Feature/communit snapshots page#1130
Conversation
Summary by CodeRabbit
WalkthroughThis pull request introduces changes to enhance snapshot-related functionality in both the backend and frontend. The backend modifies the GraphQL query by renaming the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
frontend/src/types/seo.ts (1)
27-27: Fix formatting inconsistency in property declaration.The new
snapshotproperty has inconsistent spacing around the colon compared to other properties in the interface. Also consider making it plural for consistency with other properties (chapters, projects, etc.).- snapshot : PageMetadata + snapshot: PageMetadatafrontend/src/api/queries/snapshotQueries.ts (1)
61-61: Consider adding pagination options.The query is hardcoded to fetch 24 snapshots. Consider adding pagination parameters (like offset/cursor) to handle cases where you need to load more snapshots or adjust the limit dynamically.
- snapshots(limit: 24) { + snapshots(limit: $limit, offset: $offset) {And update the query definition to include variables:
- query GetCommunitySnapshots { + query GetCommunitySnapshots($limit: Int = 24, $offset: Int = 0) {frontend/src/types/snapshot.ts (1)
21-26: Rename interface to follow singular naming convention.TypeScript interfaces typically use singular names when they represent a single entity. Consider renaming
SnapshotstoSnapshotfor consistency with naming conventions. Also, consider extending from or reusing the existingSnapshotDetailsPropsinterface since they share common properties.-export interface Snapshots { +export interface Snapshot { endAt: string key: string startAt: string title: string }Alternatively, define a base interface that both can extend:
export interface BaseSnapshot { endAt: string key: string startAt: string title: string } export interface Snapshot extends BaseSnapshot {} export interface SnapshotDetailsProps extends BaseSnapshot { newReleases: ReleaseType[] newProjects: ProjectTypeGraphql[] newChapters: ChapterTypeGraphQL[] }frontend/src/utils/metadata.ts (1)
41-46: Good addition for the new snapshots featureThe metadata entry for snapshots follows the established pattern in the codebase, which is excellent for consistency.
Consider enhancing the description to be more informative about what snapshots represent, perhaps something like "Community snapshots of OWASP contributions and activities" to give users better context.
frontend/src/index.css (2)
121-135: Clean implementation of dropdown stylingThe dropdown styling follows best practices with proper positioning context, transitions for smooth interactions, and consistent use of Tailwind's utility classes.
The hover-based implementation works well for desktop experiences. If mobile support is important, you may want to consider adding touch-friendly interactions as hover doesn't work the same way on touch devices.
136-136: Remove extra empty lineThere's an unnecessary blank line that could be removed for cleaner code.
backend/apps/owasp/graphql/queries/snapshot.py (1)
30-32: Update the docstring to match the method name.The method has been renamed from
resolve_recent_snapshotstoresolve_snapshots, but the docstring still says "Resolve recent snapshots." It should be updated for consistency.def resolve_snapshots(root, info, limit): - """Resolve recent snapshots.""" + """Resolve snapshots.""" return Snapshot.objects.order_by("-created_at")[:limit]frontend/__tests__/unit/pages/Snapshots.test.tsx (1)
1-111: Consider adding accessibility tests.While the current test suite covers many functional aspects, consider adding tests for accessibility concerns such as keyboard navigation and proper ARIA attributes.
frontend/src/pages/Snapshots.tsx (1)
67-70: Use optional chaining for better code readability.The static analysis tool correctly suggests using optional chaining here for better readability and safety.
- {snapshots && - snapshots.map((snapshot: Snapshots) => ( - <div key={snapshot.key}>{renderSnapshotCard(snapshot)}</div> - ))} + {snapshots?.map((snapshot: Snapshots) => ( + <div key={snapshot.key}>{renderSnapshotCard(snapshot)}</div> + ))}🧰 Tools
🪛 Biome (1.9.4)
[error] 67-70: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/apps/owasp/graphql/queries/snapshot.py(2 hunks)frontend/__tests__/unit/pages/Snapshots.test.tsx(1 hunks)frontend/src/App.tsx(2 hunks)frontend/src/api/queries/snapshotQueries.ts(1 hunks)frontend/src/components/Header.tsx(1 hunks)frontend/src/components/SnapshotCard.tsx(1 hunks)frontend/src/index.css(1 hunks)frontend/src/pages/Snapshots.tsx(1 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/link.ts(1 hunks)frontend/src/types/seo.ts(1 hunks)frontend/src/types/snapshot.ts(1 hunks)frontend/src/utils/constants.ts(1 hunks)frontend/src/utils/metadata.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
frontend/src/App.tsx (3)
frontend/src/types/snapshot.ts (1) (1)
Snapshots(21:26)frontend/src/types/snapshot.ts (1) (1)
Snapshots(21:26)frontend/src/types/snapshot.ts (1) (1)
Snapshots(21:26)
frontend/src/components/SnapshotCard.tsx (4)
frontend/src/types/card.ts (1) (1)
SnapshotCardProps(56:62)frontend/src/types/card.ts (1) (1)
SnapshotCardProps(56:62)frontend/src/types/card.ts (1) (1)
SnapshotCardProps(56:62)frontend/src/types/card.ts (1) (1)
SnapshotCardProps(56:62)
frontend/src/components/Header.tsx (1)
frontend/src/utils/constants.ts (1) (1)
headerLinks(4:24)
🪛 GitHub Actions: Run CI/CD
frontend/src/api/queries/snapshotQueries.ts
[error] 1-1: pre-commit hook 'end-of-file-fixer' failed. Files were modified by this hook.
frontend/src/components/Header.tsx
[error] 1-1: pre-commit hook 'trailing-whitespace' failed. Files were modified by this hook.
🪛 Biome (1.9.4)
frontend/src/pages/Snapshots.tsx
[error] 67-70: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (21)
frontend/src/types/link.ts (1)
3-5: Well-designed interface changes to support navigation hierarchyMaking
hrefoptional and adding thesubmenuproperty enables a flexible navigation structure that supports dropdowns.One thing to consider: with both properties being optional, it's technically possible to create a Link with neither an href nor a submenu. You might want to add validation elsewhere to ensure at least one is provided for each link.
frontend/src/utils/constants.ts (1)
15-18:Details
❓ Verification inconclusive
Good organization of navigation structure
The change to add a submenu under "Community" is well-implemented and creates a logical grouping of related links. The structure aligns with the interface changes and new dropdown styling.
I'd recommend verifying that the Header component correctly implements this new navigation structure with the dropdown menu.
🏁 Script executed:
#!/bin/bash # Check if the Header component has been updated to handle submenu rendering rg -A 5 "submenu" --type=tsx --glob="**/Header*"Length of output: 76
Action: Manual verification needed for the Header dropdown implementation
The submenu additions in
frontend/src/utils/constants.tsare well organized and align with the new navigation structure. However, the initial verification using thergcommand with--type=tsxreturned an error ("unrecognized file type: tsx"). This prevents us from confirming that theHeadercomponent (likely defined in a.tsxfile, e.g.,Header.tsx) has been updated to properly render the submenu.To resolve this, please verify that the updated
Headercomponent implements the dropdown behavior. For example, you might re-run the following command:rg -A 5 "submenu" --glob="*Header*.tsx"Alternatively, perform a manual inspection of the relevant Header component file(s) to ensure that the submenu logic is correctly integrated.
frontend/src/components/SnapshotCard.tsx (1)
1-40: Well-structured component implementation!The SnapshotCard component is cleanly implemented with proper prop typing, responsive design patterns, and meaningful visual feedback mechanisms. The hover effects provide good user interaction cues, and the layout hierarchy is clear.
frontend/src/App.tsx (2)
14-14: Correct import addition.The Snapshots component is properly imported from the pages directory.
51-51: Logical route placement.The route for community snapshots is correctly positioned alongside other community-related routes, maintaining a consistent routing structure.
backend/apps/owasp/graphql/queries/snapshot.py (1)
18-18: Field renaming looks good.Changing from
recent_snapshotstosnapshotsmakes the API more clear and consistent.frontend/__tests__/unit/pages/Snapshots.test.tsx (8)
1-21: Well structured test setup with proper mocks.The test file correctly imports and mocks the necessary dependencies for testing the Snapshots component. The approach of mocking external dependencies like
useQuery,useNavigate, andtoastis a good practice for isolated unit testing.
22-26: Good test organization with proper cleanup.The test suite is well organized with a proper
describeblock, and includes anafterEachhook to clear mocks, which is important for maintaining test isolation between test cases.
27-33: Loading state correctly tested.This test properly verifies that a loading spinner is displayed when data is being fetched. Good practice to test the component's loading behavior.
35-44: Data rendering test is concise and effective.The test correctly verifies that snapshots are properly rendered after data is loaded. Using
waitForis appropriate for asynchronous rendering.
46-56: Error handling test is well implemented.This test correctly verifies that error notifications are displayed when a GraphQL error occurs. It properly checks that the toast function is called with the expected parameters.
58-72: Navigation test is well structured.The test effectively verifies that clicking the "View Details" button navigates to the correct snapshot details page. Good use of
waitForto wait for async rendering before attempting to interact with elements.
74-82: Good test for empty state handling.The test verifies that an appropriate message is displayed when no snapshots are available. This is important for a good user experience.
98-111: Good test for fallback title.The test correctly verifies that a fallback title is displayed when a snapshot lacks a title. This helps ensure a good user experience even with incomplete data.
frontend/src/pages/Snapshots.tsx (7)
1-10: Well organized imports.The imports are properly organized and include all necessary dependencies for the component.
11-15: State management looks good.The component properly initializes state for snapshots and loading status, and sets up the GraphQL query.
16-29: Data fetching logic is well implemented.The useEffect hook correctly updates component state based on GraphQL query results and handles errors appropriately by showing a toast notification.
31-35: Navigation logic is clear and concise.The navigation setup is well implemented, using React Router's useNavigate hook to navigate to the snapshot details page.
37-53: Card rendering logic is clean and reusable.The renderSnapshotCard function is well structured and provides a clean way to render snapshot cards with consistent styling and behavior.
55-61: Loading state rendering is user-friendly.The component shows a loading spinner while data is being fetched, which provides good feedback to users during the loading process.
1-77:Details
❓ Verification inconclusive
Add pagination functionality.
The test file includes a test for pagination, but the component doesn't implement any pagination controls. Consider adding pagination to handle large numbers of snapshots.
Let's check if the API returns pagination information:
🏁 Script executed:
#!/bin/bash # Check if the GraphQL query includes pagination parameters grep -A 5 "GET_COMMUNITY_SNAPSHOTS" src/api/queries/snapshotQueries.tsLength of output: 138
Action Required: Add pagination controls for Snapshots Page.
The test suite includes a pagination test, but the component currently lacks any pagination-related UI or logic. Additionally, our initial search for the
GET_COMMUNITY_SNAPSHOTSquery file at the expected path (src/api/queries/snapshotQueries.ts) did not succeed. This raises the following points for clarification:
- GraphQL Query Verification: Verify if the
GET_COMMUNITY_SNAPSHOTSquery—presumably responsible for fetching snapshots—actually supports pagination (e.g., by exposing parameters such as limit, offset, or page). It would be helpful to confirm the query’s definition and parameters from its actual location if it exists elsewhere.- Component Enhancements: If pagination is supported by the backend/API, please implement the necessary pagination controls (e.g., buttons or infinite scrolling) in the component to manage large sets of snapshots.
- Testing Alignment: Ensure that the component’s behavior aligns with the pagination expectations defined in the test suite. If the backend does not support pagination, consider updating the expected behavior in the tests.
Please verify the source and capabilities of the GraphQL query and adjust the Snapshots Page implementation accordingly.
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-70: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/__tests__/unit/pages/Snapshots.test.tsx(1 hunks)frontend/src/api/queries/snapshotQueries.ts(1 hunks)frontend/src/components/Header.tsx(2 hunks)frontend/src/components/SnapshotCard.tsx(1 hunks)frontend/src/components/UserCard.tsx(1 hunks)frontend/src/pages/Snapshots.tsx(1 hunks)frontend/src/pages/index.ts(2 hunks)frontend/src/types/card.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/api/queries/snapshotQueries.ts
- frontend/src/types/card.ts
- frontend/src/components/Header.tsx
- frontend/src/components/SnapshotCard.tsx
🧰 Additional context used
🪛 GitHub Actions: Run CI/CD
frontend/src/components/UserCard.tsx
[error] 11-11: Trailing whitespace found. Please remove unnecessary whitespace at the end of the line.
🔇 Additional comments (8)
frontend/src/pages/index.ts (1)
17-17: LGTM: Clean integration of the new Snapshots pageThe import and export of the Snapshots component are correctly added to the barrel file, maintaining the established pattern for exposing pages.
Also applies to: 31-31
frontend/src/components/UserCard.tsx (1)
10-10: Good responsive design improvementChanging from fixed dimensions to responsive classes improves the component's adaptability across different screen sizes.
frontend/__tests__/unit/pages/Snapshots.test.tsx (3)
1-23: LGTM: Well-structured test setupThe test file has proper imports and mocks set up for all the necessary dependencies, which is crucial for isolated unit testing.
24-58: Good test coverage for core functionalityThe tests effectively verify loading state, successful data rendering, and error handling scenarios, which are critical paths for this component.
76-98: LGTM: Comprehensive edge case handlingThe tests for empty state and fallback title ensure the component handles edge cases gracefully, providing a good user experience.
frontend/src/pages/Snapshots.tsx (3)
11-36: LGTM: Well-structured component setup and navigationThe component has well-organized state management, proper GraphQL query integration, and clean navigation handling. The error handling with toast notifications is a good practice.
37-53: LGTM: Clean rendering functionThe
renderSnapshotCardfunction is well-implemented, creating a consistent card with proper navigation functionality.
55-77: LGTM: Good UI handling for different statesThe component properly handles loading state with a spinner and displays a meaningful message when no snapshots are found. The responsive grid layout is well-designed.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/Snapshots.test.tsx(1 hunks)
🔇 Additional comments (9)
frontend/__tests__/unit/pages/Snapshots.test.tsx (9)
1-7: Clean import structure for testing.The imports are well organized, bringing in necessary testing utilities, component dependencies, and mock data. Good practice to separate test library imports from application imports.
8-22: Comprehensive mock setup for dependencies.The mocks for Apollo Client, React Router, and the toaster component are properly implemented. Using
jest.requireActual()ensures that only specific functions are mocked while preserving the rest of the module's functionality.
24-28: Good test organization with proper cleanup.The afterEach hook ensures that all mocks are reset between tests, which prevents test contamination and improves test isolation.
29-35: Loading state test looks good.The test correctly verifies that a loading spinner is displayed when the component is in a loading state.
37-46: Data rendering test is comprehensive.The test effectively verifies that both snapshot titles are rendered after data is loaded, providing good coverage of the success path.
60-74: Navigation test implemented correctly.The test properly verifies that clicking on the "View Details" button navigates to the correct snapshot details page. The implementation correctly uses
mockReturnValuefor the navigate function, addressing a previous review comment.
76-84: Empty state test is well implemented.The test effectively verifies that the "No Snapshots found" message is displayed when there are no snapshots available.
86-98: Fallback title test provides good coverage.The test correctly verifies that a fallback title is displayed when a snapshot lacks a title, which is a good edge case to test.
1-99:Details
❓ Verification inconclusive
Missing pagination test mentioned in previous reviews.
The pagination test that was mentioned in a past review comment appears to be completely removed rather than fixed. If pagination is still a feature of the Snapshots component, you should include a test for it.
If pagination functionality exists in the component, please add a test following this pattern:
test('handles pagination correctly', async () => { const navigateMock = jest.fn(); (useNavigate as jest.Mock).mockReturnValue(navigateMock); // Mock with pagination data (useQuery as jest.Mock).mockReturnValue({ data: { snapshots: mockSnapshotDetailsData, totalPages: 2, currentPage: 1 }, loading: false, error: null }); render(<Snapshots />); // Find and click pagination controls await waitFor(() => { const nextPageButton = screen.getByText('Next Page'); fireEvent.click(nextPageButton); }); // Verify navigation or page change expect(navigateMock).toHaveBeenCalledWith(expect.stringContaining('page=2')); });Run the following script to check if pagination is implemented in the Snapshots component:
🏁 Script executed:
#!/bin/bash # Check if pagination is implemented in the Snapshots component echo "Checking for pagination implementation in Snapshots component..." rg -A 3 "pagination|Pagination|page" --glob "frontend/src/pages/Snapshots.tsx"Length of output: 217
Pagination Testing Verification Needed
The current test suite does not include any pagination-related tests, and our search for pagination indicators in the Snapshots component did not return any evidence of pagination functionality. If pagination is still intended as a feature in the component, please verify its implementation manually and update the test suite accordingly. For example, if pagination is supported, you might add a test similar to the following:
test('handles pagination correctly', async () => { const navigateMock = jest.fn(); (useNavigate as jest.Mock).mockReturnValue(navigateMock); // Mock with pagination data (useQuery as jest.Mock).mockReturnValue({ data: { snapshots: mockSnapshotDetailsData, totalPages: 2, currentPage: 1 }, loading: false, error: null }); render(<Snapshots />); // Find and click pagination controls await waitFor(() => { const nextPageButton = screen.getByText('Next Page'); fireEvent.click(nextPageButton); }); // Verify navigation or page change expect(navigateMock).toHaveBeenCalledWith(expect.stringContaining('page=2')); });If, however, the Snapshots component is not expected to support pagination at this time, please confirm that this removal was intentional. Manual verification is required to ensure the component’s functionality aligns with the product requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/__tests__/unit/pages/Snapshots.test.tsx (2)
54-64:⚠️ Potential issueFix inconsistency in error handling test.
The test expects
toasterto be called directly, but the mock setup is fortoaster.create. This inconsistency will cause the test to fail.- expect(toaster).toHaveBeenCalledWith( + expect(toaster.create).toHaveBeenCalledWith( expect.objectContaining({ title: 'GraphQL Request Failed' }) );
66-80:⚠️ Potential issueFix the useNavigate mock implementation.
The navigation test has an incorrect mock implementation that will cause the test to fail.
- (useNavigate as jest.Mock)(navigateMock); + (useNavigate as jest.Mock).mockReturnValue(navigateMock);
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/Snapshots.test.tsx (1)
1-109: Consider adding pagination tests.The test suite lacks tests for pagination functionality, which would be important for a snapshots listing page.
Consider adding a test that verifies pagination behavior, similar to what was mentioned in past reviews:
test('handles pagination correctly', async () => { // Mocking the return value with snapshot data and pagination (useQuery as jest.Mock).mockReturnValue({ data: { snapshots: mockSnapshotDetailsData, totalPages: 2 }, loading: false, error: null }); const navigateMock = jest.fn(); (useNavigate as jest.Mock).mockReturnValue(navigateMock); render(<Snapshots />); await waitFor(() => { const nextPageButton = screen.getByText('Next Page'); fireEvent.click(nextPageButton); }); // Verify navigation or page change behavior expect(navigateMock).toHaveBeenCalledWith(expect.stringContaining('page=2')); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/pages/Snapshots.test.tsx(1 hunks)frontend/src/pages/Snapshots.tsx(1 hunks)frontend/src/pages/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/pages/index.ts
- frontend/src/pages/Snapshots.tsx
🔇 Additional comments (4)
frontend/__tests__/unit/pages/Snapshots.test.tsx (4)
1-24: Good setup of mock dependencies.The imports and mock setup for Apollo client, React Router, and the toaster component are well-structured, providing clear isolation for your tests.
26-29: Good test hygiene with afterEach cleanup.Using
jest.clearAllMocks()after each test ensures proper isolation between tests and prevents potential mock state interference.
31-52: Well-structured loading and data rendering tests.The tests for loading state and successful data rendering are properly implemented, with clear expectations that test the component's behavior.
82-108: Good edge case testing.Your tests for handling empty data and fallback titles demonstrate thorough consideration of edge cases that might occur in production.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/__tests__/unit/pages/Snapshots.test.tsx (2)
54-68:⚠️ Potential issueIssue with toaster verification in error test.
The test expects
toasterto be called directly, but your mock setup is fortoaster.create. This inconsistency will cause the test to fail.- expect(toaster).toHaveBeenCalledWith( + expect(toaster.create).toHaveBeenCalledWith( expect.objectContaining({ title: 'GraphQL Request Failed' }) )
70-88:⚠️ Potential issueFix the useNavigate mock implementation.
The navigation test has an incorrect mock implementation that will cause the test to fail.
- (useNavigate as jest.Mock)(navigateMock) + (useNavigate as jest.Mock).mockReturnValue(navigateMock)
🧹 Nitpick comments (2)
frontend/__tests__/unit/pages/Snapshots.test.tsx (2)
26-117: Missing pagination test.The test suite doesn't include a test for the pagination functionality. Based on previous review comments, it appears a pagination test was implemented but had issues and was subsequently removed. Consider adding a fixed version of the pagination test to ensure this functionality is covered.
test('handles pagination correctly', async () => { // Mocking the return value with snapshot data and pagination (useQuery as jest.Mock).mockReturnValue({ data: { snapshots: mockSnapshotDetailsData, totalPages: 2, currentPage: 1 }, loading: false, error: null }); const navigateMock = jest.fn(); (useNavigate as jest.Mock).mockReturnValue(navigateMock); render(<Snapshots />); await waitFor(() => { const nextPageButton = screen.getByText('Next Page'); fireEvent.click(nextPageButton); }); // Verify navigation to the next page URL expect(navigateMock).toHaveBeenCalledWith(expect.stringContaining('page=2')); });
1-117: Overall test coverage is good, but add type definitions.The test file provides good coverage of component rendering states, error handling, and user interactions. Consider adding TypeScript type definitions for the mock data and return values to improve type safety.
For example:
- (useQuery as jest.Mock).mockReturnValue({ + (useQuery as jest.Mock<any, any>).mockReturnValue({ data: { snapshots: mockSnapshotDetailsData }, loading: false, error: null, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/pages/Snapshots.test.tsx(1 hunks)frontend/src/pages/Snapshots.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/Snapshots.tsx
🔇 Additional comments (2)
frontend/__tests__/unit/pages/Snapshots.test.tsx (2)
18-24: The toaster mock implementation is correctly set up.The mock includes both the
createmethod and default export, which addresses the issues mentioned in previous reviews. The__esModule: trueflag ensures proper module interoperability.
31-37: Good test for loading state.This test effectively verifies that the loading spinner is displayed while data is being fetched. Using the role-based query is a best practice for accessibility-oriented testing.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please follow https://github.com/OWASP/Nest/blob/main/CONTRIBUTING.md#contributing-workflow -- make sure style and test checks are green at your localhost.
Yes, sure. i just created the draft PR will push the final changes by EOD. |
|
Working on the failing test cases will update soon |
I am working on it, can you wait till EOD? |
Yes, as long as it's not abandoned I can keep it opened. |
Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/__tests__/unit/App.test.tsx (1)
18-18: Add consistent testid naming for SnapshotsPage mockThe testid "snapshot-page" doesn't follow the pattern used by other mocked pages. Most other pages use a testid that matches their component name (like "projects-page" for ProjectsPage).
- SnapshotsPage: () => <div data-testid="snapshot-page">Snapshots Page</div>, + SnapshotsPage: () => <div data-testid="snapshots-page">Snapshots Page</div>,This will ensure consistent testid naming convention across all page components.
frontend/__tests__/unit/pages/Snapshots.test.tsx (3)
65-65: Remove commented codeThis line appears to be leftover commented code. Either uncomment it if it's meant to be part of the test, or remove it entirely to maintain clean test code.
- // expect(screen.getByText('Snapshot 2')).toBeInTheDocument();
99-105: Remove redundant test caseThis test appears to be redundant with the test on lines 60-67, as both are checking for the presence of 'Snapshot 1' in the document. Consider removing this duplicated test to maintain a clean and focused test suite.
- it('renders a specific snapshot title when data is fetched successfully', async () => { - render(<SnapshotsPage />) - - await waitFor(() => { - expect(screen.getByText('Snapshot 1')).toBeInTheDocument() - }) - })
123-123: Focus on behavior over implementation detailsTesting the exact number of times a mock is called makes the test more brittle. Consider removing this assertion and focusing instead on whether the navigation occurred correctly.
- expect(navigateMock).toHaveBeenCalledTimes(1) // + // Only check that navigation happened with the correct path
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/__tests__/unit/App.test.tsx(1 hunks)frontend/__tests__/unit/data/mockSnapshotData.ts(1 hunks)frontend/__tests__/unit/pages/Snapshots.test.tsx(1 hunks)frontend/src/App.tsx(2 hunks)frontend/src/pages/Snapshots.tsx(1 hunks)frontend/src/pages/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/pages/index.ts
- frontend/src/App.tsx
- frontend/src/pages/Snapshots.tsx
🧰 Additional context used
🧬 Code Definitions (1)
frontend/__tests__/unit/pages/Snapshots.test.tsx (1)
frontend/src/pages/index.ts (1)
SnapshotsPage(28-28)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
frontend/__tests__/unit/data/mockSnapshotData.ts (1)
89-98: Good implementation of mockSnapshotDataThe mock data structure is appropriate and follows the expected pattern for snapshot data. The exported mock aligns well with the existing patterns in the codebase.
frontend/__tests__/unit/pages/Snapshots.test.tsx (1)
34-127: Well-structured test suite with good coverageThe test suite is well-organized and covers all the important scenarios: loading state, successful data rendering, empty state, error handling, and navigation. The setup/teardown logic is appropriate, and the tests follow good practices of isolating behaviors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/__tests__/unit/pages/Snapshots.test.tsx (4)
1-8: The imports look good, but you might want to consider adding test-id imports.The test file is well-structured with all necessary imports. Consider using data-testid attributes for more reliable element selection in your tests, which would require importing
getByTestIdalongside the other testing library functions.
25-32: Consider expanding test data with more diverse examples.While the current mock data is sufficient for basic tests, adding more snapshots with varied properties would strengthen your test coverage and ensure the component handles different scenarios correctly.
const mockSnapshots = [ { key: '2024-12', title: 'Snapshot 1', startAt: '2023-01-01T00:00:00Z', endAt: '2023-01-02T00:00:00Z', }, + { + key: '2024-11', + title: 'Snapshot 2', + startAt: '2022-12-01T00:00:00Z', + endAt: '2022-12-31T23:59:59Z', + }, + { + key: '2024-10', + title: 'Very Long Snapshot Title That Tests Text Wrapping', + startAt: '2022-11-01T00:00:00Z', + endAt: '2022-11-30T23:59:59Z', + } ]
46-58: Loading state test is incomplete.The test verifies that loading spinners appear initially, but doesn't verify they disappear when data is loaded. A more complete test would simulate the full loading cycle.
it('renders loading spinner initially, then shows content when loaded', async () => { // First return loading state (useQuery as jest.Mock).mockReturnValue({ data: null, error: null, loading: true }) render(<SnapshotsPage />) // Verify loading state await waitFor(() => { const loadingSpinners = screen.getAllByAltText('Loading indicator') expect(loadingSpinners.length).toBeGreaterThan(0) }) + // Then update mock to return loaded state + act(() => { + (useQuery as jest.Mock).mockReturnValue({ + data: { snapshots: mockSnapshots }, + error: null, + loading: false + }) + }) + + // Verify content appears and loading spinner disappears + await waitFor(() => { + expect(screen.getByText('Snapshot 1')).toBeInTheDocument() + expect(screen.queryAllByAltText('Loading indicator').length).toBe(0) + }) })
81-96: Error handling test is good, but could be improved.The test correctly verifies that the toaster is called when an error occurs. Consider also checking if any error UI is rendered in the component itself.
it('shows an error toaster when GraphQL request fails', async () => { ;(useQuery as jest.Mock).mockReturnValue({ data: null, error: new Error('GraphQL error'), }) render(<SnapshotsPage />) await waitFor(() => { expect(toaster.create).toHaveBeenCalledWith({ description: 'Unable to complete the requested operation.', title: 'GraphQL Request Failed', type: 'error', }) + + // If the component shows an error message in the UI, check for that too + // For example: + // expect(screen.getByText(/error loading snapshots/i)).toBeInTheDocument() }) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/App.test.tsx(1 hunks)frontend/__tests__/unit/pages/Snapshots.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/App.test.tsx
🧰 Additional context used
🧬 Code Definitions (1)
frontend/__tests__/unit/pages/Snapshots.test.tsx (1)
frontend/src/pages/index.ts (1)
SnapshotsPage(28-28)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
frontend/__tests__/unit/pages/Snapshots.test.tsx (4)
9-13: Mock implementation for toaster is appropriate.Your toaster mock correctly implements the
createmethod that's used in the component. This fixes the previous issue where the test was expecting a different API than what was mocked.
15-18: Navigation mocking looks good.The mock implementation for React Router's useNavigate is correctly set up to return a mock function that can be tested later.
98-117: Navigation test looks solid.The test properly verifies that clicking the "View Snapshot" button navigates to the correct URL path based on the snapshot key.
34-118:Details
❌ Incorrect review comment
Missing pagination test.
Previous comments mentioned a pagination test that was problematic. If the SnapshotsPage component includes pagination functionality, you should add a test for it. The pagination test should verify that clicking pagination controls updates the current page and triggers the appropriate query with updated variables.
🏁 Script executed:
#!/bin/bash # Check if the SnapshotsPage component has pagination functionality echo "Searching for pagination-related code in the SnapshotsPage component..." cat frontend/src/pages/Snapshots.tsx 2>/dev/null | grep -E 'pagination|Pagination|page|Page' || echo "No direct pagination references found" echo "Checking if the GraphQL query includes pagination parameters..." cat frontend/src/api/queries/snapshotQueries.ts 2>/dev/null | grep -E 'page|limit|offset|cursor' || echo "No pagination parameters found in queries"Length of output: 555
Clarification on Pagination Test Requirement
After verifying the codebase, it appears that the current implementation of
SnapshotsPagedoes not include dynamic pagination functionality. Specifically:
- The component (
frontend/src/pages/Snapshots.tsx) does not contain any pagination controls or logic.- The GraphQL query (
frontend/src/api/queries/snapshotQueries.ts) uses a fixedlimit(e.g.,snapshots(limit: 24)) without any dynamic pagination parameters such as page number, cursor, or offset.Given this, adding a pagination test is not applicable at this time. If pagination features are introduced in the future, please add tests that verify the behavior of pagination controls and ensure the query variables update accordingly.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/__tests__/unit/pages/Snapshots.test.tsx (3)
25-32: Consider expanding mock data for more diverse test scenarios.While the current mock data works for basic tests, consider adding more varied snapshots (different dates, titles) to ensure the component handles diverse data correctly.
const mockSnapshots = [ { key: '2024-12', title: 'Snapshot 1', startAt: '2023-01-01T00:00:00Z', endAt: '2023-01-02T00:00:00Z', }, + { + key: '2024-11', + title: 'Snapshot 2', + startAt: '2022-12-01T00:00:00Z', + endAt: '2022-12-31T23:59:59Z', + }, ]
46-58: Be more specific about loading spinner expectations.The test checks for "loadingSpinners.length > 0" which is less precise than it could be. If the component is expected to show exactly one loading spinner, the test should verify that specific expectation.
await waitFor(() => { const loadingSpinners = screen.getAllByAltText('Loading indicator') - expect(loadingSpinners.length).toBeGreaterThan(0) + expect(loadingSpinners.length).toBe(1) // Or the exact number expected })
98-116: Simplify navigation test assertions.The test includes a redundant check for the button's presence after clicking it. Since you've already confirmed the button exists before clicking it, this check is unnecessary.
// Check if navigate was called with the correct argument await waitFor(() => { - expect(viewSnapshotButton).toBeInTheDocument() expect(navigateMock).toHaveBeenCalledTimes(1) expect(navigateMock).toHaveBeenCalledWith('/community/snapshots/2024-12') })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/Snapshots.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/__tests__/unit/pages/Snapshots.test.tsx (1)
frontend/src/pages/index.ts (1)
SnapshotsPage(28-28)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
frontend/__tests__/unit/pages/Snapshots.test.tsx (8)
1-8: LGTM: Imports are appropriate and well-organized.The imports cover all the necessary testing libraries and components needed for testing the SnapshotsPage.
9-13: LGTM: Toaster mock is correctly implemented.The toaster mock is properly set up to track when the create method is called, which aligns with how it's verified in the error handling test. This was previously flagged in earlier reviews and has been correctly addressed.
15-23: LGTM: Router and Apollo Client mocks are properly configured.The mocks preserve the actual implementations while only replacing the specific hooks needed for testing. This is the correct approach for targeted mocking.
34-44: LGTM: Test suite setup follows best practices.Good use of beforeEach to set up the default query response and afterEach to clear mocks between tests. This ensures test isolation and prevents test interference.
60-66: LGTM: Data rendering test is concise and effective.This test correctly verifies that the snapshot title appears in the rendered component when data is successfully fetched.
68-79: LGTM: Empty state handling test is well-implemented.The test properly verifies the "No Snapshots found" message appears when the snapshots array is empty.
81-96: LGTM: Error handling test is correctly implemented.The test properly verifies that toaster.create is called with the expected error message when a GraphQL error occurs. This addresses the previous review comment about checking toaster.create instead of toaster.
1-118:Details
❌ Incorrect review comment
Missing pagination test.
There's no test for pagination functionality in the current file. Previous review comments mentioned pagination test issues that were addressed, but the actual pagination test appears to be missing from the current implementation.
Consider adding a test that verifies pagination functionality:
🏁 Script executed:
#!/bin/bash # Check if the SnapshotsPage component contains pagination related code or components # Look for pagination related code in the SnapshotsPage component echo "Checking for pagination code in SnapshotsPage component..." rg -p "pagination|paginate|page|useNavigate.*page|setPage" frontend/src/pages/Snapshots.tsx || echo "No pagination code found in SnapshotsPage component" # Check if there are pagination UI elements that would need testing echo "Checking for pagination UI elements..." rg -p "next page|previous page|page \d|pagination" frontend/src/pages/Snapshots.tsx || echo "No pagination UI elements found in SnapshotsPage component"Length of output: 633
Clarification on Pagination Testing in SnapshotsPage
Our investigation shows that no pagination logic or UI elements exist in the
SnapshotsPagecomponent. Please clarify the intended behavior:
- If pagination is expected in this component, you'll need to implement the feature along with appropriate tests.
- If pagination is not required, the review comment regarding missing pagination tests is not applicable and can be dismissed.
Likely an incorrect or invalid review comment.
|
* trying out stuff * basic implementation completed * snapshots page completed * test cases added * improved code quality * corrections * mobile navigation implemented * code smell removed * code smell removed * formatting issue * removed trailing spaces * code formatting improved * code formatting improved * test cases updated * updated branch * test cases updated * code structure updated * updated few cases * test coverage completed * code cleanup * nitpicks resolved * formating * updated test case * Update code --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
* trying out stuff * basic implementation completed * snapshots page completed * test cases added * improved code quality * corrections * mobile navigation implemented * code smell removed * code smell removed * formatting issue * removed trailing spaces * code formatting improved * code formatting improved * test cases updated * updated branch * test cases updated * code structure updated * updated few cases * test coverage completed * code cleanup * nitpicks resolved * formating * updated test case * Update code --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
2 participants
Resolves #1092