feat(onboarding): pre-add teammates to the team invite list - #5496
Open
jbecke wants to merge 3 commits into
Open
feat(onboarding): pre-add teammates to the team invite list#5496jbecke wants to merge 3 commits into
jbecke wants to merge 3 commits into
Conversation
The team step offered same-domain contacts as pills you had to click to add, so the default outcome of the screen was "nobody gets invited". Flip the default: as soon as contacts and the suggested team domain resolve, those teammates land in the invite list themselves, and each row gets an X to take one back out. Doing nothing now sends the invites. A line above the list says so outright, so the pre-added rows read as a default and not a trick. Invite-list logic (who is prefillable, how prefills merge with typed rows, row removal, address validation) moves into teamInvites.ts with unit tests. The created event also reports invites_prefilled/invites_removed so the prefill's effect on invite volume is measurable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fw4qvAPAPybHoM32XJcMS
…osed Rendering the step for real (see apps/web/harness) showed three things the prefill broke or muddled: - the invite list's max-h-48 scroller was sized for two rows and sliced the fourth row in half now that the list opens with seven; the flow's own scroll container takes the height instead - the X sat on blank rows too, where "don't invite" means nothing; it now appears only on rows naming someone - rows with no X stretched wider than the rest, so the remove gutter is now always reserved — including beside the team-name field, which puts every input in the form on one width The harness renders TeamStep with fixture hooks (no backend) and its shoot.mjs drives the screen, asserts what gets pre-added, removed, and submitted, and writes screenshots. Dev tooling only — the directory sits outside tsconfig include and knip's project globs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fw4qvAPAPybHoM32XJcMS
Title becomes "Macro is meant for teams".
The name field now carries a visible "Team name" label: it arrives
pre-filled from the email domain, and a placeholder disappears exactly when
the field most needs identifying. Its placeholder becomes an example
("Acme Inc.") rather than repeating the label in the empty state.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017fw4qvAPAPybHoM32XJcMS
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded shared utilities for same-domain invite prefilling, invite-slot removal, and email validation with Vitest coverage. Updated 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/web/harness/shoot.mjs (1)
8-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse Playwright browser discovery unless the fixed browser path is documented.
.github/actions/.setup-reqs-web/action.ymlinstalls Chromium throughbunx playwright install --with-deps chromium, butapps/web/harness/shoot.mjsexpects/opt/pw-browsers/chromiumandapps/web/harness/README.mdonly listsbun harness/shoot.mjs ./shotswithout explaining that provisioning. Either removeexecutablePathor document and enforce the required provisioned path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/harness/shoot.mjs` around lines 8 - 10, The browser launch in the harness currently depends on an undocumented fixed executable path. Update the chromium.launch configuration in shoot.mjs to use Playwright’s installed-browser discovery by removing executablePath, unless the repository explicitly documents and enforces provisioning at /opt/pw-browsers/chromium.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/harness/mocks/contacts.ts`:
- Around line 13-14: Update PERSONAL_CONTACTS so every fixture uses a
non-macro.com domain, replacing nikhil@macro.com with an appropriate personal
address while preserving the scenario contract described by the adjacent
comment.
In `@apps/web/harness/mocks/invitations.ts`:
- Around line 3-6: Update the useJoinTeamMutation mock to accept mutation
options containing onSuccess, then invoke that callback after mutate completes
successfully while preserving the existing logging and isPending behavior.
In `@apps/web/harness/shoot.mjs`:
- Around line 30-75: The smoke test in the top-level interaction flow must
assert expected fixture values instead of only logging them. Import
node:assert/strict and add assertions after prefill, after removing Tom and Ade,
after typing newhire@macro.com, and after submission to validate inviteValues(),
CTA state, and the mocked create-team and analytics payloads against the fixture
values.
In `@apps/web/src/features/setup/flow/TeamStep.tsx`:
- Around line 175-189: Refactor the TeamStep prefill flow around
prefillableTeammates and the invite-slot derivation: store pending and dismissed
prefills as explicit state, then derive visible invite slots from contacts(),
customDomain(), email(), and inviteSlots() without createEffect assigning
derived teammates back into setPrefilled or setInviteSlots. Preserve the
existing behavior of waiting for asynchronously available suggestions and
excluding dismissed prefills.
---
Nitpick comments:
In `@apps/web/harness/shoot.mjs`:
- Around line 8-10: The browser launch in the harness currently depends on an
undocumented fixed executable path. Update the chromium.launch configuration in
shoot.mjs to use Playwright’s installed-browser discovery by removing
executablePath, unless the repository explicitly documents and enforces
provisioning at /opt/pw-browsers/chromium.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b69faac-e6a4-4ba3-b873-2853c5da7482
📒 Files selected for processing (18)
apps/web/harness/README.mdapps/web/harness/harness.cssapps/web/harness/index.htmlapps/web/harness/main.tsxapps/web/harness/mocks/analytics.tsapps/web/harness/mocks/contacts.tsapps/web/harness/mocks/invitations.tsapps/web/harness/mocks/onboarding.tsapps/web/harness/mocks/teams.tsapps/web/harness/mocks/user.tsapps/web/harness/scenario.tsapps/web/harness/shoot.mjsapps/web/harness/vite.config.tsapps/web/src/features/setup/flow/OnboardingFlow.tsxapps/web/src/features/setup/flow/TeamStep.tsxapps/web/src/features/setup/flow/teamInvites.test.tsapps/web/src/features/setup/flow/teamInvites.tsapps/web/src/lib/analytics/app-events.ts
Comment on lines
+13
to
+14
| // Personal-email users still have contacts — just none on a team domain. | ||
| const PERSONAL_CONTACTS = ['mom@aol.com', 'nikhil@macro.com']; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep personal fixtures off the team domain.
PERSONAL_CONTACTS contains nikhil@macro.com, although the comment says that personal-email users have no team-domain contacts. Replace this address with a non-macro.com fixture, or update the scenario contract if the cross-domain contact is intentional.
Proposed fixture correction
-const PERSONAL_CONTACTS = ['mom@aol.com', 'nikhil@macro.com'];
+const PERSONAL_CONTACTS = ['mom@aol.com', 'friend@example.com'];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Personal-email users still have contacts — just none on a team domain. | |
| const PERSONAL_CONTACTS = ['mom@aol.com', 'nikhil@macro.com']; | |
| // Personal-email users still have contacts — just none on a team domain. | |
| const PERSONAL_CONTACTS = ['mom@aol.com', 'friend@example.com']; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/harness/mocks/contacts.ts` around lines 13 - 14, Update
PERSONAL_CONTACTS so every fixture uses a non-macro.com domain, replacing
nikhil@macro.com with an appropriate personal address while preserving the
scenario contract described by the adjacent comment.
Comment on lines
+3
to
+6
| export const useJoinTeamMutation = () => ({ | ||
| isPending: false, | ||
| mutate: (v: unknown) => console.log('[join-team]', v), | ||
| }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the onSuccess contract in the mutation mock.
The consumer in apps/web/src/features/setup/flow/TeamStep.tsx passes an onSuccess callback at Lines 112-116. This mock discards that callback, so the harness never records joined_invite after mutate. If success handling owns continuation, the join flow also cannot advance.
Accept the options and invoke onSuccess after a successful mutate.
Proposed mock correction
-export const useJoinTeamMutation = () => ({
+export const useJoinTeamMutation = (
+ options?: { onSuccess?: () => void }
+) => ({
isPending: false,
- mutate: (v: unknown) => console.log('[join-team]', v),
+ mutate: (v: unknown) => {
+ console.log('[join-team]', v);
+ options?.onSuccess?.();
+ },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const useJoinTeamMutation = () => ({ | |
| isPending: false, | |
| mutate: (v: unknown) => console.log('[join-team]', v), | |
| }); | |
| export const useJoinTeamMutation = ( | |
| options?: { onSuccess?: () => void } | |
| ) => ({ | |
| isPending: false, | |
| mutate: (v: unknown) => { | |
| console.log('[join-team]', v); | |
| options?.onSuccess?.(); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/harness/mocks/invitations.ts` around lines 3 - 6, Update the
useJoinTeamMutation mock to accept mutation options containing onSuccess, then
invoke that callback after mutate completes successfully while preserving the
existing logging and isPending behavior.
Comment on lines
+30
to
+75
| const inviteValues = () => | ||
| page.$$eval('input[id^="invite-"]', (els) => els.map((e) => e.value)); | ||
|
|
||
| // 1. The new default: same-domain teammates already in the list. | ||
| await open('?scenario=prefill'); | ||
| console.log('prefilled rows:', await inviteValues()); | ||
| console.log('cta:', await page.textContent('button:has-text("Create team")')); | ||
| await shot('01-prefilled-light'); | ||
| console.log( | ||
| 'remove buttons:', | ||
| await page.$$eval('button[aria-label^="Don\'t invite"]', (e) => e.length), | ||
| 'of', | ||
| (await inviteValues()).length, | ||
| 'rows' | ||
| ); | ||
| console.log('page scrollable:', await page.evaluate(() => document.body.scrollHeight > window.innerHeight)); | ||
|
|
||
| // 2. Hover state on a remove button. | ||
| await page.hover('button[aria-label="Don\'t invite tom@macro.com"]'); | ||
| await page.waitForTimeout(200); | ||
| await shot('02-remove-hover'); | ||
|
|
||
| // 3. Two teammates removed — the X actually drops the right rows. | ||
| await page.click('button[aria-label="Don\'t invite tom@macro.com"]'); | ||
| await page.click('button[aria-label="Don\'t invite ade@macro.com"]'); | ||
| await page.waitForTimeout(200); | ||
| console.log('after removing 2:', await inviteValues()); | ||
| console.log('cta:', await page.textContent('button:has-text("Create team")')); | ||
| await shot('03-after-removing-two'); | ||
|
|
||
| // 4. Typing into the trailing empty row still works, and the row grows an X. | ||
| const slots = await page.$$('input[id^="invite-"]'); | ||
| await slots[slots.length - 1].fill('newhire@macro.com'); | ||
| await page.waitForTimeout(200); | ||
| console.log('after typing:', await inviteValues()); | ||
| await shot('04-typed-into-empty-row'); | ||
|
|
||
| // 5. What actually gets submitted (logged by the mocked mutation). | ||
| const submitted = []; | ||
| page.on('console', (m) => { | ||
| if (m.text().startsWith('[create-team]')) submitted.push(m.text()); | ||
| if (m.text().startsWith('[analytics]')) submitted.push(m.text()); | ||
| }); | ||
| await page.click('button:has-text("Create team")'); | ||
| await page.waitForTimeout(500); | ||
| console.log('submitted:', submitted.join('\n ')); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the expected invite and submission values.
The script only logs inviteValues() and submitted console messages. A wrong prefill, removal result, or analytics payload can pass this smoke test. Add node:assert/strict checks against the fixture values after each interaction. This also makes the README assertion claim accurate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/harness/shoot.mjs` around lines 30 - 75, The smoke test in the
top-level interaction flow must assert expected fixture values instead of only
logging them. Import node:assert/strict and add assertions after prefill, after
removing Tom and Ade, after typing newhire@macro.com, and after submission to
validate inviteValues(), CTA state, and the mocked create-team and analytics
payloads against the fixture values.
Comment on lines
+175
to
189
| createEffect(() => { | ||
| if (hasPrefilled) return; | ||
| const teammates = prefillableTeammates({ | ||
| contacts: contacts(), | ||
| domain: customDomain(), | ||
| ownEmail: email(), | ||
| slots: inviteSlots(), | ||
| }); | ||
| // Contacts and the domain suggestion land asynchronously — keep waiting | ||
| // (and keep tracking) until there's actually someone to pre-add. | ||
| if (teammates.length === 0) return; | ||
| hasPrefilled = true; | ||
| setPrefilled(teammates); | ||
| setInviteSlots((slots) => withPrefilledTeammates(slots, teammates)); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate TeamStep.tsx =="
fd -a 'TeamStep\.tsx$' . || true
echo "== file stats =="
wc -l apps/web/src/features/setup/flow/TeamStep.tsx 2>/dev/null || true
echo "== outline =="
ast-grep outline apps/web/src/features/setup/flow/TeamStep.tsx --view compact 2>/dev/null || true
echo "== relevant TeamStep lines =="
sed -n '1,340p' apps/web/src/features/setup/flow/TeamStep.tsx | cat -n
echo "== search invite/prefill utilities =="
rg -n "prefill|inviteSlots|createSignal|createEffect|createMemo|customDomain|invite_slots|prefilledTeammates|withPrefilledTeammates" apps/web -g '*.{ts,tsx}' | head -200Repository: macro-inc/macro
Length of output: 34919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate teamInvites files =="
fd -a 'teamInvites\.(ts|tsx)$' apps/web/src/features/setup || true
fd -a 'teamInvites/test|teamInvites.spec' apps/web/src/features/setup || true
echo "== files under setup flow =="
git ls-files apps/web/src/features/setup/flow | sed -n '1,120p'
echo "== read teamInvites module and tests =="
for f in $(git ls-files | grep 'apps/web/src/features/setup/flow/teamInvites\.' | sed -n '1,10p'); do
echo "--- $f ---"
wc -l "$f"
cat -n "$f"
done
echo "== contacts query implementation =="
sed -n '1,180p' apps/web/src/lib/queries/contacts/contacts.ts | cat -n
echo "== onboarding domain hint implementation =="
rg -n "suggested_team_domain|onboarding|domain" apps/web/src/lib/queries/onboarding apps/web/src/features/setup -g '*.{ts,tsx}' | head -160Repository: macro-inc/macro
Length of output: 18216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files under queries onboarding =="
fd -a 'onboarding' apps/web/src/lib/queries apps/web/src -g '*.{ts,tsx}' || true
find apps/web/src -path '*onboarding*' -maxdepth 4 -type f | sed -n '1,120p'
echo "== locate useOnboardingQuery definition =="
rg -n "function useOnboardingQuery|const useOnboardingQuery|export function useOnboardingQuery|export const useOnboardingQuery" apps/web/src -g '*.{ts,tsx}'
echo "== useOnboardingQuery implementation and surrounding context =="
for f in $(rg -l "function useOnboardingQuery|const useOnboardingQuery|export function useOnboardingQuery|export const useOnboardingQuery" apps/web/src -g '*.{ts,tsx}' | sed -n '1,10p'); do
echo "--- $f ---"
rg -n -C 120 "function useOnboardingQuery|const useOnboardingQuery|export function useOnboardingQuery|export const useOnboardingQuery" "$f"
done
echo "== Suspense usage in TeamStep vicinity / whole component =="
rg -n "Suspense|createEffect|derived|source|createRoot|createReaction" apps/web/src/lib/urql-solid apps/web/src/lib/queries apps/web/src/features/setup/flow/TeamStep.tsx | sed -n '1,160p'Repository: macro-inc/macro
Length of output: 21804
Derive prefills instead of copying them through a createEffect.
The invite rows depend on active query state (contacts, customDomain, inviteSlots). Store pending prefills and dismissed prefills as explicit state, derive the visible invite slots from that state, and avoid this side effect from assigning derived data back to local signals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/features/setup/flow/TeamStep.tsx` around lines 175 - 189,
Refactor the TeamStep prefill flow around prefillableTeammates and the
invite-slot derivation: store pending and dismissed prefills as explicit state,
then derive visible invite slots from contacts(), customDomain(), email(), and
inviteSlots() without createEffect assigning derived teammates back into
setPrefilled or setInviteSlots. Preserve the existing behavior of waiting for
asynchronously available suggestions and excluding dismissed prefills.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The team step offered same-domain contacts as pills you had to click to
add, so the default outcome of the screen was "nobody gets invited".
Flip the default: as soon as contacts and the suggested team domain
resolve, those teammates land in the invite list themselves, and each row
gets an X to take one back out. Doing nothing now sends the invites.
A line above the list says so outright, so the pre-added rows read as a
default and not a trick.
Invite-list logic (who is prefillable, how prefills merge with typed rows,
row removal, address validation) moves into teamInvites.ts with unit
tests. The created event also reports invites_prefilled/invites_removed so
the prefill's effect on invite volume is measurable.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_017fw4qvAPAPybHoM32XJcMS