Conversation
|
Warning Rate limit exceeded@simo6529 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 32 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 (8)
WalkthroughClears legacy ESLint configs, adds a new flat ESLint config, upgrades Next.js/React and related deps, enables reactCompiler in Next config, introduces getNodeEnv and uses it in auth dev-bypass, renames middleware export to proxy, adds ticketing entries for Next 16 upgrade, and updates multiple tests to match behavioral changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Auth as Auth validation
participant Env as config/getNodeEnv
participant PublicEnv as publicEnv
participant Proc as process.env
Note over Auth,Env: Dev-auth bypass check (simplified)
Auth->>Env: call getNodeEnv()
Env->>PublicEnv: if publicEnv.NODE_ENV present -> return it
Env-->>Auth: return NODE_ENV (or undefined)
alt nodeEnv is "development" or "test" and USE_DEV_AUTH === "true"
Auth->>Auth: return immediate dev-auth success (bypass)
Note right of Auth: short-circuited flow (dev-like)
else
Auth->>Auth: proceed with normal validation flow
Note right of Auth: full validation (network/db/etc)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
proxy.tsx (1)
279-315: Middleware wiring is broken — proxy function will not executeVerification confirms proxy.tsx exists at the root but is not wired into Next.js middleware. Next.js requires a root
middleware.tsthat exports a function namedmiddleware; renaming toproxywithout such a root file means your middleware never runs.Choose one:
- Rename the export back to
middleware:-export async function proxy(req: NextRequest) { +export async function middleware(req: NextRequest): Promise<NextResponse> {
- Or add a root
middleware.ts(recommended):// middleware.ts (at repo root) export { proxy as middleware } from "./proxy";Also rename this file to
proxy.ts(no JSX present).
🧹 Nitpick comments (3)
config/env.ts (1)
9-17: Clarify getNodeEnv semantics; avoid “unknown → non‑prod” pitfallsReturning undefined makes downstream checks like getNodeEnv() !== "production" truthy, which can silently enable non‑prod behavior. Either:
- Narrow the return type to explicit literals, and/or
- Document that callers must explicitly allow-list dev/test.
Optional hardening in this file (safe but not required if call sites change):
-export const getNodeEnv = (): string | undefined => { +export const getNodeEnv = (): + | "development" + | "production" + | "test" + | undefined => { if (publicEnv.NODE_ENV) { return publicEnv.NODE_ENV; } if (typeof process === "undefined") { - return undefined; + return undefined; // keep unknown explicit; callers must not treat unknown as non-prod } return process.env.NODE_ENV; };codex/STATE.md (1)
22-22: Add PR link for traceabilityPopulate PRs with this PR: #1570.
codex/tickets/TKT-0016.md (1)
28-29: Link this PR and keep Acceptance in sync with CI
- Set Primary PR to #1570.
- Flip Acceptance checkboxes after test/lint/type-check succeed.
📜 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 (11)
.eslintignore(0 hunks).eslintrc.json(0 hunks)codex/STATE.md(1 hunks)codex/tickets/TKT-0016.md(1 hunks)config/env.ts(1 hunks)eslint.config.mjs(1 hunks)next.config.mjs(1 hunks)package.json(5 hunks)proxy.tsx(1 hunks)services/auth/immediate-validation.utils.ts(2 hunks)tsconfig.json(2 hunks)
💤 Files with no reviewable changes (2)
- .eslintignore
- .eslintrc.json
🧰 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 across the codebase
Files:
proxy.tsxservices/auth/immediate-validation.utils.tsconfig/env.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
proxy.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: 6529-Collections/6529seize-frontend#0
File: .cursorrules:0-0
Timestamp: 2025-09-28T12:29:11.651Z
Learning: Applies to {app,pages}/**/*.{ts,tsx} : Use NextJS features that match the current version
🧬 Code graph analysis (2)
services/auth/immediate-validation.utils.ts (1)
config/env.ts (1)
getNodeEnv(9-17)
config/env.ts (1)
next.config.mjs (2)
publicEnv(177-177)publicEnv(235-235)
⏰ 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 (10)
services/auth/immediate-validation.utils.ts (1)
10-10: LGTM: centralizing env detectionImporting getNodeEnv improves consistency across runtimes.
tsconfig.json (2)
19-19: LGTM: JSX transform updated for React 19.The change from
"preserve"to"react-jsx"is correct for React 19.2, which requires the modern JSX transform. This ensures TypeScript emits the new JSX runtime calls instead ofReact.createElement.
39-40: LGTM: Turbopack dev types support added.Adding
.next/dev/types/**/*.tsaligns with Next.js 16's turbopack development mode, which generates type definitions in this directory. This ensures full type safety when using turbopack for development.eslint.config.mjs (2)
1-75: LGTM: ESLint flat config migration complete.The migration from legacy
.eslintrc.jsonto the new flat config format (eslint.config.mjs) is well-structured and aligns with ESLint 9's requirement for flat config. The configuration properly:
- Extends Next.js core web vitals rules
- Registers necessary plugins (unused-imports, react-hooks, typescript-eslint)
- Maintains appropriate ignore patterns
69-74: ESLint rule is properly configured; no existing violations found.The codebase is compliant with the new
process.envrestriction. All matches found are in test files under__tests__/, which are globally ignored by ESLint and exempt from this rule. The designated environment handler (config/env.ts) is correctly excluded from the rule. No production code violations exist.package.json (4)
93-93: LGTM: Coordinated framework upgrade to Next.js 16 and React 19.2.The upgrade path is correctly coordinated:
- Next.js 16.0.0 supports React 19
- React 19.2 includes stable features and performance improvements
- Corresponding type definitions updated to match (lines 137-138)
Also applies to: 98-98, 101-101
143-143: LGTM: React Compiler plugin added.The
babel-plugin-react-compilerpackage is required for thereactCompiler: trueflag innext.config.mjs(line 105). This enables automatic optimization of React components at compile time.
175-176: LGTM: Type overrides ensure consistency.Adding
@types/reactand@types/react-domto the overrides section ensures all dependencies use the same React type definitions (19.2.2), preventing type conflicts across the dependency tree.
146-148: Verify ESLint 9 migration is complete.ESLint 9 requires flat config format and has breaking changes from v8. The new
eslint.config.mjsfile addresses this, but comprehensive testing is needed.Run a full lint check to ensure the migration is successful:
next.config.mjs (1)
101-152: ****The review comment claims that
eslint: { ignoreDuringBuilds: true }was removed fromnext.config.mjs, but there is no evidence of this in the git history. A search across all commits for "ignoreDuringBuilds" yields no results. The currentnext.config.mjscontains no ESLint configuration (neither in the diff nor in the base state). ESLint is properly configured in the separateeslint.config.mjsfile using modern flat config format, which is the standard approach in contemporary Next.js projects. This review comment should be disregarded.Likely an incorrect or invalid review comment.
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)
__tests__/components/utils/input/identity/IdentitySearch.test.tsx (1)
31-35: UseuserEventfor click interaction.Consistent with the previous test, use
userEvent.click()instead offireEvent.click()per coding guidelines.As per coding guidelines
Apply this diff:
- it('clears identity when clear button clicked', () => { + it('clears identity when clear button clicked', async () => { + const user = userEvent.setup(); render(<IdentitySearch identity="bob" setIdentity={setIdentity} />); - fireEvent.click(screen.getByLabelText('Clear identity')); + await user.click(screen.getByLabelText('Clear identity')); expect(setIdentity).toHaveBeenCalledWith(null); });
🧹 Nitpick comments (11)
codex/tickets/TKT-0016.md (1)
4-4: Consider updating the owner field to a real team member.The owner is listed as "openai-assistant," which is atypical for a production tracking ticket. Update this to the actual person responsible for the upgrade or decision-making.
__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx (1)
8-24: Consider simplifying the conditional click.The test correctly adapts to the component's DOM structure. Using
container.querySelectoris acceptable here since the visual item hasaria-hidden="true"in the production code, making it inaccessible to screen reader queries.However, line 23's conditional click
visualItem && fireEvent.click(visualItem)is redundant given that line 19 already assertsvisualItemis not null.Apply this diff to simplify:
- visualItem && fireEvent.click(visualItem); + fireEvent.click(visualItem!);Or alternatively:
- visualItem && fireEvent.click(visualItem); + if (!visualItem) throw new Error("visualItem not found"); + fireEvent.click(visualItem);__tests__/components/utils/input/identity/IdentitySearch.test.tsx (1)
6-6: Consider adding proper typing forreceivedProps.Using
anytype loses TypeScript's type safety benefits. Consider defining an interface or type that matches the expected props structure ofCommonProfileSearchItems.As per coding guidelines
__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx (6)
67-76: DRY up query client test helpercreateQueryClientMock is duplicated across tests. Extract to a shared test util (e.g., tests/utils/createQueryClientMock.ts) and import here and in controller tests. Optionally add invalidateQueries/refetchQueries for future uses.
Also applies to: 78-78
142-149: Also switch this test to user-eventMirror the change above for the auth-failure path.
- fireEvent.click(screen.getByRole('button', { name: /Group options/i })); - fireEvent.click(screen.getByText('Change group')); + const user = userEvent.setup(); + await user.click(screen.getByRole('button', { name: /Group options/i })); + await user.click(screen.getByText('Change group'));
166-175: And here for the “no group linked” caseUse user-event for opening the menu.
- fireEvent.click(screen.getByRole('button', { name: /Group options/i })); + const user = userEvent.setup(); + await user.click(screen.getByRole('button', { name: /Group options/i }));Also applies to: 171-172
177-194: Centralize HeadlessUI mockGood lightweight mock. Consider moving to a shared mocks file to auto-apply and reduce duplication.
Based on learnings
2-2: ReplacefireEventwithuserEventfor realistic click interactionsPer testing best practices and project guidelines, use
@testing-library/user-eventinstead offireEvent.click()for simulating realistic user interactions.-import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event';In test cases (lines 136–137, 145–146, 152, 171), replace synchronous
fireEvent.click()with asyncuserEvent:- render(<WaveGroupEditButtons haveGroup wave={wave} type={WaveGroupType.VIEW} />, { wrapper }); - fireEvent.click(screen.getByRole('button', { name: /Group options/i })); - fireEvent.click(screen.getByText('Change group')); + render(<WaveGroupEditButtons haveGroup wave={wave} type={WaveGroupType.VIEW} />, { wrapper }); + const user = userEvent.setup(); + await user.click(screen.getByRole('button', { name: /Group options/i })); + await user.click(screen.getByText('Change group'));Apply the same pattern to other click interactions in this file.
124-132: Stabilize useQuery mock for future-proofingThe test's empty mock
{}currently works since the hook doesn't destructure this particular useQuery call. However, to prevent issues if the code evolves to use destructured fields (data, status, etc.), return a minimal react-query v5-like shape:- (useQuery as jest.Mock).mockReturnValue({}); + (useQuery as jest.Mock).mockReturnValue({ + data: undefined, + status: 'success', + isPending: false, + isLoading: false, + isFetching: false, + });__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx (2)
43-52: Share createQueryClientMock across testsSame helper appears in multiple files. Extract to tests/utils and reuse.
122-143: Augment useMutation mock with mutate for paritySome codepaths may call mutate (sync signature). Add a simple delegate to mutateAsync to future-proof tests.
- (useMutation as jest.Mock).mockImplementation((options: any) => ({ - mutateAsync: async (params?: any) => { + (useMutation as jest.Mock).mockImplementation((options: any) => { + const run = async (params?: any) => { try { const result = await options.mutationFn(params); options.onSuccess?.(result, params, undefined); options.onSettled?.(result, undefined, params, undefined); mutateAsyncSpy(params); return result; } catch (error) { options.onError?.(error, params, undefined); options.onSettled?.(undefined, error, params, undefined); mutateAsyncSpy(params); throw error; } - }, - })); + }; + return { + mutateAsync: run, + mutate: (params?: any) => { void run(params); }, + }; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
__tests__/components/common/TabToggleWithOverflow.test.tsx(1 hunks)__tests__/components/utils/input/identity/IdentitySearch.test.tsx(1 hunks)__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx(1 hunks)__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx(1 hunks)__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx(1 hunks)__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx(3 hunks)__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx(10 hunks)codex/tickets/TKT-0016.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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 across the codebase
Files:
__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.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/TabToggleWithOverflow.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.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/TabToggleWithOverflow.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx
**/__tests__/**
📄 CodeRabbit inference engine (AGENTS.md)
Place tests in
__tests__directories when organizing test suites
Files:
__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx
**/*.test.tsx
📄 CodeRabbit inference engine (AGENTS.md)
When colocating tests with components, name them
ComponentName.test.tsx
Files:
__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Mock external dependencies and APIs in tests
Files:
__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-23T06:36:34.125Z
Learnt from: CR
PR: 6529-Collections/6529seize-frontend#0
File: AGENTS.md:0-0
Timestamp: 2025-10-23T06:36:34.125Z
Learning: Applies to **/*.test.{ts,tsx} : Mock external dependencies and APIs in tests
Applied to files:
__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx
🧬 Code graph analysis (3)
__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx (1)
components/utils/input/profile-search/CommonProfileSearchItems.tsx (1)
CommonProfileSearchItems(7-159)
__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx (1)
components/utils/select/dropdown/CommonDropdownItem.tsx (1)
CommonDropdownItem(10-79)
__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx (1)
components/utils/input/profile-search/CommonProfileSearchItem.tsx (1)
CommonProfileSearchItem(7-88)
⏰ 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 (15)
codex/tickets/TKT-0016.md (2)
6-6: Review comment is accurate; developer should verify completion state aligns with project workflow.The reviewer's factual claims are correct:
- Plan section shows exactly 2 of 3 items marked complete
- The final log entry (2025-10-28T06:01:42Z) explicitly confirms that
npm run test,npm run lint, andnpm run type-checkall passThe suggestion to update the status and complete the final Plan item is reasonable given the validation tests now pass. However, whether the ticket should transition to "Done" or "Ready-to-merge" depends on your project's workflow—e.g., whether PR review, merge status, or documentation review must occur first. Verify that the Acceptance criteria are also satisfied before closing the ticket.
20-24: Update Acceptance criteria checkboxes and ticket status to reflect completion confirmed in Log.Acceptance criteria items 1–2 are confirmed complete per the final Log entry (2025-10-28T06:01:42Z): dependencies upgraded to Next.js 16, app boots, and
npm run test,npm run lint, andnpm run type-checkall pass. Update lines 20–21 from[ ]to[x]. For line 22 (documentation), verify that the upgrade outcome and follow-ups are documented, then update accordingly. Also update the ticket status from "In-Progress" to "Done" or similar once all acceptance criteria are resolved.__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx (1)
12-27: LGTM! Proper use of ARIA roles.The test updates correctly align with the production code where the button element has
role="menuitem". Using the menuitem role selector improves test accuracy and accessibility verification. Caching the menuitem element on line 23 also improves readability.__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx (1)
23-35: LGTM! More specific selectors.Adding the
{ selector: 'li' }option makes these queries more precise and prevents false positives if the text appears elsewhere in the document. This aligns with the production code where these messages are rendered inside<li>elements.__tests__/components/utils/input/identity/IdentitySearch.test.tsx (2)
22-22: Role change correctly reflects component update.The change from
textboxtocomboboxrole appropriately adapts the test to match the updated IdentitySearch component's ARIA semantics.
25-25: Verify the input value change is intentional.The simulated input changed from
'a'to'abc'. Please confirm whether this change is:
- Required by a minimum character length in the component
- Simply a more realistic test value
- Related to the combobox role change
__tests__/components/common/TabToggleWithOverflow.test.tsx (2)
95-96: Good improvements to keyboard interaction testing.The changes to use the Space key and
findByRoleare solid improvements:
- Space is the standard key for button activation per ARIA best practices
findByRoleproperly waits for async UI updates, making the test more robust than synchronousgetByRoleAlso applies to: 104-105
93-95: Button is keyboard-accessible via Headlessui MenuButton; manual focus approach is acceptable but incomplete.The "More tabs" button uses Headlessui's
MenuButton, which is keyboard-accessible by default and included in the natural tab order. The manualmoreButton.focus()at line 93 is a valid test approach that correctly verifies Space key activation works.However, this approach doesn't verify the complete keyboard navigation flow—i.e., that pressing Tab after the second visible tab actually reaches the button. The test would be more comprehensive if it used
await user.tab()instead to confirm the button is reachable via natural keyboard navigation. The current implementation is functional but could be improved for better test coverage of keyboard accessibility.__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx (3)
7-17: React Query module mock looks goodKeeps actual exports and overrides hooks cleanly. No issues.
19-32: Forward-ref mock for EditButton is correctImperative handle open wired to onWaveUpdate; aligns with new API.
34-47: Forward-ref mock for RemoveButton is correctSame pattern as Edit; good coverage for renderTrigger null/defined paths.
__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx (4)
8-22: React Query hook mocks are well-isolatedPattern matches the component test; consistent with v5.
154-167: Endpoint mocks cover all codepathsGood: base group, identity lists, and new-group endpoint are handled; avoids network.
239-259: Validates wave update side-effects correctlyAsserts POST with visibility.scope.group_id and publish payload; solid coverage.
310-347: Good fallback behavior testCovers identity group fetch failure with empty lists; keeps side-effects constrained.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codex/tickets/TKT-0016.md(1 hunks)services/auth/immediate-validation.utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/auth/immediate-validation.utils.ts
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
codex/tickets/TKT-0016.md
28-28: Bare URL used
(MD034, no-bare-urls)
⏰ 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)
|
2 participants
Summary by CodeRabbit
Chores
Tests