fix for dashboard not updating on same role diff org - #452
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughOrganization switching now has shared loading state, guarded async handling, organization-scoped query caches, dependent-query invalidation, failure recovery, and visible loading feedback across authenticated views. ChangesOrganization switching
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change updates the dashboard when switching organizations with the same role, and no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant User
participant OrgSwitcher
participant AppStore
participant QueryClient
participant AuthenticatedLayout
User->>OrgSwitcher: Select organization or role
OrgSwitcher->>AppStore: Set switching state
AppStore-->>AuthenticatedLayout: isOrgSwitching = true
AuthenticatedLayout-->>User: Show switching overlay
OrgSwitcher->>QueryClient: Invalidate organization-scoped queries
QueryClient-->>User: Refetch organization data
OrgSwitcher->>AppStore: Clear switching state
AppStore-->>AuthenticatedLayout: isOrgSwitching = false
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
kaseywright
left a comment
kaseywright
left a comment
There was a problem hiding this comment.
Reviewed the diff. The root cause is correctly identified, and the approach is sound.
The real bug was that OrgSwitcher invalidated ['chapter-assignments'] while the translator dashboard's query is keyed ['userChapterAssignments', userId] — the keys never matched, so the switch never refetched. (For what it's worth, ['chapter-assignments', user.id] belongs to useChapterAssignments(user) in useProjects.ts:123, which nothing imports — so the old invalidation was a no-op on dead code.)
Adding orgId to the query keys is the better half of the fix: per-org cache segregation means cached data is always correct-for-that-org rather than depending on an invalidation firing.
A few things below — one I'd consider a blocker, the rest are smaller.
Also checked and clear:
isOrgSwitchingis correctly excluded from the persistpartialize, so there's no risk of a persistedtruewedging the app behind the overlay on reload.['projects', orgId]still matchesinvalidateQueries({ queryKey: ['projects'] })by prefix, souseCreateProject.onSuccessis unaffected.- Skipping
updateActiveOrgon a same-org role change is safe — the endpoint only acceptsorgId. - Dropping the
['userDetails']invalidation here is fine;useUpdateActiveOrg.onSuccessalready does it.
Two notes, not blockers:
- The
orgIdin the query key is a client-side fiction —/users/{id}/chapter-assignmentscarries no org param and is scoped server-side from the active org. It lines up today because the PATCH is awaited first, but a switch made in another tab could leave a cache key labelled with the wrong org's data. - No regression test was added, even though
UserHomePage.test.tsxalready has the mock scaffolding for this hook.
|
|
||
| <div className='bg-card flex flex-1 flex-col overflow-hidden rounded-lg border shadow'> | ||
| {loading ? ( | ||
| {loading || isFetching ? ( |
There was a problem hiding this comment.
This one I'd call a blocker. Adding isFetching here is a regression, and I don't think it's buying anything.
There are no defaultOptions on the QueryClient (main.tsx:18), so refetchOnWindowFocus is at its default of true. That means every window refocus now blanks the assignment table and replaces it with a full-panel "Loading…" spinner. Worse, useToggleChapterAi invalidates ['userChapterAssignments'] by prefix (useToggleChapterAi.ts:39), so toggling AI on a chapter wipes the entire list out from under the user mid-interaction.
And it isn't needed for the bug this PR fixes: now that orgId is in the key, switching to a new org already yields isLoading === true (no cache entry), and switching back hits a cache entry that is correct for that org. So isFetching buys a brief extra spinner in the back-switch case in exchange for a real regression on every refocus and every AI toggle.
Suggest either dropping it, or gating it so it only fires when there's nothing to show:
| {loading || isFetching ? ( | |
| {loading || (isFetching && !chapterAssignmentsData) ? ( |
| role: prevRoleName, | ||
| }); | ||
|
|
||
| navigateForRole(prevOrgId, prevRoleName); |
There was a problem hiding this comment.
This navigation fires on a path where nothing actually changed, and it can move the user off the page they were on.
The only thing in the try that can reject is updateActiveOrg.mutateAsync, and it runs before both the setUserDetail and the navigateForRole. So when we land here, no state and no navigation has happened yet — meaning the rollback setUserDetail above is a no-op (userdetail is still the pre-switch closure value), and this navigateForRole is a fresh side effect rather than a rollback. Concretely: a user sitting on a project detail page opens the switcher, the PATCH fails, and they get bounced to their role home page even though the switch never took effect.
I'd drop both the rollback setUserDetail and this navigateForRole, and just surface the toast.
| }); | ||
|
|
||
| navigateForRole(prevOrgId, prevRoleName); | ||
| toast.error('Failed to switch role. Please try again.'); |
There was a problem hiding this comment.
Minor wording: the failure being caught is the updateActiveOrg PATCH, which is an org switch — but the message says "Failed to switch role". Since a role-only change no longer touches the network at all (the isOrgChange guard), this branch is only ever reachable for an org switch. Worth saying "organization".
| <Header /> | ||
| <main className='flex-1 overflow-hidden p-4'> | ||
| <main className='relative flex-1 overflow-hidden p-4'> | ||
| {isOrgSwitching && ( |
There was a problem hiding this comment.
There's no failsafe on this overlay. It blocks all of <main> and is driven solely by isOrgSwitching, which is only cleared in the finally of handleSelectRole. finally covers a throw or a rejection, but not a hang — if the PATCH /users/me/active-org never settles (no timeout or AbortSignal on that fetch), the user is stuck behind this overlay with no way out but a page reload.
A timeout on the request, or a safety setTimeout that clears the flag, would bound the worst case.
| const navigate = useNavigate(); | ||
|
|
||
| const [isExpanded, setIsExpanded] = useState(false); | ||
| const [isSwitching, setIsSwitching] = useState(false); |
There was a problem hiding this comment.
Nit: isSwitching and the store's isOrgSwitching are always set and cleared together, so this is two sources of truth for one piece of state that have to be kept in sync by hand. Since you're already pulling from the store in this component, reading isOrgSwitching directly would let this local state go away.
2 participants
Fix for Issue statement : When a user has the Translator role in both organizations and switches from one organization to the other, the dashboard for the newly selected organization is not displayed automatically. The user needs to manually refresh the page to view the selected organization's dashboard.
Summary by CodeRabbit
New Features
Bug Fixes