Add version check to UI#691
Conversation
WalkthroughThis pull request introduces a client-side version polling mechanism to detect and notify users of application updates. The workflow injects a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Detailed assessmentThe changes introduce a new feature spanning multiple concerns: CI/CD configuration, Vite build plugin, async polling logic with failure counting, DOM API integration (visibility listener), cache storage manipulation, and React component integration. While each individual section is relatively straightforward, the heterogeneity of changes (workflow config, plugin code, module-level logic, component hooks, and test suite) and the interaction between these layers require distributed reasoning across different file types and architectural concerns. The comprehensive test suite (268 lines) demonstrates good coverage but itself requires careful review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 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 `@src/ui/App.tsx`:
- Around line 23-40: The effect calls startVersionPolling which sets up
long-lived polling/listeners but never cleans them up; change
startVersionPolling to return a stop/cleanup function (e.g., stopVersionPolling)
and update the useEffect in App.tsx to capture that return value and return it
as the cleanup (return () => stop()). Keep existing callback that calls
notifications.show and preserve forceRefresh usage when wiring the cleanup.
In `@src/ui/versionCheck.ts`:
- Around line 44-49: The forceRefresh function currently calls
window.location.reload() without awaiting cache deletions; make forceRefresh
async, await caches.keys(), map those keys to caches.delete(k) and await
Promise.all(...) so all deletions complete before calling
window.location.reload(), referencing the forceRefresh function and the
caches.keys()/caches.delete calls to locate where to add the awaits.
- Around line 35-38: The failure cutoff is off-by-one: the condition uses
fetchFailures > 10 which triggers on the 11th failure; change the conditional in
the version polling logic to use fetchFailures >= 10 so stopPolling() is invoked
after ten consecutive failures (update the conditional that references
fetchFailures and stopPolling in versionCheck.ts accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 838cd2c7-98ce-4572-8cf6-b5a212e827ed
📒 Files selected for processing (6)
.github/workflows/deploy-qa.ymlpackage.jsonsrc/ui/App.tsxsrc/ui/versionCheck.test.tssrc/ui/versionCheck.tssrc/ui/vite.config.mjs
| useEffect(() => { | ||
| startVersionPolling(() => { | ||
| notifications.show({ | ||
| id: "version-update", | ||
| title: "Update available", | ||
| message: ( | ||
| <Group gap="xs" mt={4}> | ||
| <Text size="sm">A new version of the app is available.</Text> | ||
| <Button size="xs" variant="light" onClick={forceRefresh}> | ||
| Refresh to update | ||
| </Button> | ||
| </Group> | ||
| ), | ||
| color: "blue", | ||
| autoClose: false, | ||
| }); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Return a cleanup from the polling effect.
startVersionPolling() registers long-lived state, but this effect never tears it down. In React Strict Mode or on remount, that can leave duplicate timers/listeners and duplicate update notifications.
Have the helper return a stop function and return it from this effect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/App.tsx` around lines 23 - 40, The effect calls startVersionPolling
which sets up long-lived polling/listeners but never cleans them up; change
startVersionPolling to return a stop/cleanup function (e.g., stopVersionPolling)
and update the useEffect in App.tsx to capture that return value and return it
as the cleanup (return () => stop()). Keep existing callback that calls
notifications.show and preserve forceRefresh usage when wiring the cleanup.
| if (fetchFailures > 10) { | ||
| console.warn("Failed to fetch version 10 times; giving up."); | ||
| stopPolling(); | ||
| } else { |
There was a problem hiding this comment.
Fix the failure threshold off by one.
fetchFailures > 10 stops on the 11th rejected request, not the 10th. If the intended cutoff is 10 consecutive failures, this should be >= 10.
🛠️ Suggested fix
- if (fetchFailures > 10) {
+ if (fetchFailures >= 10) {📝 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.
| if (fetchFailures > 10) { | |
| console.warn("Failed to fetch version 10 times; giving up."); | |
| stopPolling(); | |
| } else { | |
| if (fetchFailures >= 10) { | |
| console.warn("Failed to fetch version 10 times; giving up."); | |
| stopPolling(); | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/versionCheck.ts` around lines 35 - 38, The failure cutoff is
off-by-one: the condition uses fetchFailures > 10 which triggers on the 11th
failure; change the conditional in the version polling logic to use
fetchFailures >= 10 so stopPolling() is invoked after ten consecutive failures
(update the conditional that references fetchFailures and stopPolling in
versionCheck.ts accordingly).
| export function forceRefresh() { | ||
| if ("caches" in window) { | ||
| caches.keys().then((keys) => keys.forEach((k) => caches.delete(k))); | ||
| } | ||
| window.location.reload(); | ||
| } |
There was a problem hiding this comment.
Await cache deletion before reloading.
window.location.reload() runs before the Cache Storage deletions finish, so stale assets can survive the refresh. Make this async and wait for the deletions to complete first.
♻️ Suggested fix
-export function forceRefresh() {
- if ("caches" in window) {
- caches.keys().then((keys) => keys.forEach((k) => caches.delete(k)));
- }
- window.location.reload();
+export async function forceRefresh() {
+ if ("caches" in window) {
+ const keys = await caches.keys();
+ await Promise.all(keys.map((k) => caches.delete(k)));
+ }
+ window.location.reload();
}🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 48-48: Prefer globalThis over window.
[warning] 45-45: Prefer globalThis over window.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/versionCheck.ts` around lines 44 - 49, The forceRefresh function
currently calls window.location.reload() without awaiting cache deletions; make
forceRefresh async, await caches.keys(), map those keys to caches.delete(k) and
await Promise.all(...) so all deletions complete before calling
window.location.reload(), referencing the forceRefresh function and the
caches.keys()/caches.delete calls to locate where to add the awaits.
1 participant
Generate a version.json file in the UI - have the UI poll for a change to this file and prompt the user to refresh if there's a change.
Summary by CodeRabbit
Release Notes
New Features
Chores
Tests