fix(board): render error state instead of infinite loader when data is null #3084
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaced a local Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/board/[year]/candidates/page.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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/board/[year]/candidates/page.tsx
🔇 Additional comments (3)
frontend/src/app/board/[year]/candidates/page.tsx (3)
675-692: Core fix looks good — conditional order correctly addresses the infinite loader bug.The reordering ensures:
- Show spinner only while
loadingis true (active query)- Handle errors when loading completes with an error
- Show 404 when data is null/unavailable
This correctly fixes the issue where the spinner would persist indefinitely when
boardOfDirectorswas null.
513-524: LGTM — className template adjustments.These are minor formatting changes to the template literal concatenation pattern. No functional changes.
Also applies to: 548-559
635-670: LGTM — "Additional Information" section reorganization.The section is now consolidated with consistent structure and styling. The logic remains the same, displaying status tags for OWASP board members, former staff, and GSoC mentors, with the flagship project note rendered conditionally.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/src/app/board/[year]/candidates/page.tsx (2)
90-90: Dead code: Remove unusedisLoadingstate.The
isLoadingstate variable is no longer used since the component now relies on theloadingflag fromuseQuery(line 676). This state and its setter at line 103 should be removed.
96-99: Duplicate error handling creates poor user experience.The error is handled here with
handleAppError(graphQLRequestError), which likely shows a toast notification. However, the render path at lines 680-682 then returnsnull, leaving the user with a blank screen and only a toast message. This was already flagged in previous review comments.A better approach would be to remove the error handling from the
useEffectentirely and show an<ErrorDisplay>component in the render path (similar to the 404 case at lines 684-692), providing a more informative error page.🔎 Suggested approach
Remove error handling from useEffect:
useEffect(() => { - if (graphQLRequestError) { - handleAppError(graphQLRequestError) - return - } - if (graphQLData?.boardOfDirectors) { setCandidates(graphQLData.boardOfDirectors.candidates || []) - setIsLoading(false) } - }, [graphQLData, graphQLRequestError]) + }, [graphQLData])Then update the render path to show ErrorDisplay:
if (graphQLRequestError) { - return null + handleAppError(graphQLRequestError) + return ( + <ErrorDisplay + statusCode={500} + title="Failed to load board candidates" + message="An error occurred while fetching the board information. Please try again later." + /> + ) }
🧹 Nitpick comments (2)
frontend/src/app/board/[year]/candidates/page.tsx (2)
514-560: Stylistic improvement: Consolidated className templates.The className formatting has been updated to use template literals with ternary operators, which is more concise than the previous approach. This is a stylistic improvement with no functional changes.
636-671: Improved structure: Consolidated Additional Information section.The Additional Information section has been restructured for better consistency and readability. The conditional rendering logic remains the same, but the layout is now cleaner and more maintainable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/board/[year]/candidates/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/board/[year]/candidates/page.tsx (2)
frontend/src/types/__generated__/boardQueries.generated.ts (1)
GetBoardCandidatesDocument(33-33)frontend/src/app/global-error.tsx (1)
handleAppError(67-91)
🔇 Additional comments (3)
frontend/src/app/board/[year]/candidates/page.tsx (3)
92-92: Good addition: Usingloadingfrom useQuery.Capturing the
loadingflag fromuseQueryis the correct approach to fix the infinite loading issue. This enables proper loading state management based on the actual query status rather than local state.
676-678: Core fix: Loading state correctly managed.This is the key fix that resolves the infinite loading issue described in the PR objectives. By checking the
loadingflag fromuseQuerydirectly, the spinner only displays while the query is actively loading, preventing the infinite loading state when data isnull.
684-692: Correct 404 handling for missing data.The 404 error display is correctly shown when
boardOfDirectorsdata doesn't exist for the selected year. This provides clear feedback to the user about why the data isn't available, which aligns with the PR objectives.
|
Hey @arkid15r. There was a minor bug which was not soo important to fix. But i closed that pr worked on that bug fixed it and now you can see there is now. |
compressed.mp4 |
|
Everything is working fine as the video explains it. You can further see the code changes. |
|
@nios-x please do not close PRs the have requested changes - address all comments in this PR. I will close a duplicate if it's open. |
🤷♂️ |
|
So what i do next |
|
@nios-x next you should read our guidelines more closely https://github.com/OWASP/Nest/blob/main/CONTRIBUTING.md#contributing-workflow I will address issues with this PR. For example - you did not run If you're considering contributing to OWASP Nest in the future I highly recommend you familiarizing yourself with our rules - it would be easier for you and for us. p.s. we are not enforcing that but you might want to create separate branches for tasks you're working on. It's not a good practice to work straight from main branch. Just an advice 👌🏼 |
…s null (#3084) * fix(board): render error state instead of infinite loader when data is null * Address make-check --------- Co-authored-by: Kate <kate@kgthreads.com>
3 participants
Proposed change
Resolves #3052
Contributor: Soumya (GitHub: @nios-x)
Role: Student contributor (GSoC aspirant)
This PR fixes an issue on the Board page where the UI enters an infinite loading state when the GraphQL response for a selected year returns
null.Previously, the loading state was rendered before validating whether
boardOfDirectorsdata existed. As a result, when the query completed successfully but returnednull, the page remained stuck on the loading spinner.This change updates the conditional rendering logic to:
nullboard data before rendering the loading stateChecklist
make check-testlocally and all tests passedPrevious ScreenShots:
Current ScreenShots
