feat: add app-wide unlock gate#1076
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a shell-level unlock gate: a hook that checks and triggers wallet unlocks, a UI gate wrapping routed content, English/Spanish translations for the prompt, and a small vault-provider credential input type change. ChangesShell Unlock Gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
BundleMonFiles removed (2)
Files updated (8)
Unchanged files (65)
Total files change +2.32KB +0.25% Groups updated (2)
Final result: ✅ View report in BundleMon website ➡️ |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d8ec5d27e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| state: { | ||
| isChecking: statusQuery.isLoading, | ||
| isLocked: statusQuery.data === false, |
There was a problem hiding this comment.
This gate currently treats every state except data === false as unlocked, while only showing a loader for isLoading. In this app, queries are persisted via PersistQueryClientProvider (packages/feature-shell/src/data-access/shell-providers.tsx), so a previously cached true can be hydrated on startup before requireWalletKey rechecks the new in-memory vault state. That means locked wallets can briefly render routed content (and run child effects) until refetch flips the value, which defeats the purpose of an app-wide lock gate.
Useful? React with 👍 / 👎.
Add a shell unlock gate that checks the active wallet key and blocks app content with the shared unlock dialog when needed. Add shell translations for the locked state and hide PIN input values in the shared unlock dialog.
beeman
force-pushed
the
beeman/app-wide-unlock-gate
branch
from
2d8ec5d to
59aca34
Compare
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
samui-wallet-web | 59aca34 | Commit Preview URL Branch Preview URL |
May 14 2026, 01:32 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
samui-wallet-api | 59aca34 | Commit Preview URL Branch Preview URL |
May 14 2026, 01:32 PM |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| confirmPasswordLabel: copy.confirmPasswordLabel, | ||
| credential, | ||
| credentialInputType: pending?.mode === 'pin' ? 'text' : 'password', | ||
| credentialInputType: 'password', |
There was a problem hiding this comment.
🟡 PIN input type changed to 'password', inconsistent with request unlock dialog
The credentialInputType was changed from pending?.mode === 'pin' ? 'text' : 'password' to a hardcoded 'password'. This means PIN-protected wallets now show a masked password input (dots) in the vault unlock dialog instead of visible digits. This is inconsistent with packages/feature-request/src/ui/request-ui-unlock-dialog.tsx:35 which still uses type={state.mode === 'pin' ? 'text' : 'password'} along with inputMode="numeric" and pattern="[0-9]*" for PIN mode. The previous code deliberately differentiated between PIN and password modes; this change removes that distinction.
| credentialInputType: 'password', | |
| credentialInputType: pending?.mode === 'pin' ? 'text' : 'password', |
Was this helpful? React with 👍 or 👎 to provide feedback.
| isChecking: statusQuery.isLoading, | ||
| isLocked: statusQuery.data === false, |
There was a problem hiding this comment.
🟡 Unlock gate fails open on query error instead of showing locked state
When statusQuery errors (e.g., unlockWallet throws for an unsecured wallet at use-shell-unlock-gate.tsx:41), statusQuery.data is undefined and statusQuery.isLoading is false. This means isChecking is false and isLocked is undefined === false → false. The gate then renders children (shell-ui-unlock-gate.tsx:16-17) even though the wallet wasn't actually unlocked. The gate should fail closed (show locked/error state) rather than fail open.
How the error propagates
In the queryFn, the catch block for requireWalletKey does not wrap the unlockWallet call in its own try-catch. If unlockWallet throws, the error propagates out of the queryFn, causing the query to enter an error state with data === undefined. Since the component only checks statusQuery.data === false for the locked state, the undefined value falls through to show content.
| isChecking: statusQuery.isLoading, | |
| isLocked: statusQuery.data === false, | |
| isChecking: statusQuery.isLoading, | |
| isLocked: statusQuery.data !== true, |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59aca34318
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <ShellUiUnlockGate> | ||
| <Outlet /> | ||
| </ShellUiUnlockGate> |
There was a problem hiding this comment.
ShellUiUnlockGate is applied only around this layout’s <Outlet />, so it protects child routes under ShellUiLayout but not sibling top-level routes like /modals/* (defined in packages/feature-shell/src/create-router.tsx as a sibling of the layout route). As a result, a locked wallet can still navigate directly to modal screens (e.g. receive/send flows), which breaks the new “app-wide unlock gate” behavior and exposes app content outside the lock gate.
Useful? React with 👍 / 👎.
![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
🤖 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 `@packages/feature-shell/src/data-access/use-shell-unlock-gate.tsx`:
- Around line 72-74: The gate currently treats undefined as unlocked via
`isLocked: statusQuery.data === false`; change this to fail-closed by setting
`isLocked: statusQuery.data !== true` so only an explicit `true` marks unlocked.
While editing use-shell-unlock-gate.tsx, update the `isLocked` assignment that
references `statusQuery.data`, and optionally adjust `isChecking` (which uses
`statusQuery.isLoading`) to include `statusQuery.isError` if you want the UI to
remain in a checking state on query failures; keep `isUnlocking` tied to
`unlockMutation.isPending`.
🪄 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 Plus
Run ID: e2f5c778-b295-424a-ac47-7b63db0e1c52
📒 Files selected for processing (7)
packages/feature-shell/src/data-access/use-shell-unlock-gate.tsxpackages/feature-shell/src/ui/shell-ui-layout.tsxpackages/feature-shell/src/ui/shell-ui-unlock-gate.tsxpackages/i18n/locales/en/shell.jsonpackages/i18n/locales/es/shell.jsonpackages/i18n/src/resources.d.tspackages/vault-react/src/data-access/use-vault-unlock-provider.ts
| isChecking: statusQuery.isLoading, | ||
| isLocked: statusQuery.data === false, | ||
| isUnlocking: unlockMutation.isPending, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TanStack React Query v5, when a query function throws (with retry disabled), what are the values of data, isError, and isLoading?
💡 Result:
If a TanStack React Query v5 query function throws (and retries are disabled with retry: false), then after the failed attempt settles: - data: undefined [1] - isError: true [1] - isLoading: false [1] Reasoning: In v5, isError is derived from status === 'error' [1], and data is only available in status === 'success' (otherwise it’s undefined) [1]. Also, isLoading is only true when the first fetch is in-flight, defined as isFetching && isPending [1]; once the query ends in status: 'error', it’s no longer pending/in-flight, so isLoading becomes false.
Citations:
🏁 Script executed:
# Check if file exists and view the relevant lines
if [ -f "packages/feature-shell/src/data-access/use-shell-unlock-gate.tsx" ]; then
echo "=== File found ==="
wc -l "packages/feature-shell/src/data-access/use-shell-unlock-gate.tsx"
echo ""
echo "=== Lines 60-85 context ==="
sed -n '60,85p' "packages/feature-shell/src/data-access/use-shell-unlock-gate.tsx"
else
echo "File not found at specified path"
# Try to find it
find . -name "use-shell-unlock-gate.tsx" -type f 2>/dev/null
fiRepository: samui-build/samui-wallet
Length of output: 682
🏁 Script executed:
head -50 "packages/feature-shell/src/data-access/use-shell-unlock-gate.tsx"Repository: samui-build/samui-wallet
Length of output: 1740
Fail closed when unlock status is not explicitly true.
isLocked: statusQuery.data === false treats undefined (after query error) as unlocked, exposing app content while lock status is unknown. With retry: false, query failures leave data: undefined, causing the gate to fail open.
💡 Proposed fix
state: {
isChecking: statusQuery.isLoading,
- isLocked: statusQuery.data === false,
+ isLocked: statusQuery.data !== true,
isUnlocking: unlockMutation.isPending,
walletName: wallet.name,
walletProtectionMode: wallet.protectionMode,📝 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.
| isChecking: statusQuery.isLoading, | |
| isLocked: statusQuery.data === false, | |
| isUnlocking: unlockMutation.isPending, | |
| isChecking: statusQuery.isLoading, | |
| isLocked: statusQuery.data !== true, | |
| isUnlocking: unlockMutation.isPending, |
🤖 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 `@packages/feature-shell/src/data-access/use-shell-unlock-gate.tsx` around
lines 72 - 74, The gate currently treats undefined as unlocked via `isLocked:
statusQuery.data === false`; change this to fail-closed by setting `isLocked:
statusQuery.data !== true` so only an explicit `true` marks unlocked. While
editing use-shell-unlock-gate.tsx, update the `isLocked` assignment that
references `statusQuery.data`, and optionally adjust `isChecking` (which uses
`statusQuery.isLoading`) to include `statusQuery.isError` if you want the UI to
remain in a checking state on query failures; keep `isUnlocking` tied to
`unlockMutation.isPending`.
1 participant
Add a shell unlock gate that checks the active wallet key and blocks app content with the shared unlock dialog when needed.
Add shell translations for the locked state and hide PIN input values in the shared unlock dialog.
Summary by CodeRabbit
New Features
Translations