fix(onboarding): remove Screen & Accessibility Permissions step#557
Conversation
The permissions step added friction to onboarding without being essential — users can still configure screen intelligence permissions later via Settings. Steps are now: Welcome → Referral → Skills → Context Gathering.
The permissions step added friction to onboarding without being essential — users can still configure screen intelligence permissions later via Settings. Steps are now: Welcome → Referral → Skills → Context Gathering.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoved the ScreenPermissions onboarding step and its tests, reindexed subsequent onboarding steps in Onboarding.tsx, adjusted step navigation and handlers to skip the removed step, and updated the e2e login flow to match the new step ordering. Changes
Sequence Diagram(s)(omitted — changes are removal/renumbering and do not introduce a new multi-component sequential flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/test/e2e/specs/login-flow.spec.ts (2)
290-318:⚠️ Potential issue | 🟠 MajorMnemonicStep does not exist in the actual onboarding flow — this test block will silently skip.
The test looks for
'Your Recovery Phrase'text to detect MnemonicStep, but this component doesn't exist in the codebase. The actual onboarding flow has only 4 steps: WelcomeStep → ReferralApplyStep (optional) → SkillsStep → ContextGatheringStep. The final step (ContextGatheringStep) handles LinkedIn enrichment, not recovery phrases. The "Your Recovery Phrase" text only appears in a separate Mnemonic.tsx page, not in the onboarding flow.When the test runs, this block will silently skip without failure, leaving the actual final onboarding step untested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/login-flow.spec.ts` around lines 290 - 318, The test block assumes a MnemonicStep by checking textExists('Your Recovery Phrase') but that component isn't in the onboarding flow, so this block silently skips and leaves the real final step untested; update the logic in this section (where textExists and clickFirstMatch are used) to detect and exercise the actual final onboarding step (ContextGatheringStep) by looking for an element/text unique to ContextGatheringStep (e.g., the LinkedIn enrichment prompt or another stable selector), replace references to MnemonicStep handling with ContextGatheringStep actions, and add a failing assertion or explicit error when neither expected step is present so the test fails fast instead of silently skipping.
266-288:⚠️ Potential issue | 🔴 CriticalUpdate text detection patterns to match actual onboarding UI.
The test searches for
'Enable Tools'(line 268) and'Install Skills'(line 280), but these strings are not rendered by any actual onboarding component. Verification found:
ToolsStep.tsxdoes not exist in the codebaseSkillsStep.tsxrenders a heading "Connect Gmail" (line 103), not "Install Skills"- No components render the text
"Enable Tools"or"Install Skills"The test will fail when it cannot detect these steps. Update the text patterns to match the actual rendered headings in
WelcomeStep.tsx,LocalAIStep.tsx, andSkillsStep.tsxto ensure the test can identify and navigate through the correct steps during onboarding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/login-flow.spec.ts` around lines 266 - 288, The onboarding E2E checks look for non-existent texts ("Enable Tools" / "Install Skills") so textExists/clickFirstMatch in the login-flow.spec.ts should be updated to match actual headings rendered by the onboarding components; replace the checks for 'Enable Tools' and 'Install Skills' with the actual strings used in WelcomeStep, LocalAIStep and SkillsStep (e.g., the WelcomeStep heading, LocalAIStep's heading, and "Connect Gmail" from SkillsStep) so that textExists(...) and subsequent clickFirstMatch([...], 10_000) calls can correctly detect and advance those steps during the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/test/e2e/specs/login-flow.spec.ts`:
- Around line 12-17: The test's onboarding step comments and detection logic are
out of sync with the app; update the comments to reflect the real Onboarding.tsx
sequence (WelcomeStep → LocalAIStep → ReferralApplyStep → SkillsStep →
ContextGatheringStep) and replace the broken text checks that look for
non-existent UI strings ("Enable Tools", "Install Skills", "Your Recovery
Phrase") with selectors that target the actual onboarding components or their
real UI labels (e.g., assert presence of WelcomeStep, LocalAIStep,
ReferralApplyStep, SkillsStep, ContextGatheringStep by using component-specific
testids, heading text, or roles), and update any assertions that assumed a
ToolsStep or MnemonicStep to instead expect the
ReferralApplyStep/ContextGatheringStep behaviors; locate the checks in
app/test/e2e/specs/login-flow.spec.ts (the step-detection blocks around the
current checks) and change them to query real labels or testids exposed by those
components.
---
Outside diff comments:
In `@app/test/e2e/specs/login-flow.spec.ts`:
- Around line 290-318: The test block assumes a MnemonicStep by checking
textExists('Your Recovery Phrase') but that component isn't in the onboarding
flow, so this block silently skips and leaves the real final step untested;
update the logic in this section (where textExists and clickFirstMatch are used)
to detect and exercise the actual final onboarding step (ContextGatheringStep)
by looking for an element/text unique to ContextGatheringStep (e.g., the
LinkedIn enrichment prompt or another stable selector), replace references to
MnemonicStep handling with ContextGatheringStep actions, and add a failing
assertion or explicit error when neither expected step is present so the test
fails fast instead of silently skipping.
- Around line 266-288: The onboarding E2E checks look for non-existent texts
("Enable Tools" / "Install Skills") so textExists/clickFirstMatch in the
login-flow.spec.ts should be updated to match actual headings rendered by the
onboarding components; replace the checks for 'Enable Tools' and 'Install
Skills' with the actual strings used in WelcomeStep, LocalAIStep and SkillsStep
(e.g., the WelcomeStep heading, LocalAIStep's heading, and "Connect Gmail" from
SkillsStep) so that textExists(...) and subsequent clickFirstMatch([...],
10_000) calls can correctly detect and advance those steps during the test.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8a58081-b13b-4366-81f0-7f6a30c41f60
📒 Files selected for processing (4)
app/src/pages/onboarding/Onboarding.tsxapp/src/pages/onboarding/steps/ScreenPermissionsStep.tsxapp/src/pages/onboarding/steps/__tests__/ScreenPermissionsStep.test.tsxapp/test/e2e/specs/login-flow.spec.ts
💤 Files with no reviewable changes (2)
- app/src/pages/onboarding/steps/ScreenPermissionsStep.tsx
- app/src/pages/onboarding/steps/tests/ScreenPermissionsStep.test.tsx
When the user clicks "Skip for Now" on the Gmail step, complete onboarding immediately instead of showing the context gathering step.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/pages/onboarding/Onboarding.tsx (1)
123-128: Skip the context step when Gmail is absent, not only when the array is empty.
ContextGatheringStepalready short-circuits on!hasGmail(app/src/pages/onboarding/steps/ContextGatheringStep.tsx:94-114). With the current branch, users who connect only non-Gmail sources still get routed through a step that immediately marks everything skipped. If this screen is still Gmail-only, mirror that predicate here too.Suggested diff
const handleSkillsNext = async (connectedSources: string[]) => { console.debug('[onboarding:handleSkillsNext]', { connectedSources }); setDraft(prev => ({ ...prev, connectedSources })); - if (connectedSources.length === 0) { - // No sources connected — skip context gathering and finish onboarding. + const hasGmailSource = connectedSources.some(source => source.includes('gmail')); + if (!hasGmailSource) { + // No Gmail source connected — skip context gathering and finish onboarding. await handleContextNext(connectedSources); } else { handleNext(); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/onboarding/Onboarding.tsx` around lines 123 - 128, The onboarding flow should skip the context-gathering step not only when connectedSources is empty but also when there is no Gmail source; change the condition around handleContextNext/handleNext to check for a Gmail presence (e.g., compute hasGmail = connectedSources.some(s => s.type === 'gmail') or reuse the same predicate used by ContextGatheringStep) and call await handleContextNext(connectedSources) only when hasGmail is true (and connectedSources.length > 0), otherwise call handleNext(); update the branch that currently uses connectedSources.length === 0 to use this hasGmail check and reference the connectedSources array and handleContextNext/handleNext functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/pages/onboarding/Onboarding.tsx`:
- Around line 134-140: When calling setOnboardingTasks in Onboarding.tsx,
preserve the existing accessibilityPermissionGranted value instead of
unconditionally setting it to false; read the current onboardingTasks (or the
source of truth used elsewhere) and pass accessibilityPermissionGranted:
onboardingTasks?.accessibilityPermissionGranted ?? false (or equivalent) so the
reset mirrors ToolsPanel.tsx’s defensive pattern, while still resetting other
fields like localModelConsentGiven, localModelDownloadStarted, enabledTools
(getDefaultEnabledTools()), connectedSources, and updatedAtMs.
---
Nitpick comments:
In `@app/src/pages/onboarding/Onboarding.tsx`:
- Around line 123-128: The onboarding flow should skip the context-gathering
step not only when connectedSources is empty but also when there is no Gmail
source; change the condition around handleContextNext/handleNext to check for a
Gmail presence (e.g., compute hasGmail = connectedSources.some(s => s.type ===
'gmail') or reuse the same predicate used by ContextGatheringStep) and call
await handleContextNext(connectedSources) only when hasGmail is true (and
connectedSources.length > 0), otherwise call handleNext(); update the branch
that currently uses connectedSources.length === 0 to use this hasGmail check and
reference the connectedSources array and handleContextNext/handleNext functions.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0ae3c63-8782-41be-8ead-d77af0bdf6b6
📒 Files selected for processing (1)
app/src/pages/onboarding/Onboarding.tsx
- Preserve accessibilityPermissionGranted from existing state instead of hardcoding false (matches ToolsPanel defensive pattern) - Update E2E test step comments and detection logic to match the real onboarding flow (WelcomeStep → ReferralApplyStep → SkillsStep → ContextGatheringStep) and use actual UI text
|
Addressed both CodeRabbit findings in 7d93407:
|
2 participants
Summary
Changes
Onboarding.tsx: removed ScreenPermissionsStep, renumbered steps (max 3 instead of 4)ScreenPermissionsStep.tsxand its testTest plan
yarn typecheckpassesyarn lintpasses (only pre-existing warnings)Summary by CodeRabbit
Onboarding
Tests