Remove wallet cookie migration, Update AGENTS about clean code, Improve DateAccordion accessibility#1516
Remove wallet cookie migration, Update AGENTS about clean code, Improve DateAccordion accessibility#1516
Conversation
…ve DateAccordion accessibility Signed-off-by: prxt6529 <prxt@6529.io>
|
Warning Rate limit exceeded@prxt6529 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughUnified wallet state machine and address validation with debounced reconciliation; DateAccordion made accessible via a button and ARIA linkage; removed Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as SeizeConnectProvider
participant AppKit as AppKit Hooks
participant Wallet as Wallet Provider
participant Validator as Address Validator
participant Logger as Event Logger
rect rgb(245,248,255)
note over UI: Initialization & reconciliation
UI->>AppKit: read account/status
AppKit-->>UI: account, status
UI->>UI: set walletState = "initializing"
end
User->>UI: Connect wallet
UI->>Wallet: request connection
Wallet-->>UI: address (raw)
UI->>Validator: validate / checksum (getAddress/isAddress)
alt valid
Validator-->>UI: normalized address
UI->>Logger: log "wallet_initialization" success
UI->>UI: walletState = "connected"
else invalid
Validator-->>UI: errorContext
UI->>Logger: log "wallet_initialization" error
UI->>UI: walletState = "error"
end
AppKit-->>UI: account/status change
UI->>UI: debounced reconcile -> update walletState
User->>UI: Disconnect
UI->>Wallet: disconnect
UI->>Logger: log "seizeDisconnect"
UI->>UI: walletState = "disconnected"
sequenceDiagram
autonumber
actor User
participant Accordion as DateAccordion
participant DOM as DOM/ARIA
User->>Accordion: Click toggle button
Accordion->>DOM: set aria-expanded true/false
Accordion->>DOM: set aria-controls -> contentId
DOM-->>User: animated region with id=contentId shown/hidden
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Signed-off-by: prxt6529 <prxt@6529.io>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/auth/auth.utils.ts (1)
18-21: Ensure cookie removal always succeeds by fixing attributesExplicitly set path so remove matches set. Without consistent path, WALLET_AUTH_COOKIE may not be deleted across routes.
Apply this diff:
const COOKIE_OPTIONS = { secure: true, sameSite: "strict" as const, + path: "/", };components/auth/SeizeConnectContext.tsx (1)
1-625: Remove inline code comments in TS/TSX filesThe code contains numerous inline comments; project guidelines specify no comments in TypeScript/TSX. Move explanations to docs or keep minimal JSDoc in .md.
As per coding guidelines
🧹 Nitpick comments (7)
services/auth/auth.utils.ts (2)
40-49: Guard against already‑expired JWTs before setting cookieIf exp <= now, you’ll set an immediately expired cookie. Prefer skipping set and clearing auth or short‑circuiting.
Example:
const expiresInSeconds = jwtExpiration - now; const expiresInDays = expiresInSeconds / 86400; + if (expiresInSeconds <= 0) { + // Token expired; ensure no stale cookie remains. + Cookies.remove(WALLET_AUTH_COOKIE, COOKIE_OPTIONS); + return; + }
9-13: Remove legacy cookie constants and TODOMigration is done; these are unused. Drop the TODO and constants to avoid confusion.
-// TODO: remove these cookies once migration is complete -const WALLET_ADDRESS_COOKIE = "wallet-address"; -const WALLET_REFRESH_TOKEN_COOKIE = "wallet-refresh-token"; -const WALLET_ROLE_COOKIE = "wallet-role";components/common/DateAccordion.tsx (1)
31-36: Strengthen accordion a11y: add region + labelled‑byCurrent pattern links button→content via aria-controls. Also label the content region via aria-labelledby for assistive tech.
- <button + <button type="button" onClick={onToggle} aria-expanded={isExpanded} - aria-controls={contentId} + aria-controls={contentId} + id={`${contentId}-button`} className="tw-w-full ... > ... - {isExpanded && ( - <motion.div + {isExpanded && ( + <motion.div initial={{ height: 0, opacity: 0 }} animate={{ height: "auto", opacity: 1 }} exit={{ height: 0, opacity: 0 }} transition={{ duration: 0.3, ease: [0.04, 0.62, 0.23, 0.98] }} - id={contentId}> + id={contentId} + role="region" + aria-labelledby={`${contentId}-button`}> {children} </motion.div> )}Also applies to: 64-71
components/auth/SeizeConnectContext.tsx (4)
31-60: Avoid duplicating error classes; import from a single moduleWalletConnectionError/WalletDisconnectionError are defined here but also live in src/errors/wallet.ts. Centralize to prevent instanceof/type divergence.
361-361: Use cross‑platform timer typeNodeJS.Timeout can mismatch in browser TS configs. Prefer ReturnType.
- const debounceTimeoutRef = useRef<NodeJS.Timeout | null>(null); + const debounceTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
519-523: Use centralized logger instead of console.errorFor consistency and sanitization, replace console.error with logError.
- console.error( - "Failed to reopen wallet connection after logout:", - openError - ); + logError("seizeDisconnectAndLogout_reconnect", createWalletError(WalletConnectionError, "reopen wallet connection after logout", openError));
342-344: Add readonly to provider props per guidelinesAligns with project rule “Always add readonly before props”.
-export const SeizeConnectProvider: React.FC<{ children: React.ReactNode }> = ({ +export const SeizeConnectProvider: React.FC<{ readonly children: React.ReactNode }> = ({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
AGENTS.md(1 hunks)__tests__/components/auth/SeizeConnectContext.test.tsx(1 hunks)__tests__/components/common/DateAccordion.test.tsx(1 hunks)__tests__/components/header/share/HeaderShare.test.tsx(0 hunks)__tests__/services/auth.utils.test.ts(4 hunks)components/auth/SeizeConnectContext.tsx(22 hunks)components/common/DateAccordion.tsx(4 hunks)services/auth/auth.utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- tests/components/header/share/HeaderShare.test.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript for implementation code
Files:
__tests__/services/auth.utils.test.tscomponents/common/DateAccordion.tsx__tests__/components/common/DateAccordion.test.tsx__tests__/components/auth/SeizeConnectContext.test.tsxcomponents/auth/SeizeConnectContext.tsxservices/auth/auth.utils.ts
**/{__tests__/**/*.{ts,tsx},*.test.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/{__tests__/**/*.{ts,tsx},*.test.tsx}: Place tests in tests directories or alongside components as ComponentName.test.tsx
Mock external dependencies and APIs in tests
Files:
__tests__/services/auth.utils.test.ts__tests__/components/common/DateAccordion.test.tsx__tests__/components/auth/SeizeConnectContext.test.tsx
__tests__/**
📄 CodeRabbit inference engine (tests/AGENTS.md)
Place Jest test suites under the
__tests__directory mirroring source folders (e.g., components, contexts, hooks, utils)
Files:
__tests__/services/auth.utils.test.ts__tests__/components/common/DateAccordion.test.tsx__tests__/components/auth/SeizeConnectContext.test.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
components/common/DateAccordion.tsx__tests__/components/common/DateAccordion.test.tsx__tests__/components/auth/SeizeConnectContext.test.tsxcomponents/auth/SeizeConnectContext.tsx
__tests__/components/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (tests/AGENTS.md)
Use
@testing-library/reactand@testing-library/user-eventfor React component tests
Files:
__tests__/components/common/DateAccordion.test.tsx__tests__/components/auth/SeizeConnectContext.test.tsx
🧠 Learnings (3)
📚 Learning: 2025-09-28T12:33:30.950Z
Learnt from: CR
PR: 6529-Collections/6529seize-frontend#0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-09-28T12:33:30.950Z
Learning: Applies to __tests__/components/**/*.{ts,tsx,js,jsx} : Use `testing-library/react` and `testing-library/user-event` for React component tests
Applied to files:
__tests__/components/auth/SeizeConnectContext.test.tsxAGENTS.md
📚 Learning: 2025-09-28T12:32:36.083Z
Learnt from: CR
PR: 6529-Collections/6529seize-frontend#0
File: AGENTS.md:0-0
Timestamp: 2025-09-28T12:32:36.083Z
Learning: Applies to **/*.tsx : Use React functional components with hooks
Applied to files:
AGENTS.md
📚 Learning: 2025-09-28T12:32:36.083Z
Learnt from: CR
PR: 6529-Collections/6529seize-frontend#0
File: AGENTS.md:0-0
Timestamp: 2025-09-28T12:32:36.083Z
Learning: Applies to **/{__tests__/**/*.{ts,tsx},*.test.tsx} : Place tests in __tests__ directories or alongside components as ComponentName.test.tsx
Applied to files:
AGENTS.md
🧬 Code graph analysis (3)
__tests__/services/auth.utils.test.ts (1)
helpers/safeLocalStorage.ts (1)
safeLocalStorage(3-11)
__tests__/components/common/DateAccordion.test.tsx (1)
components/common/DateAccordion.tsx (1)
DateAccordion(15-76)
components/auth/SeizeConnectContext.tsx (2)
src/errors/wallet.ts (2)
WalletInitializationError(16-26)WalletConnectionError(1-7)src/utils/security-logger.ts (4)
logSecurityEvent(96-116)createValidationEventContext(197-210)logError(125-178)createConnectionEventContext(184-191)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
AGENTS.md (1)
54-54: Docs update LGTMClear, actionable addition. No further changes needed.
services/auth/auth.utils.ts (1)
118-136: Verify the JWT claim used for roleYou read role from decodedJwt.id. Confirm server issues role in id; if not, switch to the correct claim (e.g., role) or make it configurable.
Would you like a targeted unit test for validateJwtRole covering match/mismatch and missing-claim cases?
__tests__/services/auth.utils.test.ts (1)
10-11: Tests aligned with new API surfaceMocks and assertions look correct; casing/normalization expectations match implementation.
Also applies to: 66-67, 86-87, 134-135
__tests__/components/auth/SeizeConnectContext.test.tsx (1)
1-11: Solid coverage for security logging and error flowsImports/mocks reflect the new context API and migration removal. Tests exercise init, connect, accept, and logging paths thoroughly.
__tests__/components/common/DateAccordion.test.tsx (1)
16-56: Accessibility tests LGTMValidates aria-expanded/aria-controls and region presence. Mocks keep DOM deterministic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/auth/SeizeConnectContext.tsx (2)
32-52: Consolidate error classes with the errors module.
WalletConnectionErrorandWalletDisconnectionErrorare redefined here but also exist insrc/errors/wallet.ts. This duplication can lead to version skew and confusion. Consider importing the base classes from the errors module and extending them if you need the additionalcodeproperty.Apply this diff to import and extend the base classes:
+import { + WalletConnectionError as BaseWalletConnectionError, + WalletDisconnectionError as BaseWalletDisconnectionError +} from "@/src/errors/wallet"; + -export class WalletConnectionError extends Error { - constructor( - message: string, - public readonly cause?: unknown, - public readonly code?: string - ) { - super(message); - this.name = "WalletConnectionError"; - } -} +export class WalletConnectionError extends BaseWalletConnectionError { + constructor( + message: string, + cause?: unknown, + public readonly code?: string + ) { + super(message, cause); + } +} -export class WalletDisconnectionError extends Error { - constructor( - message: string, - public readonly cause?: unknown, - public readonly code?: string - ) { - super(message); - this.name = "WalletDisconnectionError"; - } -} +export class WalletDisconnectionError extends BaseWalletDisconnectionError { + constructor( + message: string, + cause?: unknown, + public readonly code?: string + ) { + super(message, cause); + } +}
544-561: Consider error handling strategy for public API.
seizeAcceptConnectionthrows anAuthenticationErrorwhen the address is invalid. Since this is part of the public context API, throwing could crash consuming components that don't wrap the call in try-catch. Consider either:
- Documenting the throwing behavior clearly in JSDoc (already present at line 93-95, but could be more explicit about the exception type), or
- Handling the error internally by setting an error state that consumers can check
The current implementation is acceptable if consumers are expected to handle exceptions, but ensure this is well-documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/auth/SeizeConnectContext.tsx(22 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript for implementation code
Files:
components/auth/SeizeConnectContext.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
components/auth/SeizeConnectContext.tsx
🧬 Code graph analysis (1)
components/auth/SeizeConnectContext.tsx (2)
src/errors/wallet.ts (2)
WalletInitializationError(16-26)WalletConnectionError(1-7)src/utils/security-logger.ts (4)
logSecurityEvent(96-116)createValidationEventContext(197-210)logError(125-178)createConnectionEventContext(184-191)
🔇 Additional comments (3)
components/auth/SeizeConnectContext.tsx (3)
259-340: LGTM: Clean state machine refactor.The unified
WalletStatetype eliminates multiple boolean flags and provides a clear state machine with explicit transitions. The initialization logic properly validates stored addresses and handles errors, and the transition methods provide a clean API for state updates.
363-440: LGTM: Proper debounced synchronization.The debounced effect correctly waits for initialization, validates addresses using
isAddressand normalizes them withgetAddress, and only updates state when necessary. The 50ms debounce delay and comprehensive dependency array ensure proper synchronization with external wallet state changes.
576-606: LGTM: Proper memoization and API surface.The context value is properly memoized with a comprehensive dependency array. Exposing both
connectionState(string literal) andwalletState(full state object) provides backward compatibility while enabling advanced consumers to access the full state machine.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/common/FallbackImage.tsx (1)
30-39: Remove inline comment.The inline comment on line 36 violates the coding guideline "Do not include any comments in the code" for
**/*.{ts,tsx}files.As per coding guidelines
Apply this diff:if (!usedFallback) { onPrimaryError?.(e); setSrc(fallbackSrc); setUsedFallback(true); } else { - // If fallback also fails, call the external onError handler onError?.(e); }
🧹 Nitpick comments (2)
__tests__/components/header/HeaderSearchModal.test.tsx (1)
173-173: Consider using consistent query pattern.For consistency with other tests (lines 146-148, 160), consider querying by accessible name:
- const input = screen.getByRole("textbox"); + const input = screen.getByRole("textbox", { name: "Search" });This makes the test more robust and explicit about what element it's targeting.
__tests__/components/common/FallbackImage.test.tsx (1)
10-34: Consider adding test coverage for the fallback error scenario.The test correctly verifies the primary image error handling and callback invocation. However, it would be beneficial to add a test case for when the fallback image also fails, which should trigger the
onErrorprop according to the component implementation (line 37 in FallbackImage.tsx).Example test case to add:
it("calls onError when both primary and fallback fail", async () => { const onPrimaryError = jest.fn(); const onError = jest.fn(); render( <FallbackImage primarySrc="primary.gif" fallbackSrc="fallback.gif" alt="fallback example" onPrimaryError={onPrimaryError} onError={onError} /> ); const image = screen.getByRole("img", { name: "fallback example" }); fireEvent.error(image); await waitFor(() => { expect(image.getAttribute("src")).toBe("fallback.gif"); }); fireEvent.error(image); expect(onPrimaryError).toHaveBeenCalledTimes(1); expect(onError).toHaveBeenCalledTimes(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
__tests__/components/common/FallbackImage.test.tsx(1 hunks)__tests__/components/header/HeaderSearchModal.test.tsx(4 hunks)components/common/FallbackImage.tsx(3 hunks)components/drops/view/item/content/media/DropListItemContentMediaImage.tsx(10 hunks)components/header/header-search/HeaderSearchModal.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript for implementation code
Files:
__tests__/components/common/FallbackImage.test.tsxcomponents/common/FallbackImage.tsx__tests__/components/header/HeaderSearchModal.test.tsxcomponents/header/header-search/HeaderSearchModal.tsxcomponents/drops/view/item/content/media/DropListItemContentMediaImage.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
__tests__/components/common/FallbackImage.test.tsxcomponents/common/FallbackImage.tsx__tests__/components/header/HeaderSearchModal.test.tsxcomponents/header/header-search/HeaderSearchModal.tsxcomponents/drops/view/item/content/media/DropListItemContentMediaImage.tsx
**/{__tests__/**/*.{ts,tsx},*.test.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/{__tests__/**/*.{ts,tsx},*.test.tsx}: Place tests in tests directories or alongside components as ComponentName.test.tsx
Mock external dependencies and APIs in tests
Files:
__tests__/components/common/FallbackImage.test.tsx__tests__/components/header/HeaderSearchModal.test.tsx
__tests__/**
📄 CodeRabbit inference engine (tests/AGENTS.md)
Place Jest test suites under the
__tests__directory mirroring source folders (e.g., components, contexts, hooks, utils)
Files:
__tests__/components/common/FallbackImage.test.tsx__tests__/components/header/HeaderSearchModal.test.tsx
__tests__/components/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (tests/AGENTS.md)
Use
@testing-library/reactand@testing-library/user-eventfor React component tests
Files:
__tests__/components/common/FallbackImage.test.tsx__tests__/components/header/HeaderSearchModal.test.tsx
🧬 Code graph analysis (1)
__tests__/components/common/FallbackImage.test.tsx (1)
components/common/FallbackImage.tsx (1)
FallbackImage(13-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
__tests__/components/header/HeaderSearchModal.test.tsx (2)
52-58: LGTM! Mock component improvements.The addition of
displayNameto the mock component and using a named constant improves debuggability and aligns with React best practices.
143-150: LGTM! Accessibility verification added.This test properly validates the accessible label association introduced in the component, ensuring the search input is accessible to assistive technologies.
components/header/header-search/HeaderSearchModal.tsx (1)
340-344: LGTM! Accessibility improvements properly implemented.The label-input association using
htmlForandidcorrectly implements accessibility best practices. Thetw-sr-onlyclass ensures the label is available to screen readers while maintaining the visual design with the placeholder.components/common/FallbackImage.tsx (1)
13-52: LGTM! The error handling logic is well-structured.The implementation correctly:
- Calls
onPrimaryErroron the first failure before falling back- Calls
onErroronly when the fallback also fails- Resets state when
primarySrcchanges for retry scenarios- Uses
forwardRefproperly for imperative accesscomponents/drops/view/item/content/media/DropListItemContentMediaImage.tsx (2)
163-221: Excellent accessibility improvements!The addition of
aria-labelattributes to buttons (lines 163, 175, 184, 197, 206) andaria-hidden="true"to decorative SVG elements (line 213) significantly improves the accessibility of the modal controls. These changes ensure screen reader users can understand the purpose of each button.
3-19: LGTM! Import reorganization is clean.The reorganization of imports improves code readability by grouping related imports together. The new imports (
FallbackImage,fullScreenSupported,getScaledImageUri,ImageScale,useCapacitor,useInView) are appropriately placed and support the enhanced functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
__tests__/components/common/FallbackImage.test.tsx (2)
36-61: Test logic is correct.The test properly verifies that
onPrimaryErroris called on the first failure andonErroris called when the fallback also fails.
Consider using jest-dom matchers here as well.
For consistency with the suggested refactor in the first test, you can use
toHaveAttributeinstead ofgetAttribute().toBe().Apply this diff:
- expect(image.getAttribute("src")).toBe("fallback.gif"); + expect(image).toHaveAttribute("src", "fallback.gif");Based on learnings.
5-62: Consider adding tests for the reset behavior and happy path.The implementation includes a
useEffectthat resets state whenprimarySrcchanges, and there's a happy path scenario when images load successfully. Adding test cases for these would improve coverage:
- Test primarySrc change reset: Verify that when
primarySrcchanges after a fallback, the component resets to try the new primary image.- Test successful image load: Verify that when the primary image loads successfully, no callbacks are invoked and the image remains on the primary source.
Example test for reset behavior:
it("resets to primary source when primarySrc changes after fallback", async () => { const { rerender } = render( <FallbackImage primarySrc="primary.gif" fallbackSrc="fallback.gif" alt="fallback example" /> ); const image = screen.getByRole("img", { name: "fallback example" }); fireEvent.error(image); await waitFor(() => { expect(image).toHaveAttribute("src", "fallback.gif"); }); rerender( <FallbackImage primarySrc="new-primary.gif" fallbackSrc="fallback.gif" alt="fallback example" /> ); expect(image).toHaveAttribute("src", "new-primary.gif"); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
__tests__/components/common/FallbackImage.test.tsx(1 hunks)__tests__/components/header/HeaderSearchModal.test.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript for implementation code
Files:
__tests__/components/header/HeaderSearchModal.test.tsx__tests__/components/common/FallbackImage.test.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
__tests__/components/header/HeaderSearchModal.test.tsx__tests__/components/common/FallbackImage.test.tsx
**/{__tests__/**/*.{ts,tsx},*.test.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/{__tests__/**/*.{ts,tsx},*.test.tsx}: Place tests in tests directories or alongside components as ComponentName.test.tsx
Mock external dependencies and APIs in tests
Files:
__tests__/components/header/HeaderSearchModal.test.tsx__tests__/components/common/FallbackImage.test.tsx
__tests__/**
📄 CodeRabbit inference engine (tests/AGENTS.md)
Place Jest test suites under the
__tests__directory mirroring source folders (e.g., components, contexts, hooks, utils)
Files:
__tests__/components/header/HeaderSearchModal.test.tsx__tests__/components/common/FallbackImage.test.tsx
__tests__/components/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (tests/AGENTS.md)
Use
@testing-library/reactand@testing-library/user-eventfor React component tests
Files:
__tests__/components/header/HeaderSearchModal.test.tsx__tests__/components/common/FallbackImage.test.tsx
🧬 Code graph analysis (1)
__tests__/components/common/FallbackImage.test.tsx (1)
components/common/FallbackImage.tsx (1)
FallbackImage(13-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
__tests__/components/header/HeaderSearchModal.test.tsx (4)
52-58: LGTM! Improved mock component with displayName.The refactored mock now uses a named constant and sets
displayName, which improves debugging and aligns with React best practices for component identification in DevTools and error messages.
143-150: LGTM! Solid accessibility test.This test validates that the search input is properly associated with an accessible label, ensuring keyboard and screen reader users can identify the input. This aligns with WCAG accessibility guidelines and complements the labeling changes in the component.
160-160: LGTM! More specific and reliable queries.The updated queries now explicitly target the textbox by its accessible name "Search", making the tests more specific and reducing the risk of false positives. As a bonus, these queries also validate the accessibility of the input across multiple test scenarios.
Also applies to: 173-173, 203-203
212-214: LGTM! Formatting improvement.The multi-line formatting improves readability without changing behavior.
__tests__/components/common/FallbackImage.test.tsx (1)
1-8: LGTM! Test setup follows best practices.The imports and cleanup setup are correct, following the coding guidelines for using
@testing-library/reactin component tests.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/auth/SeizeConnectContext.tsx (1)
161-200: Address normalization inconsistency persists (Capacitor vs standard path).The Capacitor branch (line 169) returns a lowercased address while the standard branch (line 178) returns an EIP-55 checksummed address via
getAddress(). This mismatch can lead to comparison failures when the same address is used across Capacitor and non-Capacitor contexts.As noted in the previous review, apply this fix to unify the format:
if (isCapacitorPlatform()) { if (storedAddress.startsWith("0x") && storedAddress.length === 42) { return { isValid: true, - normalizedAddress: storedAddress.toLowerCase(), + normalizedAddress: getAddress(storedAddress), }; } }
🧹 Nitpick comments (1)
__tests__/components/common/FallbackImage.test.tsx (1)
10-34: Previous feedback addressed correctly.The test now spies on
console.errorinstead ofconsole.log(line 11) and uses the idiomatictoHaveAttributematcher (lines 24, 29), as suggested in the previous review. The test effectively validates the fallback mechanism, the single invocation ofonPrimaryError, and the absence of console logging.Optional: Consider testing primarySrc change behavior.
The component's
useEffectresets state whenprimarySrcchanges. Adding a test that renders with oneprimarySrc, triggers fallback, then updatesprimarySrcand verifies the reset would provide additional confidence in the retry mechanism.Example test structure:
it("resets to primary when primarySrc changes after fallback", async () => { const { rerender } = render( <FallbackImage primarySrc="primary1.gif" fallbackSrc="fallback.gif" alt="test" /> ); const image = screen.getByRole("img", { name: "test" }); fireEvent.error(image); await waitFor(() => expect(image).toHaveAttribute("src", "fallback.gif")); rerender( <FallbackImage primarySrc="primary2.gif" fallbackSrc="fallback.gif" alt="test" /> ); expect(image).toHaveAttribute("src", "primary2.gif"); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
__tests__/components/common/FallbackImage.test.tsx(1 hunks)components/auth/SeizeConnectContext.tsx(22 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript for implementation code
Files:
__tests__/components/common/FallbackImage.test.tsxcomponents/auth/SeizeConnectContext.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
__tests__/components/common/FallbackImage.test.tsxcomponents/auth/SeizeConnectContext.tsx
**/{__tests__/**/*.{ts,tsx},*.test.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/{__tests__/**/*.{ts,tsx},*.test.tsx}: Place tests in tests directories or alongside components as ComponentName.test.tsx
Mock external dependencies and APIs in tests
Files:
__tests__/components/common/FallbackImage.test.tsx
__tests__/**
📄 CodeRabbit inference engine (tests/AGENTS.md)
Place Jest test suites under the
__tests__directory mirroring source folders (e.g., components, contexts, hooks, utils)
Files:
__tests__/components/common/FallbackImage.test.tsx
__tests__/components/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (tests/AGENTS.md)
Use
@testing-library/reactand@testing-library/user-eventfor React component tests
Files:
__tests__/components/common/FallbackImage.test.tsx
🧬 Code graph analysis (2)
__tests__/components/common/FallbackImage.test.tsx (1)
components/common/FallbackImage.tsx (1)
FallbackImage(13-52)
components/auth/SeizeConnectContext.tsx (2)
src/errors/wallet.ts (2)
WalletInitializationError(16-26)WalletConnectionError(1-7)src/utils/security-logger.ts (4)
logSecurityEvent(96-116)createValidationEventContext(197-210)logError(125-178)createConnectionEventContext(184-191)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
__tests__/components/common/FallbackImage.test.tsx (2)
6-8: LGTM!The
afterEachcleanup ensures test isolation by restoring all mocks between tests, preventing cross-test pollution.
36-61: LGTM!The test correctly validates the error cascade:
onPrimaryErroris called once when the primary fails, thenonErroris called when the fallback also fails. The assertions confirm each callback is invoked exactly once, ensuring proper error handling at both levels.components/auth/SeizeConnectContext.tsx (6)
1-59: LGTM! Clean error class definitions and organized imports.The consolidated imports and standardized error classes with double-quoted strings align well with the codebase conventions. The error classes properly extend Error and set prototypes for instanceof checks.
61-155: LGTM! Well-structured type definitions.The unified state machine types (
"initializing" | "disconnected" | "connecting" | "connected" | "error") and theWalletStatediscriminated union provide clear state transitions. TheAddressValidationResultinterface properly captures validation outcomes.
203-253: LGTM! Solid state machine and error handling.The discriminated union
WalletStatetype enables type-safe state transitions. ThehandleInitializationErrorfunction properly logs security events, clears invalid auth state, and creates descriptive error objects.
256-337: LGTM! Excellent state consolidation.The unified state hook eliminates scattered state variables and provides clear state transitions. The initialization effect properly validates stored addresses and handles both valid and invalid cases. Computed properties maintain backward compatibility.
360-437: LGTM! Robust debounced wallet reconciliation.The debounced synchronization effect (50ms delay) prevents race conditions from rapid account changes. Address validation using
isAddress()and normalization viagetAddress()ensures consistent checksummed format. The checks forisAlreadyConnectedavoid unnecessary state updates.
535-571: LGTM! Thorough validation in seizeAcceptConnection.The function properly validates the address format, logs security events (both failures and successes), normalizes to checksummed format via
getAddress(), and throws descriptive errors for invalid addresses.
Signed-off-by: prxt6529 <prxt@6529.io>
|
2 participants
Summary by CodeRabbit
New Features
Accessibility
Refactor
Documentation
Tests
Chores