fix: prevent blank new tab on startup when chrome.storage stalls#340
Conversation
The initialization chain is: storage.js creates __storageBridgeReady → calls chrome.storage.local.get() → bootstrap.js waits for the promise → loads all scripts. If chrome.storage.local.get() never calls back (extension reload, context invalidation, transient API failure), the promise never resolves and NO scripts load — the page stays blank. Fix by adding two independent safety timeout layers: 1. In storage.js: a 3-second timeout on __storageBridgeReady that resolves the promise if chrome.storage doesn't respond, falling back to the native localStorage snapshot already captured. 2. In bootstrap.js: an 8-second Promise.race with __storageBridgeReady that forces script loading regardless, as a belt-and-suspenders catch-all. Both layers log warnings when activated so the fallback path is observable in DevTools. Also adds a regression test that simulates a stalled chrome.storage.local.get() and verifies the bridge recovers via the timeout. Fixes #314 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LWWZH
left a comment
LWWZH
left a comment
There was a problem hiding this comment.
Findings
-
Low: Redundant
clearTimeoutin timeout callback —src/core/storage.js:26(diff line)- Inside the
setTimeoutcallback,clearTimeout(storageBridgeTimeoutId)is called after the timer has already fired. This is a no-op. Not a bug, but slightly redundant code.
- Inside the
-
Low: Late chrome.storage callback silently updates cache after bridge resolution —
src/core/storage.js(diff line aroundhydrateFromChromeStoragecallback)- If
chrome.storage.local.get()eventually calls back after the 3s timeout,applySnapshot(mergedSnapshot)still executes and updates the cache. Code that read settings between T=3s and when chrome.storage finally responds will have stale values. This is acceptable and documented in the warning message, but worth noting as a behavioral side effect.
- If
-
Low: Bootstrap 8s timer never cleared —
src/core/bootstrap.js:62(diff line)- The
setTimeoutinsidePromise.racealways fires after 8s even ifreadyresolved earlier. Theresolve()on an already-settled promise is a harmless no-op, but the timer still runs on every page load.
- The
-
Medium: Test timeout is tight —
tests/bootstrap-timeout.test.js:30(diff line)- Test timeout is 5000ms while the storage bridge timeout is 3000ms, leaving only ~2s of margin for setup, eval, and assertions. In slow CI environments this could flake. Consider bumping the test timeout to 8000ms.
Open Questions
- The 3s timeout in
storage.jsand the 8s timeout inbootstrap.jsare independent layers. Is the 8s belt-and-suspenders intentionally much longer than 3s, or should it be closer (e.g., 5s) to reduce the window where the page renders with default settings before chrome.storage responds?
Summary
Approve with minor notes. The fix is well-designed and correctly addresses the root cause. The two-layer timeout approach (3s storage + 8s bootstrap) is sound defense-in-depth. The idempotency guard (storageBridgeResolved flag) properly prevents double-resolution races. The test adequately covers the stalling scenario. The low-severity findings are style/efficiency nits, not correctness issues. The tight test timeout is the only actionable concern for CI reliability.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a 3s storage watchdog and an 8s bootstrap fallback to avoid startup hangs, introduces a motion preference singleton (prefersReducedMotion / onReducedMotionChange / crossfadeDelayMs), applies reduced-motion branches across UI and video background flows, updates tests to inject motion, and refreshes docs and lint globals. ChangesStartup Resilience and Reduced-Motion Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/storage.js`:
- Around line 18-30: The timeout callback for storageBridgeTimeoutId redundantly
calls clearTimeout(storageBridgeTimeoutId) after the timer has already fired;
remove the clearTimeout(storageBridgeTimeoutId) call from the function inside
the setTimeout so the block just sets storageBridgeResolved = true, nulls
storageBridgeTimeoutId if desired, and calls resolveStorageBridge() (refer to
storageBridgeTimeoutId, storageBridgeResolved, and resolveStorageBridge).
In `@tests/bootstrap-timeout.test.js`:
- Line 73: The test timeout literal 5_000 is too close to the storage bridge
timeout (3_000) and should be increased for CI reliability; update the timeout
value from 5_000 to 7_000 (or at least 6_000) in tests/bootstrap-timeout.test.js
where the timeout is set (replace the numeric literal 5_000) so tests have a
larger buffer.
- Line 56: Replace the unsafe eval re-injection of STORAGE_CODE (the line "(0,
eval)(STORAGE_CODE)") with the vm.Script pattern used in tests/storage.test.js:
create a new JSDOM window/context, create a new vm.Script(STORAGE_CODE) and run
it in that context so the storage module is evaluated in an isolated DOM
environment; update the test to use the same helper/context creation used by
storage.test.js so STORAGE_CODE is executed with vm.Script.runInContext instead
of eval.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9934ed8c-1034-4b72-bbdf-aec47dea43a6
📒 Files selected for processing (3)
src/core/bootstrap.jssrc/core/storage.jstests/bootstrap-timeout.test.js
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/bootstrap-timeout.test.js`:
- Around line 51-53: Replace the direct vm.Script injection with the shared
injectScript helper: remove the vm.Script instantiation and runInContext call
and instead call injectScript with STORAGE_CODE and the test's VM context (e.g.
injectScript(STORAGE_CODE, dom.getInternalVMContext())); this aligns this spec
with the repository test contract and uses the common injectScript helper rather
than vm.Script.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85e4429f-f10e-40e9-93ff-0b8c4fa30a82
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/core/storage.jssrc/core/utils.jstests/bootstrap-timeout.test.js
💤 Files with no reviewable changes (1)
- src/core/storage.js
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@coderabbitai rate limit |
|
Your plan includes 5 PR reviews per hour. Reviews are available now. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/core/bootstrap.js`:
- Around line 56-72: The fallback timer created in the Promise.race (used to
build readyWithFallback) can still fire after the real ready promise resolves,
producing misleading console warnings; fix this by capturing the timeout id
returned from setTimeout and clearing it when the original ready promise
fulfills or rejects. Concretely, store the timer id in a variable (from
setTimeout inside the new Promise), attach ready.then/ready.catch (or finally)
to call clearTimeout(timerId), and keep the Promise.race construction for
readyWithFallback using ready and the fallback promise; reference the
readyWithFallback, ready, and SCRIPT_BOOTSTRAP_TIMEOUT_MS symbols when making
the change.
In `@src/core/storage.js`:
- Around line 18-29: The timeout callback duplicates the logic that exists in
resolveStorageReady(): it manually sets storageBridgeResolved, nulls
storageBridgeTimeoutId, and calls resolveStorageBridge; replace that manual
block with a single call to resolveStorageReady() so the timeout handler just
calls resolveStorageReady(), reducing duplication and ensuring centralized
behavior for storage readiness handling (reference symbols:
storageBridgeTimeoutId, storageBridgeResolved, resolveStorageBridge,
resolveStorageReady).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2783909-9462-4f0f-bb7c-6cd3981447fd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
src/core/bootstrap.jssrc/core/storage.jssrc/core/utils.jstests/bootstrap-timeout.test.jstests/helpers/inject-script.js
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/core/storage.js`:
- Around line 18-25: Extract the magic timeout value (3000) into a named
constant (e.g., STORAGE_BRIDGE_TIMEOUT_MS) and use it in both the setTimeout
call and the warning message; locate the setTimeout usage that assigns
storageBridgeTimeoutId and calls resolveStorageReady(), replace the literal 3000
with STORAGE_BRIDGE_TIMEOUT_MS and interpolate or compute the human-readable "3
s" from that constant so the message and timer remain consistent.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4df77439-c332-4961-b333-f2db023cae9c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
src/core/bootstrap.jssrc/core/storage.jssrc/core/utils.jstests/bootstrap-timeout.test.jstests/helpers/inject-script.js
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 6 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ui/settings.js (1)
1385-1399:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuilt-in video backgrounds still cannot recover from an initial reduced-motion load.
triggerCrossfade()'s reduced-motion branch leaves built-in videos withoutactive/ready, butsyncVideoToMotionPreference()returns unless the element is alreadyactive. If the page starts with reduced motion enabled, flipping the OS preference back off later is still a no-op for that path. This subscriber needs the same "resume fromdata-reducedMotionPausedeven when inactive" recovery thatsrc/data/custom-backgrounds.jsnow applies, including restoring the visible post-crossfade state.♻️ Minimal fix sketch
function syncVideoToMotionPreference(reduced) { const videoEl = document.getElementById('bg-video'); - if (!videoEl || !videoEl.currentSrc || !videoEl.classList.contains('active')) return; + if (!videoEl || !videoEl.currentSrc) return; + if (!videoEl.classList.contains('active') && videoEl.dataset.reducedMotionPaused !== 'true') return; if (window._customBackgrounds && typeof window._customBackgrounds.isCustom === 'function' && window._customBackgrounds.isCustom(loadBg())) return; if (reduced) { if (!videoEl.paused) safePause(videoEl); videoEl.dataset.reducedMotionPaused = 'true'; } else { const wasReducedPaused = videoEl.dataset.reducedMotionPaused === 'true'; delete videoEl.dataset.reducedMotionPaused; - if (wasReducedPaused && loadVideoAutoplay() && !document.hidden && (!window.loadSimpleMode || !window.loadSimpleMode()) && videoEl.readyState >= 3) { - videoEl.play().catch(() => {}); + if (wasReducedPaused) { + const thumbnailEl = document.getElementById('bg-thumbnail'); + videoEl.classList.add('active', 'ready'); + if (thumbnailEl && !thumbnailEl.classList.contains('hidden')) { + thumbnailEl.classList.add('clearing'); + backgroundTransitionTimeout = setTimeout(() => { + thumbnailEl.classList.add('hidden'); + thumbnailEl.classList.remove('clearing'); + backgroundTransitionTimeout = null; + }, crossfadeDelayMs(VIDEO_THUMBNAIL_HIDE_DELAY_MS)); + } + if (loadVideoAutoplay() && !document.hidden && (!window.loadSimpleMode || !window.loadSimpleMode()) && videoEl.readyState >= 3) { + videoEl.play().catch(() => {}); + } } } }🤖 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/ui/settings.js` around lines 1385 - 1399, syncVideoToMotionPreference currently bails out early if the video element isn't active, preventing built-in videos from recovering after an initial reduced-motion load; change the logic so that when reduced is false and videoEl.dataset.reducedMotionPaused === 'true' we perform the same recovery as custom-backgrounds: restore the visible post-crossfade state (e.g. ensure the element has the expected active/ready classes/state that triggerCrossfade would set), delete data-reducedMotionPaused, and attempt playback (videoEl.play().catch(() => {})) if loadVideoAutoplay() and other checks pass; in short, don't return immediately on !active — handle the data-reducedMotionPaused resume path for built-in videos inside syncVideoToMotionPreference while keeping the reduced=true pause behavior using safePause.
🧹 Nitpick comments (4)
tests/bootstrap-timeout.test.js (2)
10-14: ⚡ Quick winDrop the Vitest globals import in this spec.
This repo already enables
describe/it/expectglobally, so importing them here just violates the shared test convention.♻️ Proposed cleanup
-import { describe, it, expect } from 'vitest'; import { readFileSync } from 'fs'; import { resolve } from 'path'; import { JSDOM } from 'jsdom'; import { injectScript } from './helpers/inject-script.js';As per coding guidelines, "Use Vitest 3 with jsdom in
vitest.config.js; globals are enabled so do not importdescribe/it/expectin test files".🤖 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 `@tests/bootstrap-timeout.test.js` around lines 10 - 14, Remove the explicit import of Vitest globals from the top of tests/bootstrap-timeout.test.js: drop "import { describe, it, expect } from 'vitest';" and rely on the project-level Vitest globals (describe, it, expect) enabled in vitest.config.js so the rest of the test (including functions like injectScript, JSDOM usage, readFileSync/resolve) continues to work unchanged.Source: Coding guidelines
19-74: 🏗️ Heavy liftThis regression never exercises the bootstrap fallback.
The blank-tab failure happens when
src/core/bootstrap.jsstays blocked onwindow.__storageBridgeReady, but this spec only proves that re-injectedsrc/core/storage.jsresolves after 3 seconds.bootstrap.js's new 8-secondPromise.racecan regress independently and this test would still stay green. Please add a smoke path that injectssrc/core/bootstrap.jsand asserts script loading continues after a stalled bridge.Based on learnings,
src/core/storage.jsexposeswindow.__storageBridgeReadythat bootstrap waits on before loading other scripts. Based on PR objectives, this fix should be covered by a regression that fails when startup would still render blank.🤖 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 `@tests/bootstrap-timeout.test.js` around lines 19 - 74, Add a smoke test that injects src/core/bootstrap.js (in addition to STORAGE_CODE) into the JSDOM context so we exercise bootstrap's Promise.race fallback rather than only storage.js: after stubbing chrome.storage.local.get to never call back, inject STORAGE_CODE, then inject the bootstrap code (the module that waits on window.__storageBridgeReady) using injectScript(dom.getInternalVMContext()), and assert that bootstrap's continuation side‑effect (e.g. the global it normally sets when it finishes booting—replace with the actual symbol from bootstrap such as a window.__bootstrapFinished or the script-loading side effect present in bootstrap.js) occurs and that script loading continues despite the stalled bridge; keep the existing timeout assertions on window.__storageBridgeReady to ensure the 3s safety timeout still resolves.Source: Learnings
tests/prefers-reduced-motion.test.js (2)
13-39: ⚡ Quick winAdd the standard per-test storage/DOM cleanup for this file.
These cases mutate
localStorageand append DOM nodes across multiple suites, but the file never installs the repo's requiredbeforeEachcleanup. That makes the spec more order-dependent than the rest of the test suite.As per coding guidelines, "In tests, use
beforeAllandbeforeEachto clearlocalStorage; usebeforeEachto also remove transient toast/notification/validation DOM nodes between tests."🤖 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 `@tests/prefers-reduced-motion.test.js` around lines 13 - 39, Add a beforeEach that performs the repo-standard per-test cleanup: clear localStorage, remove transient DOM nodes (e.g. query and remove toasts/notifications/validation elements appended by tests), reset motion state (call setReduced(false) if needed) and restore timers (vi.useRealTimers()) so each test starts with a clean DOM/storage/timer environment; place this beforeEach alongside the existing beforeAll/afterEach and reference the existing beforeAll, afterEach, setReduced and vi.useRealTimers symbols when implementing.Source: Coding guidelines
8-9: ⚡ Quick winDrop the explicit Vitest globals import here.
This spec imports
describe/it/expecteven though the repo runs Vitest with globals enabled, so the file is out of step with the test convention.♻️ Minimal cleanup
-import { describe, it, expect, beforeAll, beforeEach, afterEach, vi } from 'vitest'; +import { beforeAll, beforeEach, afterEach, vi } from 'vitest';As per coding guidelines, "Use Vitest 3 with jsdom in
vitest.config.js; globals are enabled so do not importdescribe/it/expectin test files."🤖 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 `@tests/prefers-reduced-motion.test.js` around lines 8 - 9, Remove the explicit Vitest globals from the import statement: drop describe, it, expect, beforeAll, beforeEach, afterEach (the repo runs Vitest with globals enabled) and only import what’s actually needed from 'vitest' (e.g., keep vi if used); keep the import of injectScript from './helpers/inject-script.js' unchanged so the file uses global describe/it/expect/etc. instead of importing them.Source: Coding guidelines
🤖 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.
Inline comments:
In `@New-Tab.html`:
- Line 20: New-Tab.html currently includes src/core/motion.js via a static
<script> tag; remove that tag and instead add "src/core/motion.js" to the
scriptSources array in src/core/bootstrap.js so bootstrap loads it at the end of
the body, while leaving the inline motion stubs in New-Tab.html intact; update
src/core/bootstrap.js's scriptSources (the array referenced in bootstrap
initialization) to include the motion path and ensure ordering matches other
runtime scripts.
---
Duplicate comments:
In `@src/ui/settings.js`:
- Around line 1385-1399: syncVideoToMotionPreference currently bails out early
if the video element isn't active, preventing built-in videos from recovering
after an initial reduced-motion load; change the logic so that when reduced is
false and videoEl.dataset.reducedMotionPaused === 'true' we perform the same
recovery as custom-backgrounds: restore the visible post-crossfade state (e.g.
ensure the element has the expected active/ready classes/state that
triggerCrossfade would set), delete data-reducedMotionPaused, and attempt
playback (videoEl.play().catch(() => {})) if loadVideoAutoplay() and other
checks pass; in short, don't return immediately on !active — handle the
data-reducedMotionPaused resume path for built-in videos inside
syncVideoToMotionPreference while keeping the reduced=true pause behavior using
safePause.
---
Nitpick comments:
In `@tests/bootstrap-timeout.test.js`:
- Around line 10-14: Remove the explicit import of Vitest globals from the top
of tests/bootstrap-timeout.test.js: drop "import { describe, it, expect } from
'vitest';" and rely on the project-level Vitest globals (describe, it, expect)
enabled in vitest.config.js so the rest of the test (including functions like
injectScript, JSDOM usage, readFileSync/resolve) continues to work unchanged.
- Around line 19-74: Add a smoke test that injects src/core/bootstrap.js (in
addition to STORAGE_CODE) into the JSDOM context so we exercise bootstrap's
Promise.race fallback rather than only storage.js: after stubbing
chrome.storage.local.get to never call back, inject STORAGE_CODE, then inject
the bootstrap code (the module that waits on window.__storageBridgeReady) using
injectScript(dom.getInternalVMContext()), and assert that bootstrap's
continuation side‑effect (e.g. the global it normally sets when it finishes
booting—replace with the actual symbol from bootstrap such as a
window.__bootstrapFinished or the script-loading side effect present in
bootstrap.js) occurs and that script loading continues despite the stalled
bridge; keep the existing timeout assertions on window.__storageBridgeReady to
ensure the 3s safety timeout still resolves.
In `@tests/prefers-reduced-motion.test.js`:
- Around line 13-39: Add a beforeEach that performs the repo-standard per-test
cleanup: clear localStorage, remove transient DOM nodes (e.g. query and remove
toasts/notifications/validation elements appended by tests), reset motion state
(call setReduced(false) if needed) and restore timers (vi.useRealTimers()) so
each test starts with a clean DOM/storage/timer environment; place this
beforeEach alongside the existing beforeAll/afterEach and reference the existing
beforeAll, afterEach, setReduced and vi.useRealTimers symbols when implementing.
- Around line 8-9: Remove the explicit Vitest globals from the import statement:
drop describe, it, expect, beforeAll, beforeEach, afterEach (the repo runs
Vitest with globals enabled) and only import what’s actually needed from
'vitest' (e.g., keep vi if used); keep the import of injectScript from
'./helpers/inject-script.js' unchanged so the file uses global
describe/it/expect/etc. instead of importing them.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25db4ab5-30df-4b7f-8aa7-737fa8475bd9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
AGENTS.mdCHANGELOG.mdNew-Tab.htmlcss/core.csscss/features.csseslint.config.jssrc/core/bootstrap.jssrc/core/main.jssrc/core/motion.jssrc/core/storage.jssrc/core/utils.jssrc/data/custom-backgrounds.jssrc/features/drag-drop.jssrc/features/notes.jssrc/features/todo.jssrc/ui/add-app-modal.jssrc/ui/settings.jstests/bootstrap-timeout.test.jstests/global-collision-regression.test.jstests/helpers/inject-script.jstests/prefers-reduced-motion.test.jstests/settings.test.js
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 5 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 5 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ui/settings.js (1)
541-545:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReduced-motion resume is still blocked for videos initialized in reduced mode.
At Line 1387,
syncVideoToMotionPreferencereturns unless#bg-videoalready hasactive. But at Line 541-Line 544, the reduced-motion branch intriggerCrossfadeintentionally never setsactive. Result: toggling reduced motion off can leave built-in video backgrounds stuck on the thumbnail path.♻️ Minimal fix sketch
function syncVideoToMotionPreference(reduced) { const videoEl = document.getElementById('bg-video'); - if (!videoEl || !videoEl.currentSrc || !videoEl.classList.contains('active')) return; + if (!videoEl || !videoEl.currentSrc) return; + const wasReducedPaused = videoEl.dataset.reducedMotionPaused === 'true'; + if (!videoEl.classList.contains('active') && !wasReducedPaused) return; if (window._customBackgrounds && typeof window._customBackgrounds.isCustom === 'function' && window._customBackgrounds.isCustom(loadBg())) return; if (reduced) { if (!videoEl.paused) safePause(videoEl); videoEl.dataset.reducedMotionPaused = 'true'; } else { - const wasReducedPaused = videoEl.dataset.reducedMotionPaused === 'true'; delete videoEl.dataset.reducedMotionPaused; if (wasReducedPaused && loadVideoAutoplay() && !document.hidden && (!window.loadSimpleMode || !window.loadSimpleMode()) && videoEl.readyState >= 3) { + videoEl.classList.add('active', 'ready'); + const thumbnailEl = document.getElementById('bg-thumbnail'); + if (thumbnailEl && !thumbnailEl.classList.contains('hidden')) { + thumbnailEl.classList.add('clearing'); + clearBackgroundTransitionTimeout(); + backgroundTransitionTimeout = setTimeout(() => { + thumbnailEl.classList.add('hidden'); + thumbnailEl.classList.remove('clearing'); + backgroundTransitionTimeout = null; + }, crossfadeDelayMs(VIDEO_THUMBNAIL_HIDE_DELAY_MS)); + } videoEl.play().catch(() => {}); } } }Also applies to: 1385-1397
🤖 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/ui/settings.js` around lines 541 - 545, triggerCrossfade's reduced-motion branch sets videoEl.dataset.reducedMotionPaused = 'true' and hides the overlay but never marks the background video as active, which causes syncVideoToMotionPreference to return early (it checks for dataset.active) and prevents resuming when reduced-motion is toggled off; fix by marking the same element as active in the reduced-motion path (set videoEl.dataset.active = 'true' or call the same activation helper used in the normal path) so that syncVideoToMotionPreference will proceed to resume playback, and ensure any teardown/overlay logic remains correct (update triggerCrossfade where videoEl.dataset.reducedMotionPaused is set and adjust the activation logic used by syncVideoToMotionPreference).
🤖 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.
Inline comments:
In `@New-Tab.html`:
- Around line 10-19: The inline fallback script in New-Tab.html defining
window.crossfadeDelayMs, window.prefersReducedMotion, and
window.onReducedMotionChange must be removed and its logic moved into a core JS
file; add the fallback wiring into src/core/bootstrap.js (or src/core/motion.js)
so New-Tab.html keeps exactly two <script> tags. Implement the same safe-noop
assignments for the global symbols (window.crossfadeDelayMs,
window.prefersReducedMotion, window.onReducedMotionChange) inside bootstrap.js
(or motion.js) and, if adding a new source file is required, append its path to
the scriptSources array used by bootstrap.js rather than inserting another
<script> tag in New-Tab.html.
---
Duplicate comments:
In `@src/ui/settings.js`:
- Around line 541-545: triggerCrossfade's reduced-motion branch sets
videoEl.dataset.reducedMotionPaused = 'true' and hides the overlay but never
marks the background video as active, which causes syncVideoToMotionPreference
to return early (it checks for dataset.active) and prevents resuming when
reduced-motion is toggled off; fix by marking the same element as active in the
reduced-motion path (set videoEl.dataset.active = 'true' or call the same
activation helper used in the normal path) so that syncVideoToMotionPreference
will proceed to resume playback, and ensure any teardown/overlay logic remains
correct (update triggerCrossfade where videoEl.dataset.reducedMotionPaused is
set and adjust the activation logic used by syncVideoToMotionPreference).
🪄 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: ebef743b-c325-49ae-a45f-7298f30dd423
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
AGENTS.mdCHANGELOG.mdNew-Tab.htmlcss/core.csscss/features.csseslint.config.jssrc/core/bootstrap.jssrc/core/main.jssrc/core/motion.jssrc/core/storage.jssrc/core/utils.jssrc/data/custom-backgrounds.jssrc/features/drag-drop.jssrc/features/notes.jssrc/features/todo.jssrc/ui/add-app-modal.jssrc/ui/settings.jstests/bootstrap-timeout.test.jstests/global-collision-regression.test.jstests/helpers/inject-script.jstests/prefers-reduced-motion.test.jstests/settings.test.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/data/custom-backgrounds.js`:
- Around line 759-767: Clear any existing customBackgroundTransitionTimeout
before calling setTimeout, then schedule the new timeout with
crossfadeDelayMs(VIDEO_THUMBNAIL_HIDE_DELAY_MS); inside the timeout callback
re-check that thumbEl still exists and is not already hidden (e.g., verify
thumbEl && !thumbEl.classList.contains('hidden')) before adding 'hidden' or
removing 'clearing', and finally set customBackgroundTransitionTimeout = null;
use the existing identifiers thumbEl, customBackgroundTransitionTimeout,
crossfadeDelayMs, and VIDEO_THUMBNAIL_HIDE_DELAY_MS to locate and update the
logic.
In `@src/features/drag-drop.js`:
- Around line 453-473: The timeout fallback currently calls cleanup but doesn't
remove the animationend listener, so if the timer runs first the `{ once: true
}` listener remains attached and can accumulate; fix by capturing the listener
reference when adding it (e.g., const onAnimationEnd = cleanup or a wrapper) and
call movedEl.removeEventListener('animationend', onAnimationEnd) from the
cleanup path (before/after clearing cleanupTimer) inside the
requestAnimationFrame block where movedEl, cleanupTimer and cleanup are defined,
ensuring the listener is removed whether the animationend or the timeout
triggers.
🪄 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: d6be811e-960e-4e49-90e8-63695f79405a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
AGENTS.mdCHANGELOG.mdcss/core.csscss/features.csseslint.config.jssrc/core/bootstrap.jssrc/core/main.jssrc/core/motion.jssrc/core/storage.jssrc/core/utils.jssrc/data/custom-backgrounds.jssrc/features/drag-drop.jssrc/features/notes.jssrc/features/todo.jssrc/ui/add-app-modal.jssrc/ui/settings.jstests/bootstrap-timeout.test.jstests/global-collision-regression.test.jstests/helpers/inject-script.jstests/prefers-reduced-motion.test.jstests/settings.test.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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/ui/settings.js (2)
1375-1436:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPersist the motion unsubscribe handle across full script re-evals.
unsubscribeVideoMotiononly dedupes repeatedinitSettings()calls in the same eval. A secondinjectScript()/HMR reload starts fromnull, leaves the oldonReducedMotionChangesubscriber attached, and motion flips will runsyncVideoToMotionPreference()twice.Store the handle on
windowthe same waycustom-backgrounds.jsdoes so teardown survives a full script reinjection.As per coding guidelines, tests load source files by calling
injectScript(), so this teardown needs to survive a full script re-evaluation, not just anotherinitSettings()call.🤖 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/ui/settings.js` around lines 1375 - 1436, The unsubscribe handle unsubscribeVideoMotion is only module-scoped so it is lost across full script re-evaluations and duplicate onReducedMotionChange subscribers persist; instead persist the handle on window (mirror how custom-backgrounds.js does) so teardown survives reinjection: in initSettings, replace the module-local unsubscribeVideoMotion usage with a window-scoped slot (e.g. window._unsubscribeVideoMotion) and use window._unsubscribeVideoMotion = window.onReducedMotionChange(syncVideoToMotionPreference) when registering and call it to teardown before reassigning; ensure any existing window._unsubscribeVideoMotion is invoked if present and remove the module-level reliance while keeping syncVideoToMotionPreference and window.onReducedMotionChange references.Source: Coding guidelines
1010-1029:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the static thumbnail when autoplay is enabled under reduced motion.
This branch schedules thumbnail hiding before it checks
prefersReducedMotion(). SincecrossfadeDelayMs(...)collapses to0, turning autoplay on while reduced motion is enabled immediately hides the thumbnail and exposes a paused video frame, which disagrees with the reduced-motion path intriggerCrossfade().Move the reduced-motion branch ahead of the thumbnail cleanup and leave the thumbnail visible whenever
reducedMotionPausedis set.♻️ Minimal fix sketch
if (videoAutoplaySetting.checked) { if (videoEl.readyState >= 2) { // Video already loaded — show it immediately videoEl.dataset.crossfadeTriggered = 'true'; videoEl.classList.add('active', 'ready'); - if (thumbnailEl && !thumbnailEl.classList.contains('hidden')) { - thumbnailEl.classList.add('clearing'); - setTimeout(function () { - thumbnailEl.classList.add('hidden'); - thumbnailEl.classList.remove('clearing'); - }, crossfadeDelayMs(VIDEO_THUMBNAIL_HIDE_DELAY_MS)); - } if (window.prefersReducedMotion && window.prefersReducedMotion()) { videoEl.dataset.reducedMotionPaused = 'true'; + if (thumbnailEl) { + thumbnailEl.classList.remove('clearing'); + thumbnailEl.classList.remove('hidden'); + } } else { + if (thumbnailEl && !thumbnailEl.classList.contains('hidden')) { + thumbnailEl.classList.add('clearing'); + setTimeout(function () { + thumbnailEl.classList.add('hidden'); + thumbnailEl.classList.remove('clearing'); + }, crossfadeDelayMs(VIDEO_THUMBNAIL_HIDE_DELAY_MS)); + } delete videoEl.dataset.reducedMotionPaused; if (!window.loadSimpleMode || !window.loadSimpleMode()) { videoEl.play().catch(function () {}); }🤖 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/ui/settings.js` around lines 1010 - 1029, In the videoAutoplaySetting.checked branch, move the reduced-motion check (window.prefersReducedMotion && window.prefersReducedMotion()) so it runs before any thumbnail hiding logic; if reduced motion is active set videoEl.dataset.reducedMotionPaused = 'true' and skip the thumbnail clearing/hidden class toggles and setTimeout(crossfadeDelayMs(...)) so the static thumbnail remains visible, otherwise delete videoEl.dataset.reducedMotionPaused and proceed with the existing thumbnail clearing and videoEl.play() logic (respecting window.loadSimpleMode). Refer to videoAutoplaySetting.checked, videoEl.dataset.reducedMotionPaused, thumbnailEl, crossfadeDelayMs and VIDEO_THUMBNAIL_HIDE_DELAY_MS to locate and reorder the code.
♻️ Duplicate comments (1)
src/core/storage.js (1)
19-26:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGuard late hydration after the watchdog path.
Once Line 25 resolves
__storageBridgeReady, the rest of the app can start callinglocalStorage.setItem(). Ifchrome.storage.local.get()finally returns later, Lines 142-143 rebuildcachefrom the stale pre-timeout snapshot and can drop writes made after startup resumed.Track mutations between
storageArea.get()start and callback, then skip stale hydration or merge only untouched keys before callingapplySnapshot(...).Based on learnings,
src/core/storage.jsis the bridge that unblocks bootstrap throughwindow.__storageBridgeReady, so this callback races with real app writes after startup.Also applies to: 111-145
🤖 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/core/storage.js` around lines 19 - 26, When the STORAGE bridge timeout fires you currently call resolveStorageReady and later the chrome.storage.local.get callback blindly calls applySnapshot which can overwrite writes made after resolveStorageReady; to fix, record a pre-get snapshot and a mutation set (track keys mutated via localStorage.setItem/removeItem or instrument the cache mutation points) between starting storageArea.get and its callback, then in the storageArea.get callback compare incoming snapshot keys to the mutation set and only call applySnapshot for keys that were not mutated (or merge by taking mutated values over snapshot); update code around storageBridgeTimeoutId/resolveStorageReady, the storageArea.get callback, and applySnapshot/cache logic to consult that mutation set so late hydration is skipped or merged safely.Source: Learnings
🧹 Nitpick comments (1)
tests/global-collision-regression.test.js (1)
21-23: 💤 Low valueConsider removing the unused helper.
The
_flushMicrotaskshelper is defined but never invoked in this test file. Removing it would reduce clutter and improve maintainability.🤖 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 `@tests/global-collision-regression.test.js` around lines 21 - 23, Remove the unused helper function _flushMicrotasks from the test file: delete the async function _flushMicrotasks() { await Promise.resolve(); } declaration (and remove any leftover references/imports if they exist) so the test file has no unused helper clutter.
🤖 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/ui/settings.js`:
- Around line 1375-1436: The unsubscribe handle unsubscribeVideoMotion is only
module-scoped so it is lost across full script re-evaluations and duplicate
onReducedMotionChange subscribers persist; instead persist the handle on window
(mirror how custom-backgrounds.js does) so teardown survives reinjection: in
initSettings, replace the module-local unsubscribeVideoMotion usage with a
window-scoped slot (e.g. window._unsubscribeVideoMotion) and use
window._unsubscribeVideoMotion =
window.onReducedMotionChange(syncVideoToMotionPreference) when registering and
call it to teardown before reassigning; ensure any existing
window._unsubscribeVideoMotion is invoked if present and remove the module-level
reliance while keeping syncVideoToMotionPreference and
window.onReducedMotionChange references.
- Around line 1010-1029: In the videoAutoplaySetting.checked branch, move the
reduced-motion check (window.prefersReducedMotion &&
window.prefersReducedMotion()) so it runs before any thumbnail hiding logic; if
reduced motion is active set videoEl.dataset.reducedMotionPaused = 'true' and
skip the thumbnail clearing/hidden class toggles and
setTimeout(crossfadeDelayMs(...)) so the static thumbnail remains visible,
otherwise delete videoEl.dataset.reducedMotionPaused and proceed with the
existing thumbnail clearing and videoEl.play() logic (respecting
window.loadSimpleMode). Refer to videoAutoplaySetting.checked,
videoEl.dataset.reducedMotionPaused, thumbnailEl, crossfadeDelayMs and
VIDEO_THUMBNAIL_HIDE_DELAY_MS to locate and reorder the code.
---
Duplicate comments:
In `@src/core/storage.js`:
- Around line 19-26: When the STORAGE bridge timeout fires you currently call
resolveStorageReady and later the chrome.storage.local.get callback blindly
calls applySnapshot which can overwrite writes made after resolveStorageReady;
to fix, record a pre-get snapshot and a mutation set (track keys mutated via
localStorage.setItem/removeItem or instrument the cache mutation points) between
starting storageArea.get and its callback, then in the storageArea.get callback
compare incoming snapshot keys to the mutation set and only call applySnapshot
for keys that were not mutated (or merge by taking mutated values over
snapshot); update code around storageBridgeTimeoutId/resolveStorageReady, the
storageArea.get callback, and applySnapshot/cache logic to consult that mutation
set so late hydration is skipped or merged safely.
---
Nitpick comments:
In `@tests/global-collision-regression.test.js`:
- Around line 21-23: Remove the unused helper function _flushMicrotasks from the
test file: delete the async function _flushMicrotasks() { await
Promise.resolve(); } declaration (and remove any leftover references/imports if
they exist) so the test file has no unused helper clutter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f15509e9-fca6-4019-ba3b-b4a1eb0921fa
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
AGENTS.mdCHANGELOG.mdcss/core.csscss/features.csseslint.config.jssrc/core/bootstrap.jssrc/core/main.jssrc/core/motion.jssrc/core/storage.jssrc/core/utils.jssrc/data/custom-backgrounds.jssrc/features/drag-drop.jssrc/features/notes.jssrc/features/todo.jssrc/ui/add-app-modal.jssrc/ui/settings.jstests/bootstrap-timeout.test.jstests/global-collision-regression.test.jstests/helpers/inject-script.jstests/prefers-reduced-motion.test.jstests/settings.test.js
2 participants
Summary
Fixes #314 — the New Tab page sometimes renders blank because the entire initialization chain depends on a single Promise (
__storageBridgeReady) that never resolves whenchrome.storage.local.get()fails to call back.Root Cause
If
chrome.storage.local.get()never calls its callback (extension reload, context invalidation, transient API failure),__storageBridgeReadynever resolves, bootstrap loads zero scripts, and the page stays blank.Fix
Two independent safety timeouts:
storage.js (3s timeout) — resolves
__storageBridgeReadyafter 3 seconds if chrome.storage hasn't responded, falling back to the native localStorage snapshot already captured earlier in initialization. Writes made through the bridge still reach chrome.storage when available.bootstrap.js (8s Promise.race) — races
__storageBridgeReadyagainst an 8-second timeout as a belt-and-suspenders catch-all.Both layers log a warning when activated so the fallback path is observable in DevTools.
Testing
Added
tests/bootstrap-timeout.test.js— a regression test that simulates a stalledchrome.storage.local.get()(callback never invoked) and verifies:__storageBridgeReadyresolves via the 3-second timeoutAll 396 existing tests continue to pass.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests