feat: respect prefers reduced motion

[LWWZH]

Summary by CodeRabbit

• Accessibility
  • Improved respect for OS "reduce motion" across the new-tab UI: animations, transitions and smooth scrolling are disabled; background videos pause; preference changes apply immediately.
• Stability
  • Added runtime fallbacks to prevent errors if motion handling fails to load.
• Tests
  • Expanded tests to cover reduced-motion behavior for video resume, animations, and preference change handling.

[404-Page-Found]

Findings

1. Bug: simpleModePaused not cleared when user switches back to normal motion

   • File: src/ui/settings.js (PR diff line ~1001-1006)
   • When reduced motion is detected, videoEl.dataset.simpleModePaused = 'true' is set. When the user later switches back to normal motion, the else if branch calls videoEl.play() — which works for the immediate event — but simpleModePaused is never cleared. The visibility-resume handler at settings.js:630 checks simpleModePaused !== 'true' and will block video resume on subsequent tab hide/show cycles.
   • Fix: Add delete videoEl.dataset.simpleModePaused; (or set to 'false') in the non-reduced-motion branch.

2. Medium: Duplicated isReducedMotion() / crossfadeDelayMs() wrappers

   • Files: src/data/custom-backgrounds.js:14-21, src/ui/settings.js:85-87
   • Both files define local isReducedMotion() wrapping window.prefersReducedMotion(), and custom-backgrounds.js defines a local crossfadeDelayMs() that shadows the global from src/core/motion.js. This duplication means the two modules can drift. They should both use the window.crossfadeDelayMs / window.prefersReducedMotion helpers directly.

3. Medium: Misuse of simpleModePaused flag for reduced motion

   • Files: src/ui/settings.js:521, src/data/custom-backgrounds.js:495
   • The PR reuses videoEl.dataset.simpleModePaused = 'true' to mean "video paused because of reduced motion." This flag was designed for the simple-mode feature and is checked in the visibility handler and in simple-mode.js. Reusing it for a different purpose creates semantic confusion. A dedicated reducedMotionPaused data attribute would be cleaner.

4. Low: showDateUpdateFeedback reduced-motion path provides no visual feedback

   • File: src/features/todo.js (PR diff ~line 1083-1094)
   • The reduced-motion branch sets backgroundColor then clears it in the same requestAnimationFrame callback, so the highlight is visible for essentially zero frames. Consider either showing a brief non-animated highlight (e.g., clear after ~50ms) or removing the highlight entirely and relying on the toast.

5. Low: custom-backgrounds.js doesn't handle dynamic reduced-motion changes

   • File: src/data/custom-backgrounds.js
   • settings.js's change event listener was updated to handle the user toggling prefers-reduced-motion while a background video is active. The equivalent code path in custom-backgrounds.js was not. Minor gap for an uncommon edge case.

6. Cosmetic: window.showDateUpdateFeedback exported solely for tests

   • File: src/features/todo.js:1812 — this function isn't called outside the module. The export exists only for testability. Follows the existing pattern, so non-blocking.

Positive notes: The motion.js helper is well-structured with proper error handling, test hooks, and a subscriber API. The CSS scroll-behavior: auto and .drag-drop-landed/.note-item defensive rules are thorough. The drag-drop.js fallback setTimeout(cleanup, 500) is a smart guard against class leaks when animation: none prevents animationend. Test coverage for the core helper and main.js paths looks solid.

Open Questions

• Should the simpleModePaused flag be cleared when switching back to normal motion, or should a new dedicated flag be introduced? The latter is cleaner but requires touching the visibility-resume handler in both settings.js and custom-backgrounds.js.
• Is the intent of the reduced-motion showDateUpdateFeedback to show zero visual highlight (toast only), or a brief non-animated highlight?

Summary

Request changes. The PR is well-structured and the motion helper is solid. However, the simpleModePaused bug (finding 1) will break video resume on tab visibility changes when the user has toggled their motion preference. The duplicated wrappers and semantic misuse of simpleModePaused (findings 2–3) are worth cleaning up to prevent drift between the two background modules. Non-blocking items (4–6) can be addressed in follow-up if preferred.

[404-Page-Found]

Findings

1. High: core.css defensive rules force opacity: 1 on .note-item — could break hidden/collapsed notes

   • File: css/core.css — the new .note-item { opacity: 1 !important } under @media (prefers-reduced-motion: reduce) will override any JS or CSS that relies on opacity to hide, collapse, or dim a note. If simple mode or any future feature hides notes via opacity, this rule will keep them visible. Consider a more targeted selector or removing the opacity override since the global * block already handles suppressed transitions ending at full opacity.

2. High: Two competing @media (prefers-reduced-motion: reduce) blocks in features.css vs core.css may diverge

   • Files: css/features.css:2018 and css/core.css:261
   • The same media query is split across two files with overlapping responsibilities. features.css sets animation-duration: 0.01ms / transition-duration: 0.01ms on *, while core.css sets animation: none / opacity: 1 on specific selectors. The features.css approach (near-zero duration) lets transitions "complete" instantly, while the core.css approach (animation: none) removes them entirely. These two strategies conflict for .app-icon.drag-drop-landed and .note-item: the features.css rule would create a 0.01ms transition, while core.css would suppress the animation entirely. The !important on both makes the result order-dependent. Consolidate into one block to avoid subtle cascade issues.

3. Medium: drag-drop.js setTimeout(cleanup, 500) is never cleared — leaks a timer per drop

   • File: src/features/drag-drop.js:453-460
   • After each drop, both animationend and setTimeout(cleanup, 500) are scheduled. The setTimeout handle is never stored or cleared. For heavy drag-and-drop sessions this accumulates pending timers. Consider storing the timeout handle and clearing it inside the animationend callback.

4. Medium: showDateUpdateFeedback reduced-motion path still flashes a highlight for 150ms

   • File: src/features/todo.js:1086-1090
   • The reduced-motion branch applies a backgroundColor change and clears it after 150ms via setTimeout. While this isn't "motion" per se, WCAG 2.3.3 cautions against flashes. Users who enabled reduced motion specifically to avoid sudden visual changes may find the 150ms color flash jarring. Consider making the highlight permanent until the user navigates away, or removing it entirely in reduced-motion mode.

5. Medium: syncVideoToMotionPreference and syncCustomVideoToMotionPreference both subscribe but discard the unsubscribe handle

   • Files: src/ui/settings.js:1408-1410 and src/data/custom-backgrounds.js:736-738
   • window.onReducedMotionChange(fn) returns an unsubscribe function, but both call sites discard it. If initSettings() were ever called twice (e.g., during testing or re-initialization), the handler would be registered twice, causing duplicate play/pause cycles. The risk is low in production (initSettings runs once), but it's a latent bug for test harnesses.

6. Medium: Test load order comment may be inaccurate

   • File: tests/prefers-reduced-motion.test.js:30-35
   • The comment says "Production order (New-Tab.html) loads custom-backgrounds.js BEFORE settings.js". Please verify this matches the actual <script> tag ordering in New-Tab.html.

7. Low: crossfadeDelayMs exposed on window but defined inside an IIFE — fragile global coupling

   • File: src/core/motion.js:83-85
   • Every consumer calls crossfadeDelayMs as a bare global. If motion.js ever fails to load (e.g., CSP blocking), all crossfade timers would throw a ReferenceError. Consider adding a guard or exporting a safe fallback.

8. Low: Test hooks on window in production

   • File: src/core/motion.js:120-138
   • _setReducedForTests, _setMqlForTests, _resetSubscribersForTests are accessible from the devtools console. Not a security concern for a browser extension, but worth noting.

Open Questions

• Is the core.css defensive block (.app-icon.drag-drop-landed, .note-item) meant to be permanent, or is it scaffolding for future CSS that might loosen the * selector?
• Should the reduced-motion path in showDateUpdateFeedback skip the highlight entirely, or is the 150ms color flash intentional?
• Are there plans to consolidate the two @media (prefers-reduced-motion: reduce) blocks into a single file?

Summary

The overall approach is solid: a centralized motion.js helper with a subscription API, CSS class toggling, and JS-side animation guards. The test suite is thorough. However, the two high-severity findings (the .note-item { opacity: 1 !important } override that could break note hiding, and the conflicting strategies in the two media-query blocks) should be resolved before merge. The medium issues (timer leaks, discarded unsubscribe handles, misleading comment) are worth addressing but won't cause user-facing bugs in the near term.

[LWWZH]

Thanks for the thorough re-review @404-Page-Found. Six follow-up commits address every finding from the second review pass:

HIGH (core.css + features.css)

• 6a19d9d fix(motion): drop redundant defensive CSS overrides
  • Removes the .app-icon.drag-drop-landed, .note-item { ... } block. Both animations use forwards fill mode and the global near-zero-duration override in features.css already collapses them to the final state, so the defensive rules were redundant. The .note-item { opacity: 1 !important } rule in particular would have shadowed any future opacity-based hiding of notes.
  • Adds comments to both reduced-motion media-query blocks documenting the division of responsibility (core.css handles entrance/glow animations that need explicit animation: none; opacity: 1; features.css handles the global * near-zero-duration override).

MEDIUM

• 7fa2b9d fix(drag-drop): clear setTimeout fallback when animationend fires. The 500ms cleanup timer handle is now tracked and clearTimeout runs inside the cleanup callback, so a heavy drag session no longer accumulates pending timers.
• 5265403 fix(todo): skip date-update highlight under reduced motion. The 150ms backgroundColor flash conflicts with the spirit of WCAG 2.3.3; the toast already communicates the update, so under reduced motion we drop the in-place highlight entirely. Test updated to assert no backgroundColor / transform / transition are applied.
• 892c04f fix(motion): capture unsubscribe handles for video motion subscribers. Both syncVideoToMotionPreference and syncCustomVideoToMotionPreference now capture the return value of onReducedMotionChange and detach the previous subscriber before re-subscribing, so a re-injected module (test harness, HMR) doesn't register a duplicate handler.
• 27f68cd test(motion): clarify load-order rationale. The previous comment was self-contradictory (it claimed the test loads custom-backgrounds.js after settings.js "so that _customBackgrounds is available before the settings.js test setup runs", which is the opposite of what loading-after means). Rewritten to state the actual intent and confirm the order is safe.

LOW

• 9f0999f fix(motion): install safety stubs before motion.js loads. Inline <script> in New-Tab.html defines no-op window.crossfadeDelayMs / prefersReducedMotion / onReducedMotionChange so call sites degrade gracefully if motion.js fails to load (CSP block, network error).

Verified

• npm run lint clean (no new warnings, one pre-existing eslint-disable removed by lint:fix).
• npm test — 411/411 pass.

Re-requesting review. The only items from the original review pass that remain unaddressed are the two cosmetic ones you marked non-blocking: window.showDateUpdateFeedback exported solely for tests (#6) and the _setReducedForTests family of test hooks on window (Review 2 #8).

[404-Page-Found]

Findings

1. Low: Unrelated drag-drop refactor bundled into an a11y PR

   • File: src/features/drag-drop.js:450-477
   • The PR replaces the animationend-only cleanup with a dual animationend/setTimeout + mutual-cancellation pattern. This is a solid robustness improvement (prevents stale timers during heavy drag sessions), but it's unrelated to prefers-reduced-motion. Under reduced motion, the CSS global override (animation-duration: 0.01ms) still fires animationend, and .app-icon.drag-drop-landed is explicitly not overridden in core.css (the comments explain why). This should ideally be a separate commit or PR so the change is easy to revert independently.

2. Low: Minor scope expansion — window.showDateUpdateFeedback export

   • File: src/features/todo.js:1810
   • The PR exposes showDateUpdateFeedback on window solely for test access. This function was previously internal-only. The export is harmless, but the convention in this codebase is to avoid leaking helpers to window unless they're part of the public API. The test harness's injectScript already makes all IIFE-level var/function declarations visible within the eval scope — verify whether showDateUpdateFeedback actually needs the window. assignment for the test to reach it, or if it's accessible via the existing scope.

3. Low: eslint-disable-next-line no-useless-assignment in custom-backgrounds.js

   • File: src/data/custom-backgrounds.js (motion subscriber registration)
   • The unsubscribeCustomVideoMotion = window.onReducedMotionChange(...) line is suppressed with no-useless-assignment. The explanation is correct (re-injection teardown), but it would read better as a dedicated comment above the suppression explaining why the captured return value serves a future teardown purpose rather than an inline disable.

4. Nit: crossfadeDelayMs wrapping on already-bypassed timeouts

   • Files: src/data/custom-backgrounds.js:507, src/ui/settings.js:412,558,684
   • In triggerCrossfade, when reduced motion is on, the early return prevents the thumbnail-hide timeout from ever being created. The crossfadeDelayMs() wrapper around those timeouts is therefore unreachable in that code path. It acts as a safety net if the preference toggles mid-flight, but the motion subscriber handles the live toggle independently. The wrappers are harmless but add cognitive overhead — a brief comment noting "belt-and-suspenders" would help.

Open Questions

• Is the drag-drop.js cleanup refactor intentionally part of this PR (e.g. to guarantee cleanup when animationend is suppressed), or should it be split out?

Summary

The PR is well-structured and thorough. The motion.js helper is cleanly designed with a subscription API, test hooks, and a reduce-motion class on <html>. The JS-driven animation paths (motto fade, todo FLIP, date highlight, background video crossfade) all correctly check prefersReducedMotion() and resolve instantly when the preference is active. The background video pause/resume is correctly partitioned between settings.js (built-in videos) and custom-backgrounds.js (custom videos) via mutual isCustom guards. CSS changes are well-commented explaining the backwards vs forwards fill-mode distinction. Test coverage is strong (403 lines covering the helper, each animation path, and a regression test for the flag-clearing bug). No correctness bugs found. Recommend approval with minor cleanup — consider splitting the drag-drop refactor into a separate commit and verifying the window.showDateUpdateFeedback export is necessary.

[coderabbitai]

PR changed again? Review this PR in Change Stack to compare snapshots and stay oriented.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3c7a94f6-f1a3-48ab-b7c6-cf08928c64bb

📥 Commits

Reviewing files that changed from the base of the PR and between 45b3da4 and a17c29b.

📒 Files selected for processing (3)

• CHANGELOG.md
• src/ui/settings.js
• tests/prefers-reduced-motion.test.js

✅ Files skipped from review due to trivial changes (1)

• CHANGELOG.md

---

📝 Walkthrough

Walkthrough

Adds pervasive prefers-reduced-motion support: new motion.js exposes prefersReducedMotion/onReducedMotionChange/crossfadeDelayMs and initializes root state; CSS and JS animation paths (motto, todos, drag-drop) and background video playback now respect and synchronize with the OS reduced-motion preference.

Changes

Accessibility: Reduced-Motion Support

Layer / File(s)	Summary
Motion detection module and fallback stubs src/core/motion.js, New-Tab.html	Centralizes prefers-reduced-motion detection with cached state, root-class toggling, a subscription API (onReducedMotionChange), and crossfadeDelayMs(); New-Tab.html inlines safe fallback stubs to avoid ReferenceErrors.
CSS and configuration updates css/core.css, css/features.css, eslint.config.js, CHANGELOG.md	Expands reduced-motion docs, enforces scroll-behavior: auto !important under reduced motion, adds crossfadeDelayMs to ESLint globals, and records the change in CHANGELOG.
Core UI motto animations respect reduced-motion src/core/main.js	Motto display and refresh animations check prefersReducedMotion() to skip transitions and set opacity immediately when reduced motion is enabled.
Background video motion-aware playback src/data/custom-backgrounds.js, src/ui/settings.js	Video crossfades, autoplay toggles, visibility/resume, and thumbnail timing now use crossfadeDelayMs() and dataset.reducedMotionPaused to pause/resume and avoid autoplay when reduced motion is active; modules subscribe to motion changes to synchronize active videos.
Feature animations respect reduced-motion src/features/todo.js, src/features/drag-drop.js	FLIP reordering, date-update highlight, and drag-drop landing cleanup are gated by reduced-motion; landing cleanup adds a timeout fallback and test exports adjusted.
Comprehensive reduced-motion test suite tests/prefers-reduced-motion.test.js, tests/settings.test.js	Tests expanded to cover motion helper state and subscribers, motto/todo behaviors under reduced motion, background autoplay/resume, custom-backgrounds sync, visibility-resume, and settings test setup now injects motion.js.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• Feature: Respect prefers-reduced-motion across the new tab UI #196 — This PR implements a centralized prefers-reduced-motion solution (motion.js and consumer updates), directly addressing the feature requested in that issue.

Poem

🐰 I sniff the code for motion that's kind,
When screens should be gentle, I hop and remind.
Videos pause, transitions take breath,
A little helper keeps flashes from death.
Hooray for calm UI — hop, skip, and unwind!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.83% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'feat: respect prefers reduced motion' accurately summarizes the main objective of the changeset, which implements comprehensive support for the prefers-reduced-motion accessibility preference across CSS, JavaScript, and video handling throughout the codebase.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[LWWZH]

Thanks for the re-review @404-Page-Found. Per your "approval with minor cleanup" recommendation, this pass addresses all four low/nit findings. The history was rebased to keep the a11y PR focused and split the drag-drop refactor out as its own commit.

LOW

• 3707ed1 fix(drag-drop): clean up bounce-landing animation. Split out of the a11y PR per finding 1 so the change is easy to revert independently. Combines the setTimeout fallback and the timer-handle cleanup that were previously in 556035e and 7fa2b9d (both have been removed in the rebase).
• 56f1cba chore(todo): document test-only window exports. Per finding 2, the window.* re-exports (showDateUpdateFeedback, handleDragStart/End/Over/Drop) ARE required by the test harness: tests/helpers/inject-script.js loads todo.js via globalThis.eval(code), and an eval'd function declaration is scoped to the eval scope rather than the global. The window.* assignment is the only path the test can reach these helpers through. Added a comment block above the test-only exports explaining this so future readers know the leak is intentional and a test affordance.

LOW

• 45b3da4 chore(motion): clarify reduced-motion wrapper rationale. Per finding 3, expanded the comment above the eslint-disable-next-line no-useless-assignment on the unsubscribeCustomVideoMotion capture so the suppression reads as a documented design choice. Per finding 4, added a brief "belt-and-suspenders" note at all four crossfadeDelayMs() call sites (src/data/custom-backgrounds.js and three in src/ui/settings.js) so future readers know why the wrapper is kept when the reduced-motion early return already prevents the timer from being created.

Verified

• npm run lint clean (no new warnings; same 8 pre-existing warnings in unrelated files).
• npm test — 411/411 pass.
• Working tree state matches the previous head byte-for-byte for code files; the only delta is the three new comment improvements.

Re-requesting review.

[coderabbitai]

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 `@CHANGELOG.md`:
- Around line 16-18: Update the changelog entry so it correctly reflects the
final CSS behavior: replace the sentence claiming targeted defensive rules were
added for `.app-icon.drag-drop-landed` and `.note-item` with wording that states
those defensive overrides were removed (or never present in the final build) and
explain that the implementation now relies on the consolidated/global styles
instead; ensure the text mentions the two class selectors
`.app-icon.drag-drop-landed` and `.note-item` so readers can map the note to the
code.

In `@src/ui/settings.js`:
- Around line 1379-1390: syncVideoToMotionPreference sets
videoEl.dataset.reducedMotionPaused = 'true' when reduced motion is enabled but
the visibility-resume logic later (the tab-visible subscriber that replays based
on data-wasPlaying) does not check this flag; update that visibility-resume
handler to skip restarting the video if videoEl.dataset.reducedMotionPaused ===
'true' (and continue to respect custom backgrounds via
window._customBackgrounds.isCustom and loadVideoAutoplay() checks). In short: in
the visibility resume path that calls videoEl.play() when data-wasPlaying ===
'true', add a guard that returns early when data-reducedMotionPaused === 'true'
so reduced-motion preference remains honored across background/foreground
transitions.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4868977b-8fda-4526-a643-daf8df240766

📥 Commits

Reviewing files that changed from the base of the PR and between 73c85b2 and 45b3da4.

📒 Files selected for processing (13)

• CHANGELOG.md
• New-Tab.html
• css/core.css
• css/features.css
• eslint.config.js
• src/core/main.js
• src/core/motion.js
• src/data/custom-backgrounds.js
• src/features/drag-drop.js
• src/features/todo.js
• src/ui/settings.js
• tests/prefers-reduced-motion.test.js
• tests/settings.test.js

[LWWZH]

Addressed both CodeRabbit findings from the latest review (�17c29b):

CHANGELOG.md (lines 16–18)

• Replaced the inaccurate claim that defensive CSS overrides were added for .app-icon.drag-drop-landed and .note-item. The new wording explains the division of responsibility: eatures.css handles orwards/�oth fill-mode animations via the global near-zero-duration override, and core.css handles entrance/glow animations via �nimation: none; opacity: 1. No redundant overrides were introduced.

src/ui/settings.js visibility-resume path

• initVideoVisibilityHandler now checks simpleModePaused !== 'true' and
  educedMotionPaused !== 'true' before resuming the video on tab-visible. This closes the bug where a user who enables reduced motion while the tab is hidden would still get video playback when returning to the tab.

Tests

• 3 new regression tests in prefers-reduced-motion.test.js covering the visibility-resume guard for
  educedMotionPaused, simpleModePaused, and the normal resume path. All 414 tests pass, lint clean.

[404-Page-Found]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.