refactor(onboarding): remove MnemonicStep and Open Accessibility button, add Recovery Phrase settings panel#279
Merged
graycyrus merged 4 commits intoApr 2, 2026
Conversation
…on, add Recovery Phrase settings panel - Remove "Open Accessibility" button from ScreenPermissionsStep (step 2) - Remove MnemonicStep (step 5) from onboarding, reducing total steps from 6 to 5 - Move onboarding completion logic (setOnboardedForUser + setOnboardingCompleted) into handleSkillsNext - Add RecoveryPhrasePanel in Settings with the same BIP39 generate/import functionality - Wire recovery-phrase route into Settings.tsx, SettingsHome menu, and useSettingsNavigation
…nd dictation routes)
Contributor
📝 WalkthroughWalkthroughMoves BIP39 recovery phrase management out of onboarding into a new Settings panel (Recovery Phrase), removes the Mnemonic onboarding step (onboarding now 5 steps), adds a Settings route and menu entry for Recovery Phrase, and removes a navigation button from the Screen Permissions step. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as RecoveryPhrasePanel
participant Crypto as Crypto/KeyDerivation
participant Store as Redux
participant Skills as skillManager
participant Router as Navigation
User->>UI: select mode (generate / import)
UI-->>User: show generated phrase OR input fields
alt Generate -> Save
User->>UI: confirm & save
UI->>Crypto: derive AES key & wallet from mnemonic
else Import -> Save
User->>UI: paste/type full phrase
UI->>Crypto: validate & derive AES key & wallet
end
Crypto-->>UI: returns key, walletAddress
UI->>Store: dispatch setEncryptionKeyForUser(userId, key)
Store-->>UI: ack
UI->>Skills: skillManager.setWalletAddress(walletAddress)
Skills-->>UI: ack
UI->>Router: navigate back / show success
Router-->>User: return to Settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🤖 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/components/settings/panels/RecoveryPhrasePanel.tsx`:
- Around line 35-37: The import mode always initializes importWords to
IMPORT_SLOTS_INITIAL (12) so longer phrases can't be typed; add a word-count
selector state (e.g., selectedWordCount) and use setImportWords to resize the
importWords array to the selected length (fill new slots with '') before any
validation or rendering; update the input rendering to map over importWords (so
it grows/shrinks with selectedWordCount) and ensure validation logic that sets
importValid reads importWords.length (not a hardcoded 12) and runs only after
resizing; also keep existing multi-word paste growth logic but make the manual
selector the authoritative way to expand slots for 15/18/21/24.
- Around line 169-178: The current flow unconditionally overwrites the stored
encryption key and wallet address (deriveAesKeyFromMnemonic,
deriveEvmAddressFromMnemonic) before ensuring downstream runtime sync succeeds;
change it so you first attempt the runtime operations and only persist on
success: capture the current key/wallet from the store, call
skillManager.setWalletAddress(walletAddress) and await the runtime sync/load
call used in manager.ts (the same runtime.load or sync method that can fail) as
a dry-run, and only then dispatch setEncryptionKeyForUser({ userId: user._id,
key: aesKey }) and set the new wallet; if the runtime step fails, do not persist
the new key/wallet and surface the error, and if you must persist then implement
a rollback by restoring the previous key/wallet with setEncryptionKeyForUser
using the captured backups on failure.
- Around line 303-325: The recovery-word inputs are unlabeled for assistive
tech; update the input rendering inside the map that uses importWords,
inputRefs, handleImportWordChange and handleImportKeyDown to provide accessible
labels—either give each input an explicit aria-label like `Recovery word ${index
+ 1}` or create a unique id on the numbered <span> and set input aria-labelledby
to that id (ensuring the span is not aria-hidden), so screen readers can
identify each field; keep the visual numbering but add the aria attribute based
on index and do not change other handlers or refs.
🪄 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: df39fbde-361f-4010-83fd-bfdc5d014ee5
📒 Files selected for processing (7)
.claude/memory.mdapp/src/components/settings/SettingsHome.tsxapp/src/components/settings/hooks/useSettingsNavigation.tsapp/src/components/settings/panels/RecoveryPhrasePanel.tsxapp/src/pages/Settings.tsxapp/src/pages/onboarding/Onboarding.tsxapp/src/pages/onboarding/steps/ScreenPermissionsStep.tsx
Comment on lines
+169
to
+178
| const aesKey = deriveAesKeyFromMnemonic(phraseToUse); | ||
| const walletAddress = deriveEvmAddressFromMnemonic(phraseToUse); | ||
|
|
||
| if (!user?._id) { | ||
| setError('User not loaded. Please sign in again or refresh the page.'); | ||
| return; | ||
| } | ||
| dispatch(setEncryptionKeyForUser({ userId: user._id, key: aesKey })); | ||
| await skillManager.setWalletAddress(walletAddress); | ||
| setSuccess(true); |
Contributor
There was a problem hiding this comment.
Guard this settings save path against destructive overwrite and partial writes.
Now that recovery phrase setup lives in Settings, this code can run long after a user already has encrypted data. It unconditionally replaces the current encryption key and wallet address, and app/src/store/authSlice.ts:82-85 plus app/src/lib/skills/manager.ts:462-478 show both values are persisted before any downstream runtime error is surfaced. A failed runtime.load() can therefore show an error after the new key material is already stored. Please block silent rotation here, or make the save path atomic/non-fatal for runtime sync failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/RecoveryPhrasePanel.tsx` around lines 169
- 178, The current flow unconditionally overwrites the stored encryption key and
wallet address (deriveAesKeyFromMnemonic, deriveEvmAddressFromMnemonic) before
ensuring downstream runtime sync succeeds; change it so you first attempt the
runtime operations and only persist on success: capture the current key/wallet
from the store, call skillManager.setWalletAddress(walletAddress) and await the
runtime sync/load call used in manager.ts (the same runtime.load or sync method
that can fail) as a dry-run, and only then dispatch setEncryptionKeyForUser({
userId: user._id, key: aesKey }) and set the new wallet; if the runtime step
fails, do not persist the new key/wallet and surface the error, and if you must
persist then implement a rollback by restoring the previous key/wallet with
setEncryptionKeyForUser using the captured backups on failure.
Contributor
Author
There was a problem hiding this comment.
This matches the original MnemonicStep behavior exactly — the onboarding step also unconditionally wrote the key and wallet address without a confirmation dialog. Since this is a 1:1 port of existing functionality into Settings, adding atomic save/rollback or a confirmation gate would be a scope expansion. Happy to address in a follow-up if desired.
Contributor
There was a problem hiding this comment.
@graycyrus, that's a fair point — if MnemonicStep had the same write-then-surface-errors pattern and nothing has broken in practice, keeping this a faithful port makes sense for this PR's scope.
Since you mentioned being open to a follow-up, would you like me to open a GitHub issue to track adding atomic save / rollback to RecoveryPhrasePanel? I can include a backlink to this PR and the relevant comment.
🧠 Learnings used
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 167
File: app/src/components/OnboardingOverlay.tsx:30-39
Timestamp: 2026-04-01T13:52:17.936Z
Learning: In React components, do not perform synchronous `setState` (or other state-updating calls) directly inside `useEffect` bodies. The lint rule `react-hooks/set-state-in-effect` (enforced in this codebase) disallows patterns such as resetting state flags at the top of an effect before returning. Refactor to avoid calling state setters in the effect; prefer deriving state from props/render, using refs, or restructuring logic so state updates occur outside `useEffect`.
… selector Address CodeRabbit review feedback: - Add aria-label to each recovery phrase word input for screen readers - Add word-count selector (12/15/18/21/24) so longer phrases can be entered manually, not just via paste
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/components/settings/panels/RecoveryPhrasePanel.tsx (1)
184-193:⚠️ Potential issue | 🟠 MajorThis save path is still non-atomic.
Lines 191-192 persist the new AES key immediately, and
skillManager.setWalletAddress()persists the wallet address before its runtime reload can throw (app/src/store/authSlice.ts:82-92,app/src/lib/skills/manager.ts:467-478). In Settings that can silently rotate or partially overwrite an existing account. Please gate overwrite, or roll both fields back on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/RecoveryPhrasePanel.tsx` around lines 184 - 193, The save flow is non-atomic: setEncryptionKeyForUser is dispatched before skillManager.setWalletAddress can fail, risking partial overwrite; fix by making the update atomic—capture the current AES key and wallet address, derive aesKey via deriveAesKeyFromMnemonic and walletAddress via deriveEvmAddressFromMnemonic, then call skillManager.setWalletAddress(walletAddress) and only after it resolves dispatch setEncryptionKeyForUser({ userId: user._id, key: aesKey }); if skillManager.setWalletAddress throws, do not dispatch; if you must optimistically dispatch first, store the previous key and wallet and on any subsequent failure roll them back by dispatching the previous values (use setEncryptionKeyForUser to restore) and surface the error to the user.
🤖 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/components/settings/panels/RecoveryPhrasePanel.tsx`:
- Around line 82-91: The fallback copy catch block currently ignores
document.execCommand('copy')'s boolean return and unconditionally calls
setCopied(true); update the catch block in RecoveryPhrasePanel (around the
textarea fallback) to capture the result of document.execCommand('copy'), call
setCopied(true) only when it returns true, and otherwise set an error state
(e.g., setCopied(false) and/or invoke an existing error handler or setCopyError)
so the UI displays a failure instead of a false "Copied" success; ensure you
still clean up the textarea element in all paths.
- Around line 119-126: The current handler auto-advances when value.trim() is
non-empty, which advances after the first character; change the logic in the
input change/paste handler around importWords/setImportWords/setImportValid so
that focus only moves to inputRefs.current[index + 1] when the new input
represents a completed word (e.g., the input contains a delimiter/whitespace or
multiple characters) or when the change originates from a paste event, not
merely any non-empty value; detect paste via the paste event or check for
delimiters in value and only call inputRefs.current[index + 1]?.focus() in those
cases.
---
Duplicate comments:
In `@app/src/components/settings/panels/RecoveryPhrasePanel.tsx`:
- Around line 184-193: The save flow is non-atomic: setEncryptionKeyForUser is
dispatched before skillManager.setWalletAddress can fail, risking partial
overwrite; fix by making the update atomic—capture the current AES key and
wallet address, derive aesKey via deriveAesKeyFromMnemonic and walletAddress via
deriveEvmAddressFromMnemonic, then call
skillManager.setWalletAddress(walletAddress) and only after it resolves dispatch
setEncryptionKeyForUser({ userId: user._id, key: aesKey }); if
skillManager.setWalletAddress throws, do not dispatch; if you must
optimistically dispatch first, store the previous key and wallet and on any
subsequent failure roll them back by dispatching the previous values (use
setEncryptionKeyForUser to restore) and surface the error to the user.
🪄 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: 8fe765f4-b83a-45c2-8a25-8f023cfaddc1
📒 Files selected for processing (1)
app/src/components/settings/panels/RecoveryPhrasePanel.tsx
…ipboard fallback
Address CodeRabbit round 2:
- Remove auto-focus-advance after single character input (was breaking
manual word entry — typing 'abandon' would split across slots)
- Guard execCommand('copy') fallback with return value check
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/components/settings/panels/RecoveryPhrasePanel.tsx (2)
166-178: RedundantsetLoading(false)calls.The
finallyblock at line 193 always executes—even on earlyreturn—so the explicitsetLoading(false)at lines 168 and 174 are unnecessary.♻️ Suggested cleanup
if (mode === 'import') { if (!handleValidateImport()) { - setLoading(false); return; } phraseToUse = importWords.join(' ').trim(); } else { if (!confirmed) { - setLoading(false); return; } phraseToUse = mnemonic; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/RecoveryPhrasePanel.tsx` around lines 166 - 178, The code calls setLoading(false) inside both early-return branches of the submit flow (when mode === 'import' fails handleValidateImport() and when confirmed is false), but the outer try/finally already clears loading; remove these redundant setLoading(false) calls so only the finally block handles clearing loading; update the branches around mode, handleValidateImport(), importWords, confirmed, and mnemonic to return early without calling setLoading(false).
315-326: Addaria-pressedto word-count selector buttons.These buttons act as toggle/selection controls. Adding
aria-pressedcommunicates the selected state to assistive technology.♿ Suggested fix
<button key={len} type="button" + aria-pressed={selectedWordCount === len} onClick={() => handleWordCountChange(len)} className={`px-2.5 py-1 text-xs font-medium rounded-lg transition-colors ${ selectedWordCount === len🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/RecoveryPhrasePanel.tsx` around lines 315 - 326, The word-count selector buttons in RecoveryPhrasePanel act as toggles but lack ARIA state; update the button element rendered in the map (the one using handleWordCountChange and selectedWordCount) to include an aria-pressed attribute whose value is a boolean expression comparing selectedWordCount === len (true when selected, false otherwise) so assistive tech can perceive the pressed/selected state; keep the existing className logic unchanged and ensure aria-pressed is present on each generated button.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/components/settings/panels/RecoveryPhrasePanel.tsx`:
- Around line 166-178: The code calls setLoading(false) inside both early-return
branches of the submit flow (when mode === 'import' fails handleValidateImport()
and when confirmed is false), but the outer try/finally already clears loading;
remove these redundant setLoading(false) calls so only the finally block handles
clearing loading; update the branches around mode, handleValidateImport(),
importWords, confirmed, and mnemonic to return early without calling
setLoading(false).
- Around line 315-326: The word-count selector buttons in RecoveryPhrasePanel
act as toggles but lack ARIA state; update the button element rendered in the
map (the one using handleWordCountChange and selectedWordCount) to include an
aria-pressed attribute whose value is a boolean expression comparing
selectedWordCount === len (true when selected, false otherwise) so assistive
tech can perceive the pressed/selected state; keep the existing className logic
unchanged and ensure aria-pressed is present on each generated button.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00f6d9e2-b323-498c-9404-83c457d9c74f
📒 Files selected for processing (1)
app/src/components/settings/panels/RecoveryPhrasePanel.tsx
2 tasks
YellowSnnowmann
added a commit
to YellowSnnowmann/openhuman
that referenced
this pull request
Apr 3, 2026
… walkOnboarding The previous implementation did a single onboardingOverlayLikelyVisible() check before calling walkOnboarding. This created a timing race: if onboarding had not yet rendered at call time the helper returned early, leaving the overlay blocking subsequent steps. walkOnboarding already handles both cases correctly — it polls up to 8 × 400 ms for the overlay before giving up, then no-ops gracefully if not visible. completeOnboardingIfVisible now delegates to it unconditionally, eliminating the race without changing observable behaviour. Also updates the section comment to accurately describe the current 5-step onboarding sequence (Welcome → Local AI → Screen & Accessibility → Tools → Skills) following the removal of MnemonicStep in tinyhumansai#279. Closes part of tinyhumansai#201 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
graycyrus
pushed a commit
that referenced
this pull request
Apr 3, 2026
* fix(e2e): make completeOnboardingIfVisible resilient by delegating to walkOnboarding The previous implementation did a single onboardingOverlayLikelyVisible() check before calling walkOnboarding. This created a timing race: if onboarding had not yet rendered at call time the helper returned early, leaving the overlay blocking subsequent steps. walkOnboarding already handles both cases correctly — it polls up to 8 × 400 ms for the overlay before giving up, then no-ops gracefully if not visible. completeOnboardingIfVisible now delegates to it unconditionally, eliminating the race without changing observable behaviour. Also updates the section comment to accurately describe the current 5-step onboarding sequence (Welcome → Local AI → Screen & Accessibility → Tools → Skills) following the removal of MnemonicStep in #279. Closes part of #201 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(e2e): use completeOnboardingIfVisible in conversations spec conversations-web-channel-flow.spec.ts was the only spec that called walkOnboarding directly instead of the shared completeOnboardingIfVisible helper. Align it with all other specs so the single resilient code path is used consistently. Closes #201 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(e2e): add clang and cmake to Dockerfile dependencies Updated the e2e Dockerfile to include clang and cmake as additional dependencies, enhancing the build environment for end-to-end testing. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Apr 3, 2026
senamakel
deleted the
refactor/onboarding-remove-mnemonic-and-accessibility-button
branch
This was referenced Apr 7, 2026
Merged
This was referenced May 6, 2026
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
…on, add Recovery Phrase settings panel (tinyhumansai#279) * refactor(onboarding): remove MnemonicStep and Open Accessibility button, add Recovery Phrase settings panel - Remove "Open Accessibility" button from ScreenPermissionsStep (step 2) - Remove MnemonicStep (step 5) from onboarding, reducing total steps from 6 to 5 - Move onboarding completion logic (setOnboardedForUser + setOnboardingCompleted) into handleSkillsNext - Add RecoveryPhrasePanel in Settings with the same BIP39 generate/import functionality - Wire recovery-phrase route into Settings.tsx, SettingsHome menu, and useSettingsNavigation * fix(recovery-phrase): add aria-labels to import inputs and word-count selector Address CodeRabbit review feedback: - Add aria-label to each recovery phrase word input for screen readers - Add word-count selector (12/15/18/21/24) so longer phrases can be entered manually, not just via paste * fix(recovery-phrase): remove auto-advance on single char and guard clipboard fallback Address CodeRabbit round 2: - Remove auto-focus-advance after single character input (was breaking manual word entry — typing 'abandon' would split across slots) - Guard execCommand('copy') fallback with return value check
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
* fix(e2e): make completeOnboardingIfVisible resilient by delegating to walkOnboarding The previous implementation did a single onboardingOverlayLikelyVisible() check before calling walkOnboarding. This created a timing race: if onboarding had not yet rendered at call time the helper returned early, leaving the overlay blocking subsequent steps. walkOnboarding already handles both cases correctly — it polls up to 8 × 400 ms for the overlay before giving up, then no-ops gracefully if not visible. completeOnboardingIfVisible now delegates to it unconditionally, eliminating the race without changing observable behaviour. Also updates the section comment to accurately describe the current 5-step onboarding sequence (Welcome → Local AI → Screen & Accessibility → Tools → Skills) following the removal of MnemonicStep in tinyhumansai#279. Closes part of tinyhumansai#201 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(e2e): use completeOnboardingIfVisible in conversations spec conversations-web-channel-flow.spec.ts was the only spec that called walkOnboarding directly instead of the shared completeOnboardingIfVisible helper. Align it with all other specs so the single resilient code path is used consistently. Closes tinyhumansai#201 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(e2e): add clang and cmake to Dockerfile dependencies Updated the e2e Dockerfile to include clang and cmake as additional dependencies, enhancing the build environment for end-to-end testing. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Summary
setOnboardedForUser+setOnboardingCompleted) moved intohandleSkillsNextChanges
ScreenPermissionsStep.tsxuseNavigate, simplify grid to single buttonOnboarding.tsxtotalSteps6→5, move completion logic tohandleSkillsNext, removecase 5RecoveryPhrasePanel.tsxSettings.tsxrecovery-phraserouteSettingsHome.tsxuseSettingsNavigation.tsrecovery-phraseto route type + path detection.claude/memory.mdTest plan
yarn typecheckpassesyarn lintpasses (0 errors)yarn format:checkpassesyarn buildsucceedsSummary by CodeRabbit
New Features
Improvements