CI/CD: Refresh Frontend Test Suites#1056
Merged
rahulharpal1603 merged 9 commits intoMay 30, 2026
Merged
Conversation
rohan-pandeyy
changed the title
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:
WalkthroughAdds extensive frontend test scaffolding and deterministic mocks (Tauri, Axios, window.matchMedia), shared React test utilities/providers, many new unit/integration tests for pages and components, exports a combined Redux Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/__tests__/Sidebar.test.tsx`:
- Around line 21-28: Update the test assertions in Sidebar.test.tsx to use the
ROUTES constants instead of hard-coded string paths: replace assertions that
expect literal paths like "/settings", "/home", "/ai-tagging", "/favourites",
"/videos", "/albums", "/memories" with assertions that build the expected path
from ROUTES (e.g., `"/" + ROUTES.HOME` or `"/" + ROUTES.SETTINGS` as
appropriate) so the tests reference the same source-of-truth ROUTES constants
used by the Route definitions; update every navigation assertion in the file
that currently uses string literals to use the ROUTES.* symbols to avoid
coupling to hard-coded paths.
🧹 Nitpick comments (1)
frontend/src/components/__tests__/Sidebar.test.tsx (1)
54-149: Consider table‑driven navigation tests to reduce repetition.The interaction tests follow the same pattern; a data‑driven approach trims boilerplate and makes it easier to add new routes.
♻️ Example refactor using
test.each+ const navCases = [ + { label: 'Settings', start: '/', target: ROUTES.SETTINGS, page: 'Settings Page' }, + { label: 'Home', start: ROUTES.SETTINGS, target: ROUTES.HOME, page: 'Home Page' }, + { label: 'AI Tagging', start: '/', target: ROUTES.AI, page: 'AI Tagging Page' }, + { label: 'Favourites', start: '/', target: ROUTES.FAVOURITES, page: 'Favourites Page' }, + { label: 'Videos', start: '/', target: ROUTES.VIDEOS, page: 'Videos Page' }, + { label: 'Albums', start: '/', target: ROUTES.ALBUMS, page: 'Albums Page' }, + { label: 'Memories', start: '/', target: ROUTES.MEMORIES, page: 'Memories Page' }, + ]; + - test('clicking Settings link navigates to /settings', async () => { - const user = userEvent.setup(); - render(<SidebarWithRoutes />, { initialRoutes: ['/'] }); - expect(screen.getByTestId('location-display')).toHaveTextContent('/'); // start location - await user.click(screen.getByText('Settings')); // click settings - expect(screen.getByTestId('location-display')).toHaveTextContent('/settings'); - expect(screen.getByText('Settings Page')).toBeInTheDocument(); - }); + test.each(navCases)( + 'clicking %s navigates to %s', + async ({ label, start, target, page }) => { + const user = userEvent.setup(); + render(<SidebarWithRoutes />, { initialRoutes: [start] }); + await user.click(screen.getByText(label)); + expect(screen.getByTestId('location-display')).toHaveTextContent(target); + expect(screen.getByText(page)).toBeInTheDocument(); + }, + );
Contributor
Author
|
Updated PR description. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/package.json`:
- Line 91: Remove the unnecessary direct devDependency
"baseline-browser-mapping" from package.json: open package.json, locate the
dependency entry "baseline-browser-mapping": "^2.9.19" and delete that line so
it is no longer listed under devDependencies; rely on the transitive dependency
provided by browserslist and run a quick npm/yarn install and lockfile update
(npm install or yarn install) to ensure the lockfile is updated accordingly.
In `@frontend/src/components/__tests__/ThemeToggle.test.tsx`:
- Around line 1-2: Tests in ThemeToggle.test.tsx are racing against Radix UI
portal mounting and exit animations; update the test to use async-safe queries
and waits: replace immediate synchronous queries (e.g., getByText / queryByText)
with await findByText to wait for elements to appear and wrap presence/absence
assertions in waitFor to wait for state/animation transitions after
userEvent.click on the ThemeToggle component; target the ThemeToggle test
utilities and any assertions that check the dropdown/menu text to ensure they
use findByText and waitFor around visibility checks.
Consolidates and stabilizes frontend tests: suppress console.log in jest.setup to reduce noise; replace repetitive Sidebar navigation specs with a parameterized test.each suite and fix route usage; refactor ThemeToggle tests to use parameterized cases, findByText/waitFor assertions for more reliability. Adds comprehensive SettingsPage unit tests covering interactions and state changes. Removes a redundant matchMedia mock from allPages tests. Updates test-utils to adjust react-query defaults (disable retries, set gc/stale times) and return the test store and queryClient alongside the render result for easier test introspection.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/pages/__tests__/SettingsPage.test.tsx`:
- Around line 119-126: A lint violation is caused by the "TODO" comment in
SettingsPage.test.tsx; remove or modify it: either (a) add an inline suppression
by placing "// eslint-disable-next-line no-warning-comments" directly above the
TODO comment, (b) move the note into an external tracker (create a GitHub issue
or README entry) and delete the comment, or (c) reword the comment to avoid the
TODO keyword (e.g., change to "NOTE" or "FUTURE") so the lint rule no longer
triggers; target the comment block in
frontend/src/pages/__tests__/SettingsPage.test.tsx that lists "System
integrations (future scope)".
Add comprehensive Jest tests for several frontend pieces: new tests for DeleteImageDialog (rendering and yes/no interactions), EmptyStates (gallery and AI tagging), GlobalLoader (conditional rendering and messages), and InfoDialog (rendering, variants, and close action updating store). Also add lib/utils tests for cn and getErrorMessage (including Axios-like errors) and dateUtils tests for getTimeAgo (uses fake timers) and groupImagesByYearMonthFromMetadata (grouping and edge cases). Minor comment wording tweak in SettingsPage.test.tsx.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/components/__tests__/DeleteImageDialog.test.tsx (1)
23-37: Extract duplicate confirmation text to a constant.The confirmation text string is duplicated across tests. Extract it to reduce duplication and simplify future maintenance if the text changes.
♻️ Proposed fix
describe('DeleteImagesDialog', () => { const mockSetIsOpen = jest.fn(); const mockExecuteDeleteImages = jest.fn(); + const confirmationText = 'Do you also want to delete these images from Device ?'; const defaultProps = { isOpen: true, setIsOpen: mockSetIsOpen, executeDeleteImages: mockExecuteDeleteImages, }; // ... describe('Rendering', () => { test('renders confirmation text when open', () => { render(<DeleteImagesDialog {...defaultProps} />); - expect( - screen.getByText( - 'Do you also want to delete these images from Device ?', - ), - ).toBeInTheDocument(); + expect(screen.getByText(confirmationText)).toBeInTheDocument(); }); test('does not render content when closed', () => { render(<DeleteImagesDialog {...defaultProps} isOpen={false} />); - expect( - screen.queryByText( - 'Do you also want to delete these images from Device ?', - ), - ).not.toBeInTheDocument(); + expect(screen.queryByText(confirmationText)).not.toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/DeleteImageDialog.test.tsx` around lines 23 - 37, Extract the duplicated confirmation string used in the tests into a shared constant (e.g., CONFIRMATION_TEXT) at the top of the DeleteImageDialog.test.tsx file and replace the two inline occurrences inside the expect/getByText and queryByText calls with that constant; update any other tests in this file that use the same literal to reference CONFIRMATION_TEXT so all assertions use the single source of truth for the message shown by the DeleteImagesDialog component.frontend/src/components/__tests__/GlobalLoader.test.tsx (1)
17-18: Remove the redundant inline comment in this assertion block.The comment on Line 17 restates exactly what Line 18 already asserts, so it adds noise without clarifying intent.
As per coding guidelines, "Point out redundant obvious comments that do not add clarity to the code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/GlobalLoader.test.tsx` around lines 17 - 18, The inline comment that duplicates the assertion is redundant; remove the comment line that says "renders an empty div" above the assertion expect(container.firstChild).toBeEmptyDOMElement() in the GlobalLoader test so the test reads only the assertion (or replace it with a brief intent-focused comment if you want to add clarifying context).frontend/src/lib/__tests__/utils.test.ts (1)
54-75: Add single-field Axios payload cases to close a branch-coverage gap.Current cases validate
{ error, message }together, but not payloads containing only one of them. Adding those two cases would lock in expected formatting behavior.📌 Suggested test additions
+ test.each([ + [{ error: 'Not Found' }, 'Not Found'], + [{ message: 'Image does not exist' }, 'Image does not exist'], + ])( + 'extracts single-field Axios response data correctly', + (responseData, expected) => { + const axiosErr = fakeAxiosError('Request failed', 'ERR_BAD_REQUEST', responseData); + expect(getErrorMessage(axiosErr)).toBe(expected); + }, + );As per coding guidelines, "Ensure that test code is automated, comprehensive, and follows testing best practices."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/__tests__/utils.test.ts` around lines 54 - 75, Add two unit tests in frontend/src/lib/__tests__/utils.test.ts using the existing fakeAxiosError and getErrorMessage helpers to cover single-field response payloads: one where the response data contains only { error: 'Not Found' } and assert getErrorMessage(...) returns 'Not Found', and one where the response data contains only { message: 'Image does not exist' } and assert getErrorMessage(...) returns 'Image does not exist'; place these alongside the existing AxiosError tests so the branch where only one field is present is exercised.
🤖 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/src/components/__tests__/InfoDialog.test.tsx`:
- Around line 59-68: Replace the unsafe cast by importing and using the explicit
RootState type from your store and type the retrieved state (e.g. const state =
store.getState() as RootState) instead of using any; also stop inspecting the
implementation detail data-slot attribute—locate the close button via a
user-facing query by finding the dialog element (e.g. the element returned by
screen.getByRole('dialog') or the component wrapper) and then using
within(dialog).getByRole('button', { name: /close/i }) (or an equivalent
user-facing selector) before calling user.click on that button; update
references to store.getState and infoDialog accordingly.
In `@frontend/src/test-utils.tsx`:
- Around line 10-13: Replace weak ReturnType<typeof configureStore> usage with
an explicit Redux store type to preserve RootState typing: change the
CustomRenderOptions property "store?: ReturnType<typeof configureStore>" to
"store?: EnhancedStore<RootState, UnknownAction>" (and update the same typing at
the other occurrences and the test render return type). Also add imports for
EnhancedStore and UnknownAction from `@reduxjs/toolkit` and ensure any factory
function that creates the test store returns EnhancedStore<RootState,
UnknownAction> so callers can use store.getState() without casting.
---
Nitpick comments:
In `@frontend/src/components/__tests__/DeleteImageDialog.test.tsx`:
- Around line 23-37: Extract the duplicated confirmation string used in the
tests into a shared constant (e.g., CONFIRMATION_TEXT) at the top of the
DeleteImageDialog.test.tsx file and replace the two inline occurrences inside
the expect/getByText and queryByText calls with that constant; update any other
tests in this file that use the same literal to reference CONFIRMATION_TEXT so
all assertions use the single source of truth for the message shown by the
DeleteImagesDialog component.
In `@frontend/src/components/__tests__/GlobalLoader.test.tsx`:
- Around line 17-18: The inline comment that duplicates the assertion is
redundant; remove the comment line that says "renders an empty div" above the
assertion expect(container.firstChild).toBeEmptyDOMElement() in the GlobalLoader
test so the test reads only the assertion (or replace it with a brief
intent-focused comment if you want to add clarifying context).
In `@frontend/src/lib/__tests__/utils.test.ts`:
- Around line 54-75: Add two unit tests in
frontend/src/lib/__tests__/utils.test.ts using the existing fakeAxiosError and
getErrorMessage helpers to cover single-field response payloads: one where the
response data contains only { error: 'Not Found' } and assert
getErrorMessage(...) returns 'Not Found', and one where the response data
contains only { message: 'Image does not exist' } and assert
getErrorMessage(...) returns 'Image does not exist'; place these alongside the
existing AxiosError tests so the branch where only one field is present is
exercised.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/components/__tests__/DeleteImageDialog.test.tsxfrontend/src/components/__tests__/EmptyStates.test.tsxfrontend/src/components/__tests__/GlobalLoader.test.tsxfrontend/src/components/__tests__/InfoDialog.test.tsxfrontend/src/lib/__tests__/utils.test.tsfrontend/src/pages/__tests__/SettingsPage.test.tsxfrontend/src/test-utils.tsxfrontend/src/utils/__tests__/dateUtils.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/tests/SettingsPage.test.tsx
|
Hello 👋 |
Contributor
Author
|
Hey @JITENDRAGAHERWAR03, thanks for your interest in contributing! Just a couple of quick pointers:
That said, if you're comfortable contributing in TypeScript as well, feel free to leave a comment on issue #889. |
Contributor
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
Contributor
|
@rohan-pandeyy Is this PR still in development? otherwise I can move forward in removing the conflicts in this one. |
Contributor
Author
|
@akshajtiwari, don't worry, I'll take care of this one. You can focus on the others👍 |
rahulharpal1603
approved these changes
May 30, 2026
github-project-automation
Bot
moved this from Todo
to Done
in Issues tracker for PictoPy
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
4 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.
closes #889
Description
Addressing the need for a reliable, modern testing baseline for the frontend. This PR introduces a layered frontend testing strategy and incrementally expands it with navigation interaction sanity tests.
The intent is to provide fast, stable and high-signal checks.
Changes
Infrastructure
src/test-utils.tsxwith acustomRenderfunction that wraps components in Redux, Router, and Theme providers.src/app/store.tsand test utils to create a fresh Redux store for every test run. prevents state leakage.New Tests
Page Sanity Tests
Sidebar Navigation
Checklist
Summary by CodeRabbit
Tests
Refactor
Chores