feat(extension-wallet): add popup router and navigation#139
Open
David-patrick-chuks wants to merge 6 commits intoancore-org:mainfrom
Open
feat(extension-wallet): add popup router and navigation#139David-patrick-chuks wants to merge 6 commits intoancore-org:mainfrom
David-patrick-chuks wants to merge 6 commits intoancore-org:mainfrom
Conversation
|
@David-patrick-chuks Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThe PR implements React Router v6 for the extension wallet with comprehensive authentication guards and navigation, replaces the demo application with a routing-based architecture, adds a navigation bar component, strengthens TypeScript type safety across storage and crypto modules by removing Changes
Sequence DiagramsequenceDiagram
participant Client as Browser/Client
participant ER as ExtensionRouter
participant Auth as AuthGuard
participant Storage as localStorage
participant Screen as Screen Component
participant Nav as NavBar
Client->>ER: Navigate to /
ER->>Storage: readAuthState()
Storage-->>ER: AuthState (onboarded, unlocked)
ER->>Auth: Route through AuthGuard
alt First-time user
Auth->>Storage: Check hasOnboarded
Auth-->>Client: Redirect to /welcome
else Onboarded but locked
Auth->>Storage: Check isUnlocked
Auth-->>Client: Redirect to /unlock
else Unlocked user
Auth->>Storage: Verify auth state
Auth->>Screen: Render protected route
Screen-->>Client: Display content + NavBar
Client->>Nav: Click navigation link (e.g., /settings)
Nav->>ER: Navigate via NavLink
ER->>Screen: Update screen component
Screen-->>Client: Render settings page
end
Client->>Auth: User action (lock/unlock/reset)
Auth->>Storage: writeAuthState(updated)
Storage-->>Auth: State persisted
Auth->>ER: Trigger redirect if needed
ER-->>Client: Navigate to appropriate route
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
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 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
🧹 Nitpick comments (4)
apps/extension-wallet/src/router/AuthGuard.tsx (2)
72-101:useMemodependency onauthStatenegates memoization benefit.The
useMemohook includesauthStatein its dependency array (line 100), which means the memoized value is recreated on every state change. Since the action functions (completeOnboarding,unlockWallet, etc.) usesetAuthStatewith functional updates, they don't actually needauthStatein their closures.⚡ Proposed optimization
const value = React.useMemo<AuthContextValue>( () => ({ authState, completeOnboarding: (walletName: string) => { setAuthState({ hasOnboarded: true, isUnlocked: true, walletName: walletName.trim() || DEFAULT_AUTH_STATE.walletName, accountAddress: DEFAULT_AUTH_STATE.accountAddress, }); }, unlockWallet: () => { setAuthState((current) => ({ ...current, hasOnboarded: true, isUnlocked: true, })); }, lockWallet: () => { setAuthState((current) => ({ ...current, isUnlocked: false, })); }, resetWallet: () => { setAuthState(DEFAULT_AUTH_STATE); }, }), - [authState] + [] // Action functions are stable; authState is read directly in consumers ); + + // Return a new object each render so consumers get the latest authState + return ( + <AuthContext.Provider value={{ ...value, authState }}> + {children} + </AuthContext.Provider> + );Alternatively, since
authStatemust be in the context value and will change, consider removinguseMemoentirely as the optimization provides no benefit here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/router/AuthGuard.tsx` around lines 72 - 101, The useMemo around value (React.useMemo<AuthContextValue>(... ,[authState])) is pointless because authState is included in the dependency array so the memoized object is recreated on every change; either remove useMemo entirely or remove authState from the dependency array and instead memoize only the action functions that rely on setAuthState (completeOnboarding, unlockWallet, lockWallet, resetWallet) and include DEFAULT_AUTH_STATE where needed so the context value remains stable without depending on authState in the closure. Ensure you keep references to authState in the context value itself (so consumers update), but avoid capturing authState inside the memoized callbacks—use functional setAuthState updates and stable memoization for the functions or drop useMemo for simplicity.
57-59: Initial mount writes existing state back to storage.On first render,
readAuthState()reads from localStorage, then the effect immediately writes the same value back. This is harmless but unnecessary.🔧 Optional: skip initial write
+ const isInitialMount = React.useRef(true); + React.useEffect(() => { + if (isInitialMount.current) { + isInitialMount.current = false; + return; + } writeAuthState(authState); }, [authState]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/router/AuthGuard.tsx` around lines 57 - 59, The effect in AuthGuard.tsx currently calls writeAuthState(authState) on initial render (after readAuthState()) which is unnecessary; update the React.useEffect that writes authState so it skips the initial mount (only writes on subsequent changes). Implement this by introducing an initialization flag (e.g., useRef<boolean> or an isInitialized state) checked inside the effect that sets the flag on first render and prevents calling writeAuthState(authState) until after that initial load; reference the existing React.useEffect, authState, readAuthState(), and writeAuthState() when making the change.apps/extension-wallet/src/router/index.tsx (2)
254-262: Type assertion for location state could be more defensive.The
fromstate extraction uses a type assertion that could throw iflocation.statehas an unexpected shape in edge cases.🛡️ Suggested defensive extraction
- const from = (location.state as { from?: string } | null)?.from ?? '/home'; + const from = + typeof location.state === 'object' && + location.state !== null && + 'from' in location.state && + typeof (location.state as Record<string, unknown>).from === 'string' + ? (location.state as { from: string }).from + : '/home';Alternatively, consider a small helper function for type-safe state extraction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/router/index.tsx` around lines 254 - 262, The extraction of from using a direct type assertion on location.state is unsafe; change it to defensively read location?.state and verify the shape before using it (e.g., check that (location?.state as any)?.from is a string) so you fallback to '/home' if the value is missing or has the wrong type; update the code around the from declaration (and any helper you add) to use optional chaining and a type/typeof check, and ensure handleUnlock/navigation behavior remains the same (unlockWallet then navigate(from, { replace: true })) so unexpected location.state shapes no longer throw.
345-355: Uncontrolled form inputs missing accessibility attributes.The recipient address and amount inputs in
SendScreenare uncontrolled (novalue/onChange) and lackname,id, and associated<label>elements. While this is a placeholder, consider adding minimal accessibility attributes for consistency with other screens.♿ Suggested accessibility improvement
<div className="space-y-3"> + <label className="block text-sm font-medium text-foreground"> + Recipient <input + name="recipient" className="w-full rounded-xl border border-border bg-background px-3 py-3 text-sm outline-none transition focus:border-primary" placeholder="Recipient address" /> + </label> + <label className="block text-sm font-medium text-foreground"> + Amount <input + name="amount" className="w-full rounded-xl border border-border bg-background px-3 py-3 text-sm outline-none transition focus:border-primary" placeholder="Amount" /> + </label> <PrimaryButton>Review transaction</PrimaryButton> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/router/index.tsx` around lines 345 - 355, In SendScreen, the two inputs for recipient address and amount are uncontrolled and missing accessibility attributes; add minimal accessible markup by giving each input a unique id and name (e.g., id="recipient" name="recipient", id="amount" name="amount") and include associated <label> elements for each (or at minimum aria-label attributes) and wire them to the inputs so screen readers can announce them; if you intend to manage state, convert the inputs to controlled components by adding value/onChange handlers in the SendScreen component and keep the labels/ids in place; ensure PrimaryButton usage remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/extension-wallet/src/screens/Settings/SecuritySettings.tsx`:
- Around line 168-177: The ExportWarningView component declares an unused title
prop; remove it from the component signature (the typed props object for
ExportWarningView) and delete any title usage in the JSX props list where
ExportWarningView is rendered (remove the title prop from both call sites that
pass it), leaving only warningText, onConfirm and onCancel; ensure the prop type
and all callers are updated so no unused/undeclared prop remains.
---
Nitpick comments:
In `@apps/extension-wallet/src/router/AuthGuard.tsx`:
- Around line 72-101: The useMemo around value
(React.useMemo<AuthContextValue>(... ,[authState])) is pointless because
authState is included in the dependency array so the memoized object is
recreated on every change; either remove useMemo entirely or remove authState
from the dependency array and instead memoize only the action functions that
rely on setAuthState (completeOnboarding, unlockWallet, lockWallet, resetWallet)
and include DEFAULT_AUTH_STATE where needed so the context value remains stable
without depending on authState in the closure. Ensure you keep references to
authState in the context value itself (so consumers update), but avoid capturing
authState inside the memoized callbacks—use functional setAuthState updates and
stable memoization for the functions or drop useMemo for simplicity.
- Around line 57-59: The effect in AuthGuard.tsx currently calls
writeAuthState(authState) on initial render (after readAuthState()) which is
unnecessary; update the React.useEffect that writes authState so it skips the
initial mount (only writes on subsequent changes). Implement this by introducing
an initialization flag (e.g., useRef<boolean> or an isInitialized state) checked
inside the effect that sets the flag on first render and prevents calling
writeAuthState(authState) until after that initial load; reference the existing
React.useEffect, authState, readAuthState(), and writeAuthState() when making
the change.
In `@apps/extension-wallet/src/router/index.tsx`:
- Around line 254-262: The extraction of from using a direct type assertion on
location.state is unsafe; change it to defensively read location?.state and
verify the shape before using it (e.g., check that (location?.state as
any)?.from is a string) so you fallback to '/home' if the value is missing or
has the wrong type; update the code around the from declaration (and any helper
you add) to use optional chaining and a type/typeof check, and ensure
handleUnlock/navigation behavior remains the same (unlockWallet then
navigate(from, { replace: true })) so unexpected location.state shapes no longer
throw.
- Around line 345-355: In SendScreen, the two inputs for recipient address and
amount are uncontrolled and missing accessibility attributes; add minimal
accessible markup by giving each input a unique id and name (e.g.,
id="recipient" name="recipient", id="amount" name="amount") and include
associated <label> elements for each (or at minimum aria-label attributes) and
wire them to the inputs so screen readers can announce them; if you intend to
manage state, convert the inputs to controlled components by adding
value/onChange handlers in the SendScreen component and keep the labels/ids in
place; ensure PrimaryButton usage remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddc0a3b2-523a-4079-941e-e0ba1f319552
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
apps/extension-wallet/package.jsonapps/extension-wallet/postcss.config.jsapps/extension-wallet/src/App.tsxapps/extension-wallet/src/components/Navigation/NavBar.tsxapps/extension-wallet/src/errors/__tests__/errors.test.tsapps/extension-wallet/src/errors/error-handler.tsapps/extension-wallet/src/index.tsapps/extension-wallet/src/main.tsxapps/extension-wallet/src/router/AuthGuard.tsxapps/extension-wallet/src/router/__tests__/router.test.tsxapps/extension-wallet/src/router/index.tsxapps/extension-wallet/src/screens/Settings/SecuritySettings.tsxapps/extension-wallet/src/screens/__tests__/TransactionDetail.test.tsxapps/extension-wallet/tailwind.config.jsapps/extension-wallet/tsconfig.jsonapps/extension-wallet/vite.config.tsapps/extension-wallet/vitest.config.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/core-sdk/eslint.config.cjs (1)
33-40: Consider addingafterAllandafterEachto test globals.The test lifecycle hooks
afterAllandafterEachare missing from the globals list. If any tests in this package use these hooks, ESLint may flag them as undefined (thoughno-undefis disabled in the parent override, so this is low risk).🔧 Suggested addition for completeness
languageOptions: { globals: { describe: 'readonly', it: 'readonly', expect: 'readonly', beforeAll: 'readonly', beforeEach: 'readonly', + afterAll: 'readonly', + afterEach: 'readonly', jest: 'readonly', }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/eslint.config.cjs` around lines 33 - 40, The globals object in eslint config is missing the test lifecycle hooks afterAll and afterEach; update the globals mapping (the globals block where describe, it, expect, beforeAll, beforeEach, jest are defined) to add afterAll: 'readonly' and afterEach: 'readonly' so ESLint recognizes those Jest hooks as globals.apps/extension-wallet/src/errors/ErrorBoundary.tsx (1)
206-206: Consider clarifying the ErrorInfo re-export.This re-exports React's
ErrorInfotype (from the import on line 9), whilehandleError()returns a differentErrorInfotype defined inerror-handler.ts. The naming overlap could confuse consumers of this module.Consider either:
- Renaming the export to clarify its origin:
export type { ErrorInfo as ReactErrorInfo }- Documenting which
ErrorInfois which in the module's exportsThis is pre-existing and not introduced by this PR, so it's optional to address now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/errors/ErrorBoundary.tsx` at line 206, The exported type ErrorInfo conflicts with a different ErrorInfo returned by handleError in error-handler.ts; update the re-export to disambiguate by renaming it (e.g., export type { ErrorInfo as ReactErrorInfo }) or add a clear export comment/documentation indicating this export is React's ErrorInfo; ensure references in this file to the original ErrorInfo import (from React) are updated if the alias is used elsewhere in the module so that consumers and internal usage point to the correct symbol.packages/ui-kit/eslint.config.cjs (3)
52-52: Use recursive glob for config ignores if planning to place config files in subdirectories.Line 52 currently uses
*.config.{js,cjs,ts}, which matches only root-level files. No nested config files exist inpackages/ui-kit, but if config files may be placed in subdirectories in the future, use**/*.config.{js,cjs,ts}to ensure they are also ignored.Proposed ignore pattern update
- ignores: ['dist/**', 'node_modules/**', '*.config.js', '*.config.cjs', '*.config.ts'], + ignores: ['dist/**', 'node_modules/**', '**/*.config.js', '**/*.config.cjs', '**/*.config.ts'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-kit/eslint.config.cjs` at line 52, The ignores entry currently uses '*.config.js', '*.config.cjs', '*.config.ts' which only matches root-level config files; update the ignores array (the ignores: [...] entry) to use a recursive glob pattern like '**/*.config.{js,cjs,ts}' so config files in subdirectories are also ignored by ESLint.
21-24: Optional: Scope Node globals to test/config files only.Line 21–24 enables both browser and node globals for all TS/TSX files. While no source code currently uses Node APIs, tightening this scope would prevent accidental Node global usage in browser code. Node globals could be enabled only for test and config files where they're actually needed.
Proposed config split
{ files: ['**/*.{ts,tsx}'], languageOptions: { parser: tsparser, parserOptions: { ecmaVersion: 2020, sourceType: 'module', ecmaFeatures: { jsx: true, }, }, globals: { - ...globals.browser, - ...globals.node, + ...globals.browser, }, }, @@ }, + { + files: ['**/*.{test,spec}.{ts,tsx}', '**/*.stories.tsx'], + languageOptions: { + globals: { + ...globals.browser, + ...globals.node, + }, + }, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-kit/eslint.config.cjs` around lines 21 - 24, The ESLint config currently spreads both globals.browser and globals.node into the top-level globals, which enables Node globals project-wide; change this by removing ...globals.node from the top-level globals and instead add an overrides block that applies globals: { ...globals.node } only to test/config patterns (e.g., files matching **/*.{test,spec}.{ts,tsx,js,jsx} and config/**/*), so update the globals definition and add an override entry referencing the same globals object to restrict Node globals to tests/configs.
45-49: Replace blanket ESLint rule disable with targeted suppressions for specific stories.Disabling
react-hooks/rules-of-hooksglobally for all*.stories.tsxfiles masks real hook-order and conditional-hook bugs. React's ESLint plugin documentation includes this rule in its recommended set for good reason. Instead, keep the rule enabled and use targetedeslint-disable-next-line react-hooks/rules-of-hookscomments on specific stories where they genuinely conflict with Storybook patterns, documenting why the suppression is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-kit/eslint.config.cjs` around lines 45 - 49, Remove the blanket disable of 'react-hooks/rules-of-hooks' for all '*.stories.tsx' in the ESLint config (the object that currently sets files: ['**/*.stories.tsx'] and rules: {'react-hooks/rules-of-hooks': 'off'}) and restore the rule to the recommended config; instead, for any individual story that genuinely conflicts with Storybook, add a single-line file-local suppression comment (eslint-disable-next-line react-hooks/rules-of-hooks) immediately above the offending hook usage in that specific .stories.tsx and include a brief inline comment explaining why the suppression is necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui-kit/README.md`:
- Around line 94-101: The Card example imports Card, CardHeader, CardTitle,
CardDescription, CardContent, and CardFooter but uses the Button component
without importing it; update the import statement that currently lists these
symbols to also include Button (i.e., add Button to the destructured import from
'@ancore/ui-kit') so the TSX example compiles when copy-pasted and Button is
available in the example JSX.
---
Nitpick comments:
In `@apps/extension-wallet/src/errors/ErrorBoundary.tsx`:
- Line 206: The exported type ErrorInfo conflicts with a different ErrorInfo
returned by handleError in error-handler.ts; update the re-export to
disambiguate by renaming it (e.g., export type { ErrorInfo as ReactErrorInfo })
or add a clear export comment/documentation indicating this export is React's
ErrorInfo; ensure references in this file to the original ErrorInfo import (from
React) are updated if the alias is used elsewhere in the module so that
consumers and internal usage point to the correct symbol.
In `@packages/core-sdk/eslint.config.cjs`:
- Around line 33-40: The globals object in eslint config is missing the test
lifecycle hooks afterAll and afterEach; update the globals mapping (the globals
block where describe, it, expect, beforeAll, beforeEach, jest are defined) to
add afterAll: 'readonly' and afterEach: 'readonly' so ESLint recognizes those
Jest hooks as globals.
In `@packages/ui-kit/eslint.config.cjs`:
- Line 52: The ignores entry currently uses '*.config.js', '*.config.cjs',
'*.config.ts' which only matches root-level config files; update the ignores
array (the ignores: [...] entry) to use a recursive glob pattern like
'**/*.config.{js,cjs,ts}' so config files in subdirectories are also ignored by
ESLint.
- Around line 21-24: The ESLint config currently spreads both globals.browser
and globals.node into the top-level globals, which enables Node globals
project-wide; change this by removing ...globals.node from the top-level globals
and instead add an overrides block that applies globals: { ...globals.node }
only to test/config patterns (e.g., files matching
**/*.{test,spec}.{ts,tsx,js,jsx} and config/**/*), so update the globals
definition and add an override entry referencing the same globals object to
restrict Node globals to tests/configs.
- Around line 45-49: Remove the blanket disable of 'react-hooks/rules-of-hooks'
for all '*.stories.tsx' in the ESLint config (the object that currently sets
files: ['**/*.stories.tsx'] and rules: {'react-hooks/rules-of-hooks': 'off'})
and restore the rule to the recommended config; instead, for any individual
story that genuinely conflicts with Storybook, add a single-line file-local
suppression comment (eslint-disable-next-line react-hooks/rules-of-hooks)
immediately above the offending hook usage in that specific .stories.tsx and
include a brief inline comment explaining why the suppression is necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37137faf-c288-4cb8-aef8-0d8e600cf31b
📒 Files selected for processing (70)
apps/extension-wallet/src/components/SettingsGroup.tsxapps/extension-wallet/src/components/TransactionStatus.tsxapps/extension-wallet/src/errors/ErrorBoundary.tsxapps/extension-wallet/src/errors/ErrorScreen.tsxapps/extension-wallet/src/errors/error-messages.tsapps/extension-wallet/src/errors/index.tsapps/extension-wallet/src/screens/Settings/AboutScreen.tsxapps/extension-wallet/src/screens/Settings/NetworkSettings.tsxapps/extension-wallet/src/screens/Settings/SettingsScreen.tsxapps/extension-wallet/src/screens/Settings/__tests__/settings.test.tsxapps/extension-wallet/src/screens/TransactionDetail.tsxcontracts/README.mdcontracts/account/test_snapshots/test/test_add_session_key.1.jsoncontracts/account/test_snapshots/test/test_add_session_key_emits_event.1.jsoncontracts/account/test_snapshots/test/test_double_initialize.1.jsoncontracts/account/test_snapshots/test/test_execute_emits_event.1.jsoncontracts/account/test_snapshots/test/test_execute_rejects_invalid_nonce.1.jsoncontracts/account/test_snapshots/test/test_execute_validates_nonce_then_increments.1.jsoncontracts/account/test_snapshots/test/test_initialize.1.jsoncontracts/account/test_snapshots/test/test_initialize_emits_event.1.jsoncontracts/account/test_snapshots/test/test_refresh_session_key_ttl.1.jsoncontracts/account/test_snapshots/test/test_revoke_session_key_emits_event.1.jsondocs/PULL_REQUEST_WORKFLOW.mdpackages/account-abstraction/eslint.config.cjspackages/account-abstraction/package.jsonpackages/account-abstraction/src/__tests__/account-contract.test.tspackages/account-abstraction/src/account-contract.tspackages/account-abstraction/src/errors.tspackages/account-abstraction/src/index.tspackages/account-abstraction/src/xdr-utils.tspackages/core-sdk/README.mdpackages/core-sdk/eslint.config.cjspackages/core-sdk/src/__tests__/builder.test.tspackages/core-sdk/src/__tests__/integration.test.tspackages/core-sdk/src/account-transaction-builder.tspackages/core-sdk/src/contract-params.tspackages/core-sdk/src/errors.tspackages/core-sdk/src/storage/__tests__/manager.test.tspackages/core-sdk/src/storage/secure-storage-manager.tspackages/core-sdk/src/storage/types.tspackages/core-sdk/tsconfig.jsonpackages/crypto/src/__tests__/password-strengh.test.tspackages/crypto/src/__tests__/verify-signature.test.tspackages/crypto/src/index.tspackages/crypto/src/password.tspackages/stellar/package.jsonpackages/stellar/src/errors.tspackages/stellar/src/retry.tspackages/types/package.jsonpackages/types/src/__tests__/user-operation.test.tspackages/types/src/index.tspackages/ui-kit/DESIGN_TOKENS.mdpackages/ui-kit/README.mdpackages/ui-kit/eslint.config.cjspackages/ui-kit/src/components/address-display.stories.tsxpackages/ui-kit/src/components/address-display.test.tsxpackages/ui-kit/src/components/address-display.tsxpackages/ui-kit/src/components/amount-input.stories.tsxpackages/ui-kit/src/components/amount-input.tsxpackages/ui-kit/src/components/ui/badge.tsxpackages/ui-kit/src/components/ui/button.tsxpackages/ui-kit/src/components/ui/card.stories.tsxpackages/ui-kit/src/components/ui/card.tsxpackages/ui-kit/src/components/ui/input.test.tsxpackages/ui-kit/src/components/ui/input.tsxpackages/ui-kit/src/components/ui/separator.stories.tsxpackages/ui-kit/src/components/ui/separator.tsxpackages/ui-kit/tailwind.config.jspackages/ui-kit/tsconfig.jsonpackages/ui-kit/tsup.config.ts
💤 Files with no reviewable changes (3)
- packages/types/src/index.ts
- packages/crypto/src/tests/password-strengh.test.ts
- packages/ui-kit/tsup.config.ts
✅ Files skipped from review due to trivial changes (58)
- apps/extension-wallet/src/screens/Settings/AboutScreen.tsx
- apps/extension-wallet/src/screens/Settings/tests/settings.test.tsx
- contracts/account/test_snapshots/test/test_add_session_key.1.json
- contracts/account/test_snapshots/test/test_add_session_key_emits_event.1.json
- contracts/account/test_snapshots/test/test_revoke_session_key_emits_event.1.json
- apps/extension-wallet/src/components/SettingsGroup.tsx
- apps/extension-wallet/src/screens/Settings/SettingsScreen.tsx
- contracts/README.md
- contracts/account/test_snapshots/test/test_double_initialize.1.json
- contracts/account/test_snapshots/test/test_execute_emits_event.1.json
- contracts/account/test_snapshots/test/test_execute_validates_nonce_then_increments.1.json
- contracts/account/test_snapshots/test/test_initialize.1.json
- contracts/account/test_snapshots/test/test_refresh_session_key_ttl.1.json
- packages/core-sdk/tsconfig.json
- packages/stellar/package.json
- apps/extension-wallet/src/errors/index.ts
- docs/PULL_REQUEST_WORKFLOW.md
- packages/account-abstraction/src/errors.ts
- packages/core-sdk/src/errors.ts
- packages/crypto/src/index.ts
- packages/types/package.json
- apps/extension-wallet/src/screens/Settings/NetworkSettings.tsx
- contracts/account/test_snapshots/test/test_initialize_emits_event.1.json
- packages/account-abstraction/src/index.ts
- packages/core-sdk/README.md
- packages/types/src/tests/user-operation.test.ts
- contracts/account/test_snapshots/test/test_execute_rejects_invalid_nonce.1.json
- packages/account-abstraction/eslint.config.cjs
- packages/ui-kit/src/components/address-display.test.tsx
- packages/account-abstraction/package.json
- packages/account-abstraction/src/tests/account-contract.test.ts
- packages/ui-kit/src/components/ui/input.test.tsx
- packages/crypto/src/tests/verify-signature.test.ts
- packages/ui-kit/src/components/ui/separator.stories.tsx
- packages/ui-kit/tailwind.config.js
- packages/ui-kit/src/components/address-display.stories.tsx
- packages/core-sdk/src/contract-params.ts
- apps/extension-wallet/src/components/TransactionStatus.tsx
- packages/stellar/src/retry.ts
- packages/ui-kit/tsconfig.json
- packages/stellar/src/errors.ts
- packages/core-sdk/src/tests/integration.test.ts
- packages/ui-kit/src/components/ui/card.stories.tsx
- packages/ui-kit/src/components/ui/input.tsx
- packages/crypto/src/password.ts
- apps/extension-wallet/src/errors/ErrorScreen.tsx
- packages/ui-kit/src/components/ui/separator.tsx
- packages/ui-kit/src/components/ui/badge.tsx
- packages/account-abstraction/src/account-contract.ts
- packages/ui-kit/src/components/ui/button.tsx
- packages/ui-kit/src/components/ui/card.tsx
- packages/account-abstraction/src/xdr-utils.ts
- packages/ui-kit/src/components/amount-input.stories.tsx
- packages/ui-kit/src/components/amount-input.tsx
- packages/ui-kit/DESIGN_TOKENS.md
- packages/ui-kit/src/components/address-display.tsx
- packages/core-sdk/src/tests/builder.test.ts
- packages/core-sdk/src/account-transaction-builder.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/extension-wallet/src/router/index.tsx (1)
346-356: Accessibility: form inputs missing associated labels.The placeholder inputs in
SendScreenlack proper<label>associations, unlike the inputs inCreateAccountScreenandUnlockScreenwhich correctly wrap inputs with labels. While this is a placeholder screen, adding proper labels now prevents accessibility debt when the real form is implemented.♿ Suggested fix for accessible form inputs
<div className="space-y-3"> - <input - className="w-full rounded-xl border border-border bg-background px-3 py-3 text-sm outline-none transition focus:border-primary" - placeholder="Recipient address" - /> - <input - className="w-full rounded-xl border border-border bg-background px-3 py-3 text-sm outline-none transition focus:border-primary" - placeholder="Amount" - /> + <label className="block text-sm font-medium text-foreground"> + Recipient address + <input + className="mt-2 w-full rounded-xl border border-border bg-background px-3 py-3 text-sm outline-none transition focus:border-primary" + placeholder="Enter recipient address" + /> + </label> + <label className="block text-sm font-medium text-foreground"> + Amount + <input + className="mt-2 w-full rounded-xl border border-border bg-background px-3 py-3 text-sm outline-none transition focus:border-primary" + placeholder="Enter amount" + /> + </label> <PrimaryButton>Review transaction</PrimaryButton> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/router/index.tsx` around lines 346 - 356, SendScreen's two inputs (the "Recipient address" and "Amount" placeholders) lack associated labels which breaks accessibility; update the SendScreen component to add proper labels for these fields by either wrapping each input in a <label> or adding <label htmlFor="..."> plus matching id attributes on the inputs (use descriptive ids like recipientAddress and amount). Ensure the labels are visible to screen readers (can be visually hidden if needed) and keep the PrimaryButton usage unchanged.apps/extension-wallet/eslint.config.cjs (1)
37-42: Consider keeping@typescript-eslint/no-explicit-anyenabled with gradual exceptions.Disabling
no-explicit-anyglobally for all production code removes a guardrail against type safety regressions. While the PR summary indicates type safety improvements are being made, fully disabling this rule could allow newanyusages to slip in unnoticed.Consider either:
- Keeping the rule as
'warn'to surface new usages without blocking builds- Using targeted
// eslint-disable-next-linecomments for legitimate casesThe
no-undef: offis appropriate for TypeScript projects since TS handles undefined variable checking and ESLint can produce false positives for TS-specific constructs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/eslint.config.cjs` around lines 37 - 42, The config currently disables '@typescript-eslint/no-explicit-any' globally; change its setting from 'off' to 'warn' in the ESLint config (the rule name '@typescript-eslint/no-explicit-any' in the config block) so new any usages are surfaced but do not fail CI, and for true exceptions use targeted inline comments (// eslint-disable-next-line `@typescript-eslint/no-explicit-any`) or file/folder-level overrides rather than a global disable.packages/crypto/src/encryption.ts (1)
2-3: Use a type-only import forwebcryptoto clarify intent.
webcryptois used only in type positions (return type annotations). While TypeScript's default configuration already elides this import, explicitly marking it asimport typemakes the intent clear and improves maintainability.Suggested fix
-import { webcrypto } from 'node:crypto'; +import type { webcrypto } from 'node:crypto';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/crypto/src/encryption.ts` around lines 2 - 3, Change the top-level import so that webcrypto is imported as a type-only import: replace the current import of webcrypto from 'node:crypto' with an explicit type import (e.g., import type { webcrypto } from 'node:crypto';) while keeping TextDecoder and TextEncoder as runtime imports from 'node:util'; update any return type annotations already referencing webcrypto to use the same symbol so the type-only import is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/extension-wallet/eslint.config.cjs`:
- Around line 37-42: The config currently disables
'@typescript-eslint/no-explicit-any' globally; change its setting from 'off' to
'warn' in the ESLint config (the rule name '@typescript-eslint/no-explicit-any'
in the config block) so new any usages are surfaced but do not fail CI, and for
true exceptions use targeted inline comments (// eslint-disable-next-line
`@typescript-eslint/no-explicit-any`) or file/folder-level overrides rather than a
global disable.
In `@apps/extension-wallet/src/router/index.tsx`:
- Around line 346-356: SendScreen's two inputs (the "Recipient address" and
"Amount" placeholders) lack associated labels which breaks accessibility; update
the SendScreen component to add proper labels for these fields by either
wrapping each input in a <label> or adding <label htmlFor="..."> plus matching
id attributes on the inputs (use descriptive ids like recipientAddress and
amount). Ensure the labels are visible to screen readers (can be visually hidden
if needed) and keep the PrimaryButton usage unchanged.
In `@packages/crypto/src/encryption.ts`:
- Around line 2-3: Change the top-level import so that webcrypto is imported as
a type-only import: replace the current import of webcrypto from 'node:crypto'
with an explicit type import (e.g., import type { webcrypto } from
'node:crypto';) while keeping TextDecoder and TextEncoder as runtime imports
from 'node:util'; update any return type annotations already referencing
webcrypto to use the same symbol so the type-only import is correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8438b1f-e322-4f57-9ca2-a659b1871e1d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
apps/extension-wallet/eslint.config.cjsapps/extension-wallet/package.jsonapps/extension-wallet/src/components/PaymentQRCode.tsxapps/extension-wallet/src/index.tsapps/extension-wallet/src/main.tsxapps/extension-wallet/src/router/index.tsxapps/extension-wallet/src/screens/ReceiveScreen.tsxapps/extension-wallet/src/screens/__tests__/ReceiveScreen.test.tsxapps/extension-wallet/vite.config.tspackages/account-abstraction/eslint.config.cjspackages/core-sdk/eslint.config.cjspackages/crypto/eslint.config.cjspackages/crypto/src/__tests__/encryption-roundtrip.test.tspackages/crypto/src/encryption.tspackages/crypto/src/index.tspackages/ui-kit/eslint.config.cjspackages/ui-kit/src/__tests__/Form/validation.test.tspackages/ui-kit/src/components/Form/AddressInput.tsxpackages/ui-kit/src/components/Form/AmountInput.tsxpackages/ui-kit/src/components/Form/Form.stories.tsxpackages/ui-kit/src/components/Form/Form.tsxpackages/ui-kit/src/components/Form/PasswordInput.tsxpackages/ui-kit/src/components/Form/validation.tspackages/ui-kit/src/index.ts
✅ Files skipped from review due to trivial changes (21)
- packages/crypto/eslint.config.cjs
- packages/ui-kit/src/index.ts
- apps/extension-wallet/src/components/PaymentQRCode.tsx
- packages/account-abstraction/eslint.config.cjs
- apps/extension-wallet/package.json
- packages/ui-kit/src/components/Form/validation.ts
- packages/crypto/src/tests/encryption-roundtrip.test.ts
- apps/extension-wallet/src/screens/ReceiveScreen.tsx
- packages/ui-kit/src/tests/Form/validation.test.ts
- packages/core-sdk/eslint.config.cjs
- packages/ui-kit/eslint.config.cjs
- apps/extension-wallet/src/index.ts
- apps/extension-wallet/vite.config.ts
- packages/ui-kit/src/components/Form/Form.stories.tsx
- packages/crypto/src/index.ts
- apps/extension-wallet/src/main.tsx
- packages/ui-kit/src/components/Form/Form.tsx
- packages/ui-kit/src/components/Form/AddressInput.tsx
- packages/ui-kit/src/components/Form/AmountInput.tsx
- apps/extension-wallet/src/screens/tests/ReceiveScreen.test.tsx
- packages/ui-kit/src/components/Form/PasswordInput.tsx
Contributor
|
@David-patrick-chuks please |
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.
Description
Adds client-side routing for the extension popup using React Router v6, including protected/auth-aware navigation flows for welcome, unlock, create account, home, send, receive, history, settings, and session keys.
This PR also adds a reusable popup navigation bar, route-based page titles, a 404 recovery screen, and focused routing tests. A few extension-only config/type issues on
mainwere cleaned up so the extension app can build and test successfully.Type of Change
Security Impact
This PR introduces route guards for protected extension screens and separates public flows (
/welcome,/create-account,/unlock) from authenticated flows. The current auth state is a local demo/session layer for popup routing and is not yet connected to real wallet unlock or key management logic.Testing
Test Coverage
Manual Testing Steps
/welcome,/create-account, and/unlockrender correctly for unauthenticated users./homeplus navigation to/send,/receive,/history,/settings, and/session-keys./unlock, and unknown routes show the 404 recovery screen.Breaking Changes
Checklist
For High-Security Changes
Related Issues
Closes #79
Related to #
Additional Context
Local validation completed with:
pnpm --filter @ancore/extension-wallet testpnpm --filter @ancore/extension-wallet buildReviewer Notes
Please focus on:
Summary by CodeRabbit
Release Notes
New Features
Chores