refactor: use loading property from useQuery instead of useState #3208
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces local useState-based loading/data handling with Apollo useQuery’s Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-07-12T17:36:57.255ZApplied to files:
📚 Learning: 2025-11-17T16:47:05.578ZApplied to files:
📚 Learning: 2025-07-12T17:14:28.536ZApplied to files:
📚 Learning: 2025-11-17T17:30:42.139ZApplied to files:
🧬 Code graph analysis (2)frontend/__tests__/unit/pages/ModuleDetails.test.tsx (1)
frontend/__tests__/unit/pages/ChapterDetails.test.tsx (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @frontend/src/components/CardDetailsPage.tsx:
- Line 14: The RepositoryCardProps type import and the explicit cast to
RepositoryCardProps[] are out of scope for this loading-state refactor—remove
the RepositoryCardProps import and undo the cast at the code that coerces the
repository list (the site where you wrote "as RepositoryCardProps[]"); instead
preserve the existing DetailsCardProps shape (Partial<RepositoryCardProps>[]) by
either keeping the repositories value uncast or casting it to
Partial<RepositoryCardProps>[] if a cast is necessary; leave any proper type
cleanup (removing Partial<> or changing prop types) to a separate PR.
In @frontend/src/types/card.ts:
- Line 69: The repositories property was relaxed to
Partial<RepositoryCardProps>[] but CardDetailsPage casts it back to
RepositoryCardProps[] (unsafe) and RepositoryCard expects required fields;
decide which model is true: if repository data is always complete, revert
repositories to RepositoryCardProps[] and remove the cast in CardDetailsPage,
ensuring types align; if data can be partial, keep
Partial<RepositoryCardProps>[], remove the cast in CardDetailsPage, and update
RepositoryCard (props and usages) to accept optional fields and guard against
missing values; avoid mixing both approaches in this PR—defer unrelated
type-only refactors to a separate change.
🧹 Nitpick comments (5)
frontend/src/app/members/[memberKey]/page.tsx (1)
105-113: Redundant!isLoadingcheck.Since the component already returns early on line 97 when
isLoadingis true, the!isLoadingcheck here is always true and can be simplified.🔎 Suggested simplification
- if (!isLoading && user == null) { + if (user == null) {frontend/__tests__/unit/pages/ChapterDetails.test.tsx (1)
31-36: Consider adding explicitloading: falsefor consistency.The
beforeEachmock doesn't include theloadingproperty. While this works (undefined is falsy), addingloading: falseexplicitly would improve test clarity and consistency with the loading test case.🔎 Suggested improvement
beforeEach(() => { ;(useQuery as unknown as jest.Mock).mockReturnValue({ data: mockChapterDetailsData, error: null, + loading: false, }) })frontend/src/app/page.tsx (1)
47-55: Loading refactor is correct, but alias is redundant.The loading state refactoring is correctly implemented. However, the alias
const data = graphQLDataon line 55 adds unnecessary indirection. Consider usinggraphQLDatadirectly throughout, or rename it in the destructuring.🔎 Suggested simplification
Either rename in destructuring:
const { - data: graphQLData, + data, error: graphQLRequestError, loading: isLoading, } = useQuery(GetMainPageDataDocument, { variables: { distinct: true }, }) - - const data = graphQLDataOr use
graphQLDatadirectly throughout the component and remove line 55.frontend/src/app/organizations/[organizationKey]/page.tsx (2)
24-28: Consider removing organizationKey from the dependency array.The
organizationKeyis not used within the effect body. Since the query automatically re-runs whenorganizationKeychanges, andgraphQLRequestErrorwill have a new reference if the query fails, includingorganizationKeyin the dependencies is redundant.🔎 Proposed refinement
useEffect(() => { if (graphQLRequestError) { handleAppError(graphQLRequestError) } - }, [graphQLRequestError, organizationKey]) + }, [graphQLRequestError])
73-73: String conversion appears defensive.The
String()conversion onfollowersCountseems unnecessary if the GraphQL schema returns a number and the display component handles numeric values. However, if this ensures type consistency for the label-value pairs inorganizationDetails, it's acceptable defensive programming.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
frontend/__tests__/unit/pages/ChapterDetails.test.tsxfrontend/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/__tests__/unit/pages/ProjectDetails.test.tsxfrontend/__tests__/unit/pages/RepositoryDetails.test.tsxfrontend/__tests__/unit/pages/Snapshots.test.tsxfrontend/src/app/chapters/[chapterKey]/page.tsxfrontend/src/app/committees/[committeeKey]/page.tsxfrontend/src/app/community/snapshots/[id]/page.tsxfrontend/src/app/community/snapshots/page.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/organizations/[organizationKey]/page.tsxfrontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsxfrontend/src/app/page.tsxfrontend/src/app/projects/[projectKey]/page.tsxfrontend/src/app/projects/dashboard/page.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/types/card.ts
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/organizations/[organizationKey]/page.tsxfrontend/src/app/chapters/[chapterKey]/page.tsxfrontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsxfrontend/src/app/projects/[projectKey]/page.tsxfrontend/src/app/community/snapshots/[id]/page.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/src/app/organizations/[organizationKey]/page.tsxfrontend/src/app/chapters/[chapterKey]/page.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/organizations/[organizationKey]/page.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/ChapterDetails.test.tsxfrontend/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/__tests__/unit/pages/ProjectDetails.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/projects/dashboard/page.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/components/CardDetailsPage.tsxfrontend/src/app/members/[memberKey]/page.tsx
🧬 Code graph analysis (14)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (1)
frontend/src/types/__generated__/moduleQueries.generated.ts (1)
GetProgramAdminsAndModulesDocument(41-41)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx (1)
frontend/src/types/__generated__/menteeQueries.generated.ts (1)
GetModuleMenteeDetailsDocument(14-14)
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (1)
frontend/src/types/__generated__/moduleQueries.generated.ts (1)
GetProgramAdminsAndModulesDocument(41-41)
frontend/src/app/organizations/[organizationKey]/page.tsx (1)
frontend/src/types/__generated__/organizationQueries.generated.ts (1)
GetOrganizationDataDocument(19-19)
frontend/src/app/chapters/[chapterKey]/page.tsx (3)
frontend/src/types/__generated__/chapterQueries.generated.ts (1)
GetChapterDataDocument(19-19)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)frontend/src/app/global-error.tsx (1)
handleAppError(67-91)
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (1)
frontend/src/types/__generated__/repositoryQueries.generated.ts (1)
GetRepositoryDataDocument(21-21)
frontend/src/app/projects/[projectKey]/page.tsx (1)
frontend/src/types/__generated__/projectQueries.generated.ts (1)
GetProjectDocument(36-36)
frontend/src/components/CardDetailsPage.tsx (2)
backend/apps/owasp/api/internal/nodes/project.py (1)
repositories(100-107)frontend/src/types/project.ts (1)
RepositoryCardProps(56-67)
frontend/src/app/committees/[committeeKey]/page.tsx (1)
frontend/src/types/__generated__/committeeQueries.generated.ts (1)
GetCommitteeDataDocument(19-19)
frontend/src/app/page.tsx (1)
frontend/src/types/__generated__/homeQueries.generated.ts (1)
GetMainPageDataDocument(12-12)
frontend/src/app/community/snapshots/[id]/page.tsx (1)
frontend/src/types/__generated__/snapshotQueries.generated.ts (1)
GetSnapshotDetailsDocument(24-24)
frontend/src/types/card.ts (1)
frontend/src/types/project.ts (1)
RepositoryCardProps(56-67)
frontend/src/app/members/[memberKey]/page.tsx (1)
frontend/src/types/__generated__/userQueries.generated.ts (1)
GetUserDataDocument(27-27)
frontend/src/app/community/snapshots/page.tsx (1)
frontend/src/types/__generated__/snapshotQueries.generated.ts (1)
GetCommunitySnapshotsDocument(26-26)
🔇 Additional comments (22)
frontend/__tests__/unit/pages/RepositoryDetails.test.tsx (1)
44-57: LGTM! Loading state mock is now explicit.The addition of
loading: trueto the mock correctly simulates Apollo Client's loading state, ensuring the loading spinner assertion validates the expected UI behavior.frontend/__tests__/unit/pages/OrganizationDetails.test.tsx (1)
181-201: LGTM! Error state mock is complete.Adding
loading: falseto the error scenario mock correctly represents Apollo Client's behavior when a query fails—loading completes (false) and error is populated. This makes the test state explicit and robust.frontend/__tests__/unit/pages/ProjectDetails.test.tsx (1)
57-70: LGTM! Loading state mock is explicit.The addition of
loading: truecorrectly simulates the initial loading phase, allowing the test to validate that the loading indicator renders as expected.frontend/src/app/community/snapshots/page.tsx (1)
15-19: Successful refactor! Loading state now driven by Apollo Client.The destructuring of
loading: isLoadingfromuseQuerycorrectly replaces the manualuseStateloading management, eliminating the need forsetIsLoading(false)calls. The component now relies on Apollo Client's built-in loading state, which is more reliable and reduces complexity.frontend/__tests__/unit/pages/Snapshots.test.tsx (1)
53-66: LGTM! Loading state test properly configured.The explicit
loading: truein the mock ensures the test accurately validates the loading skeleton rendering behavior, completing the test suite updates for this refactor.frontend/src/app/committees/[committeeKey]/page.tsx (1)
22-28: LGTM! Clean refactor to use Apollo's loading state.The destructuring of
loadingasisLoadingfromuseQuerycorrectly replaces the previous manual loading state management. This simplifies the code by eliminating redundant state tracking while maintaining the same loading behavior for the UI.frontend/src/app/community/snapshots/[id]/page.tsx (1)
24-30: LGTM! Consistent refactor pattern.The loading state is now correctly sourced from Apollo's
useQueryhook. This change eliminates manual loading state management and aligns with the refactor pattern used across other page components in this PR.frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (1)
24-30: LGTM! Loading state refactor applied correctly.The component now uses Apollo's
loadingflag fromuseQueryinstead of manual state management. This maintains the existing loading behavior while reducing complexity and potential bugs from manual state synchronization.frontend/src/app/projects/dashboard/page.tsx (1)
29-33: LGTM! Consistent loading state management.The refactor correctly replaces manual loading state with Apollo's built-in
loadingproperty. This change simplifies state management and aligns with Apollo Client best practices.frontend/src/app/projects/[projectKey]/page.tsx (1)
22-28: LGTM! Well-executed refactor across all files.This refactor successfully replaces manual loading state with Apollo's
loadingproperty fromuseQuery. The change is consistent across all modified files in this PR, improving maintainability and reducing the potential for loading-state-related bugs.frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx (1)
37-49: Refactoring looks correct.The loading state is now properly derived from Apollo's
useQueryhook. ThefetchPolicy: 'cache-and-network'will show cached data immediately while refetching from the network in the background, which is appropriate for this use case.frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (1)
20-29: LGTM!The loading state refactoring is correctly implemented. The component now derives loading state directly from the
useQueryhook while maintaining proper data population and error handling.frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (1)
19-28: LGTM!The loading state refactoring follows the same correct pattern as the public module details page.
frontend/src/app/members/[memberKey]/page.tsx (1)
37-43: LGTM!The loading state refactoring is correctly implemented with appropriate skeleton loading UI.
frontend/src/app/chapters/[chapterKey]/page.tsx (2)
15-24: Clean refactoring approach.This file takes a cleaner approach by deriving
chapterandtopContributorsdirectly from the query data without intermediate state. This eliminates unnecessary re-renders from state updates.
45-57: Verify the type narrowing logic in your IDE or withnpm run test:unit.The control flow appears sound: after the early return on line 32 (
if (isLoading)),isLoadingis false. The check on line 36 (if (!chapter && !isLoading)) returns early, so TypeScript should narrowchapterto non-null for lines 45-57. Modern TypeScript (5.9.3) typically handles this correctly, but type errors may still occur depending on how the Apollo Client types are configured. Verify locally thatnpm run test:unit(which runstsc --noEmit) passes without errors on this file.frontend/__tests__/unit/pages/ChapterDetails.test.tsx (1)
42-53: LGTM!The test correctly mocks
loading: trueto verify the loading spinner renders. This aligns with the component's new reliance on theuseQueryloading flag.frontend/src/app/page.tsx (1)
59-70: Error handling effect is correctly scoped.The dependency array correctly only includes
graphQLRequestErrorsince this effect only handles error toasting and no longer manages loading state.frontend/src/app/organizations/[organizationKey]/page.tsx (4)
16-22: LGTM! Clean migration to Apollo's built-in loading state.The destructuring pattern is clear and the aliasing of
loadingtoisLoadingmaintains consistency with the codebase. This successfully eliminates the need for separateuseStatemanagement.
30-36: LGTM! Proper use of Apollo's loading state.The loading check correctly uses the
isLoadingflag fromuseQueryand provides appropriate skeleton feedback during data fetch.
38-56: LGTM! Clean data extraction and error handling.The destructuring pattern with nullish coalescing (
graphQLData ?? {}) safely handles the case when data is undefined. The!organizationcheck appropriately displays a 404 error when the organization doesn't exist, whilehandleAppErrorin the useEffect handles GraphQL/network errors via toast notifications.
110-124: LGTM! Props correctly wired to use GraphQL data directly.The component now passes the destructured data directly to
DetailsCard, successfully eliminating the intermediate state layer. This aligns perfectly with the refactor objectives.
frontend/src/types/card.ts
Outdated
| recentMilestones?: Milestone[] | ||
| recentReleases?: Release[] | ||
| repositories?: RepositoryCardProps[] | ||
| repositories?: Partial<RepositoryCardProps>[] |
There was a problem hiding this comment.
Type relaxation seems inconsistent with usage and out of scope.
Changing repositories to Partial<RepositoryCardProps>[] relaxes all field requirements, but in CardDetailsPage.tsx line 310, the data is immediately cast back to RepositoryCardProps[], suggesting the fields are actually required.
This creates an inconsistency: if the data can be partial, components should handle missing fields explicitly; if it's always complete, the Partial<> wrapper is unnecessary and the cast in CardDetailsPage is unsafe.
Additionally, this type change appears unrelated to the PR's stated objective of refactoring loading state management (replacing useState with useQuery.loading).
Consider one of these approaches:
- If data is always complete: Remove the
Partial<>wrapper and the cast in CardDetailsPage - If data can be partial: Keep
Partial<>, remove the cast, and updateRepositoryCardto handle optional fields
Based on learnings, consider deferring unrelated type adjustments to a separate PR to maintain focus on the loading state migration.
🤖 Prompt for AI Agents
In @frontend/src/types/card.ts around line 69, The repositories property was
relaxed to Partial<RepositoryCardProps>[] but CardDetailsPage casts it back to
RepositoryCardProps[] (unsafe) and RepositoryCard expects required fields;
decide which model is true: if repository data is always complete, revert
repositories to RepositoryCardProps[] and remove the cast in CardDetailsPage,
ensuring types align; if data can be partial, keep
Partial<RepositoryCardProps>[], remove the cast in CardDetailsPage, and update
RepositoryCard (props and usages) to accept optional fields and guard against
missing values; avoid mixing both approaches in this PR—defer unrelated
type-only refactors to a separate change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/app/committees/[committeeKey]/page.tsx (1)
18-27: LGTM! Clean refactor to Apollo's loading state.The refactor correctly eliminates local
useStateforisLoading,committee, andtopContributors, and derives all data directly from theuseQueryresult. This aligns perfectly with the PR objectives.Optional: Use nullish coalescing for semantic precision
Consider using
??instead of||for the default array on line 27:- const topContributors = data?.topContributors || [] + const topContributors = data?.topContributors ?? []While
||works correctly for arrays (since empty arrays are truthy),??more precisely expresses "use default only if null/undefined."frontend/src/app/members/[memberKey]/page.tsx (1)
25-38: LGTM! Consistent refactor pattern.The refactor successfully eliminates local
useStatefor user data and loading state, deriving all values directly from theuseQueryresult. The use ofgraphQLDataas the variable name appropriately avoids collision with the existingdatastate used for heatmap data.Optional: Consider nullish coalescing for default arrays
For semantic precision, consider using
??instead of||on lines 34-38:- const issues = graphQLData?.recentIssues || [] - const topRepositories = graphQLData?.topContributedRepositories || [] - const milestones = graphQLData?.recentMilestones || [] - const pullRequests = graphQLData?.recentPullRequests || [] - const releases = graphQLData?.recentReleases || [] + const issues = graphQLData?.recentIssues ?? [] + const topRepositories = graphQLData?.topContributedRepositories ?? [] + const milestones = graphQLData?.recentMilestones ?? [] + const pullRequests = graphQLData?.recentPullRequests ?? [] + const releases = graphQLData?.recentReleases ?? []While both operators work correctly for arrays,
??more precisely expresses "use default only if null/undefined."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/app/committees/[committeeKey]/page.tsxfrontend/src/app/community/snapshots/[id]/page.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/page.tsxfrontend/src/app/projects/[projectKey]/page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/app/projects/[projectKey]/page.tsx
- frontend/src/app/community/snapshots/[id]/page.tsx
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/page.tsxfrontend/src/app/committees/[committeeKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
📚 Learning: 2025-07-08T17:24:36.501Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/page.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/page.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/page.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
🧬 Code graph analysis (4)
frontend/src/app/members/[memberKey]/page.tsx (3)
backend/apps/owasp/models/project.py (1)
issues(186-192)backend/apps/github/models/user.py (2)
issues(67-74)releases(82-89)frontend/src/app/global-error.tsx (1)
handleAppError(67-91)
frontend/src/app/mentorship/programs/[programKey]/page.tsx (1)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)
frontend/src/app/committees/[committeeKey]/page.tsx (3)
frontend/src/types/__generated__/committeeQueries.generated.ts (1)
GetCommitteeDataDocument(19-19)backend/apps/owasp/api/internal/queries/committee.py (1)
committee(14-28)frontend/src/app/global-error.tsx (2)
handleAppError(67-91)ErrorDisplay(28-52)
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (5)
frontend/src/types/__generated__/moduleQueries.generated.ts (1)
GetProgramAdminsAndModulesDocument(41-41)backend/apps/mentorship/api/internal/nodes/program.py (1)
admins(32-34)frontend/src/app/global-error.tsx (2)
handleAppError(67-91)ErrorDisplay(28-52)frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)frontend/src/components/ModuleCard.tsx (1)
getSimpleDuration(96-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (11)
frontend/src/app/committees/[committeeKey]/page.tsx (1)
39-47: Appropriate 404 check.The condition
!data || !committeecorrectly handles the case where the query completes but returns no data. This ensures the 404 error is shown only after loading completes and when the committee truly doesn't exist.frontend/src/app/members/[memberKey]/page.tsx (1)
92-100: Correct 404 condition.The check
!graphQLData || !userproperly handles the case where the query completes but returns no user data, ensuring the 404 error is displayed only after loading finishes and the user truly doesn't exist. This pattern is consistent with the refactor incommittees/[committeeKey]/page.tsx.frontend/src/app/mentorship/programs/[programKey]/page.tsx (3)
4-4: LGTM: Simplified imports and params extraction.The refactor correctly uses
useParamsfrom Next.js navigation, and the removal ofuseSearchParamsanduseRouteraligns with eliminating manual refresh logic per the PR objectives.Also applies to: 14-14
20-21: LGTM: Direct data access simplifies state management.Deriving
programandmodulesdirectly from the query result eliminates the need for local state and manual synchronization, aligning with the PR objectives.
23-33: LGTM: Loading and 404 logic flow is correct.The component correctly shows a loading spinner while the query executes, then displays a 404 for missing programs. The condition
(!data || !program)is explicit and handles cases where the query succeeds but returns null for the program.frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (6)
6-6: LGTM: Removed unused useState import.The removal of
useStatefrom the React import correctly reflects that loading state is now managed by Apollo'suseQueryhook, aligning with the PR's refactoring objective.
17-26: LGTM: Correctly uses Apollo's loading state.The refactor successfully replaces local
useStateloading management with Apollo's built-inloadingproperty, aliased asisLoading. This simplifies the code and ensures loading state is always synchronized with the query state.
31-47: LGTM: Improved error handling.The error handling correctly combines
handleAppError(for toast notifications and Sentry logging) withErrorDisplay(for UI rendering). This dual approach provides both immediate user feedback via toast and a full-page error state, which is appropriate for this detail page.
49-57: LGTM: 404 handling correctly checks query data.The 404 check appropriately validates the presence of query data and the module. The condition
!data || !programModulecorrectly identifies when the module doesn't exist, which is the intended behavior for this detail page.
59-80: LGTM: Data usage correctly references query results.All module field references have been properly updated to use
programModule(derived from Apollo query data) instead of local state. The field mappings align with the GraphQL query structure, and utility functions (formatDate,getSimpleDuration,capitalize) are appropriately applied.
28-29: No action needed. TheDetailsCardcomponent is designed to accept an optionaladminsprop (admins?: Contributor[]in its interface). All usages within the component safely handle undefined values using optional chaining (admins?.some(...)) or explicit null checks ({admins && admins.length > 0 && ...}). Ifdata.getProgramisnull,adminswill beundefined, which is a valid state that the component handles correctly.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (1)
31-47: Add explicit GraphQL error handling before the 404 check.Currently, network or GraphQL errors result in
databeing null/undefined, causing the condition at Line 39 to render a 404 "Module Not Found" page. This conflates transient network failures with actual missing modules. The pattern infrontend/src/app/mentorship/programs/[programKey]/page.tsx(lines 36-44) is better: it explicitly checks forerrorand renders a 500 error page before checking for 404.🔎 Proposed fix to add explicit error handling
useEffect(() => { if (error) { handleAppError(error) } }, [error]) if (isLoading) return <LoadingSpinner /> + if (error) { + return ( + <ErrorDisplay + statusCode={500} + title="Error loading module" + message="An error occurred while loading the module data" + /> + ) + } + if (!data || !programModule) { return ( <ErrorDisplay statusCode={404} title="Module Not Found" message="Sorry, the module you're looking for doesn't exist." /> ) }
🤖 Fix all issues with AI Agents
In @frontend/src/app/projects/[projectKey]/page.tsx:
- Around line 30-34: The effect watching for GraphQL errors unnecessarily
includes projectKey in its dependency array, causing re-runs when projectKey
changes even if the error is stale; in the useEffect that calls
handleAppError(graphQLRequestError) remove projectKey from the dependencies so
the hook only depends on graphQLRequestError (i.e., change the dependency array
for the useEffect that references graphQLRequestError and handleAppError to
[graphQLRequestError]).
🧹 Nitpick comments (2)
frontend/src/app/mentorship/programs/[programKey]/page.tsx (1)
28-32: RemoveprogramKeyfrom the useEffect dependency array.Including
programKeyis unnecessary because Apollo Client automatically re-executes the query when variables change, andgraphQLRequestErrorwill be reset by the new query. The dependency array should only includegraphQLRequestError.🔎 Proposed fix
useEffect(() => { if (graphQLRequestError) { handleAppError(graphQLRequestError) } - }, [graphQLRequestError, programKey]) + }, [graphQLRequestError])frontend/src/app/chapters/[chapterKey]/page.tsx (1)
31-31: Consider removingchapterKeyfrom the dependency array.The effect body only references
graphQLRequestError, so includingchapterKeyis unnecessary. WhenchapterKeychanges, the query re-runs and any error state change will trigger the effect viagraphQLRequestErroranyway.🔎 Simplified dependency array
- }, [graphQLRequestError, chapterKey]) + }, [graphQLRequestError])
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/src/app/chapters/[chapterKey]/page.tsxfrontend/src/app/committees/[committeeKey]/page.tsxfrontend/src/app/community/snapshots/[id]/page.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/page.tsxfrontend/src/app/projects/[projectKey]/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/committees/[committeeKey]/page.tsx
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/app/chapters/[chapterKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/page.tsxfrontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/src/app/chapters/[chapterKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/page.tsxfrontend/src/app/community/snapshots/[id]/page.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/page.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/page.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/mentorship/programs/[programKey]/page.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-07-08T17:24:36.501Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/page.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/mentorship/programs/[programKey]/page.tsxfrontend/src/app/community/snapshots/[id]/page.tsx
🧬 Code graph analysis (4)
frontend/src/app/chapters/[chapterKey]/page.tsx (3)
frontend/src/types/__generated__/chapterQueries.generated.ts (1)
GetChapterDataDocument(19-19)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)frontend/src/app/global-error.tsx (2)
handleAppError(67-91)ErrorDisplay(28-52)
frontend/src/app/mentorship/programs/[programKey]/page.tsx (2)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)frontend/src/app/global-error.tsx (2)
handleAppError(67-91)ErrorDisplay(28-52)
frontend/src/app/community/snapshots/[id]/page.tsx (2)
backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)frontend/src/app/global-error.tsx (2)
handleAppError(67-91)ErrorDisplay(28-52)
frontend/src/app/projects/[projectKey]/page.tsx (2)
backend/apps/github/models/repository.py (1)
project(162-164)frontend/src/app/global-error.tsx (2)
handleAppError(67-91)ErrorDisplay(28-52)
🔇 Additional comments (12)
frontend/src/app/mentorship/programs/[programKey]/page.tsx (1)
36-44: LGTM! Previous error handling concern resolved.The explicit check for
graphQLRequestErrorproperly distinguishes network/GraphQL errors (500) from actual missing programs (404), addressing the concern raised in previous reviews.frontend/src/app/chapters/[chapterKey]/page.tsx (2)
16-25: LGTM! Clean refactor to Apollo-native loading state.The refactor successfully eliminates manual state management by deriving
chapterandtopContributorsdirectly from the query result and using Apollo'sloadingproperty. This simplifies the component and aligns with the PR objective.
37-55: LGTM! Robust error handling for both network errors and missing data.The dual error handling approach (toast via
handleAppError+ persistentErrorDisplaypage) provides good UX. The 500/404 distinction is clear, and the!data || !chaptercheck correctly handles both TypeScript type narrowing and the backend's ability to returnnullfor non-existent chapters.frontend/src/app/projects/[projectKey]/page.tsx (2)
19-28: LGTM! Clean refactor to useuseQueryloading and derived data.The changes successfully eliminate local
useStatefor loading and data management, deriving everything directly from the Apollo query. This aligns perfectly with the PR objectives and simplifies the component logic.
40-58: Well-structured error handling with 500 and 404 states.The error handling pattern (toast notification via
handleAppError+ error page viaErrorDisplay) provides good UX, and the defensive checks for missing data ensure proper 404 handling. This approach is consistent with the pattern established across other pages in this PR.frontend/src/app/members/[memberKey]/page.tsx (3)
25-38: LGTM! Clean refactor to use useQuery loading state.The migration from local useState loading to Apollo's
useQueryloading property is correctly implemented. Data extraction uses safe optional chaining with appropriate fallbacks, and the destructuredisLoadingflag properly tracks query state.
84-90: LGTM! Loading state correctly derived from useQuery.The loading check properly uses
isLoadingfromuseQueryto control skeleton display, meeting the PR's objective of removing redundantuseStateloading tracking.
92-110: LGTM! Comprehensive error and 404 handling.The error-handling flow correctly covers both failure cases:
- 500 error: when
graphQLRequestErroris present- 404 error: when query succeeds but returns no data
The pattern of calling
handleAppErrorin the effect (line 42) alongside renderingErrorDisplayis standard—side effects for logging/telemetry happen separately from error UI rendering.frontend/src/app/community/snapshots/[id]/page.tsx (4)
4-4: LGTM - useState correctly removed.The removal of
useStatefrom imports aligns perfectly with the PR objective to eliminate local loading state management.
22-30: LGTM - Core refactor correctly implemented.The refactor successfully replaces local
useStateloading management with Apollo'sloadingproperty. The destructuring pattern is clean, and derivingsnapshotfromdata?.snapshotwith optional chaining is the correct approach.
32-36: LGTM - Effect dependencies are correct.The
useEffectdependencies[graphQLRequestError, snapshotKey]are appropriate. When the route parameter changes, Apollo Client automatically refetches and clears the previous error state, so the effect will correctly handle errors for each new snapshot.
94-116: Verify double error handling UX and note scope expansion.Two observations:
Double error feedback: When
graphQLRequestErroroccurs, the code shows both:
- A toast notification via
handleAppError(lines 33-34)- A full-page
ErrorDisplaycomponent (lines 98-106)This means users see both a dismissible toast AND a persistent error page. While this might be intentional (toast for immediate feedback, error page for stable state), please confirm this UX pattern is desired.
Scope beyond PR objectives: The commit message notes "Add ErrorDisplay for network error" and explicit 500/404 error handling is being introduced. The PR description states the objective is to refactor loading state management with "app behavior and UI loading indicators unchanged except for using
useQuery.loading", but adding ErrorDisplay components constitutes new error handling UI not mentioned in the PR description.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@eyad-hazem-elmorsy thanks for working on this! 👍🏼
I pushed some changes because with using both useQuery and useState many pages were showing a 404 error page for a second or so when loading the page.
3 participants
Proposed change
Resolves #3135
Checklist
make check-testlocally and all tests passed