frontend for snapshotDetailsPage #981
Conversation
Summary by CodeRabbit
WalkthroughThis pull request introduces snapshot-related enhancements across both backend and frontend systems. In the backend, updates include modifications to GraphQL nodes and queries, admin configurations, model migrations, and tests to support snapshot functionality as well as improvements to the release node. In the frontend, new routes, pages, queries, types, and utility functions have been added to display snapshot details and refine contributor components. Overall, the changes extend the snapshot feature set and adjust tests and migrations accordingly. Changes
Suggested labels
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (7)
frontend/src/types/snapshot.ts (1)
11-23: Consider adding nullable types for optional fields.The
SnapshotDetailsPropsinterface is comprehensive, but it might be helpful to mark certain fields as optional if they may not always be present (e.g.,errorMessage).export interface SnapshotDetailsProps { title: string key: string updatedAt: string createdAt: string startAt: string endAt: string status: string - errorMessage: string + errorMessage?: string newReleases: ReleaseType[] newProjects: ProjectTypeGraphql[] newChapters: ChapterTypeGraphQL[] }frontend/src/utils/data.ts (1)
87-106: Added camelCase versions of icon definitionsThese additions support a transition to camelCase naming while maintaining backward compatibility with existing snake_case names. The duplication ensures proper icon rendering throughout the application.
However, consider adding a comment above these additions explaining the duplicated properties are for supporting a transition to camelCase naming conventions while maintaining backward compatibility.
+// The following camelCase versions of the icon definitions are added to support +// a transition to camelCase naming conventions while maintaining backward compatibility +// with existing snake_case names. starsCount: { label: 'GitHub stars', icon: 'fa-regular fa-star', },backend/apps/owasp/graphql/nodes/snapshot.py (1)
23-26: Consider adding pagination for potentially large collections.The resolver methods for
new_projects,new_releases, andnew_usersdon't have limits, whilenew_issuesis limited to 100 items. If these collections grow large, it could lead to performance issues. Consider implementing pagination or consistent limits across all collection resolvers.frontend/src/api/queries/snapshotQueries.ts (1)
3-61: Well-structured query with comprehensive field selection.The
GET_SNAPSHOT_DETAILSquery is well-organized and includes all necessary fields for the snapshot and its related entities.However, consider if all this data is needed at once. For better performance, you could:
- Consider splitting this into smaller queries if all data isn't required immediately
- Implement pagination for collections like
newReleases,newProjects, andnewChapters- Use fragments to make the query more maintainable
frontend/src/pages/SnapshotDetails.tsx (2)
171-171: Add dark mode styling for New Releases headingFor consistency with other section headings, add the dark mode text color class.
- <h2 className="mb-4 text-2xl font-semibold">New Releases</h2> + <h2 className="mb-4 text-2xl font-semibold text-gray-700 dark:text-gray-200">New Releases</h2>
173-198: Consider using unique identifiers for release keysUsing a combination of tagName and index as keys could potentially cause issues if tagName is not unique or if the array order changes.
- key={`${release.tagName}-${index}`} + key={release.id || `${release.tagName}-${index}`}If release objects have a unique ID field, consider using that instead.
frontend/src/components/ContributorAvatar.tsx (1)
37-38: Consider refactoring repeated type assertions.While the component works correctly, there are multiple instances of type assertions to
TopContributorsTypeGraphql. Consider extracting GraphQL-specific properties into variables at the top of the component to reduce repetition and improve readability.const isAlgolia = isAlgoliaContributor(contributor) + const graphqlContributor = !isAlgolia ? (contributor as TopContributorsTypeGraphql) : null const avatarUrl = isAlgolia ? contributor.avatar_url - : (contributor as TopContributorsTypeGraphql).avatarUrl + : graphqlContributor?.avatarUrl const contributionsCount = isAlgolia ? contributor.contributions_count - : (contributor as TopContributorsTypeGraphql).contributionsCount + : graphqlContributor?.contributionsCount const { login, name } = contributor const displayName = name || login const repositoryInfo = - !isAlgolia && (contributor as TopContributorsTypeGraphql).repositoryName - ? ` in ${(contributor as TopContributorsTypeGraphql).repositoryName}` + !isAlgolia && graphqlContributor?.repositoryName + ? ` in ${graphqlContributor.repositoryName}` : ''Also applies to: 44-48
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
backend/apps/github/graphql/nodes/release.py(3 hunks)backend/apps/owasp/admin.py(2 hunks)backend/apps/owasp/graphql/nodes/snapshot.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(1 hunks)backend/apps/owasp/graphql/queries/snapshot.py(1 hunks)backend/apps/owasp/migrations/0015_snapshot.py(2 hunks)backend/apps/owasp/models/snapshot.py(3 hunks)backend/tests/owasp/models/snapshot_test.py(2 hunks)frontend/src/App.tsx(2 hunks)frontend/src/api/queries/snapshotQueries.ts(1 hunks)frontend/src/components/ContributorAvatar.tsx(1 hunks)frontend/src/components/DisplayIcon.tsx(1 hunks)frontend/src/pages/SnapshotDetails.tsx(1 hunks)frontend/src/pages/index.ts(2 hunks)frontend/src/types/snapshot.ts(1 hunks)frontend/src/utils/data.ts(1 hunks)frontend/src/utils/utility.ts(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/apps/owasp/graphql/queries/__init__.py
11-11: Line too long (100 > 99)
(E501)
🔇 Additional comments (29)
frontend/src/pages/index.ts (1)
14-14: Integration of SnapshotDetailsPage looks good!The import and export of the new SnapshotDetailsPage component follow the existing pattern in the file, maintaining consistency with other page components.
Also applies to: 29-29
backend/tests/owasp/models/snapshot_test.py (2)
15-16: Good addition of new Snapshot model attributes in test setup.The mock Snapshot object now includes the new
titleandkeyfields, which matches the model changes mentioned in the AI summary.
33-36: Well-structured test for new snapshot attributes.This test ensures that the newly added attributes (
titleandkey) are correctly assigned. The test is clear, concise, and follows the existing test patterns in this file.frontend/src/types/snapshot.ts (1)
1-9: Interface definitions follow TypeScript best practices.The
ReleaseTypeinterface is well-defined with appropriate types for all properties. The imports are correctly structured at the top of the file.backend/apps/owasp/graphql/queries/__init__.py (1)
7-7: Added import for SnapshotQuery properly follows ordering pattern.The import has been correctly added in alphabetical order, maintaining the organization of the file.
backend/apps/owasp/admin.py (2)
122-124: Added "title" to list_display for improved admin visibilityThe addition of "title" to the list_display tuple enhances the admin interface by making snapshot titles immediately visible in the list view.
136-139: Enhanced search capability with title and key fieldsAdding "title" and "key" to search_fields allows administrators to efficiently find snapshots using these important identifiers.
frontend/src/components/DisplayIcon.tsx (2)
12-18: Extended container class logic to support camelCase namingThe conditional logic has been updated to support both snake_case and camelCase naming conventions for icon identifiers, ensuring proper styling across the application.
26-32: Consistent icon class logic update for camelCase supportThe icon class naming logic has been aligned with the container class logic changes, maintaining consistency in how styles are applied to both elements.
backend/apps/github/graphql/nodes/release.py (3)
3-3: Import style updated to use graphene moduleChanged from importing specific Field to importing the entire graphene module, which is more maintainable when adding multiple graphene types.
13-14: Updated field definition and added project_name fieldThe field definition style has been updated to use the new import pattern, and a new project_name field has been added to enhance the GraphQL API.
26-28: Added resolver for project_name fieldThe resolver implementation correctly returns the idx_project value, providing clients with the associated project name for each release.
backend/apps/owasp/graphql/nodes/snapshot.py (2)
56-59: Good use of limiting for potentially large collections.Excellent implementation of limiting the number of issues to 100 most recent ones. This helps prevent performance issues when querying snapshots with many associated issues.
16-39: Well-structured node with clear field definitions.The SnapshotNode is well-implemented with appropriate field types and a clear Meta class definition. The code follows the project's patterns for GraphQL nodes.
backend/apps/owasp/models/snapshot.py (2)
20-22: Good field additions with appropriate constraints.The
titleandkeyfields are well-defined with appropriate constraints. The unique constraint onkeyis particularly important for the YYYY-mm format identifier.
38-43: Clean implementation of automatic key generation.The save method elegantly handles automatic key generation in the YYYY-mm format, respecting existing keys and properly calling the parent class's save method.
backend/apps/owasp/graphql/queries/snapshot.py (2)
23-29: Good error handling for non-existent snapshots.The resolver properly handles the case where a snapshot with the given key doesn't exist by returning
Noneinstead of throwing an exception.
30-32: Well-implemented recent snapshots resolver with sensible limits.The resolver for recent snapshots is well-implemented with a default limit of 8, which helps maintain good performance while providing enough data for UI display.
frontend/src/App.tsx (2)
2-14: Good organization of importsThe reordering of imports follows alphabetical ordering, which is a good practice for maintainability and readability.
47-47: LGTM on the route additionThe new route for SnapshotDetailsPage is properly integrated with the existing routing structure, following the same pattern as other detail pages. The path parameter
:idcorrectly matches the expected parameter extraction in the component.frontend/src/pages/SnapshotDetails.tsx (4)
21-45: Component structure and data fetching look goodThe component properly uses hooks for state management and data fetching. The error handling for GraphQL requests is appropriately implemented with user feedback via toast notifications.
46-72: Well-implemented card rendering functionThe renderProjectCard function effectively handles navigation and filtering of project data for display. Good use of the utility functions for icon filtering.
74-101: Good reuse of pattern for chapter card renderingThe renderChapterCard function follows the same pattern as renderProjectCard, maintaining consistency across the codebase.
103-118: Effective loading and error statesThe implementation properly handles loading and error states with appropriate UI feedback.
backend/apps/owasp/migrations/0015_snapshot.py (1)
8-8: Migration looks well-structuredThe migration correctly updates dependencies and adds necessary fields to the Snapshot model. The unique constraint on the key field and the auto timestamps for created_at and updated_at follow best practices.
Also applies to: 22-25
frontend/src/components/ContributorAvatar.tsx (4)
3-3: Well-structured type definitions for supporting multiple contributor sources.The addition of
TopContributorsTypeGraphqland creation of a unifiedContributorPropstype is a clean approach to handle different data sources. This enables the component to work with both Algolia and GraphQL data formats.Also applies to: 6-8
10-14: Good use of TypeScript type guard pattern.The
isAlgoliaContributortype guard function is well-implemented, providing proper type narrowing to safely access properties specific to each contributor type. This is a standard TypeScript pattern that improves type safety.
16-34: Clean conditional property access based on contributor type.The component implementation correctly determines the appropriate properties to access based on the contributor type. The extraction of
avatarUrl,contributionsCount, andrepositoryInfois handled properly with appropriate fallbacks.
55-55: Good practice: Setting component display name.Setting the display name explicitly is a good practice for debugging and development tooling, especially for memoized components.
| from .stats import StatsQuery | ||
|
|
||
|
|
||
| class OwaspQuery(ChapterQuery, CommitteeQuery, EventQuery, ProjectQuery, StatsQuery): | ||
| class OwaspQuery(ChapterQuery, CommitteeQuery, EventQuery, ProjectQuery, SnapshotQuery, StatsQuery): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Class declaration exceeds line length limit.
The OwaspQuery class declaration now exceeds the maximum line length of 99 characters (currently 100 characters), as flagged by the static analyzer.
Consider reformatting the inheritance list for readability:
-class OwaspQuery(ChapterQuery, CommitteeQuery, EventQuery, ProjectQuery, SnapshotQuery, StatsQuery):
+class OwaspQuery(
+ ChapterQuery,
+ CommitteeQuery,
+ EventQuery,
+ ProjectQuery,
+ SnapshotQuery,
+ StatsQuery,
+):
"""OWASP queries."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class OwaspQuery(ChapterQuery, CommitteeQuery, EventQuery, ProjectQuery, SnapshotQuery, StatsQuery): | |
| class OwaspQuery( | |
| ChapterQuery, | |
| CommitteeQuery, | |
| EventQuery, | |
| ProjectQuery, | |
| SnapshotQuery, | |
| StatsQuery, | |
| ): | |
| """OWASP queries.""" |
🧰 Tools
🪛 Ruff (0.8.2)
11-11: Line too long (100 > 99)
(E501)
| export const getFilteredIconsGraphql = ( | ||
| project: ProjectTypeGraphql | ChapterTypeGraphQL, | ||
| params: string[] | ||
| ): IconType => { | ||
| const filteredIcons = params.reduce((acc: IconType, key) => { | ||
| if (Icons[key as IconKeys] && project[key as keyof typeof project] !== undefined) { | ||
| if (key === 'createdAt') { | ||
| acc[key] = dayjs.unix(project[key as keyof projectType] as number).fromNow() | ||
| } else { | ||
| acc[key] = project[key as keyof typeof project] as number | ||
| } | ||
| } | ||
| return acc | ||
| }, {}) | ||
|
|
||
| return filteredIcons | ||
| } |
There was a problem hiding this comment.
Fix type reference in getFilteredIconsGraphql
There's an incorrect type reference on line 43 that uses projectType which is not defined in this function's context.
- acc[key] = dayjs.unix(project[key as keyof projectType] as number).fromNow()
+ acc[key] = dayjs.unix(project[key as keyof typeof project] as number).fromNow()Additionally, consider refactoring to reduce duplication between getFilteredIcons and getFilteredIconsGraphql functions, as they share very similar logic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getFilteredIconsGraphql = ( | |
| project: ProjectTypeGraphql | ChapterTypeGraphQL, | |
| params: string[] | |
| ): IconType => { | |
| const filteredIcons = params.reduce((acc: IconType, key) => { | |
| if (Icons[key as IconKeys] && project[key as keyof typeof project] !== undefined) { | |
| if (key === 'createdAt') { | |
| acc[key] = dayjs.unix(project[key as keyof projectType] as number).fromNow() | |
| } else { | |
| acc[key] = project[key as keyof typeof project] as number | |
| } | |
| } | |
| return acc | |
| }, {}) | |
| return filteredIcons | |
| } | |
| export const getFilteredIconsGraphql = ( | |
| project: ProjectTypeGraphql | ChapterTypeGraphQL, | |
| params: string[] | |
| ): IconType => { | |
| const filteredIcons = params.reduce((acc: IconType, key) => { | |
| if (Icons[key as IconKeys] && project[key as keyof typeof project] !== undefined) { | |
| if (key === 'createdAt') { | |
| acc[key] = dayjs.unix(project[key as keyof typeof project] as number).fromNow() | |
| } else { | |
| acc[key] = project[key as keyof typeof project] as number | |
| } | |
| } | |
| return acc | |
| }, {}) | |
| return filteredIcons | |
| } |
There was a problem hiding this comment.
will work on it
There was a problem hiding this comment.
@abhayymishraa Thanks for acknowledging the issue! Let me know if you need any assistance when implementing the fix. I'm here to help if you have any questions about either fixing the type reference or refactoring to reduce duplication between the functions.
(_/)
(•ᴗ•)
/ >👍
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/github/graphql/nodes/release_test.py (1)
25-25: Addition of project_name field looks good.The addition of "project_name" to the expected fields set correctly reflects the changes made to the ReleaseNode class, ensuring test coverage for this new field. This update is appropriate and necessary for maintaining test integrity.
While the field presence test is sufficient based on the pattern established in this file, consider adding a dedicated test for the resolver functionality of this new field (similar to how
test_author_fieldtests that specific field's configuration).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/github/graphql/nodes/release_test.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
frontend/src/components/ContributorAvatar.tsx (2)
21-31: Simplify type handling by leveraging TypeScript's type narrowing.The type assertion with
asis redundant after the type guard has been applied. TypeScript can infer the correct types after the guard check.const avatarUrl = isAlgolia ? contributor.avatar_url - : (contributor as TopContributorsTypeGraphql).avatarUrl + : contributor.avatarUrl const contributionsCount = isAlgolia ? contributor.contributions_count - : (contributor as TopContributorsTypeGraphql).contributionsCount + : contributor.contributionsCount
35-38: Simplify repository info logic with proper type narrowing.Similar to the previous comment, the repeated type assertions can be simplified by using a more idiomatic TypeScript approach.
const repositoryInfo = - !isAlgolia && (contributor as TopContributorsTypeGraphql).repositoryName - ? ` in ${(contributor as TopContributorsTypeGraphql).repositoryName}` + !isAlgolia && contributor.repositoryName + ? ` in ${contributor.repositoryName}` : ''frontend/__tests__/unit/data/mockSnapshotData.ts (2)
76-76: Inconsistent date format.The
updatedAtfield uses a Unix timestamp format (1727353371.0) while other date fields use ISO 8601 string format. This inconsistency could cause issues when handling dates in the application.- updatedAt: 1727353371.0, + updatedAt: '2024-07-30T15:22:51+00:00',
73-73: Consider adding a name for completeness.The
namefield is empty for this contributor. While this might be testing edge cases, it would be more representative to include a name for consistency with other contributors in your mock data.- name: '', + name: 'Test User',frontend/__tests__/unit/pages/SnapshotDetails.test.tsx (3)
45-58: Loading state test could be more robust.The loading state test uses
getAllByAltTextwhich will throw an error if no elements are found, but doesn't specifically assert what components should be present during loading.render(<SnapshotDetailsPage />) const loadingSpinner = screen.getAllByAltText('Loading indicator') await waitFor(() => { - expect(loadingSpinner.length).toBeGreaterThan(0) + expect(loadingSpinner[0]).toBeInTheDocument() + expect(screen.queryByText('New Snapshot')).not.toBeInTheDocument() })
149-169: Add positive assertion for missing data case.The test for missing data only verifies that certain elements are not in the document. Add a positive assertion to confirm that some "no data" message or empty state indicator is displayed.
render(<SnapshotDetailsPage />) await waitFor(() => { expect(screen.queryByText('New Chapters')).not.toBeInTheDocument() expect(screen.queryByText('New Projects')).not.toBeInTheDocument() expect(screen.queryByText('New Releases')).not.toBeInTheDocument() + + // Verify that empty state messages are displayed + expect(screen.getByText('No new chapters found')).toBeInTheDocument() + expect(screen.getByText('No new projects found')).toBeInTheDocument() + expect(screen.getByText('No new releases found')).toBeInTheDocument() })Note: Adjust the expected text based on your actual empty state messages.
1-170: Consider adding tests for additional edge cases.The test suite covers basic scenarios well, but might benefit from additional tests for:
- Partially missing data (e.g., when some arrays are present but others are null)
- Malformed data (e.g., missing required fields)
- Different snapshot statuses (e.g., 'in-progress', 'failed')
- Testing that release data is properly formatted and displayed
These additional tests would ensure the component is robust against various data conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/unit/App.test.tsx(1 hunks)frontend/__tests__/unit/data/mockSnapshotData.ts(1 hunks)frontend/__tests__/unit/pages/SnapshotDetails.test.tsx(1 hunks)frontend/src/components/ContributorAvatar.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (12)
frontend/src/components/ContributorAvatar.tsx (7)
3-3: Good import update to support the new contributor type.The addition of
TopContributorsTypeGraphqlalongsideTopContributorsTypeAlgoliaproperly sets up the component to handle both contributor data sources.
6-8: Well-defined props type to accommodate multiple contributor sources.Creating a dedicated
ContributorPropstype that accepts both Algolia and GraphQL contributor types is a clean approach to handle this enhancement.
10-19: Effective type guard implementation for contributor type checking.The
isAlgoliaContributortype guard is well implemented with proper property checks to distinguish between contributor types. This is a TypeScript best practice for handling union types.
42-43: Good tooltip enhancement with dynamic contributor information.The tooltip now provides more contextual information by including the contribution count and repository details when available, improving the user experience.
49-49: Clean user profile link implementation.Using the
loginproperty to create the profile link URL is a good practice, ensuring consistent navigation to user profiles regardless of the contributor data source.
60-60: Good practice setting the displayName for the memoized component.Setting the display name explicitly for the memoized component is a best practice for debugging and component identification in React DevTools.
52-52:❓ Verification inconclusive
Verify if the size parameter is required for Algolia avatars only.
The conditional addition of
&s=60for Algolia avatars suggests different handling for image sizing. Ensure this difference is intentional and documented.
🏁 Script executed:
#!/bin/bash # Check how avatar URLs are constructed in other components for both contributor types echo "Checking for avatar URL patterns in the codebase:" rg -A 2 -B 2 "avatar_url" --type ts --type tsx echo "Checking for GraphQL avatar URL patterns:" rg -A 2 -B 2 "avatarUrl" --type ts --type tsxLength of output: 350
Action Required: Verify Avatar Size Parameter for Algolia
The conditional appending of
&s=60for Algolia avatars is still ambiguous. The initial search using file type parameters resulted in errors due to misconfigured file type recognition (tsx files weren’t matched). Please re-run the search with file globbing to confirm whether any other components modify avatar URLs similarly. For instance, try executing:#!/bin/bash echo "Checking for avatar URL patterns in the codebase using file globs:" rg -A 2 -B 2 "avatar_url" -g "*.tsx" -g "*.ts" echo "Checking for GraphQL avatarUrl patterns using file globs:" rg -A 2 -B 2 "avatarUrl" -g "*.tsx" -g "*.ts"Also, verify if the
isAlgoliaflag is used elsewhere that might affect avatar URL construction:#!/bin/bash echo "Searching for 'isAlgolia' usage in the codebase:" rg -A 2 -B 2 "isAlgolia" -g "*.tsx" -g "*.ts"Once these searches confirm that only Algolia-based avatars include a size parameter (or if this pattern is consistent across the system), please ensure that the behavior is intentional and documented accordingly.
frontend/__tests__/unit/App.test.tsx (1)
20-20: Mock implementation looks good.The addition of the
SnapshotDetailsPagemock follows the established pattern of other page component mocks in this file. The implementation correctly uses a descriptivedata-testidthat will be useful for testing, and the text content matches the component name.frontend/__tests__/unit/data/mockSnapshotData.ts (2)
1-87: Overall structure is well-organized for testing purposes.The mock data provides a comprehensive representation of a snapshot object with all necessary fields for testing the component.
5-8: Verify the timestamp logic in your mock data.There appears to be a logical inconsistency in the timestamps:
createdAt(2025-03-01) is afterstartAt(2024-12-01)updatedAt(2025-03-02) is afterendAt(2024-12-31)This suggests that the snapshot was created and updated after its time period, which might not reflect real-world behavior. Consider adjusting these dates to ensure they align with the expected lifecycle of a snapshot.
frontend/__tests__/unit/pages/SnapshotDetails.test.tsx (2)
1-6: Good import organization.The imports are well organized, correctly importing testing utilities, mock data, and the component under test.
9-22: Properly mocked dependencies.All necessary dependencies are appropriately mocked, including Apollo Client's
useQuery, React Router's navigation functions, and the toast notification system.
| test('navigates to chapter page when chapter card is clicked', async () => { | ||
| ;(useQuery as jest.Mock).mockReturnValue({ | ||
| data: mockSnapshotDetailsData, | ||
| }) | ||
|
|
||
| render(<SnapshotDetailsPage />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('OWASP Sivagangai')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| const chapterCardButton = screen.getAllByRole('button', { name: /View Details/i })[0] | ||
| fireEvent.click(chapterCardButton) | ||
|
|
||
| await waitFor(() => { | ||
| expect(navigateMock).toHaveBeenCalledWith('/chapters/sivagangai') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Similar issue with index-based element selection.
As with the previous test, using array indices to select elements makes tests fragile.
await waitFor(() => {
expect(screen.getByText('OWASP Sivagangai')).toBeInTheDocument()
})
- const chapterCardButton = screen.getAllByRole('button', { name: /View Details/i })[0]
+ // Find the button within the context of the chapter card
+ const chapterCard = screen.getByText('OWASP Sivagangai').closest('.card') || document.body
+ const chapterCardButton = within(chapterCard).getByRole('button', { name: /View Details/i })
fireEvent.click(chapterCardButton)
await waitFor(() => {
expect(navigateMock).toHaveBeenCalledWith('/chapters/sivagangai')
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('navigates to chapter page when chapter card is clicked', async () => { | |
| ;(useQuery as jest.Mock).mockReturnValue({ | |
| data: mockSnapshotDetailsData, | |
| }) | |
| render(<SnapshotDetailsPage />) | |
| await waitFor(() => { | |
| expect(screen.getByText('OWASP Sivagangai')).toBeInTheDocument() | |
| }) | |
| const chapterCardButton = screen.getAllByRole('button', { name: /View Details/i })[0] | |
| fireEvent.click(chapterCardButton) | |
| await waitFor(() => { | |
| expect(navigateMock).toHaveBeenCalledWith('/chapters/sivagangai') | |
| }) | |
| }) | |
| test('navigates to chapter page when chapter card is clicked', async () => { | |
| ;(useQuery as jest.Mock).mockReturnValue({ | |
| data: mockSnapshotDetailsData, | |
| }) | |
| render(<SnapshotDetailsPage />) | |
| await waitFor(() => { | |
| expect(screen.getByText('OWASP Sivagangai')).toBeInTheDocument() | |
| }) | |
| // Find the button within the context of the chapter card | |
| const chapterCard = screen.getByText('OWASP Sivagangai').closest('.card') || document.body | |
| const chapterCardButton = within(chapterCard).getByRole('button', { name: /View Details/i }) | |
| fireEvent.click(chapterCardButton) | |
| await waitFor(() => { | |
| expect(navigateMock).toHaveBeenCalledWith('/chapters/sivagangai') | |
| }) | |
| }) |
| test('navigates to project page when project card is clicked', async () => { | ||
| ;(useQuery as jest.Mock).mockReturnValue({ | ||
| data: mockSnapshotDetailsData, | ||
| }) | ||
|
|
||
| render(<SnapshotDetailsPage />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('OWASP Nest')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| const projectCardButton = screen.getAllByRole('button', { name: /View Details/i })[1] | ||
| fireEvent.click(projectCardButton) | ||
|
|
||
| await waitFor(() => { | ||
| expect(navigateMock).toHaveBeenCalledWith('/projects/nest') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid index-based element selection for more reliable tests.
Using array indices to select buttons ([1]) makes the test fragile if the order of items changes. Instead, consider more targeted selectors.
await waitFor(() => {
expect(screen.getByText('OWASP Nest')).toBeInTheDocument()
})
- const projectCardButton = screen.getAllByRole('button', { name: /View Details/i })[1]
+ // Find the button within the context of the project card
+ const projectCard = screen.getByText('OWASP Nest').closest('.card') || document.body
+ const projectCardButton = within(projectCard).getByRole('button', { name: /View Details/i })
fireEvent.click(projectCardButton)
await waitFor(() => {
expect(navigateMock).toHaveBeenCalledWith('/projects/nest')
})Note: This assumes there's a parent element with a class 'card' containing each project. Adjust the selector based on your actual component structure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('navigates to project page when project card is clicked', async () => { | |
| ;(useQuery as jest.Mock).mockReturnValue({ | |
| data: mockSnapshotDetailsData, | |
| }) | |
| render(<SnapshotDetailsPage />) | |
| await waitFor(() => { | |
| expect(screen.getByText('OWASP Nest')).toBeInTheDocument() | |
| }) | |
| const projectCardButton = screen.getAllByRole('button', { name: /View Details/i })[1] | |
| fireEvent.click(projectCardButton) | |
| await waitFor(() => { | |
| expect(navigateMock).toHaveBeenCalledWith('/projects/nest') | |
| }) | |
| }) | |
| test('navigates to project page when project card is clicked', async () => { | |
| ;(useQuery as jest.Mock).mockReturnValue({ | |
| data: mockSnapshotDetailsData, | |
| }) | |
| render(<SnapshotDetailsPage />) | |
| await waitFor(() => { | |
| expect(screen.getByText('OWASP Nest')).toBeInTheDocument() | |
| }) | |
| // Find the button within the context of the project card | |
| const projectCard = screen.getByText('OWASP Nest').closest('.card') || document.body | |
| const projectCardButton = within(projectCard).getByRole('button', { name: /View Details/i }) | |
| fireEvent.click(projectCardButton) | |
| await waitFor(() => { | |
| expect(navigateMock).toHaveBeenCalledWith('/projects/nest') | |
| }) | |
| }) |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/apps/owasp/graphql/nodes/snapshot.py (3)
16-34: GraphQL node structure looks good but consider including key in Meta fieldsThe node structure is well-organized, but you should consider adding
keyto thefieldstuple in the Meta class. Currently, it's defined as a GraphQL field but not included in the Meta fields, which could lead to inconsistencies.class Meta: model = Snapshot fields = ( "created_at", "end_at", "start_at", "title", + "key", )
35-38: Consider removing redundant resolver for keyThe
resolve_keymethod simply returnsself.keywithout any additional logic. Since thekeyfield is defined on the model, this resolver might be redundant if the field is properly included in the Meta fields list.
43-57: Consider standardizing sort fields across resolversThe resolvers use different sorting fields (
created_atvspublished_at). This is likely intentional based on the available fields for each model, but it's worth documenting why different fields are used for sorting to maintain clarity.def resolve_new_releases(self, info): - """Resolve recent new releases.""" + """Resolve recent new releases, sorted by published_at as releases use published_at rather than created_at.""" return self.new_releases.order_by("-published_at")backend/apps/owasp/models/snapshot.py (1)
20-22: New fields are well-defined, but consider a more descriptive default titleThe new fields are well-structured, but consider providing a more descriptive default for the
titlefield rather than an empty string. This would improve usability in the admin interface and any listing views.- title = models.CharField(max_length=255, default="") + title = models.CharField(max_length=255, default="", help_text="A descriptive title for this snapshot")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/github/graphql/nodes/release.py(2 hunks)backend/apps/owasp/graphql/nodes/snapshot.py(1 hunks)backend/apps/owasp/migrations/0020_snapshot_key_snapshot_title.py(1 hunks)backend/apps/owasp/migrations/0021_alter_snapshot_key.py(1 hunks)backend/apps/owasp/models/snapshot.py(3 hunks)frontend/src/App.tsx(2 hunks)frontend/src/pages/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/App.tsx
- frontend/src/pages/index.ts
- backend/apps/github/graphql/nodes/release.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (5)
backend/apps/owasp/migrations/0021_alter_snapshot_key.py (1)
12-16: Migration looks good with appropriate field constraintsThe migration correctly alters the
keyfield to allow blank values while maintaining uniqueness, which is appropriate for a field that will be automatically populated during save operations.backend/apps/owasp/migrations/0020_snapshot_key_snapshot_title.py (2)
18-22: Title field implementation looks goodAdding a
titlefield with a default empty string is appropriate, though you may want to consider adding validation to ensure titles are provided for new snapshots.
12-17:❓ Verification inconclusive
Verify database migration on existing data
Adding a unique field with
preserve_default=Falseis correct, but be aware that if there are existing Snapshot records in the database, this migration might prompt for a value or could fail if executed without interaction. Consider running this migration with a management command that provides unique values for each existing record if there are many existing records.
🏁 Script executed:
#!/bin/bash # Check if there are existing snapshot records that might be affected by this migration python manage.py shell -c "from apps.owasp.models import Snapshot; print(f'Existing snapshots: {Snapshot.objects.count()}')"Length of output: 224
Attention: Manual Verification Required for Snapshot Migration
The migration itself (adding the unique, non-default field “key”) appears correct. However, there’s a concern that existing Snapshot records might cause issues during migration if they don’t already have unique values. The attempted command to count existing records failed because it couldn’t locate the project’s manage.py (likely due to running the command from the wrong directory).
Next Steps:
- Verify that you’re running migration-related commands from the correct project root (where manage.py resides).
- Manually check for existing Snapshot records (or use a corrected command) to ensure that a management command is executed that populates unique keys if needed.
- Consider using a shell script to locate the manage.py file, for example:
Once you confirm the correct environment and verify the Snapshot records, ensure that a management command (or a data migration) is in place to assign unique keys for pre-existing records if necessary.
🏁 Script executed:
[run_scripts] #!/bin/bash # Locate the manage.py file to determine the correct project root fd --type f "^manage.py$"Length of output: 167
Heads-up: Ensure Correct Execution Context for Snapshot Migration
- The migration adds a unique
keyfield withpreserve_default=False, which is sound. However, if Snapshot records already exist, they won't have unique keys, and the migration could fail or prompt for a value.- The original shell command failed because it was executed from the wrong directory. Since the
manage.pyfile is located atbackend/manage.py, ensure you either change your directory tobackendor adjust the command:python backend/manage.py shell -c "from apps.owasp.models import Snapshot; print(f'Existing snapshots: {Snapshot.objects.count()}')"- Consider implementing a data migration or a pre-migration management command to generate and assign unique keys to any existing Snapshot records before applying this migration.
backend/apps/owasp/graphql/nodes/snapshot.py (1)
1-14: Node structure with imports and constants looks goodThe imports are well-organized and the constant
RECENT_ISSUES_LIMITis appropriately defined at the module level.backend/apps/owasp/models/snapshot.py (1)
42-47:❓ Verification inconclusive
Key generation logic might not ensure uniqueness
The current implementation generates keys in the format "YYYY-MM", which might not ensure uniqueness if multiple snapshots are created in the same month. Consider adding a suffix or using a more granular timestamp format.
🏁 Script executed:
#!/bin/bash # Check if there are any duplicate keys with the current implementation python manage.py shell -c "from django.db.models import Count; from apps.owasp.models import Snapshot; dupes = Snapshot.objects.values('key').annotate(key_count=Count('key')).filter(key_count__gt=1); print(f'Duplicate keys: {list(dupes)}')"Length of output: 352
Snapshot Key Uniqueness – Manual Verification Required
The duplicate key check could not be executed since the command failed to locate the expected
manage.py(error: "can't open file '/home/jailuser/git/manage.py'"). This prevents automated confirmation that the current "YYYY-MM" key generation does not produce duplicates.
- Please verify the repository’s project structure to confirm the correct location of
manage.pyor the appropriate project entry point.- Once identified, manually re-run the duplicate key check. Regardless of the outcome, consider revisiting the key generation logic to either include a unique suffix or employ a more granular timestamp to ensure uniqueness within the same month.
| def __str__(self): | ||
| """Return a string representation of the snapshot.""" | ||
| return f"Snapshot {self.start_at} to {self.end_at} ({self.status})" | ||
| return self.title |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more informative str representation
The current implementation returns just the title. If titles are not set, this would result in empty strings being displayed in the admin interface. Consider a more informative string representation that includes fallback information.
def __str__(self):
"""Return a string representation of the snapshot."""
- return self.title
+ if self.title:
+ return self.title
+ return f"Snapshot {self.key or self.start_at.strftime('%Y-%m-%d')}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __str__(self): | |
| """Return a string representation of the snapshot.""" | |
| return f"Snapshot {self.start_at} to {self.end_at} ({self.status})" | |
| return self.title | |
| def __str__(self): | |
| """Return a string representation of the snapshot.""" | |
| if self.title: | |
| return self.title | |
| return f"Snapshot {self.key or self.start_at.strftime('%Y-%m-%d')}" |
* fixed some issue * backend test fix * All test-case fix * some optimization * fixed case * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Yash Goyal <[email protected]> Update backend data commit
* fixed some issue * backend test fix * All test-case fix * some optimization * fixed case * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Yash Goyal <[email protected]> Update backend data commit
* fixed some issue * backend test fix * All test-case fix * some optimization * fixed case * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Yash Goyal <[email protected]> Update backend data commit
* fixed some issue * backend test fix * All test-case fix * some optimization * fixed case * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Yash Goyal <[email protected]> Update backend data commit
Resolves #915
Add the PR description here.
Preview/Screenshots
