ENG-397 fix: refine initial update check condition in useAppVersion hook#16382
ENG-397 fix: refine initial update check condition in useAppVersion hook#16382abhimanyurajeesh wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe ChangesRoute-aware version update logic
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryFixes the unintended auto-reload behaviour where the app was silently refreshing on every page load regardless of the current route. The change adds a
Confidence Score: 4/5Single-file change with a clear, targeted fix; the core logic is correct and the only issues are stale comments. The behaviour change is minimal and well-scoped: users on No files require special attention beyond the comment/JSDoc updates noted in the review comments. Important Files Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useAppVersion.ts (2)
77-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the else branch comment to include initial non-root checks.
The comment on line 82 states "Polling check: notify user, wait for confirmation", but this else block now handles two scenarios:
- Polling checks (when
isInitialis false)- Initial checks on non-root routes (when
isInitialis true but pathname is not"/")💬 Suggested comment update
// Version differs if (isInitial && window.location.pathname === "/") { // Initial check on root route: auto-update performAppUpdate(data.version); } else { - // Polling check: notify user, wait for confirmation + // Polling check or initial check on non-root route: notify user, wait for confirmation setPendingUpdate(data.version); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useAppVersion.ts` around lines 77 - 84, Update the comment above the else block in useAppVersion to accurately describe both scenarios it handles: (a) polling checks when isInitial is false and (b) the initial check when isInitial is true but window.location.pathname !== "/", i.e., initial non-root routes—clarify that in these cases we setPendingUpdate(data.version) to notify the user and wait for confirmation rather than auto-updating via performAppUpdate.
27-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate JSDoc to reflect route-dependent initial update behavior.
The JSDoc states that on initial mount, if the stored version differs, the app "auto-updates (clears caches + reloads)". However, with the new logic, auto-update only occurs when the initial check happens on the root route (
"/"). On non-root routes, the initial check now setspendingUpdateand waits for user confirmation.📝 Suggested JSDoc update
/** * Hook for managing app version updates with React Query. * * On initial mount: * - If no stored version (first visit): stores current version without reload - * - If stored version differs from server: auto-updates (clears caches + reloads) + * - If stored version differs from server: + * - On root route ("/"): auto-updates (clears caches + reloads) + * - On non-root routes: sets pendingUpdate state (user must confirm) * * On subsequent polling checks: * - If version differs: sets pendingUpdate state (user must confirm) */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useAppVersion.ts` around lines 27 - 36, The JSDoc for useAppVersion is inaccurate about initial-mount behavior: update only auto-applies on initial checks when the current route is root ("/"); on non-root routes the hook now sets pendingUpdate instead of forcing reload. Update the comment block for useAppVersion to mention route-dependent initial behavior, explicitly state that on initial mount the stored/version mismatch will auto-update (clear caches + reload) only when location.pathname === "/" and otherwise will set pendingUpdate and await user confirmation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/hooks/useAppVersion.ts`:
- Around line 77-84: Update the comment above the else block in useAppVersion to
accurately describe both scenarios it handles: (a) polling checks when isInitial
is false and (b) the initial check when isInitial is true but
window.location.pathname !== "/", i.e., initial non-root routes—clarify that in
these cases we setPendingUpdate(data.version) to notify the user and wait for
confirmation rather than auto-updating via performAppUpdate.
- Around line 27-36: The JSDoc for useAppVersion is inaccurate about
initial-mount behavior: update only auto-applies on initial checks when the
current route is root ("/"); on non-root routes the hook now sets pendingUpdate
instead of forcing reload. Update the comment block for useAppVersion to mention
route-dependent initial behavior, explicitly state that on initial mount the
stored/version mismatch will auto-update (clear caches + reload) only when
location.pathname === "/" and otherwise will set pendingUpdate and await user
confirmation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7f8e45e-d983-4085-bd10-d8705d7b5eec
📒 Files selected for processing (1)
src/hooks/useAppVersion.ts
Deploying care-preview with
|
| Latest commit: |
bad57c7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a7757ba5.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-397.care-preview-a7w.pages.dev |
There was a problem hiding this comment.
Pull request overview
Refines the useAppVersion update flow so that automatic cache clear + reload only happens on the root route (/) during the initial version check, while non-root routes surface an “update available” prompt instead of forcing a reload.
Changes:
- Restricts initial auto-update behavior to
window.location.pathname === "/". - Updates hook documentation to reflect differing behavior for root vs non-root routes.
| if (isInitial && window.location.pathname === "/") { | ||
| // Initial check on root route: auto-update | ||
| performAppUpdate(data.version); | ||
| } else { | ||
| // Polling check: notify user, wait for confirmation | ||
| // Non-root initial check or polling check: notify user, wait for confirmation | ||
| setPendingUpdate(data.version); |
Proposed Changes
Fixes ENG-397
"App auto update is intended to be auto updated only on the root route; However, it seems to be auto-updating even in non-root routes as well now."
This PR fixes the above issue, now if the user is in a non-root route there will be the update available toast and if in root it auto updates as it was.
How to test
Test 1 : auto-update at root route
--
Test 2 :No auto-update on non-root route
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit