Update Admin > Statistics views data fetching and error handling#9416
Update Admin > Statistics views data fetching and error handling#9416hotzenklotz wants to merge 2 commits intoupdate-admin-page-designfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduced a new custom hook Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@Reviewer: Make sure to hide whitespace: |
hotzenklotz
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/javascripts/admin/statistic/available_tasks_report_view.tsx`:
- Around line 37-40: The onChange handler for the team selector currently
ignores the null case and leaves stale state; update the handler that calls
setSelectedTeamId so that when selectedTeam is null (filter cleared) you call
setSelectedTeamId(null) (or an appropriate empty value) instead of doing
nothing, otherwise keep setting setSelectedTeamId(selectedTeam.id) when a single
team object is provided; adjust the existing Array.isArray(selectedTeam) check
to branch: array -> ignore/handle arrays, null -> reset selectedTeamId, else ->
set selectedTeam id.
In `@frontend/javascripts/admin/statistic/project_progress_report_view.tsx`:
- Around line 76-79: The onChange handler for the team selector currently
ignores the null branch so clearing the filter doesn't update state; update the
handler in project_progress_report_view.tsx so that when selectedTeam is null
you explicitly call handleTeamChange(null) (while keeping the Array.isArray
guard for multi-select values) to clear the team filter and stop the previous
report from continuing to poll/render.
- Around line 21-35: The query is created with queryFn: () =>
getProjectProgressReport(team!.id) and enabled: team != null, but calling
refetch() (e.g., from the Refresh button handler) still invokes queryFn even
when team is null, causing a crash; update the Refresh handler(s) that call
refetch (and any other places using refetch with the same
useQueryWithErrorHandling pattern) to first check team != null before calling
refetch, or guard by using if (!team) return; so queryFn
(getProjectProgressReport) is never invoked with team! when no team is selected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fecbdb04-3393-4d03-b68f-7711ede1e5ac
📒 Files selected for processing (6)
frontend/javascripts/admin/statistic/available_tasks_report_view.tsxfrontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsxfrontend/javascripts/admin/statistic/project_progress_report_view.tsxfrontend/javascripts/admin/statistic/time_tracking_detail_view.tsxfrontend/javascripts/admin/statistic/time_tracking_overview.tsxfrontend/javascripts/libs/react_hooks.ts
| onChange={(selectedTeam) => { | ||
| if (!Array.isArray(selectedTeam) && selectedTeam != null) { | ||
| fetchData(selectedTeam.id); | ||
| setSelectedTeamId(selectedTeam.id); | ||
| } |
There was a problem hiding this comment.
Reset selectedTeamId when the filter is cleared.
This handler ignores null, so clearing the selector leaves the previous team id in state and the table keeps showing stale report data.
💡 Proposed fix
<TeamSelectionComponent
onChange={(selectedTeam) => {
- if (!Array.isArray(selectedTeam) && selectedTeam != null) {
- setSelectedTeamId(selectedTeam.id);
- }
+ if (Array.isArray(selectedTeam) || selectedTeam == null) {
+ setSelectedTeamId(null);
+ return;
+ }
+ setSelectedTeamId(selectedTeam.id);
}}
prefix={<FilterOutlined />}
/>📝 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.
| onChange={(selectedTeam) => { | |
| if (!Array.isArray(selectedTeam) && selectedTeam != null) { | |
| fetchData(selectedTeam.id); | |
| setSelectedTeamId(selectedTeam.id); | |
| } | |
| onChange={(selectedTeam) => { | |
| if (Array.isArray(selectedTeam) || selectedTeam == null) { | |
| setSelectedTeamId(null); | |
| return; | |
| } | |
| setSelectedTeamId(selectedTeam.id); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/javascripts/admin/statistic/available_tasks_report_view.tsx` around
lines 37 - 40, The onChange handler for the team selector currently ignores the
null case and leaves stale state; update the handler that calls
setSelectedTeamId so that when selectedTeam is null (filter cleared) you call
setSelectedTeamId(null) (or an appropriate empty value) instead of doing
nothing, otherwise keep setting setSelectedTeamId(selectedTeam.id) when a single
team object is provided; adjust the existing Array.isArray(selectedTeam) check
to branch: array -> ignore/handle arrays, null -> reset selectedTeamId, else ->
set selectedTeam id.
| const { | ||
| data = [], | ||
| isLoading, | ||
| isFetching, | ||
| refetch, | ||
| dataUpdatedAt, | ||
| } = useQueryWithErrorHandling( | ||
| { | ||
| queryKey: ["projectProgressReport", team?.id], | ||
| enabled: team != null, | ||
| queryFn: () => getProjectProgressReport(team!.id), | ||
| refetchInterval: RELOAD_INTERVAL, | ||
| }, | ||
| messages["project.report.failed_to_refresh"], | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TanStack React Query v5, does calling refetch() on a useQuery with enabled: false still execute the queryFn?
💡 Result:
Yes, calling refetch on a useQuery with enabled: false in TanStack React Query v5 still executes the queryFn.
Citations:
- 1: https://tanstack.com/query/latest/docs/framework/react/guides/disabling-queries
- 2: https://tanstack.dev/query/v5/docs/framework/react/reference/useQuery
- 3: https://tanstack.com/query/v5/docs/framework/react/reference/useQuery
- 4: https://react-query.tanstack.com/guides/disabling-queries
- 5: https://stackoverflow.com/questions/70653010/is-there-a-way-to-prevent-fetching-when-refetch-method-get-called-react-query
Disable Refresh until a team is selected.
Calling refetch() executes the queryFn even when the query is disabled. This will crash when dereferencing team!.id before a team has been selected.
Proposed fix
<Button
icon={<ReloadOutlined spin={isFetching} />}
variant="outlined"
onClick={() => refetch()}
+ disabled={team == null}
>
Refresh
</Button>Also applies to: 62-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/javascripts/admin/statistic/project_progress_report_view.tsx` around
lines 21 - 35, The query is created with queryFn: () =>
getProjectProgressReport(team!.id) and enabled: team != null, but calling
refetch() (e.g., from the Refresh button handler) still invokes queryFn even
when team is null, causing a crash; update the Refresh handler(s) that call
refetch (and any other places using refetch with the same
useQueryWithErrorHandling pattern) to first check team != null before calling
refetch, or guard by using if (!team) return; so queryFn
(getProjectProgressReport) is never invoked with team! when no team is selected.
| onChange={(selectedTeam) => { | ||
| if (!Array.isArray(selectedTeam) && selectedTeam != null) { | ||
| handleTeamChange(selectedTeam); | ||
| } |
There was a problem hiding this comment.
Handle clearing the team filter.
This handler drops the null branch as well, so reopening the filter and clearing it keeps the previous team selected and the old report continues to poll/render.
💡 Proposed fix
onChange={(selectedTeam) => {
- if (!Array.isArray(selectedTeam) && selectedTeam != null) {
- handleTeamChange(selectedTeam);
- }
+ if (Array.isArray(selectedTeam) || selectedTeam == null) {
+ setTeam(undefined);
+ return;
+ }
+ handleTeamChange(selectedTeam);
}}📝 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.
| onChange={(selectedTeam) => { | |
| if (!Array.isArray(selectedTeam) && selectedTeam != null) { | |
| handleTeamChange(selectedTeam); | |
| } | |
| onChange={(selectedTeam) => { | |
| if (Array.isArray(selectedTeam) || selectedTeam == null) { | |
| setTeam(undefined); | |
| return; | |
| } | |
| handleTeamChange(selectedTeam); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/javascripts/admin/statistic/project_progress_report_view.tsx` around
lines 76 - 79, The onChange handler for the team selector currently ignores the
null branch so clearing the filter doesn't update state; update the handler in
project_progress_report_view.tsx so that when selectedTeam is null you
explicitly call handleTeamChange(null) (while keeping the Array.isArray guard
for multi-select values) to clear the team filter and stop the previous report
from continuing to poll/render.
1 participant
I updated the Admin > Statistics views slightly:
useQueryuseQueryWithErrorHandlingtime_tracking_overview.tsx:→ fixed to filteredTimeEntries.length === 0
it while the other was still loading. Now each query has its own isLoading/isFetching
flag, combined as const isFetching = isTeamsLoading || isEntriesFetching
time_tracking_detail_view.tsx:replaced with
project_and_annotation_type_dropdown.tsx:reassigned)
initializer function — valid but highly unusual and non-obvious)
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)