Conversation
Signed-off-by: prxt6529 <prxt@6529.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multi‑wallet support and UI: persistent connectedAccounts with active-account lifecycle, account switching/adding, storage migration and events; SeizeConnectContext API expanded; Auth gains immediate-validation and abort guards; header/sidebar show connected-account UI and switching; tests, constants, styles, and lint/tooling adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Header as AppHeader
participant Context as SeizeConnectContext
participant Storage as AuthUtils
participant Auth as AuthComponent
User->>Header: double-activate avatar
Header->>Context: request next connected address / switch
Context->>Storage: setActiveAddressInStorage(address)
Storage-->>Context: emit WALLET_ACCOUNTS_UPDATED_EVENT
Context->>Auth: notify address change
Auth->>Auth: run immediate validation (abort previous ops)
Auth-->>Header: updated UI state
Header-->>User: show switched profile
sequenceDiagram
participant User
participant Page as AcceptConnectionPage
participant Storage as AuthUtils
participant Context as SeizeConnectContext
participant Auth as AuthComponent
User->>Page: Click "Accept Connection"
Page->>Storage: canStoreAnotherWalletAccount(address)?
alt can store
Page->>Context: seizeConnect/start flow
Context->>Auth: initiate sign-in flow
Auth->>Storage: setAuthJwt(address,jwt,refreshToken,role)
Storage-->>Context: persist accounts + emit WALLET_ACCOUNTS_UPDATED_EVENT
Context-->>Page: success → navigate
else cannot store
Page-->>User: show "Maximum connected profiles reached" toast
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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)
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 |
Collaborator
Author
|
@CodeRabbit review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/accept-connection-sharing/page.client.tsx (1)
51-73:⚠️ Potential issue | 🟠 MajorHandle invalid redeem responses and
setAuthJwtfailure before redirecting.Right now, an invalid-but-non-throwing redeem response can leave the button stuck in processing, and a failed
setAuthJwt(...)still proceeds toseizeAcceptConnection+ navigation.💡 Proposed fix
const acceptConnection = async () => { try { @@ if ( redeemResponse.address && redeemResponse.token && areEqualAddresses(redeemResponse.address, address) ) { @@ - setAuthJwt( + const persisted = setAuthJwt( redeemResponse.address, redeemResponse.token, token, role ?? undefined ); + if (!persisted) { + setToast({ + message: "Maximum connected profiles reached", + type: "error", + }); + return; + } seizeAcceptConnection(redeemResponse.address); router.push("/"); + return; } + setToast({ message: "Invalid connection response", type: "error" }); } catch (error) { console.error(error); setToast({ message: "Failed to accept connection", type: "error" }); - setAcceptingConnection(false); + } finally { + setAcceptingConnection(false); } };Also applies to: 120-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/accept-connection-sharing/page.client.tsx` around lines 51 - 73, Ensure redeemResponse is fully valid and handle failures from setAuthJwt before calling seizeAcceptConnection or router.push: validate redeemResponse.address/token and areEqualAddresses(redeemResponse.address, address), and if invalid call setToast(...) and setAcceptingConnection(false) then return; call setAuthJwt(...) inside a try/catch or check its boolean result and if it fails set a toast, setAcceptingConnection(false) and return instead of proceeding; only call seizeAcceptConnection(redeemResponse.address) and router.push("/") after setAuthJwt succeeded. Apply the same guard and failure handling to the same logic at the other occurrence (lines 120-142).components/header/user/proxy/HeaderUserProxyDropdown.tsx (1)
195-240:⚠️ Potential issue | 🟠 MajorAvoid fire-and-forget async disconnect/logout calls.
Both handlers call async context methods without awaiting/catching errors, so failures can become unhandled and invisible to users.
Suggested fix
{isConnected ? ( <button onClick={() => { - seizeDisconnect(); - onClose(); + void (async () => { + try { + await seizeDisconnect(); + } catch (error) { + console.error("Failed to disconnect wallet", error); + setToast({ + message: + "Failed to disconnect wallet. Please try again.", + type: "error", + }); + } finally { + onClose(); + } + })(); }} @@ <div className="tw-h-full tw-px-2 tw-pt-2"> <button - onClick={() => seizeDisconnectAndLogout()} + onClick={() => { + void (async () => { + try { + await seizeDisconnectAndLogout(); + onClose(); + } catch (error) { + console.error("Failed to disconnect and logout", error); + setToast({ + message: + "Failed to logout. Please try again.", + type: "error", + }); + } + })(); + }} type="button"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/user/proxy/HeaderUserProxyDropdown.tsx` around lines 195 - 240, The handlers call async functions (seizeDisconnect, seizeConnect, seizeDisconnectAndLogout) without awaiting or handling errors; convert the onClick handlers to async functions that await the corresponding call, wrap each await in try/catch to handle/log errors (and surface UI feedback if available), and ensure onClose is only invoked after a successful await (or still invoked in finally if intended). Also consider disabling the button during the pending state (use a local isPending state per button) to prevent double-clicks; update references to seizeDisconnect, seizeConnect, and seizeDisconnectAndLogout accordingly.components/auth/SeizeConnectContext.tsx (1)
742-852:⚠️ Potential issue | 🟠 MajorDon’t gate disconnect on
isActiveWalletConnectedin logout paths.When the live wallet is connected but differs from the active auth profile, this guard is false and logout/sign-out-all can skip
disconnect(), leaving a wallet session active.Suggested fix
const seizeDisconnect = useCallback(async (): Promise<void> => { - if (!isActiveWalletConnected) { + if (!account.isConnected) { return; } @@ - }, [disconnect, isActiveWalletConnected]); + }, [account.isConnected, disconnect]); const seizeDisconnectAndLogout = useCallback(async (): Promise<void> => { - if (isActiveWalletConnected) { + if (account.isConnected) { @@ }, [ + account.isConnected, disconnect, - isActiveWalletConnected, refreshStoredConnectedAccounts, setConnected, setDisconnected, ]); const seizeDisconnectAndLogoutAll = useCallback(async (): Promise<void> => { - if (isActiveWalletConnected) { + if (account.isConnected) { @@ }, [ + account.isConnected, disconnect, - isActiveWalletConnected, refreshStoredConnectedAccounts, setDisconnected, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/auth/SeizeConnectContext.tsx` around lines 742 - 852, The logout handlers seizeDisconnectAndLogout and seizeDisconnectAndLogoutAll currently skip calling disconnect() when isActiveWalletConnected is false, which can leave a live wallet session active; remove the guard and always attempt await disconnect() inside the existing try/catch so disconnect() runs on logout regardless of isActiveWalletConnected, keeping the existing error wrapping (createWalletError/WalletDisconnectionError) and AuthenticationError behavior and leaving the rest of the auth-cleanup logic (removeAuthJwt, refreshStoredConnectedAccounts, setConnected/setDisconnected) unchanged.
🧹 Nitpick comments (5)
components/header/AppSidebarHeader.tsx (1)
19-26: Consider adding a hover state to the close button.The button has
active:tw-text-whitebut nohover:state, which may reduce visual feedback for users before they click. Adding a hover state would improve interaction discoverability.💡 Suggested enhancement
className="tw-border-none tw-bg-transparent tw-p-0 tw-text-iron-400 tw-transition-colors tw-duration-200 focus-visible:tw-ring-2 focus-visible:tw-ring-primary-400 focus-visible:tw-ring-offset-2 focus-visible:tw-ring-offset-iron-900 active:tw-text-white" + className="tw-border-none tw-bg-transparent tw-p-0 tw-text-iron-400 tw-transition-colors tw-duration-200 hover:tw-text-iron-200 focus-visible:tw-ring-2 focus-visible:tw-ring-primary-400 focus-visible:tw-ring-offset-2 focus-visible:tw-ring-offset-iron-900 active:tw-text-white"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/AppSidebarHeader.tsx` around lines 19 - 26, The close button in AppSidebarHeader (the button element with onClose and the XMarkIcon) lacks a hover state; update its className to include a hover utility (for example add hover:tw-text-white or another appropriate hover:tw-text-... and/or a subtle hover:bg utility) so the XMarkIcon visually changes on hover and provides better interaction feedback.__tests__/components/header/user/proxy/HeaderUserProxyDropdown.test.tsx (1)
44-48: IncludesetToastin the mockedAuthContextvalue for contract completeness.The component reads
setToast; omitting it makes the mock fragile for any error-path test expansion.Suggested fix
const authValue = { activeProfileProxy: null, setActiveProfileProxy: jest.fn(), receivedProfileProxies: [{ id: "proxy-1" }], + setToast: jest.fn(), } as any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/header/user/proxy/HeaderUserProxyDropdown.test.tsx` around lines 44 - 48, The mocked AuthContext value `authValue` used in `HeaderUserProxyDropdown` tests is missing `setToast`, making the mock incomplete and brittle; add a `setToast: jest.fn()` property to the `authValue` object (alongside `activeProfileProxy`, `setActiveProfileProxy`, and `receivedProfileProxies`) so the component can call `setToast` safely in tests, keeping the `as any` typing or appropriate mock type intact.components/header/AppUserConnect.tsx (2)
98-108: Consider making aria-label dynamic to match the visible label.The visible button label changes between "Disconnect & Logout" and "Logout" based on
isConnected, but thearia-labelremains static. Screen reader users might be confused by the mismatch.♿ Proposed fix for accessibility
<button onClick={() => { seizeDisconnectAndLogout(); onNavigate(); }} className="tw-flex tw-w-full tw-items-center tw-space-x-4 tw-rounded-lg tw-border-none tw-bg-transparent tw-px-4 tw-py-3.5 tw-text-base tw-font-semibold tw-text-iron-300 tw-transition-colors tw-duration-200 active:tw-bg-iron-700 active:tw-text-iron-200" - aria-label="Disconnect & Logout" + aria-label={isConnected ? "Disconnect & Logout" : "Logout"} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/AppUserConnect.tsx` around lines 98 - 108, The aria-label should reflect the visible text that depends on isConnected; update the button's aria-label to use the same conditional expression as the span (e.g., isConnected ? "Disconnect & Logout" : "Logout") so screen readers receive the matching label, keeping the existing onClick behavior (seizeDisconnectAndLogout and onNavigate) and preserving the button's other attributes and the ArrowRightEndOnRectangleIcon.
48-59: Async IIFE pattern is appropriate here but consider extracting for reuse.The async IIFE pattern for handling disconnection is correct, but the same pattern is repeated at lines 111-120 for
seizeDisconnectAndLogoutAll. Consider extracting this into a helper function for consistency.♻️ Optional refactor to reduce duplication
+ const handleAsyncAction = async ( + action: () => Promise<void>, + errorMessage: string + ) => { + try { + await action(); + } catch (error) { + console.error(errorMessage, error); + } finally { + onNavigate(); + } + }; + const walletConnectionButton = isConnected ? ( <button - onClick={() => { - void (async () => { - try { - await seizeDisconnect(); - } catch (error) { - console.error("Failed to disconnect wallet", error); - } finally { - onNavigate(); - } - })(); - }} + onClick={() => handleAsyncAction(seizeDisconnect, "Failed to disconnect wallet")}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/AppUserConnect.tsx` around lines 48 - 59, The disconnect async IIFE used inside walletConnectionButton (calling seizeDisconnect then onNavigate) is duplicated with the pattern used for seizeDisconnectAndLogoutAll; extract a reusable helper (e.g., runAsyncAndNavigate or handleAsyncDisconnect) that accepts an async callback like seizeDisconnect or seizeDisconnectAndLogoutAll and handles try/catch/finally logging and calling onNavigate, then replace both inline IIFEs in the component with calls to that helper to remove duplication and centralize error handling.components/header/AppHeader.tsx (1)
65-77: Consider extracting the constant outside the component.
PROFILE_DOUBLE_ACTIVATE_DELAY_MSis defined inside the component body, which means it's redeclared on every render. While this doesn't cause bugs, it's cleaner to define constants outside the component.♻️ Suggested refactor
+const PROFILE_DOUBLE_ACTIVATE_DELAY_MS = 280; + export default function AppHeader() { - const PROFILE_DOUBLE_ACTIVATE_DELAY_MS = 280; const [menuOpen, setMenuOpen] = useState(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/AppHeader.tsx` around lines 65 - 77, PROFILE_DOUBLE_ACTIVATE_DELAY_MS is declared inside the AppHeader component and should be extracted to a top-level constant to avoid redeclaration on each render; move PROFILE_DOUBLE_ACTIVATE_DELAY_MS = 280 out of the component body (above the AppHeader function) keeping it as a plain number constant, then remove the in-component declaration and leave all usages (e.g., where profileClickTimeoutRef, menuOpen, setMenuOpen, or other logic reference PROFILE_DOUBLE_ACTIVATE_DELAY_MS) unchanged so the component reads the top-level constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/auth/Auth.test.tsx`:
- Around line 133-141: Remove the redeclaration via require and instead cast the
already imported commonApiPost to a Jest mock: drop the const { commonApiPost }
= require(...) line and assign mockCommonApiPost by casting the existing
imported commonApiPost (same pattern used for mockCommonApiFetch), e.g. const
mockCommonApiPost = commonApiPost as jest.MockedFunction<typeof commonApiPost>;
keep the existing mockUseIdentity assignment as-is; update any tests that
referenced the old require-based mock name to use mockCommonApiPost.
In `@__tests__/components/auth/connection-state-indicator.test.ts`:
- Around line 24-39: The test's expected CSS opacity tokens for
getConnectionProfileIndicator are mismatched with the implementation; update the
assertions in the test for the "authorized only (wallet not connected)" and
"authorized+connected" cases to match the exact class strings returned by
getConnectionProfileIndicator (adjust expectations for result.avatarClassName,
result.overlayClassName, result.buttonClassName and keep
result.state/result.title checks aligned). Locate the assertions referencing
getConnectionProfileIndicator in
__tests__/components/auth/connection-state-indicator.test.ts and change the
opacity fractions (e.g., "/40", "/30", "/60", "/35", "/25", "/55") to the tokens
returned by the function so the test compares the exact implemented class
values.
In `@components/auth/Auth.tsx`:
- Around line 584-606: The current sign-in flow treats requestSignIn success as
enough but doesn't fail when wallet/account persistence (setAuthJwt/store
account) fails, leading to inconsistent auth state; update requestSignIn to
detect and propagate persistence failures (e.g., when setAuthJwt or
canStoreAnotherWalletAccount fails) by returning success: false (or an explicit
persisted flag) and throw or return an error detail, then update this branch in
Auth.tsx to treat that returned failure as an authentication failure (hide
modal, call seizeDisconnect(), invalidateAll only on full success) so that
inability to persist an account prevents marking the user as signed in; look for
the requestSignIn function and setAuthJwt/canStoreAnotherWalletAccount usages to
implement this change.
In `@components/header/user/proxy/HeaderUserProxyDropdown.tsx`:
- Around line 77-84: The click handler onSelectConnectedAccount must catch
errors from seizeSwitchConnectedAccount to avoid breaking the dropdown flow;
update onSelectConnectedAccount to call seizeSwitchConnectedAccount inside a
try/catch (and await it if it returns a Promise) and ensure onClose() is invoked
in a finally block so the dropdown always closes; optionally log or surface the
caught error in the catch block for debugging.
In `@components/layout/sidebar/WebSidebarUser.tsx`:
- Around line 144-183: switchToNextConnectedAccount currently calls
seizeSwitchConnectedAccount synchronously and doesn't catch thrown errors; wrap
the call to seizeSwitchConnectedAccount(nextAccount.address) in a try-catch
inside switchToNextConnectedAccount, return false if an exception is caught (and
optionally log the error), so the component won't crash when
seizeSwitchConnectedAccount throws; keep the existing return true on success and
ensure connectedAccounts and activeIndex logic stays unchanged.
In `@package.json`:
- Around line 32-34: The xargs calls in the npm scripts are missing the
no-run-if-empty flag, causing eslint/prettier to run against the whole repo when
the file list is empty; update each affected script (lint:changed:fix,
lint:changed, lint:changed:tight and also lint:uncommitted:tight, lint:diff,
format:uncommitted) to add the -r option to the xargs invocation (i.e., change
xargs -0 ... to xargs -0 -r ...) so the command is skipped when there are no
filenames, preserving the intended changed-file behavior.
---
Outside diff comments:
In `@app/accept-connection-sharing/page.client.tsx`:
- Around line 51-73: Ensure redeemResponse is fully valid and handle failures
from setAuthJwt before calling seizeAcceptConnection or router.push: validate
redeemResponse.address/token and areEqualAddresses(redeemResponse.address,
address), and if invalid call setToast(...) and setAcceptingConnection(false)
then return; call setAuthJwt(...) inside a try/catch or check its boolean result
and if it fails set a toast, setAcceptingConnection(false) and return instead of
proceeding; only call seizeAcceptConnection(redeemResponse.address) and
router.push("/") after setAuthJwt succeeded. Apply the same guard and failure
handling to the same logic at the other occurrence (lines 120-142).
In `@components/auth/SeizeConnectContext.tsx`:
- Around line 742-852: The logout handlers seizeDisconnectAndLogout and
seizeDisconnectAndLogoutAll currently skip calling disconnect() when
isActiveWalletConnected is false, which can leave a live wallet session active;
remove the guard and always attempt await disconnect() inside the existing
try/catch so disconnect() runs on logout regardless of isActiveWalletConnected,
keeping the existing error wrapping (createWalletError/WalletDisconnectionError)
and AuthenticationError behavior and leaving the rest of the auth-cleanup logic
(removeAuthJwt, refreshStoredConnectedAccounts, setConnected/setDisconnected)
unchanged.
In `@components/header/user/proxy/HeaderUserProxyDropdown.tsx`:
- Around line 195-240: The handlers call async functions (seizeDisconnect,
seizeConnect, seizeDisconnectAndLogout) without awaiting or handling errors;
convert the onClick handlers to async functions that await the corresponding
call, wrap each await in try/catch to handle/log errors (and surface UI feedback
if available), and ensure onClose is only invoked after a successful await (or
still invoked in finally if intended). Also consider disabling the button during
the pending state (use a local isPending state per button) to prevent
double-clicks; update references to seizeDisconnect, seizeConnect, and
seizeDisconnectAndLogout accordingly.
---
Nitpick comments:
In `@__tests__/components/header/user/proxy/HeaderUserProxyDropdown.test.tsx`:
- Around line 44-48: The mocked AuthContext value `authValue` used in
`HeaderUserProxyDropdown` tests is missing `setToast`, making the mock
incomplete and brittle; add a `setToast: jest.fn()` property to the `authValue`
object (alongside `activeProfileProxy`, `setActiveProfileProxy`, and
`receivedProfileProxies`) so the component can call `setToast` safely in tests,
keeping the `as any` typing or appropriate mock type intact.
In `@components/header/AppHeader.tsx`:
- Around line 65-77: PROFILE_DOUBLE_ACTIVATE_DELAY_MS is declared inside the
AppHeader component and should be extracted to a top-level constant to avoid
redeclaration on each render; move PROFILE_DOUBLE_ACTIVATE_DELAY_MS = 280 out of
the component body (above the AppHeader function) keeping it as a plain number
constant, then remove the in-component declaration and leave all usages (e.g.,
where profileClickTimeoutRef, menuOpen, setMenuOpen, or other logic reference
PROFILE_DOUBLE_ACTIVATE_DELAY_MS) unchanged so the component reads the top-level
constant.
In `@components/header/AppSidebarHeader.tsx`:
- Around line 19-26: The close button in AppSidebarHeader (the button element
with onClose and the XMarkIcon) lacks a hover state; update its className to
include a hover utility (for example add hover:tw-text-white or another
appropriate hover:tw-text-... and/or a subtle hover:bg utility) so the XMarkIcon
visually changes on hover and provides better interaction feedback.
In `@components/header/AppUserConnect.tsx`:
- Around line 98-108: The aria-label should reflect the visible text that
depends on isConnected; update the button's aria-label to use the same
conditional expression as the span (e.g., isConnected ? "Disconnect & Logout" :
"Logout") so screen readers receive the matching label, keeping the existing
onClick behavior (seizeDisconnectAndLogout and onNavigate) and preserving the
button's other attributes and the ArrowRightEndOnRectangleIcon.
- Around line 48-59: The disconnect async IIFE used inside
walletConnectionButton (calling seizeDisconnect then onNavigate) is duplicated
with the pattern used for seizeDisconnectAndLogoutAll; extract a reusable helper
(e.g., runAsyncAndNavigate or handleAsyncDisconnect) that accepts an async
callback like seizeDisconnect or seizeDisconnectAndLogoutAll and handles
try/catch/finally logging and calling onNavigate, then replace both inline IIFEs
in the component with calls to that helper to remove duplication and centralize
error handling.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/intern-no-bg.pngis excluded by!**/*.png
📒 Files selected for processing (35)
__tests__/components/auth/Auth.test.tsx__tests__/components/auth/SeizeConnectContext.test.tsx__tests__/components/auth/connection-state-indicator.test.ts__tests__/components/header/AppSidebarUserInfo.test.tsx__tests__/components/header/AppUserConnect.test.tsx__tests__/components/header/user/proxy/HeaderUserProxyDropdown.test.tsx__tests__/services/auth.utils.test.tsapp/accept-connection-sharing/page.client.tsxcomponents/auth/Auth.module.scsscomponents/auth/Auth.tsxcomponents/auth/SeizeConnectContext.tsxcomponents/auth/connection-state-indicator.tscomponents/brain/my-stream/layout/LayoutContext.tsxcomponents/header/AppHeader.tsxcomponents/header/AppSidebarConnectedAccounts.tsxcomponents/header/AppSidebarHeader.tsxcomponents/header/AppSidebarUserInfo.tsxcomponents/header/AppUserConnect.tsxcomponents/header/user/connected/HeaderUserConnectedAccounts.tsxcomponents/header/user/proxy/HeaderUserProxyDropdown.tsxcomponents/layout/SmallScreenLayout.tsxcomponents/layout/WebLayout.tsxcomponents/layout/sidebar/WebSidebarUser.tsxcomponents/notifications/NotificationsContext.tsxcomponents/providers/LayoutWrapper.tsxconstants/constants.tscontexts/HeaderContext.tsxeslint.config.mjseslint.config.tight.mjshooks/isMobileDevice.tshooks/isMobileScreen.tshooks/useCapacitor.tshooks/useControlledTooltip.tspackage.jsonservices/auth/auth.utils.ts
✅ Actions performedReview triggered.
|
Signed-off-by: prxt6529 <prxt@6529.io>
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Signed-off-by: prxt6529 <prxt@6529.io>
Signed-off-by: prxt6529 <prxt@6529.io>
Signed-off-by: prxt6529 <prxt@6529.io>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/auth/Auth.tsx (1)
283-330:⚠️ Potential issue | 🟠 MajorCurrent stale-address checks are ineffective in this effect.
On Line 292 and Line 322,
currentAddress !== addresscompares two values from the same closure, so it won’t detect later address changes. This weakens the stale-response guard in concurrent auth transitions.Suggested fix
+ const latestAddressRef = useRef<string | null>(null); + useEffect(() => { + latestAddressRef.current = address ?? null; + }, [address]); @@ - if (currentAddress !== address) { + if (latestAddressRef.current !== currentAddress) { return; } @@ - if (result.wasCancelled || currentAddress !== address) { + if (result.wasCancelled || latestAddressRef.current !== currentAddress) { return; } @@ - if (!abortController.signal.aborted && currentAddress === address) { + if ( + !abortController.signal.aborted && + latestAddressRef.current === currentAddress + ) { abortControllerRef.current = null; setAuthLoadingState("idle"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/auth/Auth.tsx` around lines 283 - 330, The stale-address checks comparing currentAddress to address inside validateImmediately are invalid because both values are from the same closure; replace them with a runtime check against a mutable ref or current operation id to detect later changes. Add a ref like latestAddressRef (updated in a useEffect whenever address changes) or an operationIdRef (set to operationId at start) and then: (1) before/after awaiting validateAuthImmediate, compare currentAddress !== latestAddressRef.current (or operationIdRef.current !== operationId) to detect stale operations, and (2) when cleaning up, verify abortControllerRef.current === abortController (not abortController.signal.aborted) before clearing abortControllerRef and setting setAuthLoadingState("idle"); update usages of currentAddress !== address in validateImmediately to use the chosen ref comparison and ensure abortControllerRef is only cleared by the active operation.
♻️ Duplicate comments (1)
package.json (1)
41-41:⚠️ Potential issue | 🟠 MajorAdd
-rtoformat:changedto prevent unintended broad formatting.Line [41] still uses
xargs -0 prettier ...without-r. If the upstream file list is empty, this can run prettier without file args and break the “changed-files only” contract (also affectingquality:changedandaudit:changedflows).Proposed fix
- "format:changed": "bash -lc '{ git diff --name-only -z --diff-filter=ACMR main...HEAD -- \"*.js\" \"*.jsx\" \"*.ts\" \"*.tsx\" \"*.json\" \"*.css\" \"*.scss\" \":(exclude)generated/**\"; git diff --name-only -z --diff-filter=ACMR HEAD -- \"*.js\" \"*.jsx\" \"*.ts\" \"*.tsx\" \"*.json\" \"*.css\" \"*.scss\" \":(exclude)generated/**\"; git ls-files --others --exclude-standard -z -- \"*.js\" \"*.jsx\" \"*.ts\" \"*.tsx\" \"*.json\" \"*.css\" \"*.scss\" \":(exclude)generated/**\"; } | xargs -0 prettier --write --ignore-unknown'", + "format:changed": "bash -lc '{ git diff --name-only -z --diff-filter=ACMR main...HEAD -- \"*.js\" \"*.jsx\" \"*.ts\" \"*.tsx\" \"*.json\" \"*.css\" \"*.scss\" \":(exclude)generated/**\"; git diff --name-only -z --diff-filter=ACMR HEAD -- \"*.js\" \"*.jsx\" \"*.ts\" \"*.tsx\" \"*.json\" \"*.css\" \"*.scss\" \":(exclude)generated/**\"; git ls-files --others --exclude-standard -z -- \"*.js\" \"*.jsx\" \"*.ts\" \"*.tsx\" \"*.json\" \"*.css\" \"*.scss\" \":(exclude)generated/**\"; } | xargs -0 -r prettier --write --ignore-unknown'",#!/bin/bash # Verify whether format:changed includes the no-run-if-empty guard. rg -n '"format:changed"' package.json python - <<'PY' import json with open("package.json", "r", encoding="utf-8") as f: pkg = json.load(f) cmd = pkg["scripts"]["format:changed"] print("format:changed:", cmd) print("contains '| xargs -0 -r prettier':", "| xargs -0 -r prettier" in cmd) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 41, The "format:changed" npm script currently pipes a possibly-empty NUL-separated file list into "xargs -0 prettier" which will run prettier with no args if the list is empty; update the "format:changed" script string to use "xargs -0 -r prettier" (add the -r / --no-run-if-empty flag) so prettier is skipped when there are no files, and also scan the other related scripts "quality:changed" and "audit:changed" and add the same "-r" to any similar "xargs -0" invocations to keep changed-only behavior consistent.
🧹 Nitpick comments (5)
components/header/user/proxy/HeaderUserProxyDropdown.tsx (1)
138-150: Consider handling potential errors fromseizeAddConnectedAccount.The
onAddAccountcallback directly callsseizeAddConnectedAccount()without error handling. WhileseizeAddConnectedAccountin the context handles its own errors internally, consistency with other actions wrapped inrunMenuActionwould improve robustness.♻️ Suggested improvement
<HeaderUserConnectedAccounts accounts={availableConnectedAccounts} onSelectAccount={onSelectConnectedAccount} canAddAccount={canAddConnectedAccount} - onAddAccount={() => { - seizeAddConnectedAccount(); - onClose(); - }} + onAddAccount={() => { + void runMenuAction({ + action: seizeAddConnectedAccount, + pendingKey: "add-account", + errorMessage: "Failed to add account. Please try again.", + }); + }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/user/proxy/HeaderUserProxyDropdown.tsx` around lines 138 - 150, The onAddAccount handler in HeaderUserProxyDropdown calls seizeAddConnectedAccount() directly without running it through the existing runMenuAction wrapper; change the onAddAccount callback given to HeaderUserConnectedAccounts to invoke seizeAddConnectedAccount via runMenuAction (e.g., runMenuAction(() => seizeAddConnectedAccount())) so its errors/side-effects are handled consistently, and then call onClose after the wrapped action completes (or in a finally-equivalent post-action step) to preserve current behavior; update the onAddAccount arrow handler in HeaderUserProxyDropdown accordingly.components/auth/SeizeConnectContext.tsx (3)
952-958: Missing dependency inseizeAddConnectedAccountuseMemo array.The callback references
storedConnectedAccounts.lengthin the dependency array, but the actual check usescanStoreAnotherWalletAccount()which may have its own internal state. This is fine ifcanStoreAnotherWalletAccountreads from the same source, but the dependency on.lengthis redundant since it's not directly used in the callback body.♻️ Suggested cleanup
}, [ account.address, account.isConnected, disconnect, seizeConnect, - storedConnectedAccounts.length, ]);Alternatively, if the intent is to re-create the callback when accounts change, use the full array or rely on
refreshStoredConnectedAccountsbeing stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/auth/SeizeConnectContext.tsx` around lines 952 - 958, The useMemo for seizeAddConnectedAccount incorrectly depends on storedConnectedAccounts.length (a redundant dependency) while the callback actually uses canStoreAnotherWalletAccount and possibly the full storedConnectedAccounts; update the dependency array to include canStoreAnotherWalletAccount and/or the entire storedConnectedAccounts (or ensure refreshStoredConnectedAccounts is stable) so the memo recalculates correctly when account storage state changes; locate the seizeAddConnectedAccount definition and replace storedConnectedAccounts.length with either canStoreAnotherWalletAccount or storedConnectedAccounts (or add refreshStoredConnectedAccounts) depending on which value the callback truly relies on.
200-201: Consider using a typed constant or configuration for the grace period.The
ADD_FLOW_CANCEL_GRACE_MS = 5000is a reasonable value but could benefit from a comment explaining why 5 seconds was chosen, especially since it affects UX during wallet transitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/auth/SeizeConnectContext.tsx` around lines 200 - 201, Replace the hardcoded ADD_FLOW_CANCEL_GRACE_MS = 5000 with a clearly typed exported constant or configuration value (e.g., export const ADD_FLOW_CANCEL_GRACE_MS: number = 5000 or pull from a config object) and add a short comment explaining why 5000ms was chosen and how it impacts wallet transition UX; update any references (e.g., where ADD_FLOW_CANCEL_GRACE_MS is used in SeizeConnectContext logic) to use the new typed/centralized value so the timeout can be easily adjusted or overridden in the future.
984-1030: Potential stale closure:connectedAddressandimpersonatedAddressin dependency array may not be needed.The
contextValuememo includesconnectedAddressandimpersonatedAddressin dependencies, but these are already captured viaactiveAddress. Additionally,walletInfo?.typeis in the dependency array but not used in the memo body, which could cause unnecessary re-renders.♻️ Suggested dependency cleanup
[ activeAddress, isActiveWalletConnected, connectedAccounts, - connectedAddress, - impersonatedAddress, walletInfo?.name, walletInfo?.icon, - walletInfo?.type, seizeConnect, seizeDisconnect, seizeDisconnectAndLogout, seizeDisconnectAndLogoutAll, seizeAcceptConnection, seizeSwitchConnectedAccount, seizeAddConnectedAccount, state.open, - account.isConnected, walletState, hasInitializationError, initializationError, canAddConnectedAccount, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/auth/SeizeConnectContext.tsx` around lines 984 - 1030, The memo for contextValue includes unnecessary dependencies (connectedAddress, impersonatedAddress, and walletInfo?.type) that aren't referenced in the returned object and can cause extra re-renders; update the useMemo dependency array to remove connectedAddress, impersonatedAddress, and walletInfo?.type while keeping activeAddress, walletInfo?.name, walletInfo?.icon, all seize* handler references (seizeConnect, seizeDisconnect, seizeDisconnectAndLogout, seizeDisconnectAndLogoutAll, seizeAcceptConnection, seizeSwitchConnectedAccount, seizeAddConnectedAccount), state.open, account.isConnected, walletState, hasInitializationError, initializationError, connectedAccounts, and canAddConnectedAccount so the memo dependencies exactly match the symbols used in the SeizeConnectContextType value.__tests__/components/header/user/proxy/HeaderUserProxyDropdown.test.tsx (1)
38-42: Consider adding a test forseizeSwitchConnectedAccounterror handling.Given the past review emphasis on error handling for account switching, adding a test that verifies the error toast appears when
seizeSwitchConnectedAccountthrows would strengthen coverage.🧪 Suggested test case
it("shows error toast when switching account fails", async () => { const seizeSwitchConnectedAccount = jest.fn().mockImplementation(() => { throw new Error("Switch failed"); }); const { setToast, onClose } = renderDropdown({ profile: profileBase, address: "0xabc", isConnected: true, connectedAccounts: [ { address: "0xabc", role: null, isActive: true, isConnected: true }, { address: "0xdef", role: null, isActive: false, isConnected: false }, ], seizeSwitchConnectedAccount, }); // Trigger account switch via the mock - would need to expose this in the mock // This is a placeholder showing the pattern await waitFor(() => { expect(setToast).toHaveBeenCalledWith( expect.objectContaining({ type: "error" }) ); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/header/user/proxy/HeaderUserProxyDropdown.test.tsx` around lines 38 - 42, Add a test in HeaderUserProxyDropdown.test.tsx that verifies error handling when seizeSwitchConnectedAccount throws: mock seizeSwitchConnectedAccount to throw (jest.fn().mockImplementation(() => { throw new Error("Switch failed") })) and pass it into renderDropdown along with profileBase, isConnected true and two connectedAccounts (one active, one inactive); trigger the account-switch UI action the same way other tests trigger switching (or call the exposed handler used by the component), then await a waitFor that asserts setToast was called with an object containing type: "error" (use the existing setToast/onClose helpers returned by renderDropdown to observe the toast).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/auth/Auth.test.tsx`:
- Around line 63-72: The Modal mock currently always renders children causing
visibility tests to be invalid; modify the jest.mock for "react-bootstrap" so
the mocked Modal function accepts props (including show) and returns children
only when props.show is true (otherwise return null), and preserve the attached
static subcomponents Modal.Header, Modal.Title, Modal.Body, Modal.Footer; update
the mocked Modal signature (the Object.assign target) to check props.show before
rendering so tests that assert modal visibility behave correctly.
In `@components/auth/Auth.tsx`:
- Around line 205-206: The current render reads authRole = getRole(getAuthJwt())
which can throw on malformed/stale JWT and crash the provider tree; wrap the
derivation in a safe guard (e.g., try/catch or a safeDecode helper) inside the
Auth component so any decode/role extraction errors are caught and you return a
safe fallback (null/undefined or a "guest" role) instead of throwing; update
references to authRole to expect the fallback and ensure getRole/getAuthJwt
calls are only made inside the guarded block so the component never throws
during render.
In `@components/auth/SeizeConnectContext.tsx`:
- Around line 804-846: The while loop in seizeDisconnectAndLogoutAll can hang if
removeAuthJwt fails silently; add a defensive max-iteration guard (e.g.,
maxIterations or maxProfiles) when looping over getConnectedWalletAccounts() to
break out after a sensible limit, log an error via logError (use the same
"seizeDisconnectAndLogoutAll" tag) and throw an AuthenticationError if the loop
exceeds the limit; keep current check that compares nextRemainingProfiles to
remainingProfiles, but also increment an iteration counter each loop and use
refreshStoredConnectedAccounts()/setDisconnected() only on successful exit.
In `@components/header/AppHeader.tsx`:
- Around line 166-182: The call to seizeSwitchConnectedAccount in
switchToNextConnectedAccount is unguarded and can throw, so wrap the
seizeSwitchConnectedAccount(nextAccount.address) invocation in a try/catch; on
catch, log or handle the error, return false, and trigger the existing
account-menu fallback (invoke the component’s menu-opening handler or set the
menu state to open) so activation falls back to showing the menu instead of
breaking; references: switchToNextConnectedAccount, seizeSwitchConnectedAccount,
connectedAccounts.
In `@components/header/AppUserConnect.tsx`:
- Around line 33-47: runAsyncAndNavigate currently only console.error's caught
errors and thus swallows them; update it to use the same toast UX as
HeaderUserProxyDropdown.runMenuAction by importing and using AuthContext's
setToast, and in the catch block call setToast with a user-facing message
(errorMessage) and include error details for debugging, while preserving the
existing console.error and the final onNavigate() call; ensure you reference
runAsyncAndNavigate and HeaderUserProxyDropdown for parity when implementing the
toast behavior.
---
Outside diff comments:
In `@components/auth/Auth.tsx`:
- Around line 283-330: The stale-address checks comparing currentAddress to
address inside validateImmediately are invalid because both values are from the
same closure; replace them with a runtime check against a mutable ref or current
operation id to detect later changes. Add a ref like latestAddressRef (updated
in a useEffect whenever address changes) or an operationIdRef (set to
operationId at start) and then: (1) before/after awaiting validateAuthImmediate,
compare currentAddress !== latestAddressRef.current (or operationIdRef.current
!== operationId) to detect stale operations, and (2) when cleaning up, verify
abortControllerRef.current === abortController (not
abortController.signal.aborted) before clearing abortControllerRef and setting
setAuthLoadingState("idle"); update usages of currentAddress !== address in
validateImmediately to use the chosen ref comparison and ensure
abortControllerRef is only cleared by the active operation.
---
Duplicate comments:
In `@package.json`:
- Line 41: The "format:changed" npm script currently pipes a possibly-empty
NUL-separated file list into "xargs -0 prettier" which will run prettier with no
args if the list is empty; update the "format:changed" script string to use
"xargs -0 -r prettier" (add the -r / --no-run-if-empty flag) so prettier is
skipped when there are no files, and also scan the other related scripts
"quality:changed" and "audit:changed" and add the same "-r" to any similar
"xargs -0" invocations to keep changed-only behavior consistent.
---
Nitpick comments:
In `@__tests__/components/header/user/proxy/HeaderUserProxyDropdown.test.tsx`:
- Around line 38-42: Add a test in HeaderUserProxyDropdown.test.tsx that
verifies error handling when seizeSwitchConnectedAccount throws: mock
seizeSwitchConnectedAccount to throw (jest.fn().mockImplementation(() => { throw
new Error("Switch failed") })) and pass it into renderDropdown along with
profileBase, isConnected true and two connectedAccounts (one active, one
inactive); trigger the account-switch UI action the same way other tests trigger
switching (or call the exposed handler used by the component), then await a
waitFor that asserts setToast was called with an object containing type: "error"
(use the existing setToast/onClose helpers returned by renderDropdown to observe
the toast).
In `@components/auth/SeizeConnectContext.tsx`:
- Around line 952-958: The useMemo for seizeAddConnectedAccount incorrectly
depends on storedConnectedAccounts.length (a redundant dependency) while the
callback actually uses canStoreAnotherWalletAccount and possibly the full
storedConnectedAccounts; update the dependency array to include
canStoreAnotherWalletAccount and/or the entire storedConnectedAccounts (or
ensure refreshStoredConnectedAccounts is stable) so the memo recalculates
correctly when account storage state changes; locate the
seizeAddConnectedAccount definition and replace storedConnectedAccounts.length
with either canStoreAnotherWalletAccount or storedConnectedAccounts (or add
refreshStoredConnectedAccounts) depending on which value the callback truly
relies on.
- Around line 200-201: Replace the hardcoded ADD_FLOW_CANCEL_GRACE_MS = 5000
with a clearly typed exported constant or configuration value (e.g., export
const ADD_FLOW_CANCEL_GRACE_MS: number = 5000 or pull from a config object) and
add a short comment explaining why 5000ms was chosen and how it impacts wallet
transition UX; update any references (e.g., where ADD_FLOW_CANCEL_GRACE_MS is
used in SeizeConnectContext logic) to use the new typed/centralized value so the
timeout can be easily adjusted or overridden in the future.
- Around line 984-1030: The memo for contextValue includes unnecessary
dependencies (connectedAddress, impersonatedAddress, and walletInfo?.type) that
aren't referenced in the returned object and can cause extra re-renders; update
the useMemo dependency array to remove connectedAddress, impersonatedAddress,
and walletInfo?.type while keeping activeAddress, walletInfo?.name,
walletInfo?.icon, all seize* handler references (seizeConnect, seizeDisconnect,
seizeDisconnectAndLogout, seizeDisconnectAndLogoutAll, seizeAcceptConnection,
seizeSwitchConnectedAccount, seizeAddConnectedAccount), state.open,
account.isConnected, walletState, hasInitializationError, initializationError,
connectedAccounts, and canAddConnectedAccount so the memo dependencies exactly
match the symbols used in the SeizeConnectContextType value.
In `@components/header/user/proxy/HeaderUserProxyDropdown.tsx`:
- Around line 138-150: The onAddAccount handler in HeaderUserProxyDropdown calls
seizeAddConnectedAccount() directly without running it through the existing
runMenuAction wrapper; change the onAddAccount callback given to
HeaderUserConnectedAccounts to invoke seizeAddConnectedAccount via runMenuAction
(e.g., runMenuAction(() => seizeAddConnectedAccount())) so its
errors/side-effects are handled consistently, and then call onClose after the
wrapped action completes (or in a finally-equivalent post-action step) to
preserve current behavior; update the onAddAccount arrow handler in
HeaderUserProxyDropdown accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
__tests__/components/auth/Auth.test.tsx__tests__/components/auth/connection-state-indicator.test.ts__tests__/components/header/AppUserConnect.test.tsx__tests__/components/header/user/proxy/HeaderUserProxyDropdown.test.tsxapp/accept-connection-sharing/page.client.tsxcomponents/auth/Auth.tsxcomponents/auth/SeizeConnectContext.tsxcomponents/header/AppHeader.tsxcomponents/header/AppSidebarHeader.tsxcomponents/header/AppUserConnect.tsxcomponents/header/user/proxy/HeaderUserProxyDropdown.tsxcomponents/layout/sidebar/WebSidebarUser.tsxpackage.json
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/auth/Auth.tsx`:
- Around line 636-639: The calls to seizeDisconnect() are fire-and-forget and
can produce unhandled rejections; update places that call seizeDisconnect()
(e.g., the branches around setShowSignModal and the other call near line 737) to
await the Promise or attach a .catch handler so rejections (including
WalletDisconnectionError) are handled cleanly; specifically, wrap the call in a
try/catch when using await (try { await seizeDisconnect(); } catch (err) {
process or log the error and continue cleanup }) or use
seizeDisconnect().catch(err => { /* log/handle WalletDisconnectionError and
ensure session cleanup */ }) to avoid leaving the session in an incomplete state
while preserving the existing control flow (returning { success: false } where
applicable).
In `@components/auth/SeizeConnectContext.tsx`:
- Around line 951-957: The delayed reconnect callback currently calls
seizeConnect() directly inside the setTimeout (see
retryConnectTimeoutRef.current and isMountedRef.current) which allows its thrown
WalletConnectionError to escape and leave isAddingConnectedAccount stuck; wrap
the seizeConnect() invocation in a try/catch inside that timeout callback, catch
any errors (including WalletConnectionError), log or rethrow as appropriate, and
ensure you reset the isAddingConnectedAccount state (or clear any flags) in the
catch/finally so the add-account flow cannot remain blocked.
In `@components/header/user/proxy/HeaderUserProxyDropdown.tsx`:
- Around line 145-152: The call to onClose is invoked twice because
runMenuAction (used earlier around line 94) already triggers onClose on success;
remove the redundant .finally(() => { onClose(); }) wrapping this invocation so
onClose is only called once. Specifically, update the block that calls
runMenuAction({ action: () => seizeAddConnectedAccount(), pendingKey:
"add-account", errorMessage: ... }) to omit the .finally handler and rely on
runMenuAction's internal close behavior (or alternatively, if you intend the
caller to control closing, remove the close logic from runMenuAction
instead—choose one consistent place to call onClose).
In `@eslint.config.tight.mjs`:
- Line 149: The tight ESLint config currently sets includeTestResultsIgnore:
false which re-enables linting of Playwright/test artifacts; change the setting
back to true (or restore the "**/test-results/**" ignore entry) so the
test-results directory remains ignored—update the includeTestResultsIgnore value
referenced in eslint.config.tight.mjs to true (or re-add the test-results ignore
pattern) to prevent generated artifacts from being linted.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
__tests__/components/auth/Auth.test.tsx__tests__/components/header/user/proxy/HeaderUserProxyDropdown.test.tsxcomponents/auth/Auth.tsxcomponents/auth/SeizeConnectContext.tsxcomponents/header/AppHeader.tsxcomponents/header/AppUserConnect.tsxcomponents/header/user/proxy/HeaderUserProxyDropdown.tsxcomponents/notifications/NotificationsContext.tsxeslint.config.mjseslint.config.tight.mjspackage.json
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/auth/Auth.tsx (1)
740-751:⚠️ Potential issue | 🟠 MajorHandle rejected logout promise in cancel flow.
Line 750 invokes
seizeDisconnectAndLogout()without a rejection handler. If logout/disconnect fails, this can surface as an unhandled promise rejection and silently fail the cancel flow.Suggested fix
const onCancelSignRequest = useCallback(() => { setShowSignModal(false); if (!isAddressAuthorized) { void seizeDisconnect().catch((error) => { logErrorSecurely("onCancelSignRequest_disconnect", error); }); return; } - void seizeDisconnectAndLogout(); + void seizeDisconnectAndLogout().catch((error) => { + logErrorSecurely("onCancelSignRequest_logout", error); + setToast({ + message: "Failed to sign out. Please try again.", + type: "error", + }); + }); }, [isAddressAuthorized, seizeDisconnect, seizeDisconnectAndLogout]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/auth/Auth.tsx` around lines 740 - 751, The onCancelSignRequest handler calls seizeDisconnectAndLogout() without handling rejection; update onCancelSignRequest so after setShowSignModal(false) you await or attach a .catch to seizeDisconnectAndLogout() (similar to the seizeDisconnect branch) and call logErrorSecurely("onCancelSignRequest_disconnectAndLogout", error) on failure; keep the existing behavior for the unauthorized branch using seizeDisconnect and ensure the function dependencies (isAddressAuthorized, seizeDisconnect, seizeDisconnectAndLogout) remain unchanged.
🧹 Nitpick comments (1)
eslint.config.tight.mjs (1)
150-150: Explicit strict enforcement can silently degrade if plugin loading fails.At line 150 in
eslint.config.tight.mjs,reactCompilerRule: "error"declares strict intent. However, ineslint.config.mjs(lines 15–23, 480–482), the rule is only applied whenreactCompilerPluginsuccessfully loads. While the plugin is declared as a dependency (eslint-plugin-react-compiler: ^19.1.0-rc.2), a loading failure is caught silently without warning—causing the explicit "error" setting to be ignored.To match the caller's intent, either fail loudly when the plugin cannot load, or apply enforcement regardless of plugin presence, or at minimum emit a warning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.tight.mjs` at line 150, The config sets reactCompilerRule: "error" but the loader for reactCompilerPlugin swallows load failures, so the strict rule can be silently ignored; update the plugin-loading logic that initializes reactCompilerPlugin to either rethrow the loading error or at minimum emit a clear warning via the same logger when the plugin cannot be required, and ensure the code that applies reactCompilerRule checks the plugin load result and fails fast (throw) or explicitly applies a noop/fallback rule so the "error" intent is not silently skipped; reference reactCompilerRule and reactCompilerPlugin and adjust the loader/initializer where the plugin is required and the rule is applied (the module that conditionally applies the rule) to implement the loud failure or warning behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/auth/SeizeConnectContext.tsx`:
- Around line 930-1001: seizeAddConnectedAccount needs a re-entrancy guard to
avoid races: at the top of the function return early if an add flow is already
in progress (use the existing isAddingConnectedAccount state or, preferably, a
mutable ref like isAddingRef.current to avoid stale closures), then set that
guard immediately before starting any async work; ensure you clear the guard and
reset addFlowOriginAddressRef.current and retryConnectTimeoutRef.current in
every error path and after the final successful seizeConnect call (both the
immediate try/catch path and the disconnect().then timer branch). Update the
logic inside seizeAddConnectedAccount (and its error/then handlers) to set
isAdding (or isAddingRef) true at start and false on all exits so concurrent
invocations cannot run overlapping disconnect/retry timers.
---
Duplicate comments:
In `@components/auth/Auth.tsx`:
- Around line 740-751: The onCancelSignRequest handler calls
seizeDisconnectAndLogout() without handling rejection; update
onCancelSignRequest so after setShowSignModal(false) you await or attach a
.catch to seizeDisconnectAndLogout() (similar to the seizeDisconnect branch) and
call logErrorSecurely("onCancelSignRequest_disconnectAndLogout", error) on
failure; keep the existing behavior for the unauthorized branch using
seizeDisconnect and ensure the function dependencies (isAddressAuthorized,
seizeDisconnect, seizeDisconnectAndLogout) remain unchanged.
---
Nitpick comments:
In `@eslint.config.tight.mjs`:
- Line 150: The config sets reactCompilerRule: "error" but the loader for
reactCompilerPlugin swallows load failures, so the strict rule can be silently
ignored; update the plugin-loading logic that initializes reactCompilerPlugin to
either rethrow the loading error or at minimum emit a clear warning via the same
logger when the plugin cannot be required, and ensure the code that applies
reactCompilerRule checks the plugin load result and fails fast (throw) or
explicitly applies a noop/fallback rule so the "error" intent is not silently
skipped; reference reactCompilerRule and reactCompilerPlugin and adjust the
loader/initializer where the plugin is required and the rule is applied (the
module that conditionally applies the rule) to implement the loud failure or
warning behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/auth/Auth.tsxcomponents/auth/SeizeConnectContext.tsxcomponents/header/user/proxy/HeaderUserProxyDropdown.tsxeslint.config.tight.mjs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/auth/Auth.tsx (1)
669-678:⚠️ Potential issue | 🟡 MinorGate cache invalidation on successful re-auth.
invalidateAll()runs even ifrequestSignIn(...)fails. That creates unnecessary refetch churn while still unauthenticated.💡 Suggested patch
if (!isValid) { removeAuthJwt(); - await requestSignIn({ + const { success } = await requestSignIn({ signerAddress: address, role: activeProfileProxy ? validateRoleForAuthentication(activeProfileProxy) : null, }); - invalidateAll(); + if (success) { + invalidateAll(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/auth/Auth.tsx` around lines 669 - 678, The current flow calls invalidateAll() regardless of whether requestSignIn(...) succeeds; change it so cache invalidation only runs after a successful re-auth: await requestSignIn(...) inside a try/catch, and only call invalidateAll() in the try (after the awaited call resolves); if requestSignIn throws, catch the error (log or handle it) and do not call invalidateAll(). Keep removeAuthJwt() where it is if you intend to clear the token before re-auth, but ensure invalidateAll() is gated on requestSignIn success (use the symbols isValid, removeAuthJwt, requestSignIn, and invalidateAll to locate the code).
♻️ Duplicate comments (1)
components/auth/SeizeConnectContext.tsx (1)
975-977:⚠️ Potential issue | 🟠 MajorRe-entrancy guard is released too early in add-account flow.
clearAddConnectedAccountGuard()is called immediately afterseizeConnect()succeeds, so a secondseizeAddConnectedAccount()can start while the first add flow is still in progress.💡 Suggested patch
const seizeAddConnectedAccount = useCallback((): void => { - if (isAddingConnectedAccountRef.current) { + if (isAddingConnectedAccount || isAddingConnectedAccountRef.current) { return; } @@ try { seizeConnect(); - clearAddConnectedAccountGuard(); } catch (error: unknown) { clearAddConnectedAccountGuard(); setIsAddingConnectedAccount(false); @@ try { seizeConnect(); - clearAddConnectedAccountGuard(); } catch (error: unknown) { clearAddConnectedAccountGuard(); setIsAddingConnectedAccount(false);Also applies to: 1012-1015
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/auth/SeizeConnectContext.tsx` around lines 975 - 977, The guard is being cleared too early: when seizeConnect() succeeds we call clearAddConnectedAccountGuard() allowing another seizeAddConnectedAccount() to start while the first add-account flow is still active; instead, keep the guard set until the add-account flow fully completes (success or failure) — move the clearAddConnectedAccountGuard() call out of the immediate post-seizeConnect() path and into the finalization path (or finally block) that runs after the entire add-account flow finishes, and apply the same change to the duplicate block around lines 1012-1015 so clearAddConnectedAccountGuard() is only called once the add flow is done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/auth/SeizeConnectContext.tsx`:
- Around line 745-748: seizeDisconnect() currently returns early when
!isActiveWalletConnected which can skip disconnecting the live wallet if
activeAddress was switched via seizeSwitchConnectedAccount; update
seizeDisconnect to verify the wallet's real connection state (e.g., query the
provider/current signer or compare the live wallet address from the provider to
activeAddress) and proceed to disconnect the wallet if the provider indicates a
connected session even when isActiveWalletConnected is false; apply the same
change to the other seizeDisconnect implementation around the 905-923 block so
both use the provider-based check (or explicit address comparison to the live
wallet) instead of relying solely on isActiveWalletConnected.
In `@eslint.config.mjs`:
- Around line 441-457: DEFAULT_GLOBAL_IGNORES currently contains broad globs
("*.js", "*.mjs", "*.ts", "*.tsx") that accidentally exclude root Next.js
runtime entries like instrumentation.ts and instrumentation-client.ts; remove
those four broad patterns from DEFAULT_GLOBAL_IGNORES and instead add narrowly
scoped ignore patterns that target only config files (for example,
config/**/*.js, config/**/*.mjs, config/**/*.ts and/or root config filenames
like *.config.js, *.config.mjs, *.config.ts) so application entry files such as
instrumentation.ts and instrumentation-client.ts remain linted; update the
DEFAULT_GLOBAL_IGNORES constant accordingly.
---
Outside diff comments:
In `@components/auth/Auth.tsx`:
- Around line 669-678: The current flow calls invalidateAll() regardless of
whether requestSignIn(...) succeeds; change it so cache invalidation only runs
after a successful re-auth: await requestSignIn(...) inside a try/catch, and
only call invalidateAll() in the try (after the awaited call resolves); if
requestSignIn throws, catch the error (log or handle it) and do not call
invalidateAll(). Keep removeAuthJwt() where it is if you intend to clear the
token before re-auth, but ensure invalidateAll() is gated on requestSignIn
success (use the symbols isValid, removeAuthJwt, requestSignIn, and
invalidateAll to locate the code).
---
Duplicate comments:
In `@components/auth/SeizeConnectContext.tsx`:
- Around line 975-977: The guard is being cleared too early: when seizeConnect()
succeeds we call clearAddConnectedAccountGuard() allowing another
seizeAddConnectedAccount() to start while the first add-account flow is still
active; instead, keep the guard set until the add-account flow fully completes
(success or failure) — move the clearAddConnectedAccountGuard() call out of the
immediate post-seizeConnect() path and into the finalization path (or finally
block) that runs after the entire add-account flow finishes, and apply the same
change to the duplicate block around lines 1012-1015 so
clearAddConnectedAccountGuard() is only called once the add flow is done.
Signed-off-by: prxt6529 <prxt@6529.io>
Signed-off-by: prxt6529 <prxt@6529.io>
Signed-off-by: prxt6529 <prxt@6529.io>
Signed-off-by: prxt6529 <prxt@6529.io>
|
simo6529
approved these changes
Mar 3, 2026
This was referenced Mar 4, 2026
Merged
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.
Summary by CodeRabbit
New Features
Bug Fixes