Open
Conversation
Contributor
|
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:
Summary by CodeRabbitRelease Notes
WalkthroughAdds many unit tests for null/undefined/edge cases across frontend components and pages; adjusts runtime control flow in several components (removed early-return guards, switched falsy/nullish checks, added non-null assertions), changes ChapterMap zoom-control replacement and makes SocialLinks non-exported. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
frontend/__tests__/unit/pages/EditModule.test.tsx (1)
188-188:⚠️ Potential issue | 🟡 MinorStale test name — update to match the actual assertion.
The description
'shows loading spinner when query returns an error'is a leftover from before the error-path refactor; the test body no longer checks for a spinner and instead asserts'Access Denied'.🐛 Proposed fix
- it('shows loading spinner when query returns an error', async () => { + it('shows Access Denied when query returns an error', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/EditModule.test.tsx` at line 188, The test case name is stale: the it(...) titled 'shows loading spinner when query returns an error' no longer matches the assertion (which checks for 'Access Denied'); update the test description string to reflect the actual expectation (e.g., 'shows Access Denied when query returns an error' or similar) so the test name matches the assertion in EditModule.test.tsx (locate the it(...) block around the assertion that looks for 'Access Denied').frontend/__tests__/unit/components/ModuleForm.test.tsx (4)
657-687:⚠️ Potential issue | 🟡 MinorTest name is misleading — the Set-based selection path is never verified
After clicking
autocomplete-select-item(which dispatchesonSelectionChange(new Set(['project-1']))), the only assertion is:await waitFor(() => { expect(mockOnProjectChange).toHaveBeenCalledWith(null, 'Test') })This only confirms the earlier
handleInputChangeside-effect (typing "Test"). The actual purpose of the test — verifying thathandleSelectionChangeextracts the ID from theSetand callsonProjectChange('project-1', 'Test Project 1')— is never asserted. Add the missing expectation:🛠️ Proposed fix
- // Verify that typing triggers onProjectChange with the search term - await waitFor(() => { - expect(mockOnProjectChange).toHaveBeenCalledWith(null, 'Test') - }) + // Verify that typing triggers onProjectChange with the search term + await waitFor(() => { + expect(mockOnProjectChange).toHaveBeenCalledWith(null, 'Test') + }) + // Verify that Set-based selection resolves to the correct project + expect(mockOnProjectChange).toHaveBeenCalledWith('project-1', 'Test Project 1')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 657 - 687, The test currently only asserts the input change side-effect and never verifies the Set-based selection path; update the test around the click on the element with test id "autocomplete-select-item" (which triggers onSelectionChange(new Set(['project-1'])) and is handled by handleSelectionChange) to also assert that mockOnProjectChange was called with the selected project ID and name (expect(mockOnProjectChange).toHaveBeenCalledWith('project-1', 'Test Project 1')), using waitFor after the click to account for async handling.
657-687:⚠️ Potential issue | 🟡 Minor"handles selection via Set" only asserts the typing side-effect, not the Set selection itself
After clicking
autocomplete-select-item(which invokesonSelectionChange(new Set(['project-1']))), the sole assertion is:expect(mockOnProjectChange).toHaveBeenCalledWith(null, 'Test')This only confirms the earlier
handleInputChangecall from typing "Test". The actualhandleSelectionChangepath for aSet— which should ultimately callonProjectChange('project-1', 'Test Project 1')— is never verified. Add the missing expectation after thewaitFor:🛠️ Proposed fix
await waitFor(() => { expect(mockOnProjectChange).toHaveBeenCalledWith(null, 'Test') }) + + // Verify the Set-based selection resolved to the correct project + expect(mockOnProjectChange).toHaveBeenCalledWith('project-1', 'Test Project 1')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 657 - 687, The test "handles selection via Set" only asserts the typing side-effect and misses verifying the Set-selection path: after clicking the element with test id "autocomplete-select-item" (which triggers onSelectionChange(new Set(['project-1']))) add an assertion that mockOnProjectChange was called for the selection, e.g. expect(mockOnProjectChange).toHaveBeenCalledWith('project-1', 'Test Project 1'); place this check after the act/click and any waitFor so it validates the handleSelectionChange -> onProjectChange flow invoked by the selection; keep the existing typing assertions (renderProjectSelector, mockQuery, input change) intact.
40-85:⚠️ Potential issue | 🔴 CriticalCorrect the mock signature —
onSelectionChangeshould accept onlyReact.Key, notSet,'all', ornullHeroUI's Autocomplete
onSelectionChangecallback has the signature(key: React.Key) => void— it accepts a single key only, notnull,Set, or'all'.The mock incorrectly declares:
onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => voidThis type includes
Set<React.Key>and'all', which belong to the multi-selectSelectiontype used bySelectandDropdownMenu, notAutocomplete. Moreover, the mock invokesonSelectionChangewith invalid arguments:
- Line 68:
new Set(['project-1'])— invalid type- Line 75:
'all'— invalid type- Line 82:
null— invalid typeOnly line 89 (
'project-1'as a string) follows the correct API.🛠️ Proposed fix
- onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void + onSelectionChange?: (key: React.Key) => voidUpdate mock calls at lines 68, 75, and 82 to pass valid
React.Keyvalues instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 40 - 85, The mock's onSelectionChange type and invocations are wrong for Autocomplete: change the prop signature to onSelectionChange?: (key: React.Key) => void and update the three buttons that call onSelectionChange to pass single React.Key values (strings or numbers) instead of a Set, 'all', or null — e.g. use 'project-1', 'project-2', and 'project-none' (or similar single-key strings) for the handlers on the buttons with data-testids autocomplete-select-item, autocomplete-select-all, and autocomplete-clear so all calls and the signature match the Autocomplete API.
40-85:⚠️ Potential issue | 🟠 MajorType mismatch:
nullis not assignable to the declaredonSelectionChangesignatureThe mock's
onSelectionChangeprop is typed as:onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => voidbut the clear button at line 82 invokes it with
null:onClick={() => onSelectionChange?.(null)}
nullis not a member ofstring | number | Set<React.Key> | 'all'. This is a TypeScript compile error understrictNullChecks: true. The mock type must includenullto permit selection clearing.🛠️ Proposed fix
- onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void + onSelectionChange?: (key: React.Key | Set<React.Key> | 'all' | null) => void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 40 - 85, The mock prop onSelectionChange's type must allow null because the Clear Selection button (data-testid="autocomplete-clear") calls onSelectionChange(null); update the prop signature in the mock component from onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void to include null — e.g. onSelectionChange?: (key: React.Key | Set<React.Key> | 'all' | null) => void — and adjust any related typings/usages of onSelectionChange in the test mock to match.
🧹 Nitpick comments (12)
frontend/src/app/members/[memberKey]/page.tsx (1)
49-57:dates.at(-1)widensendDatetostring | undefinedunnecessarily
dates.at(-1)returnsT | undefined(ES2022 spec), soendDateis now typedstring | undefinedeven though thedates.length === 0early-return on lines 51-53 guarantees the array is non-empty by the time line 56 is reached. In practice this can never beundefined, but the widened type forces the extradateRange.endDate &&truthiness guard added at line 166.Using
dates[dates.length - 1]preserves thestringtype without any assertion:♻️ Proposed fix
- endDate: dates.at(-1), + endDate: dates[dates.length - 1],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/members/`[memberKey]/page.tsx around lines 49 - 57, The dateRange useMemo widens endDate to string | undefined due to dates.at(-1); update the endDate extraction in the useMemo (where dateRange is computed from contributionData inside the useMemo callback) to use dates[dates.length - 1] instead of dates.at(-1) so the non-empty-array check (dates.length === 0) guarantees a string endDate and removes the need for downstream truthiness guards.frontend/__tests__/unit/pages/SnapshotDetails.test.tsx (1)
380-403: Test names overstate what's being verified — Reactkeyprops are not observable from the DOM.Both tests claim to validate fallback
keyprop behavior ("uses fallback key","uses unknown fallback"), but React keys are purely an internal rendering hint and are never exposed in the DOM.@testing-library/reacthas no mechanism to assert which key was used. What both tests actually verify is that the component renders without crashing whenidand/orrepositoryNameare undefined, and the assertion in both cases is identical:expect(screen.getByText('v0.9.2')).toBeInTheDocument()Because the observable outcome is the same regardless of which fallback key is selected, the second test adds only marginal coverage over the first (it confirms no crash occurs with both fields undefined). Renaming to reflect the actual intent would improve readability:
🔤 Suggested renames
- test('renders release without id (uses fallback key)', async () => { + test('renders release without id without crashing', async () => {- test('renders release without id and repositoryName (uses unknown fallback)', async () => { + test('renders release without id and repositoryName without crashing', async () => {Also applies to: 405-429
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/SnapshotDetails.test.tsx` around lines 380 - 403, The two tests ('renders release without id (uses fallback key)' and the similar one at lines 405-429) incorrectly claim to assert React key fallback behavior, which isn't observable; update the test names to reflect the actual observable intent (e.g., "renders release without id" and "renders release without id and repositoryName") and/or strengthen them to assert real DOM-visible outcomes instead of key usage by keeping the render of <SnapshotDetailsPage /> and the existing assertion expect(screen.getByText('v0.9.2')).toBeInTheDocument(), or add additional DOM checks (e.g., repository text or other visible fields) if you need to distinguish the two cases; reference the test functions in SnapshotDetails.test.tsx that mock useQuery and render SnapshotDetailsPage to locate and rename/adjust the tests.frontend/__tests__/unit/pages/EditProgram.test.tsx (1)
482-482: Type cast inconsistency:as jest.Mockvs.as unknown as jest.Mock.Every other
useQuerycast in this file uses the double-cast form (as unknown as jest.Mock). The directas jest.Mockcast here may trigger a TypeScript error in strict mode if the compiler cannot find a structural overlap betweenuseQuery's overloaded type andjest.Mock. Align with the rest of the file:♻️ Proposed fix
- ;(useQuery as jest.Mock).mockImplementation(mockQueryFn) + ;(useQuery as unknown as jest.Mock).mockImplementation(mockQueryFn)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/EditProgram.test.tsx` at line 482, The test uses a direct cast "(useQuery as jest.Mock).mockImplementation(mockQueryFn)" which is inconsistent with the rest of the file and can fail in strict TypeScript; update the cast to match the other usages by changing the cast to "useQuery as unknown as jest.Mock" before calling mockImplementation so useQuery.mockImplementation(mockQueryFn) uses the double-cast form (refer to useQuery, mockImplementation, and mockQueryFn to locate the line).frontend/__tests__/unit/pages/EditModule.test.tsx (1)
136-186: Hidden test-order dependency:useMutationis not mocked in two tests.
jest.clearAllMocks()(used inafterEach) clears call history but notmockReturnValue/mockImplementation. The tests at lines 136 and 176 carry nouseMutationsetup and silently depend on the stale return value left by the first test. If either test is run in isolation (e.g., via.only) or Test 1 is removed/reordered,useMutation()returnsundefinedand the component crashes with a destructuring TypeError.Add a default
useMutationsetup tobeforeEachso every test is self-contained:♻️ Proposed fix
beforeEach(() => { jest.useFakeTimers() ;(useRouter as jest.Mock).mockReturnValue({ push: mockPush, replace: mockReplace }) ;(useParams as jest.Mock).mockReturnValue({ programKey: 'test-program', moduleKey: 'test-module', }) ;(useApolloClient as jest.Mock).mockReturnValue({ query: jest.fn().mockResolvedValue({ data: { searchProjects: [{ id: '123', name: 'Awesome Project' }], }, }), }) + ;(useMutation as unknown as jest.Mock).mockReturnValue([jest.fn(), { loading: false }]) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/EditModule.test.tsx` around lines 136 - 186, Tests have a hidden order dependency because useMutation is not mocked in every test, causing it to be undefined when tests run in isolation; add a default mock for useMutation in the test suite setup (e.g., in beforeEach) that returns the same shape the component expects (a tuple of [mutateFn, resultObj] such as [jest.fn(), { loading: false, data: null, error: null }]) so destructuring in the component never throws, and allow individual tests to override that mockReturnValue where needed; keep afterEach jest.clearAllMocks() as-is.frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
222-244:jest.clearAllMocks()mid-test and test title don't fully match the intent.Two things worth noting:
jest.clearAllMocks()at line 237 resets all mock call counts (not just zoom-control ones), making the test state harder to reason about. Splitting into twoitblocks — one for first activation and one for re-activation — would keep each assertion self-contained and avoid the silent dependency on what state was established in the first half.The test title says "removes existing zoom control before adding a new one on re-activation", but in this flow the deactivation branch already sets
zoomControlRef.current = nullbefore the second click. So line 29 (zoomControlRef.current?.remove()) is a no-op at the time of re-activation — the specific guard being tested by that production change is never exercised with a non-null ref. The test does verify the correct lifecycle end-to-end, but the title implies coverage that isn't quite there.♻️ Optional: split into two focused tests
- it('removes existing zoom control before adding a new one on re-activation', async () => { - const { getByText, container } = render(<ChapterMap {...defaultProps} />) - - fireEvent.click(getByText('Unlock map').closest('button')) - await waitFor(() => { - expect(L.control.zoom).toHaveBeenCalledTimes(1) - expect(mockZoomControl.addTo).toHaveBeenCalledTimes(1) - }) - - fireEvent.mouseLeave(container.firstChild as HTMLElement) - await waitFor(() => { - expect(mockZoomControl.remove).toHaveBeenCalled() - expect(getByText('Unlock map')).toBeInTheDocument() - }) - - jest.clearAllMocks() - - fireEvent.click(getByText('Unlock map').closest('button')) - await waitFor(() => { - expect(L.control.zoom).toHaveBeenCalledTimes(1) - expect(mockZoomControl.addTo).toHaveBeenCalledTimes(1) - }) - }) + it('creates zoom control on first activation', async () => { + const { getByText } = render(<ChapterMap {...defaultProps} />) + + fireEvent.click(getByText('Unlock map').closest('button')) + await waitFor(() => { + expect(L.control.zoom).toHaveBeenCalledTimes(1) + expect(mockZoomControl.addTo).toHaveBeenCalledTimes(1) + }) + }) + + it('recreates zoom control on re-activation after lock', async () => { + const { getByText, container } = render(<ChapterMap {...defaultProps} />) + + fireEvent.click(getByText('Unlock map').closest('button')) + await waitFor(() => expect(L.control.zoom).toHaveBeenCalledTimes(1)) + + fireEvent.mouseLeave(container.firstChild as HTMLElement) + await waitFor(() => { + expect(mockZoomControl.remove).toHaveBeenCalled() + expect(getByText('Unlock map')).toBeInTheDocument() + }) + + // counts from first cycle are already validated; beforeEach will reset between tests + fireEvent.click(getByText('Unlock map').closest('button')) + await waitFor(() => { + expect(L.control.zoom).toHaveBeenCalledTimes(2) // cumulative: 1 first + 1 second + expect(mockZoomControl.addTo).toHaveBeenCalledTimes(2) + }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ChapterMap.test.tsx` around lines 222 - 244, The test mixes two scenarios and uses jest.clearAllMocks() mid-test which hides mock state and doesn't exercise the non-null guard on zoomControlRef.current; split into two focused tests (one "activates and deactivates zoom control" and a second "re-activates and removes existing control before adding new one"), or alternatively remove jest.clearAllMocks() and explicitly set zoomControlRef.current to a mock object before the second click so zoomControlRef.current?.remove() is exercised; refer to L.control.zoom, mockZoomControl, zoomControlRef.current, and the remove() call when making the change.frontend/__tests__/unit/components/ModuleForm.test.tsx (2)
399-421:jest.advanceTimersByTimeis a no-op —jest.useFakeTimers()is never called in theModuleFormblockThe
ModuleFormdescribe'sbeforeEach(lines 279–284) does not install fake timers, sojest.advanceTimersByTime(350)insideactat line 405 has no effect. The debounce runs on the real clock and the test passes only becausewaitFor's 1 000 ms default timeout exceeds the debounce delay — making these two tests slow and prone to flake under CI load.The sibling
ProjectSelectorblock already shows the correct pattern; apply it here too:🛠️ Proposed fix
beforeEach(() => { jest.clearAllMocks() + jest.useFakeTimers() mockQuery.mockResolvedValue({ data: { searchProjects: [{ id: 'project-1', name: 'Test Project' }] }, }) }) + + afterEach(() => { + jest.useRealTimers() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 399 - 421, The ModuleForm test uses jest.advanceTimersByTime(350) but never installs fake timers; in the ModuleForm describe block add jest.useFakeTimers() in its beforeEach (the same place renderModuleForm is prepared) and add jest.useRealTimers() in afterEach to restore timers; ensure the advanceTimers call is wrapped in act (as it already is) and that any pending timers are flushed (e.g., runOnlyPendingTimers or advanceTimersByTime) so the debounce in ProjectSelector triggers deterministically when calling renderModuleForm and firing change/click events.
399-421:jest.advanceTimersByTimeis a no-op here — fake timers are never installed in theModuleFormblockThe
ModuleFormdescribe'sbeforeEach(lines 279–284) only callsjest.clearAllMocks()and sets upmockQuery; it never callsjest.useFakeTimers(). Insideact,jest.advanceTimersByTime(350)therefore has no effect, and the debounce insideProjectSelectorfires on real time. The test passes only becausewaitFor's default 1 000 ms timeout outlasts the ~300 ms debounce — this is fragile and will be unreliable under CI load.Add
jest.useFakeTimers()/jest.useRealTimers()to theModuleFormdescribe'sbeforeEach/afterEach:🛠️ Proposed fix
beforeEach(() => { jest.clearAllMocks() + jest.useFakeTimers() mockQuery.mockResolvedValue({ data: { searchProjects: [{ id: 'project-1', name: 'Test Project' }] }, }) }) + + afterEach(() => { + jest.useRealTimers() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 399 - 421, The test uses jest.advanceTimersByTime(350) but fake timers are never enabled in the ModuleForm describe; update the ModuleForm describe's setup to call jest.useFakeTimers() in its beforeEach and restore timers with jest.useRealTimers() in its afterEach so that the debounce in ProjectSelector responds to the timer advance inside the act block (the area around the existing beforeEach/afterEach that sets up mockQuery and jest.clearAllMocks()); this ensures the jest.advanceTimersByTime(350) in the 'updates project field when ProjectSelector changes' test actually triggers the debounce rather than relying on real time.frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx (1)
655-678: Consider asserting the button's disabled state before clicking to make preconditions explicit.The test relies on
disabled={!issueId || assigning}preventing the click, but doesn't assert that precondition. Adding atoBeDisabled()check beforefireEvent.clickmakes the intent self-documenting and would catch regressions if the disabled condition were inadvertently removed.✅ Suggested assertion
const assignButton = within(userGrid as HTMLElement).getByRole('button', { name: /Assign/i }) + expect(assignButton).toBeDisabled() fireEvent.click(assignButton) expect(assignIssue).not.toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx` around lines 655 - 678, Add an explicit assertion that the Assign button is disabled before clicking to make the precondition clear and prevent false positives: in the test for ModuleIssueDetailsPage (where mockUseParams sets issueId to '' and mockUseIssueMutations returns assigning: false and assignIssue is a jest.fn), locate the retrieved Assign button (within the userGrid) and assert expect(assignButton).toBeDisabled() before fireEvent.click(assignButton); keep the existing expect(assignIssue).not.toHaveBeenCalled() afterwards to verify no mutation occurred.frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (1)
228-231: Removed onClick guards rely entirely ondisabled— consider retaining a guard on mutation-invoking handlers.Removing
if (!canEditDeadline) returnfrom the deadline onClick is safe here becauseisEditingDeadline && canEditDeadline(line 182) still gates the date input from appearing, and React does not dispatchonClickfordisabledbuttons. However, the same approach on the unassign (lines 300–307) and assign (lines 367–374) handlers removes the last programmatic barrier before an actual GraphQL mutation fires. If the button'sdisabledcondition were ever mis-set or bypassed (e.g., programmatically),Number(issueId)with an emptyissueIdevaluates to0— a silent, valid-looking integer that would be forwarded to the mutation with no error at the call site.For the deadline button, the removal is fine. For the two mutation handlers, a minimal guard preserves defense-in-depth at negligible cost:
🛡️ Suggested guard on mutation handlers
// Unassign button onClick onClick={async () => { + if (!issueId || unassigning) return await unassignIssue({ variables: { programKey, moduleKey, issueNumber: Number(issueId), userLogin: a.login } }) }} // Assign button onClick onClick={async () => { + if (!issueId || assigning) return await assignIssue({ variables: { programKey, moduleKey, issueNumber: Number(issueId), userLogin: u.login } }) }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx around lines 228 - 231, Add a defensive early-return to the assign and unassign click handlers: before calling the GraphQL mutation (the code that constructs Number(issueId) and invokes the assign/unassign mutation), check the same permission/guard used to disable the buttons (e.g., if (!canEditAssignees) return) and also validate issueId (e.g., if (!issueId) return) so you never call Number(issueId) or fire the mutation when the action is not allowed or issueId is empty; keep the deadline click behavior unchanged.frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
1617-1623: Split into twoitblocks for clarity.Two independent scenarios (undefined
topicsvs undefinedlanguages) are combined in a single test with a manualcleanup()between renders. When this test fails it's harder to identify which scenario is broken. Each scenario is self-contained and deserves its own test.♻️ Suggested split
- it('handles undefined topics/languages in grid layout', () => { - render(<CardDetailsPage {...defaultProps} languages={['JavaScript']} topics={undefined} />) - expect(screen.getByTestId('toggleable-list')).toHaveTextContent('Languages: JavaScript') - cleanup() - render(<CardDetailsPage {...defaultProps} languages={undefined} topics={['web']} />) - expect(screen.getByTestId('toggleable-list')).toHaveTextContent('Topics: web') - }) + it('renders only languages when topics is undefined', () => { + render(<CardDetailsPage {...defaultProps} languages={['JavaScript']} topics={undefined} />) + expect(screen.getByTestId('toggleable-list')).toHaveTextContent('Languages: JavaScript') + }) + + it('renders only topics when languages is undefined', () => { + render(<CardDetailsPage {...defaultProps} languages={undefined} topics={['web']} />) + expect(screen.getByTestId('toggleable-list')).toHaveTextContent('Topics: web') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx` around lines 1617 - 1623, Split the combined test "handles undefined topics/languages in grid layout" into two independent it blocks: one that renders CardDetailsPage with languages=['JavaScript'] and topics=undefined and asserts the toggleable-list contains "Languages: JavaScript", and a second that renders CardDetailsPage with languages=undefined and topics=['web'] and asserts the toggleable-list contains "Topics: web"; remove the manual cleanup() between renders so each test is self-contained and has a clear, descriptive name (e.g., "renders languages when topics undefined" and "renders topics when languages undefined") to make failures easier to diagnose.frontend/__tests__/unit/pages/UserDetails.test.tsx (2)
1031-1051: Test name "validates defensive check for endDate" is unclear.The name exposes an implementation detail rather than describing the observable behavior. A reader cannot tell what's being tested without reading the body.
💡 Suggested rename
- test('validates defensive check for endDate', async () => { + test('renders heatmap when contributionData contains a single entry', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/UserDetails.test.tsx` around lines 1031 - 1051, The test name "validates defensive check for endDate" leaks an implementation detail and is unclear; rename the test in UserDetails.test.tsx to describe observable behavior (e.g., "renders contribution heatmap when contribution data has a single date" or "shows heatmap even when contributionData contains only one date"), updating the test declaration that contains the mock singleDateContribution and the render of UserDetailsPage so it reads the new descriptive name while keeping the existing mock and assertions (references: the test block currently named 'validates defensive check for endDate', the singleDateContribution mock, and the render(<UserDetailsPage />) / expect(screen.getByTestId('contribution-heatmap')).toBeInTheDocument()).
1053-1075: Near-duplicate of existing tests at lines 895–917 and 942–963.The new test uses
login: 'fallback-login'(instead of the default'testuser') to show the fallback is dynamic, which is marginally useful — but the code path (name: null → use login) and both assertion types (avatar alt text, displayed text) are already exercised by the pair of existing tests. Consider removing this test or merging itsgetByTextassertion into one of the existing tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/UserDetails.test.tsx` around lines 1053 - 1075, This test ("uses user login as fallback for avatar alt text and title when name is missing") is a near-duplicate of existing tests that already cover the code path when user.name is null and assert avatar alt text and displayed login; remove this redundant test or merge its unique assertion (the getByText('fallback-login') check) into one of the existing tests that mock userWithoutName (reuse mockUserDetailsData and the useQuery mock) so the fallback-login dynamic case is still covered without adding a duplicate test case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx`:
- Around line 1625-1634: The test fails TypeScript because you assign value:
null while DetailsCardProps.details expects string | JSX.Element; update the
test to avoid the type error by either removing the explicit DetailsCardProps
annotation so TypeScript infers the literal (like other tests) or cast the null
value (e.g., value: null as unknown as any) when constructing
propsWithNullLeader used by CardDetailsPage; keep references to defaultProps and
CardDetailsPage intact so the test still renders and asserts "Leaders:" and
"Unknown".
In `@frontend/__tests__/unit/contexts/BreadcrumbContext.test.tsx`:
- Around line 54-58: The test calling registerBreadcrumbClassName('test') should
push its unregister callback into the shared cleanupFns so teardown runs even if
the assertion fails; update the test to capture the unregister from
registerBreadcrumbClassName('test') and call cleanupFns.push(unregister) (or
equivalent) instead of calling unregister() directly, ensuring the module-level
classNameStack is cleared in afterEach; locate registerBreadcrumbClassName and
the test in BreadcrumbContext.test.tsx and use the existing cleanupFns helper to
register the unregister function for deferred cleanup.
In `@frontend/__tests__/unit/pages/EditProgram.test.tsx`:
- Around line 458-491: The test currently relies on mockReturnValueOnce calls on
mockQueryFn which assumes an exact number of renders; instead implement
mockQueryFn as a closure with an internal call counter and useQuery mocked via
mockImplementation to call that closure so responses are deterministic: have the
closure return the validData response for the first N calls (e.g., 3) and then
return the nullAdminsData response thereafter. Update references to mockQueryFn
and useQuery in the test to use this closure-based mock so rerender()
transitions predictably regardless of React render counts.
In `@frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx`:
- Around line 545-561: The test MenteeProfilePage.test.tsx currently spies on
React.useState and calls jest.restoreAllMocks() inline, which can leak the spy
if assertions throw; change the test to guarantee cleanup by either moving spy
restoration into an afterEach that calls jest.restoreAllMocks() (or at minimum
wrap the render/assert block in a try/finally that calls
jest.restoreAllMocks()), and replace the fragile React.useState interception
with a safer approach: drive the invalid filter scenario through the existing
Select testing pattern (use the select-trigger-empty helper or add a hidden
trigger that calls onSelectionChange(new Set(['invalid_filter']))), so you no
longer need to spy on React.useState and avoid hook-count/side-effect issues.
In `@frontend/__tests__/unit/pages/MyMentorship.test.tsx`:
- Around line 449-455: The test is enabling jest.useFakeTimers() mid-test which
leaves a real timer queued; move jest.useFakeTimers() to run before the
component is rendered (matching the "updates debounced search query" test) and
remove the duplicate fireEvent.change so only one change is dispatched under
fake timers, then advance the fake timer with jest.advanceTimersByTime(500)
inside React.act; update any setup/teardown to restore timers if present.
In `@frontend/src/app/my/mentorship/programs/`[programKey]/edit/page.tsx:
- Line 121: Restore the runtime fallback for the program status before sending
the GraphQL mutation: ensure the value passed as status uses formData.status ??
ProgramStatusEnum.Draft rather than only casting with as ProgramStatusEnum so
the mutation (UpdateProgramInput) never receives undefined; update the
assignment where status is built (the line currently using status:
formData.status as ProgramStatusEnum) to use the nullish fallback to
ProgramStatusEnum.Draft to provide a real runtime enum value.
In
`@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/issues/page.tsx:
- Around line 117-120: The onSelectionChange handler currently destructures an
empty Set into key and calls handleLabelChange(key as string), which lets
runtime undefined propagate into selectedLabel and URL; update the
onSelectionChange in the Select to check that the extracted key is
defined/truthy before calling handleLabelChange (e.g., const [key] =
Array.from(keys as Set<string>); if (!key) return; handleLabelChange(key)); this
restores the previous undefined guard so selectedLabel (used to build new
Set([selectedLabel])) and params.set('label', ...) never receive the literal
"undefined" string and GraphQL queries keep correct null semantics.
---
Outside diff comments:
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 657-687: The test currently only asserts the input change
side-effect and never verifies the Set-based selection path; update the test
around the click on the element with test id "autocomplete-select-item" (which
triggers onSelectionChange(new Set(['project-1'])) and is handled by
handleSelectionChange) to also assert that mockOnProjectChange was called with
the selected project ID and name
(expect(mockOnProjectChange).toHaveBeenCalledWith('project-1', 'Test Project
1')), using waitFor after the click to account for async handling.
- Around line 657-687: The test "handles selection via Set" only asserts the
typing side-effect and misses verifying the Set-selection path: after clicking
the element with test id "autocomplete-select-item" (which triggers
onSelectionChange(new Set(['project-1']))) add an assertion that
mockOnProjectChange was called for the selection, e.g.
expect(mockOnProjectChange).toHaveBeenCalledWith('project-1', 'Test Project 1');
place this check after the act/click and any waitFor so it validates the
handleSelectionChange -> onProjectChange flow invoked by the selection; keep the
existing typing assertions (renderProjectSelector, mockQuery, input change)
intact.
- Around line 40-85: The mock's onSelectionChange type and invocations are wrong
for Autocomplete: change the prop signature to onSelectionChange?: (key:
React.Key) => void and update the three buttons that call onSelectionChange to
pass single React.Key values (strings or numbers) instead of a Set, 'all', or
null — e.g. use 'project-1', 'project-2', and 'project-none' (or similar
single-key strings) for the handlers on the buttons with data-testids
autocomplete-select-item, autocomplete-select-all, and autocomplete-clear so all
calls and the signature match the Autocomplete API.
- Around line 40-85: The mock prop onSelectionChange's type must allow null
because the Clear Selection button (data-testid="autocomplete-clear") calls
onSelectionChange(null); update the prop signature in the mock component from
onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void to include
null — e.g. onSelectionChange?: (key: React.Key | Set<React.Key> | 'all' | null)
=> void — and adjust any related typings/usages of onSelectionChange in the test
mock to match.
In `@frontend/__tests__/unit/pages/EditModule.test.tsx`:
- Line 188: The test case name is stale: the it(...) titled 'shows loading
spinner when query returns an error' no longer matches the assertion (which
checks for 'Access Denied'); update the test description string to reflect the
actual expectation (e.g., 'shows Access Denied when query returns an error' or
similar) so the test name matches the assertion in EditModule.test.tsx (locate
the it(...) block around the assertion that looks for 'Access Denied').
---
Nitpick comments:
In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx`:
- Around line 1617-1623: Split the combined test "handles undefined
topics/languages in grid layout" into two independent it blocks: one that
renders CardDetailsPage with languages=['JavaScript'] and topics=undefined and
asserts the toggleable-list contains "Languages: JavaScript", and a second that
renders CardDetailsPage with languages=undefined and topics=['web'] and asserts
the toggleable-list contains "Topics: web"; remove the manual cleanup() between
renders so each test is self-contained and has a clear, descriptive name (e.g.,
"renders languages when topics undefined" and "renders topics when languages
undefined") to make failures easier to diagnose.
In `@frontend/__tests__/unit/components/ChapterMap.test.tsx`:
- Around line 222-244: The test mixes two scenarios and uses
jest.clearAllMocks() mid-test which hides mock state and doesn't exercise the
non-null guard on zoomControlRef.current; split into two focused tests (one
"activates and deactivates zoom control" and a second "re-activates and removes
existing control before adding new one"), or alternatively remove
jest.clearAllMocks() and explicitly set zoomControlRef.current to a mock object
before the second click so zoomControlRef.current?.remove() is exercised; refer
to L.control.zoom, mockZoomControl, zoomControlRef.current, and the remove()
call when making the change.
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 399-421: The ModuleForm test uses jest.advanceTimersByTime(350)
but never installs fake timers; in the ModuleForm describe block add
jest.useFakeTimers() in its beforeEach (the same place renderModuleForm is
prepared) and add jest.useRealTimers() in afterEach to restore timers; ensure
the advanceTimers call is wrapped in act (as it already is) and that any pending
timers are flushed (e.g., runOnlyPendingTimers or advanceTimersByTime) so the
debounce in ProjectSelector triggers deterministically when calling
renderModuleForm and firing change/click events.
- Around line 399-421: The test uses jest.advanceTimersByTime(350) but fake
timers are never enabled in the ModuleForm describe; update the ModuleForm
describe's setup to call jest.useFakeTimers() in its beforeEach and restore
timers with jest.useRealTimers() in its afterEach so that the debounce in
ProjectSelector responds to the timer advance inside the act block (the area
around the existing beforeEach/afterEach that sets up mockQuery and
jest.clearAllMocks()); this ensures the jest.advanceTimersByTime(350) in the
'updates project field when ProjectSelector changes' test actually triggers the
debounce rather than relying on real time.
In `@frontend/__tests__/unit/pages/EditModule.test.tsx`:
- Around line 136-186: Tests have a hidden order dependency because useMutation
is not mocked in every test, causing it to be undefined when tests run in
isolation; add a default mock for useMutation in the test suite setup (e.g., in
beforeEach) that returns the same shape the component expects (a tuple of
[mutateFn, resultObj] such as [jest.fn(), { loading: false, data: null, error:
null }]) so destructuring in the component never throws, and allow individual
tests to override that mockReturnValue where needed; keep afterEach
jest.clearAllMocks() as-is.
In `@frontend/__tests__/unit/pages/EditProgram.test.tsx`:
- Line 482: The test uses a direct cast "(useQuery as
jest.Mock).mockImplementation(mockQueryFn)" which is inconsistent with the rest
of the file and can fail in strict TypeScript; update the cast to match the
other usages by changing the cast to "useQuery as unknown as jest.Mock" before
calling mockImplementation so useQuery.mockImplementation(mockQueryFn) uses the
double-cast form (refer to useQuery, mockImplementation, and mockQueryFn to
locate the line).
In `@frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx`:
- Around line 655-678: Add an explicit assertion that the Assign button is
disabled before clicking to make the precondition clear and prevent false
positives: in the test for ModuleIssueDetailsPage (where mockUseParams sets
issueId to '' and mockUseIssueMutations returns assigning: false and assignIssue
is a jest.fn), locate the retrieved Assign button (within the userGrid) and
assert expect(assignButton).toBeDisabled() before fireEvent.click(assignButton);
keep the existing expect(assignIssue).not.toHaveBeenCalled() afterwards to
verify no mutation occurred.
In `@frontend/__tests__/unit/pages/SnapshotDetails.test.tsx`:
- Around line 380-403: The two tests ('renders release without id (uses fallback
key)' and the similar one at lines 405-429) incorrectly claim to assert React
key fallback behavior, which isn't observable; update the test names to reflect
the actual observable intent (e.g., "renders release without id" and "renders
release without id and repositoryName") and/or strengthen them to assert real
DOM-visible outcomes instead of key usage by keeping the render of
<SnapshotDetailsPage /> and the existing assertion
expect(screen.getByText('v0.9.2')).toBeInTheDocument(), or add additional DOM
checks (e.g., repository text or other visible fields) if you need to
distinguish the two cases; reference the test functions in
SnapshotDetails.test.tsx that mock useQuery and render SnapshotDetailsPage to
locate and rename/adjust the tests.
In `@frontend/__tests__/unit/pages/UserDetails.test.tsx`:
- Around line 1031-1051: The test name "validates defensive check for endDate"
leaks an implementation detail and is unclear; rename the test in
UserDetails.test.tsx to describe observable behavior (e.g., "renders
contribution heatmap when contribution data has a single date" or "shows heatmap
even when contributionData contains only one date"), updating the test
declaration that contains the mock singleDateContribution and the render of
UserDetailsPage so it reads the new descriptive name while keeping the existing
mock and assertions (references: the test block currently named 'validates
defensive check for endDate', the singleDateContribution mock, and the
render(<UserDetailsPage />) /
expect(screen.getByTestId('contribution-heatmap')).toBeInTheDocument()).
- Around line 1053-1075: This test ("uses user login as fallback for avatar alt
text and title when name is missing") is a near-duplicate of existing tests that
already cover the code path when user.name is null and assert avatar alt text
and displayed login; remove this redundant test or merge its unique assertion
(the getByText('fallback-login') check) into one of the existing tests that mock
userWithoutName (reuse mockUserDetailsData and the useQuery mock) so the
fallback-login dynamic case is still covered without adding a duplicate test
case.
In `@frontend/src/app/members/`[memberKey]/page.tsx:
- Around line 49-57: The dateRange useMemo widens endDate to string | undefined
due to dates.at(-1); update the endDate extraction in the useMemo (where
dateRange is computed from contributionData inside the useMemo callback) to use
dates[dates.length - 1] instead of dates.at(-1) so the non-empty-array check
(dates.length === 0) guarantees a string endDate and removes the need for
downstream truthiness guards.
In
`@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx:
- Around line 228-231: Add a defensive early-return to the assign and unassign
click handlers: before calling the GraphQL mutation (the code that constructs
Number(issueId) and invokes the assign/unassign mutation), check the same
permission/guard used to disable the buttons (e.g., if (!canEditAssignees)
return) and also validate issueId (e.g., if (!issueId) return) so you never call
Number(issueId) or fire the mutation when the action is not allowed or issueId
is empty; keep the deadline click behavior unchanged.
frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx
Outdated
Show resolved
Hide resolved
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 20 files
Confidence score: 3/5
- There’s some user-facing risk: removing the guard in
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsxcan lethandleLabelChangereceive undefined when the selection is cleared, which could break label changes. - Test hygiene has a mild risk:
frontend/__tests__/unit/pages/MenteeProfilePage.test.tsxmay leak a React.useState spy on failure, potentially affecting subsequent tests. - Pay close attention to
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx- empty selection handling could pass undefined tohandleLabelChange.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/pages/SnapshotDetails.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/SnapshotDetails.test.tsx:405">
P2: Test name claims to validate 'unknown fallback' but only asserts release version is rendered. The test setup passes both id and repositoryName as undefined, but the assertion only checks that 'v0.9.2' appears, which doesn't verify whether the fallback behavior for missing fields actually works.</violation>
</file>
<file name="frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx">
<violation number="1" location="frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx:119">
P1: Removing the guard allows `handleLabelChange` to receive undefined when selection is empty (e.g., user clicks the same option twice). HeroUI Select's `disallowEmptySelection` defaults to `false`, allowing empty selections. This results in `label=undefined` in the URL and incorrect backend filtering.</violation>
</file>
<file name="frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx:560">
P2: Mock cleanup is not guaranteed on test failure - React.useState spy restoration may leak to subsequent tests</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
frontend/__tests__/unit/contexts/BreadcrumbContext.test.tsx (1)
54-58: The cleanup fix is correct; consider strengthening the assertion.
cleanupFns.push(unregister)before theexpectcorrectly addresses the prior teardown concern.The lone assertion
expect(unregister).toBeDefined()can never fail —registerBreadcrumbClassNameis typed() => void, so it unconditionally returns a function. If the context ever exposes a class-name getter (e.g.getBreadcrumbClassNames()), consider replacing or supplementing this with a behavioral assertion such as verifying the registered class name is present and absent afterafterEachcleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/contexts/BreadcrumbContext.test.tsx` around lines 54 - 58, The test currently only asserts the unregister function exists; strengthen it by asserting observable behavior: after calling registerBreadcrumbClassName('test') (and pushing unregister into cleanupFns), verify the registered class name is actually present via the context's accessor (e.g., getBreadcrumbClassNames() or similar) or by checking the relevant DOM element's class list, then call the returned unregister and assert the class name is removed; use the existing registerBreadcrumbClassName and cleanupFns/unregister helpers to locate where to add these presence/absence assertions.frontend/__tests__/unit/pages/SnapshotDetails.test.tsx (1)
380-403: Consider strengthening the assertion to differentiate this test from the "without repositoryName" variant.
releaseWithoutIdstill carriesrepositoryName: 'test-project-1'from the spread, so the test could additionally assert that'test-project-1'is visible. Currently the singlegetByText('v0.9.2')assertion doesn't confirm that the repository name still renders correctly when onlyidis absent — which is the meaningful distinction between this test and the one at line 405.🔍 Suggested additional assertion
await waitFor(() => { expect(screen.getByText('v0.9.2')).toBeInTheDocument() + expect(screen.getByText('test-project-1')).toBeInTheDocument() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/SnapshotDetails.test.tsx` around lines 380 - 403, The test "renders release without id (uses fallback key)" creates releaseWithoutId from mockSnapshotDetailsData.snapshot.newReleases[0] but never asserts the repository name; update the test (in SnapshotDetails.test.tsx) to also assert that the repositoryName from releaseWithoutId (e.g., 'test-project-1') is rendered by the SnapshotDetailsPage component—use the existing screen/getByText or waitFor to check that the repository name is in the document so this case is distinguished from the "without repositoryName" variant.frontend/src/app/members/[memberKey]/page.tsx (2)
57-57:badges.slice()is redundant
.map()already returns a new array without mutating the source; the extra.slice()just allocates a throw-away copy.♻️ Proposed fix
- {badges.slice().map((badge: Badge) => ( + {badges.map((badge: Badge) => (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/members/`[memberKey]/page.tsx at line 57, The call to badges.slice() before mapping is redundant and wastes an allocation; in the component rendering where badges.slice().map((badge: Badge) => ...) is used, remove the .slice() and use badges.map((badge: Badge) => ...) instead so the original array isn't copied unnecessarily while preserving behavior; update any related occurrences of badges.slice().map to badges.map in this file (page.tsx) or nearby render helpers.
118-126:dates.at(-1)gives an imprecisestring | undefinedtype despite the non-empty guardThe branch guarding
dates.length === 0guarantees the array is non-empty whendates.at(-1)is called, yet TypeScript infersstring | undefinedfor.at(), makingendDate's type looser than necessary. Using the index form instead keeps the inferred typestringand makes intent explicit.♻️ Proposed fix
return { startDate: dates[0], - endDate: dates.at(-1), + endDate: dates[dates.length - 1], }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/members/`[memberKey]/page.tsx around lines 118 - 126, The computed dateRange inside the useMemo uses dates.at(-1) which TypeScript types as string | undefined despite the earlier guard; replace the .at(-1) call with the explicit indexed access dates[dates.length - 1] (or otherwise assert non-null) so endDate is inferred as string; update the return object in the dateRange computation (the useMemo that reads contributionData and builds dates) to use dates[dates.length - 1] instead of dates.at(-1).frontend/__tests__/unit/pages/EditProgram.test.tsx (1)
460-460: Redundantjest.clearAllMocks()call
beforeEachalready callsjest.clearAllMocks()as its last statement (Line 50), so this call adds no value.jest.clearAllMocks()resets all information stored in the mock function's usage data but does not change the mock implementation or return value. Since no mocks have been called between the end ofbeforeEachand this line, the call history is already empty.🧹 Proposed cleanup
- jest.clearAllMocks() ;(useSession as jest.Mock).mockReturnValue({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/EditProgram.test.tsx` at line 460, Remove the redundant jest.clearAllMocks() call at the test location — since the test suite's beforeEach already invokes jest.clearAllMocks() (see the beforeEach block), delete the extra jest.clearAllMocks() line to avoid unnecessary duplication and keep setup/teardown centralized; search for the standalone jest.clearAllMocks() at the location indicated and remove it, leaving the beforeEach-managed clearing intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/__tests__/unit/contexts/BreadcrumbContext.test.tsx`:
- Around line 54-58: The test currently only asserts the unregister function
exists; strengthen it by asserting observable behavior: after calling
registerBreadcrumbClassName('test') (and pushing unregister into cleanupFns),
verify the registered class name is actually present via the context's accessor
(e.g., getBreadcrumbClassNames() or similar) or by checking the relevant DOM
element's class list, then call the returned unregister and assert the class
name is removed; use the existing registerBreadcrumbClassName and
cleanupFns/unregister helpers to locate where to add these presence/absence
assertions.
In `@frontend/__tests__/unit/pages/EditProgram.test.tsx`:
- Line 460: Remove the redundant jest.clearAllMocks() call at the test location
— since the test suite's beforeEach already invokes jest.clearAllMocks() (see
the beforeEach block), delete the extra jest.clearAllMocks() line to avoid
unnecessary duplication and keep setup/teardown centralized; search for the
standalone jest.clearAllMocks() at the location indicated and remove it, leaving
the beforeEach-managed clearing intact.
In `@frontend/__tests__/unit/pages/SnapshotDetails.test.tsx`:
- Around line 380-403: The test "renders release without id (uses fallback key)"
creates releaseWithoutId from mockSnapshotDetailsData.snapshot.newReleases[0]
but never asserts the repository name; update the test (in
SnapshotDetails.test.tsx) to also assert that the repositoryName from
releaseWithoutId (e.g., 'test-project-1') is rendered by the SnapshotDetailsPage
component—use the existing screen/getByText or waitFor to check that the
repository name is in the document so this case is distinguished from the
"without repositoryName" variant.
In `@frontend/src/app/members/`[memberKey]/page.tsx:
- Line 57: The call to badges.slice() before mapping is redundant and wastes an
allocation; in the component rendering where badges.slice().map((badge: Badge)
=> ...) is used, remove the .slice() and use badges.map((badge: Badge) => ...)
instead so the original array isn't copied unnecessarily while preserving
behavior; update any related occurrences of badges.slice().map to badges.map in
this file (page.tsx) or nearby render helpers.
- Around line 118-126: The computed dateRange inside the useMemo uses
dates.at(-1) which TypeScript types as string | undefined despite the earlier
guard; replace the .at(-1) call with the explicit indexed access
dates[dates.length - 1] (or otherwise assert non-null) so endDate is inferred as
string; update the return object in the dateRange computation (the useMemo that
reads contributionData and builds dates) to use dates[dates.length - 1] instead
of dates.at(-1).
Collaborator
|
@HarshitVerma109 hi! Please resolve conflicts 🙏🏼 |
kasya
previously approved these changes
Feb 21, 2026
Collaborator
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@HarshitVerma109 looks good 👍🏼
Make sure to resolve conflicts so that we could merge this.
HarshitVerma109
dismissed stale reviews from kasya, cubic-dev-ai[bot], and coderabbitai[bot]
via
06bf360
HarshitVerma109
force-pushed
the
feat/improve-frontend-test-coverage
branch
from
4391ca1 to
06bf360
Compare
Contributor
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)
frontend/__tests__/unit/components/ModuleForm.test.tsx (1)
45-45:⚠️ Potential issue | 🟡 Minor
nullargument at line 82 does not match theonSelectionChangetype declared at line 45The declared type excludes
null(React.Key | Set<React.Key> | 'all'), but the Clear Selection button at line 82 passesnull. HeroUI Autocomplete actually emitsnullwhen clearing, so the mock's type signature must include it for correctness:🛠️ Proposed fix
- onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void + onSelectionChange?: (key: React.Key | Set<React.Key> | 'all' | null) => void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` at line 45, The test mock's onSelectionChange signature doesn't accept null while the component (HeroUI Autocomplete) can emit null when clearing; update the mock/type to include null (i.e., change the onSelectionChange type to React.Key | Set<React.Key> | 'all' | null) or adjust the mock implementation for onSelectionChange used in ModuleForm.test.tsx to accept a null argument (reference the onSelectionChange prop and the Clear Selection button/mock that passes null).
🧹 Nitpick comments (7)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx (1)
100-112: Consider replacing non-null assertions with a null guard at the top ofhandleSubmit.The assertions are logically safe today because
ModuleForm(and therefore its submit handler) is only mounted whenformData !== null(lines 145-147). But scatteringformData!across a dozen field accesses makes the code brittle: any future refactor that callshandleSubmitdirectly (e.g., a keyboard shortcut, a test, or an imperative trigger) will throw at runtime without a TypeScript-visible warning.A single guard at the top of the function is more readable and resilient:
♻️ Proposed refactor — replace assertions with an early return guard
const handleSubmit = async (e: React.FormEvent) => { e.preventDefault() + if (!formData) return try { const input: UpdateModuleInput = { - description: formData!.description, - domains: parseCommaSeparated(formData!.domains), - endedAt: formData!.endedAt || '', - experienceLevel: formData!.experienceLevel as ExperienceLevelEnum, + description: formData.description, + domains: parseCommaSeparated(formData.domains), + endedAt: formData.endedAt || '', + experienceLevel: formData.experienceLevel as ExperienceLevelEnum, key: moduleKey, - labels: parseCommaSeparated(formData!.labels), - mentorLogins: parseCommaSeparated(formData!.mentorLogins), - name: formData!.name, + labels: parseCommaSeparated(formData.labels), + mentorLogins: parseCommaSeparated(formData.mentorLogins), + name: formData.name, programKey: programKey, - projectId: formData!.projectId, - projectName: formData!.projectName, - startedAt: formData!.startedAt || '', - tags: parseCommaSeparated(formData!.tags), + projectId: formData.projectId, + projectName: formData.projectName, + startedAt: formData.startedAt || '', + tags: parseCommaSeparated(formData.tags), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/edit/page.tsx around lines 100 - 112, The handleSubmit implementation currently uses repeated non-null assertions (formData!) when building the payload; add a single null guard at the top of handleSubmit (e.g., if (!formData) { return; } or throw new Error) and then remove the individual non-null assertions so the rest of the function can safely reference formData fields (description, domains, endedAt, experienceLevel, labels, mentorLogins, name, projectId, projectName, startedAt, tags, etc.); update any callers/tests if they rely on throwing behavior to instead handle the early return or thrown error.frontend/__tests__/unit/pages/EditProgram.test.tsx (1)
436-478: Redundantjest.clearAllMocks()inside the test body
beforeEachalready callsjest.clearAllMocks()as its final statement (line 50), so the same call at line 460 is a no-op.jest.clearAllMocksonly wipes call history, notmockReturnValue/mockImplementationstate, so nothing breaks — it's just dead code.🧹 Suggested cleanup
- jest.clearAllMocks() - ;(useSession as jest.Mock).mockReturnValue({ + ;(useSession as jest.Mock).mockReturnValue({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/EditProgram.test.tsx` around lines 436 - 478, The jest.clearAllMocks() call inside the test "handles null admins when access is initially allowed" is redundant because beforeEach already calls jest.clearAllMocks(); remove the in-test call to jest.clearAllMocks() from EditProgram.test.tsx (inside that specific test) and keep the existing beforeEach behavior so mocks are cleared consistently; no other changes needed.frontend/__tests__/unit/pages/MyMentorship.test.tsx (1)
423-463:jest.useFakeTimers()placement is now correct — previous issue resolved.The timer is activated before
render(line 445 → render line 447), matching the "updates debounced search query" pattern and eliminating the earlier real-timer/fake-timer race. ThewaitFor(() => expect(mockPush).not.toHaveBeenCalled())assertion is safe under fake timers here because the debounce fires insideReact.act(() => jest.advanceTimersByTime(500))beforewaitForis entered, sowaitFor's own internal retry timer (which can stall under fake timers) is never needed.The
useSearchParamsmock (line 442) andhistory.replaceState(line 443) are set up outside thetryblock, while the "updates debounced search query" sibling keeps all setup insidetry. Moving them inside would make the two fake-timer tests consistent:♻️ Optional — align setup placement with the sibling test
- ;(useSearchParams as jest.Mock).mockReturnValue(new URLSearchParams('q=existing')) - globalThis.history.replaceState(null, '', '?q=existing') - jest.useFakeTimers() try { + ;(useSearchParams as jest.Mock).mockReturnValue(new URLSearchParams('q=existing')) + globalThis.history.replaceState(null, '', '?q=existing') + render(<MyMentorshipPage />)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/MyMentorship.test.tsx` around lines 423 - 463, Move the setup of the useSearchParams mock and globalThis.history.replaceState into the try block alongside jest.useFakeTimers() and render so the two fake-timer tests follow the same setup/teardown pattern; specifically, for the test case that calls jest.useFakeTimers() and renders MyMentorshipPage, relocate the lines that call (useSearchParams as jest.Mock).mockReturnValue(...) and globalThis.history.replaceState(...) inside the try before render(), and keep the existing finally block that restores history and calls jest.useRealTimers() to ensure consistent isolation for the debounced search behavior.frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
1617-1623: Consider splitting into two separate tests for better isolation.Two scenarios in one test can make failures harder to diagnose. The manual
cleanup()call at line 1620 between renders is redundant sinceafterEach(line 805) already handles cleanup. Splitting into two independent tests would provide clearer failure messages and keep each test focused on a single scenario.♻️ Proposed split into two tests
- it('handles undefined topics/languages in grid layout', () => { - render(<CardDetailsPage {...defaultProps} languages={['JavaScript']} topics={undefined} />) - expect(screen.getByTestId('toggleable-list')).toHaveTextContent('Languages: JavaScript') - cleanup() - render(<CardDetailsPage {...defaultProps} languages={undefined} topics={['web']} />) - expect(screen.getByTestId('toggleable-list')).toHaveTextContent('Topics: web') - }) + it('renders only languages when topics is undefined', () => { + render(<CardDetailsPage {...defaultProps} languages={['JavaScript']} topics={undefined} />) + expect(screen.getByTestId('toggleable-list')).toHaveTextContent('Languages: JavaScript') + }) + + it('renders only topics when languages is undefined', () => { + render(<CardDetailsPage {...defaultProps} languages={undefined} topics={['web']} />) + expect(screen.getByTestId('toggleable-list')).toHaveTextContent('Topics: web') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx` around lines 1617 - 1623, Split the single test "handles undefined topics/languages in grid layout" into two independent tests to improve isolation: one test rendering <CardDetailsPage {...defaultProps} languages={['JavaScript']} topics={undefined} /> and asserting screen.getByTestId('toggleable-list') contains "Languages: JavaScript", and a separate test rendering <CardDetailsPage {...defaultProps} languages={undefined} topics={['web']} /> asserting it contains "Topics: web"; remove the manual cleanup() call (it's redundant because afterEach already runs) and keep the same test ids and expectations so only the test names and bodies change.frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx (2)
655-678: Redundantassigning: falseoverride
assigning: falseis already established bybeforeEachand inherited through thebaseMocksspread, so the explicit override on line 667 adds no value.♻️ Suggested cleanup
mockUseIssueMutations.mockReturnValue({ ...baseMocks, assignIssue, - assigning: false, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx` around lines 655 - 678, The test redundantly overrides assigning: false when baseMocks already provides it; remove the explicit assigning: false property from the mockUseIssueMutations return in the 'does not trigger mutations when issueId is missing' test so the mock is returned as {...baseMocks, assignIssue} (keeping assignIssue) and rely on the beforeEach-provided assigning value from useIssueMutations/baseMocks.
655-678: Redundantassigning: falseoverride
assigning: falseis already established bybeforeEachand inherited via thebaseMocksspread, so the explicit override on line 667 is unnecessary noise. Removing it simplifies the test setup.♻️ Suggested cleanup
mockUseIssueMutations.mockReturnValue({ ...baseMocks, assignIssue, - assigning: false, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx` around lines 655 - 678, The test redundantly overrides assigning when building mockUseIssueMutations; remove the explicit assigning: false in the mockReturnValue call so it simply spreads baseMocks and only overrides assignIssue (i.e., keep mockUseIssueMutations.mockReturnValue({ ...baseMocks, assignIssue }) ), referencing assignIssue, mockUseIssueMutations, baseMocks and assigning to locate the code to change.frontend/__tests__/unit/components/ModuleForm.test.tsx (1)
657-687: Test title "handles selection via Set" does not match what is actually assertedThe
autocomplete-select-itembutton firesonSelectionChange(new Set(['project-1'])), but the assertion on lines 684–686 only checksmockOnProjectChangewas called with(null, 'Test'), which is the value emitted byonInputChange(the typing step), not the Set-based selection. The actual Set-selection path (which should callonProjectChange('project-1', 'Test Project 1')) is never verified.Either update the assertion to check the project selection result or rename the test to reflect what it actually covers:
🛠️ Proposed fix — verify the Set selection callback
- // Verify that typing triggers onProjectChange with the search term - await waitFor(() => { - expect(mockOnProjectChange).toHaveBeenCalledWith(null, 'Test') - }) + // Verify that selecting via Set calls onProjectChange with the resolved project + await waitFor(() => { + expect(mockOnProjectChange).toHaveBeenCalledWith('project-1', 'Test Project 1') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 657 - 687, The test "handles selection via Set (lines 403-404)" currently only asserts that typing triggered onProjectChange with (null, 'Test') but never verifies the Set-selection path; update the test to assert that after clicking the button with data-testid 'autocomplete-select-item' (which fires onSelectionChange(new Set(['project-1']))) the mockOnProjectChange mock is called with the selected project id and name (e.g., mockOnProjectChange('project-1', 'Test Project 1')), or alternatively rename the test to reflect it only verifies input change; locate the test block using renderProjectSelector, mockQuery, the input bound to 'autocomplete-input', and the click on 'autocomplete-select-item' and add the expectation for the Set-based selection callback (mockOnProjectChange called with 'project-1' and 'Test Project 1').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx`:
- Around line 2108-2139: The test assigns `name: null` on the milestone author
which violates the User type (name?: string), so update the test data used in
the `recentMilestones` array for the `CardDetailsPage`/`DetailsCardProps` case:
remove the `name` property or set it to `undefined` (instead of `null`) on the
author object so it conforms to the `User`/`Milestone` types and the test can
compile while still asserting tooltip falls back to `login`.
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 400-421: The ModuleForm tests rely on advancing timers but never
enable fake timers; inside the describe('ModuleForm', ...) add
jest.useFakeTimers() in its beforeEach (the same place you call
renderModuleForm) and call jest.useRealTimers() in the corresponding afterEach
to restore real timers; this ensures jest.advanceTimersByTime(350) in the
'updates project field when ProjectSelector changes' test properly triggers the
debounce in ProjectSelector (reference the renderModuleForm helper and the test
name to locate the fix).
---
Outside diff comments:
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Line 45: The test mock's onSelectionChange signature doesn't accept null while
the component (HeroUI Autocomplete) can emit null when clearing; update the
mock/type to include null (i.e., change the onSelectionChange type to React.Key
| Set<React.Key> | 'all' | null) or adjust the mock implementation for
onSelectionChange used in ModuleForm.test.tsx to accept a null argument
(reference the onSelectionChange prop and the Clear Selection button/mock that
passes null).
---
Duplicate comments:
In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx`:
- Around line 1625-1634: The test sets details.value to null but
DetailsCardProps.details is typed as { label: string; value: string |
JSX.Element }[], causing a TypeScript error; fix by either updating the type to
allow null (e.g., include null | undefined in DetailsCardProps.details value
union) or change the test to avoid the type mismatch (remove any explicit
DetailsCardProps annotation on defaultProps so TypeScript can infer the looser
type or cast propsWithNullLeader to unknown as any before calling render);
locate the type declaration for DetailsCardProps and the test variables
defaultProps and propsWithNullLeader in CardDetailsPage.test.tsx to apply the
chosen fix.
In `@frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx`:
- Around line 545-563: Remove the fragile React.useState spy in the test for
MenteeProfilePage and instead drive the invalid status through the component's
public selection handler: update the test to trigger the existing
select-trigger-empty button (or add a hidden sibling button) to call the
component's onSelectionChange handler with new Set(['invalid_filter']), keep
mockUseQuery returning mockMenteeData, render <MenteeProfilePage />, assert both
"Open Issue 1" and "Closed Issue 1" are present, and remove the
jest.spyOn(React, 'useState') mock/restore to avoid coupling to internal initial
state.
---
Nitpick comments:
In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx`:
- Around line 1617-1623: Split the single test "handles undefined
topics/languages in grid layout" into two independent tests to improve
isolation: one test rendering <CardDetailsPage {...defaultProps}
languages={['JavaScript']} topics={undefined} /> and asserting
screen.getByTestId('toggleable-list') contains "Languages: JavaScript", and a
separate test rendering <CardDetailsPage {...defaultProps} languages={undefined}
topics={['web']} /> asserting it contains "Topics: web"; remove the manual
cleanup() call (it's redundant because afterEach already runs) and keep the same
test ids and expectations so only the test names and bodies change.
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 657-687: The test "handles selection via Set (lines 403-404)"
currently only asserts that typing triggered onProjectChange with (null, 'Test')
but never verifies the Set-selection path; update the test to assert that after
clicking the button with data-testid 'autocomplete-select-item' (which fires
onSelectionChange(new Set(['project-1']))) the mockOnProjectChange mock is
called with the selected project id and name (e.g.,
mockOnProjectChange('project-1', 'Test Project 1')), or alternatively rename the
test to reflect it only verifies input change; locate the test block using
renderProjectSelector, mockQuery, the input bound to 'autocomplete-input', and
the click on 'autocomplete-select-item' and add the expectation for the
Set-based selection callback (mockOnProjectChange called with 'project-1' and
'Test Project 1').
In `@frontend/__tests__/unit/pages/EditProgram.test.tsx`:
- Around line 436-478: The jest.clearAllMocks() call inside the test "handles
null admins when access is initially allowed" is redundant because beforeEach
already calls jest.clearAllMocks(); remove the in-test call to
jest.clearAllMocks() from EditProgram.test.tsx (inside that specific test) and
keep the existing beforeEach behavior so mocks are cleared consistently; no
other changes needed.
In `@frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx`:
- Around line 655-678: The test redundantly overrides assigning: false when
baseMocks already provides it; remove the explicit assigning: false property
from the mockUseIssueMutations return in the 'does not trigger mutations when
issueId is missing' test so the mock is returned as {...baseMocks, assignIssue}
(keeping assignIssue) and rely on the beforeEach-provided assigning value from
useIssueMutations/baseMocks.
- Around line 655-678: The test redundantly overrides assigning when building
mockUseIssueMutations; remove the explicit assigning: false in the
mockReturnValue call so it simply spreads baseMocks and only overrides
assignIssue (i.e., keep mockUseIssueMutations.mockReturnValue({ ...baseMocks,
assignIssue }) ), referencing assignIssue, mockUseIssueMutations, baseMocks and
assigning to locate the code to change.
In `@frontend/__tests__/unit/pages/MyMentorship.test.tsx`:
- Around line 423-463: Move the setup of the useSearchParams mock and
globalThis.history.replaceState into the try block alongside
jest.useFakeTimers() and render so the two fake-timer tests follow the same
setup/teardown pattern; specifically, for the test case that calls
jest.useFakeTimers() and renders MyMentorshipPage, relocate the lines that call
(useSearchParams as jest.Mock).mockReturnValue(...) and
globalThis.history.replaceState(...) inside the try before render(), and keep
the existing finally block that restores history and calls jest.useRealTimers()
to ensure consistent isolation for the debounced search behavior.
In
`@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/edit/page.tsx:
- Around line 100-112: The handleSubmit implementation currently uses repeated
non-null assertions (formData!) when building the payload; add a single null
guard at the top of handleSubmit (e.g., if (!formData) { return; } or throw new
Error) and then remove the individual non-null assertions so the rest of the
function can safely reference formData fields (description, domains, endedAt,
experienceLevel, labels, mentorLogins, name, projectId, projectName, startedAt,
tags, etc.); update any callers/tests if they rely on throwing behavior to
instead handle the early return or thrown error.
Contributor
There was a problem hiding this comment.
1 issue found across 19 files
Confidence score: 3/5
- Potential test failure in
frontend/__tests__/unit/pages/UserDetails.test.tsx: assertion expectsfallback-loginbut UI renders@fallback-login, which will likely break the test and block CI. - Score reflects a concrete, user-impacting regression risk for the test suite rather than runtime behavior; fix is straightforward but needs attention before merge.
- Pay close attention to
frontend/__tests__/unit/pages/UserDetails.test.tsx- update the assertion to match the rendered login format.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/pages/UserDetails.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/UserDetails.test.tsx:1171">
P1: Test assertion expects plain 'fallback-login' text but UI renders '@fallback-login' with @ prefix. The assertion `screen.getByText('fallback-login')` will fail because UserSummary component renders the login link as `@{user?.login}` which includes the '@' prefix.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
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)
frontend/__tests__/unit/components/ModuleForm.test.tsx (1)
45-45:⚠️ Potential issue | 🟡 MinorUpdate the Autocomplete mock type to include
nullThe mock's
onSelectionChangetype (line 45) declaresReact.Key | Set<React.Key> | 'all', but line 82 invokes it withnull. While TypeScript doesn't flag this because test files are excluded from type checking, the mock type should be updated to match its actual usage.Proposed fix
- onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void + onSelectionChange?: (key: React.Key | Set<React.Key> | 'all' | null) => void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` at line 45, The Autocomplete mock's onSelectionChange signature doesn't allow null even though the test invokes it with null; update the mock type for onSelectionChange to include null (i.e., change the type from React.Key | Set<React.Key> | 'all' to React.Key | Set<React.Key> | 'all' | null) so the mock accurately reflects usage in the ModuleForm test where onSelectionChange is called with null; locate the Autocomplete mock definition referencing onSelectionChange and adjust its type accordingly.
🧹 Nitpick comments (3)
frontend/__tests__/unit/pages/UserDetails.test.tsx (2)
1129-1149: Rename test: "validates defensive check for endDate" doesn't describe what's tested.The test body only verifies that the heatmap renders when
contributionDatahas a single entry. The wordendDatedoesn't appear in any assertion, making the intent opaque to future readers. A name like"renders heatmap when contributionData has a single date entry"would be accurate.♻️ Suggested rename
- test('validates defensive check for endDate', async () => { + test('renders heatmap when contributionData has a single date entry', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/UserDetails.test.tsx` around lines 1129 - 1149, The test named "validates defensive check for endDate" is misleading; rename the jest test title to something accurate like "renders heatmap when contributionData has a single date entry" in the test that renders <UserDetailsPage /> (the test that mocks useQuery to return singleDateContribution and asserts getByTestId('contribution-heatmap')). Update only the string in the test(...) call so the behavior being verified (heatmap renders for a single contribution date) is clearly described.
1151-1173: Partial duplication with existing tests onname: nullfallback.The avatar alt-text assertion is already covered by the test at Line 1040 ("renders with login as alt text when name is null"), and the
@{login}link check is covered at Line 993 ("renders with login as title when name is null"). The sole distinction here is using a differentloginvalue. Consider removing this test or folding its unique@fallback-loginlink assertion into one of the existing tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/UserDetails.test.tsx` around lines 1151 - 1173, The new test "uses user login as fallback for avatar alt text and title when name is missing" duplicates coverage from the existing tests "renders with login as alt text when name is null" and "renders with login as title when name is null"; remove this redundant test or merge its unique assertion into an existing test by adjusting that test's mocked user login to 'fallback-login' (or adding an assertion for '@fallback-login') so the avatar alt/text and the `@login` link are both asserted in one place (look for the tests in UserDetails.test.tsx that reference the name:null fallback and update them accordingly).frontend/__tests__/unit/components/ModuleForm.test.tsx (1)
662-692: Test "handles selection via Set" doesn't assert the Set-selection outcomeThe assertion at line 690 only verifies the typing side-effect (
onProjectChange(null, 'Test')), not the result of clickingautocomplete-select-item(which firesonSelectionChange(new Set(['project-1']))). The Set-selection path — which should callonProjectChange('project-1', 'Test Project 1')— is never validated here.Consider adding a second assertion after clicking
autocomplete-select-item:♻️ Proposed addition
// Verify that typing triggers onProjectChange with the search term await waitFor(() => { expect(mockOnProjectChange).toHaveBeenCalledWith(null, 'Test') }) + // Verify that the Set-based selection resolves to the project id + name + expect(mockOnProjectChange).toHaveBeenCalledWith('project-1', 'Test Project 1') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 662 - 692, The test "handles selection via Set (lines 403-404)" currently only asserts the typing side-effect; after clicking the select button (getByTestId('autocomplete-select-item')), add a second assertion that the Set-selection path invoked onProjectChange with the selected project id and name (e.g., expect(mockOnProjectChange).toHaveBeenCalledWith('project-1', 'Test Project 1')). Wrap that assertion in a waitFor if needed to await async handlers so the click's onSelectionChange(new Set(['project-1'])) outcome is validated alongside the existing typing assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/__tests__/unit/pages/UserDetails.test.tsx`:
- Around line 1175-1196: The test for UserDetailsPage using the mock
userWithoutNameAndLogin should also assert that a null login does not render as
the string "@null"; update the test (the case that renders <UserDetailsPage />
with userWithoutNameAndLogin) to check that there is no element containing the
text "@null" (or that the login link is absent/uses the expected fallback), so
the component's rendering of @{user?.login} is validated when login is null.
---
Outside diff comments:
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Line 45: The Autocomplete mock's onSelectionChange signature doesn't allow
null even though the test invokes it with null; update the mock type for
onSelectionChange to include null (i.e., change the type from React.Key |
Set<React.Key> | 'all' to React.Key | Set<React.Key> | 'all' | null) so the mock
accurately reflects usage in the ModuleForm test where onSelectionChange is
called with null; locate the Autocomplete mock definition referencing
onSelectionChange and adjust its type accordingly.
---
Duplicate comments:
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 281-289: The test now correctly adds jest.useFakeTimers() in the
setup and jest.useRealTimers() in teardown to make jest.advanceTimersByTime(350)
effective; ensure these calls are placed inside the ModuleForm describe block's
beforeEach/afterEach (where mockQuery is prepared) so any usage of
jest.advanceTimersByTime in tests that reference mockQuery or Component
rendering will actually advance timers and reset to real timers after each test.
---
Nitpick comments:
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 662-692: The test "handles selection via Set (lines 403-404)"
currently only asserts the typing side-effect; after clicking the select button
(getByTestId('autocomplete-select-item')), add a second assertion that the
Set-selection path invoked onProjectChange with the selected project id and name
(e.g., expect(mockOnProjectChange).toHaveBeenCalledWith('project-1', 'Test
Project 1')). Wrap that assertion in a waitFor if needed to await async handlers
so the click's onSelectionChange(new Set(['project-1'])) outcome is validated
alongside the existing typing assertion.
In `@frontend/__tests__/unit/pages/UserDetails.test.tsx`:
- Around line 1129-1149: The test named "validates defensive check for endDate"
is misleading; rename the jest test title to something accurate like "renders
heatmap when contributionData has a single date entry" in the test that renders
<UserDetailsPage /> (the test that mocks useQuery to return
singleDateContribution and asserts getByTestId('contribution-heatmap')). Update
only the string in the test(...) call so the behavior being verified (heatmap
renders for a single contribution date) is clearly described.
- Around line 1151-1173: The new test "uses user login as fallback for avatar
alt text and title when name is missing" duplicates coverage from the existing
tests "renders with login as alt text when name is null" and "renders with login
as title when name is null"; remove this redundant test or merge its unique
assertion into an existing test by adjusting that test's mocked user login to
'fallback-login' (or adding an assertion for '@fallback-login') so the avatar
alt/text and the `@login` link are both asserted in one place (look for the tests
in UserDetails.test.tsx that reference the name:null fallback and update them
accordingly).
HarshitVerma109
force-pushed
the
feat/improve-frontend-test-coverage
branch
from
bba2df0 to
a29ba4d
Compare
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/__tests__/unit/components/ModuleForm.test.tsx (2)
40-86:⚠️ Potential issue | 🟡 MinorFix TypeScript error on line 82:
nullis not assignable to the mock's onSelectionChange type unionThe mock's
onSelectionChangeprop at line 45 is typed as:onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => voidCalling it with
nullat line 82 causes a TypeScript error sincenullis not in the union. The mock accepts a custom set of possible values (includingSet<React.Key>and'all') for testing purposes. Add| nullto the union to support the clear action test case:- onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void + onSelectionChange?: (key: React.Key | Set<React.Key> | 'all' | null) => void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 40 - 86, The mock's onSelectionChange prop signature doesn't allow null, causing the TypeScript error when Clear Selection calls onSelectionChange(null); update the prop type in the test component (the onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void declaration) to include null by changing the union to React.Key | Set<React.Key> | 'all' | null so the Clear Selection button call onSelectionChange?.(null) is valid.
40-86:⚠️ Potential issue | 🟡 MinorAdd
nullto mockonSelectionChangetype unionLine 45 types
onSelectionChangeas(key: React.Key | Set<React.Key> | 'all') => void, but line 82 passesnull. This is a TypeScript error. HeroUI Autocomplete'sonSelectionChangecallback receivesnullwhen a selection is cleared, so the mock's type must include it.Fix
- onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void + onSelectionChange?: (key: React.Key | Set<React.Key> | 'all' | null) => void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 40 - 86, The mock prop type for onSelectionChange is missing null; update its type union in the mock props declaration (the anonymous component props where onSelectionChange is declared) to include null — i.e. change onSelectionChange?: (key: React.Key | Set<React.Key> | 'all' | null) => void — so the existing Clear Selection button that calls onSelectionChange?.(null) is type-safe (refer to the autocomplete mock props and the onSelectionChange usage in the Clear Selection button).
🧹 Nitpick comments (3)
frontend/__tests__/unit/pages/SnapshotDetails.test.tsx (1)
380-403: Strengthen the first test's assertions and clarify the test name.The test name "uses fallback key" refers to a React list
keyprop, which isn't directly observable in DOM assertions — the test can't actually verify that behavior. More practically, the test only assertsv0.9.2is rendered, but sincerepositoryNameis still defined ('test-project-1'), there's no assertion differentiating it from the second test. Without a negative assertion for'Unknown repository', both the "no id" and the "no id + no repositoryName" cases look identical from this test's perspective.✨ Proposed improvements
- test('renders release without id (uses fallback key)', async () => { + test('renders release without id (repositoryName still displayed)', async () => { const releaseWithoutId = { ...mockSnapshotDetailsData.snapshot.newReleases[0], id: undefined, } ;(useQuery as unknown as jest.Mock).mockReturnValue({ data: { snapshot: { ...mockSnapshotDetailsData.snapshot, newReleases: [releaseWithoutId], newChapters: [], newProjects: [], }, }, error: null, loading: false, }) render(<SnapshotDetailsPage />) await waitFor(() => { expect(screen.getByText('v0.9.2')).toBeInTheDocument() + expect(screen.getByText('test-project-1')).toBeInTheDocument() + expect(screen.queryByText('Unknown repository')).not.toBeInTheDocument() }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/SnapshotDetails.test.tsx` around lines 380 - 403, Rename and strengthen the test for the "no id" release case in SnapshotDetails.test.tsx: update the test name from "renders release without id (uses fallback key)" to something like "renders release without id and shows repository name or fallback" and modify the mocked release object (currently derived from mockSnapshotDetailsData.snapshot.newReleases[0] with id: undefined) to also remove repositoryName in a separate test case; in the first test assert that the known repositoryName (e.g., 'test-project-1') is rendered and assert that 'Unknown repository' is not present, and add a second test where you set repositoryName: undefined and assert the fallback text 'Unknown repository' appears; keep using SnapshotDetailsPage, useQuery mock, and mockSnapshotDetailsData to locate where to change the mocks and expectations.frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
1129-1149: Misleading test name.
'validates defensive check for endDate'doesn't describe what's actually being exercised. The test verifies that a single-datecontributionDataentry is sufficient to render the heatmap; the name should reflect that.📝 Suggested rename
- test('validates defensive check for endDate', async () => { + test('renders heatmap when contributionData contains a single date entry', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/UserDetails.test.tsx` around lines 1129 - 1149, The test name 'validates defensive check for endDate' is misleading; rename the test to reflect that it ensures a heatmap renders when contributionData contains a single date. Update the test declaration string in the test block that creates singleDateContribution (using mockUserDetailsData and mocking useQuery to return it) — change the test(...) first argument to something like 'renders heatmap when contributionData has a single date' while leaving the rest of the test (render(<UserDetailsPage />), waitFor, and assertions) unchanged.frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx (1)
102-127: Consider retaining a defensiveformDatanull guard at the top ofhandleSubmit.The non-null assertions (
formData!) are safe today because the form is only rendered—and thereforehandleSubmitonly callable—whenformDatais non-null (guarded at lines 175-177). However, the assertions silently bypass TypeScript's null-safety: if the render guard is ever moved or the component is restructured, these will become runtime crashes with no compile-time warning.A one-line early return is cheaper than a future debugging session:
♻️ Proposed defensive guard
const handleSubmit = async (e: React.FormEvent) => { e.preventDefault() + if (!formData) return try { const currentUserLogin = sessionData?.user?.login ... const input: UpdateModuleInput = { - description: formData!.description, - domains: parseCommaSeparated(formData!.domains), - endedAt: formData!.endedAt || '', - experienceLevel: formData!.experienceLevel as ExperienceLevelEnum, + description: formData.description, + domains: parseCommaSeparated(formData.domains), + endedAt: formData.endedAt || '', + experienceLevel: formData.experienceLevel as ExperienceLevelEnum, key: moduleKey, - labels: parseCommaSeparated(formData!.labels), - name: formData!.name, + labels: parseCommaSeparated(formData.labels), + name: formData.name, programKey: programKey, - projectId: formData!.projectId, - projectName: formData!.projectName, - startedAt: formData!.startedAt || '', - tags: parseCommaSeparated(formData!.tags), + projectId: formData.projectId, + projectName: formData.projectName, + startedAt: formData.startedAt || '', + tags: parseCommaSeparated(formData.tags), } if (isAdmin) { - input.mentorLogins = parseCommaSeparated(formData!.mentorLogins) + input.mentorLogins = parseCommaSeparated(formData.mentorLogins) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/edit/page.tsx around lines 102 - 127, The handleSubmit function uses many non-null assertions on formData (formData!) which can cause runtime crashes if formData becomes null; add a one-line defensive guard at the start of handleSubmit that returns early when formData is null/undefined (e.g. if (!formData) return) before building the UpdateModuleInput and calling parseCommaSeparated for mentorLogins, so all references to formData inside handleSubmit are safe; update any dependent logic in handleSubmit (e.g., building input, isAdmin check) to assume formData exists after the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 40-86: The mock's onSelectionChange prop signature doesn't allow
null, causing the TypeScript error when Clear Selection calls
onSelectionChange(null); update the prop type in the test component (the
onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void
declaration) to include null by changing the union to React.Key | Set<React.Key>
| 'all' | null so the Clear Selection button call onSelectionChange?.(null) is
valid.
- Around line 40-86: The mock prop type for onSelectionChange is missing null;
update its type union in the mock props declaration (the anonymous component
props where onSelectionChange is declared) to include null — i.e. change
onSelectionChange?: (key: React.Key | Set<React.Key> | 'all' | null) => void —
so the existing Clear Selection button that calls onSelectionChange?.(null) is
type-safe (refer to the autocomplete mock props and the onSelectionChange usage
in the Clear Selection button).
---
Duplicate comments:
In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx`:
- Around line 1629-1638: The test passes value: null into CardDetailsPage but
DetailsCardProps.details expects value: string | JSX.Element, causing a
TypeScript error; fix the test by making the null compatible (e.g., change the
test's propsWithNullLeader to cast the value: null as any or as unknown as
string | JSX.Element), or remove any explicit DetailsCardProps annotation on
defaultProps so inference allows null — locate the test variables (defaultProps,
propsWithNullLeader) and update the value assignment accordingly.
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 279-289: Tests already properly scope fake timers: ensure the
beforeEach contains jest.useFakeTimers(), jest.clearAllMocks(), and
mockQuery.mockResolvedValue(...) and the afterEach restores timers with
jest.useRealTimers(); keep these calls (beforeEach, afterEach,
jest.useFakeTimers, jest.useRealTimers, and any jest.advanceTimersByTime(...)
usage in tests) as-is so timer-dependent tests remain deterministic.
- Around line 279-289: The beforeEach/afterEach setup correctly scopes fake
timers for the ModuleForm tests; keep the jest.useFakeTimers() call inside the
beforeEach and jest.useRealTimers() in afterEach so
jest.advanceTimersByTime(350) remains deterministic, and ensure you do not add
global fake-timer calls outside this describe block (check the beforeEach,
afterEach, jest.useFakeTimers, jest.useRealTimers, and tests that call
jest.advanceTimersByTime to confirm no conflicting timer setup).
In `@frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx`:
- Around line 545-563: Replace the fragile React.useState spy by driving the
component via the mock Select: update the Select mock (the component exposing
"select-trigger-empty") to add a hidden button that calls its onSelectionChange
handler with new Set(['invalid_filter']), then change the test case in
MenteeProfilePage.test.tsx to render the page and programmatically click that
hidden button (instead of mocking React.useState) to trigger the invalid filter
path; keep mockUseQuery return as-is and retain the try/finally cleanup around
mocks.
In `@frontend/__tests__/unit/pages/UserDetails.test.tsx`:
- Around line 1175-1197: The test "uses \"User Avatar\" fallback when both name
and login are missing" in UserDetails.test.tsx now includes the missing
assertion expect(screen.queryByText('@null')).not.toBeInTheDocument(), so no
code changes are required; leave the test as-is (refer to the test function and
the added assertion line) and mark the previous review request as resolved.
In
`@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/issues/page.tsx:
- Around line 117-120: The change tightens the guard in onSelectionChange to
only block undefined keys which is correct; keep the updated code in
onSelectionChange that converts the selected keys to a single key (const [key] =
Array.from(keys as Set<string>)) and use if (key !== undefined)
handleLabelChange(key) so empty-string keys (if ever present) are allowed while
preventing the undefined-from-empty-Set bug—no further code changes needed to
functions onSelectionChange or handleLabelChange.
---
Nitpick comments:
In `@frontend/__tests__/unit/pages/SnapshotDetails.test.tsx`:
- Around line 380-403: Rename and strengthen the test for the "no id" release
case in SnapshotDetails.test.tsx: update the test name from "renders release
without id (uses fallback key)" to something like "renders release without id
and shows repository name or fallback" and modify the mocked release object
(currently derived from mockSnapshotDetailsData.snapshot.newReleases[0] with id:
undefined) to also remove repositoryName in a separate test case; in the first
test assert that the known repositoryName (e.g., 'test-project-1') is rendered
and assert that 'Unknown repository' is not present, and add a second test where
you set repositoryName: undefined and assert the fallback text 'Unknown
repository' appears; keep using SnapshotDetailsPage, useQuery mock, and
mockSnapshotDetailsData to locate where to change the mocks and expectations.
In `@frontend/__tests__/unit/pages/UserDetails.test.tsx`:
- Around line 1129-1149: The test name 'validates defensive check for endDate'
is misleading; rename the test to reflect that it ensures a heatmap renders when
contributionData contains a single date. Update the test declaration string in
the test block that creates singleDateContribution (using mockUserDetailsData
and mocking useQuery to return it) — change the test(...) first argument to
something like 'renders heatmap when contributionData has a single date' while
leaving the rest of the test (render(<UserDetailsPage />), waitFor, and
assertions) unchanged.
In
`@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/edit/page.tsx:
- Around line 102-127: The handleSubmit function uses many non-null assertions
on formData (formData!) which can cause runtime crashes if formData becomes
null; add a one-line defensive guard at the start of handleSubmit that returns
early when formData is null/undefined (e.g. if (!formData) return) before
building the UpdateModuleInput and calling parseCommaSeparated for mentorLogins,
so all references to formData inside handleSubmit are safe; update any dependent
logic in handleSubmit (e.g., building input, isAdmin check) to assume formData
exists after the guard.
Contributor
Author
|
@kasya, please review it and let me know if any changes are required. |
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.
Proposed change
Resolves #3959
Improved frontend test coverage to 99.91%
Checklist
make check-testlocally: all warnings addressed, tests passed