[codex] animal timer growth and docs cleanup - #59
Conversation
📝 WalkthroughWalkthroughPigDetail component now accepts an optional ChangesPigDetail Timer Status & Animal Sizing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 9
🤖 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 `@CONTEXT.md`:
- Line 157: The ordered list item starting with "In-session agent via
`/checkpoint` — HTTP POST `/proposals`, then user accepts/rejects." uses the
prefix "2." but the repo's markdownlint config expects all ordered-list prefixes
to use "1."; update that list item prefix from "2." to "1." so the entry matches
the surrounding ordered-list style and satisfies markdownlint.
In `@issues/030-pig-growth-expired-visual-tray-list.md`:
- Line 19: The comment referencing pig-specific terminology should be made
animal-neutral: update the line that reads "Returns 1.0 if no timer. Otherwise
pig_scale(elapsed, duration) clamped 1.0–3.0." to use the neutral function/term
used in the code (e.g., "animal_scale(elapsed, duration)" or simply
"scale(elapsed, duration)") so it matches the useRanchAnimalScale naming; keep
the semantics about returning 1.0 if no timer and clamping the result to
1.0–3.0.
In `@README.md`:
- Around line 51-60: The fenced code block in README.md for the storage layout
is missing a language hint; update the opening triple-backtick for that block to
include a language token (e.g., change ``` to ```text) so the block becomes
fenced as ```text, preserving the existing block contents; locate the block
showing "~/.adhd-ranch/" and update its opening fence accordingly.
In `@src/components/App.tsx`:
- Around line 46-55: The tray-event subscription logic in App's useEffect should
be moved into a dedicated hook (e.g., useOpenFocusSubscription) so App remains
view-only; create a hook in src/hooks that accepts the focuses array and a
callback (or setter) for selection, calls subscribeOpenFocusDetail(focusId)
inside an effect, handles the promise/unsubscribe correctly (catching errors and
returning the unsubscribe function), and ensures cleanup by invoking the
resolved unsubscribe on unmount; then replace the useEffect in App with a call
to this new hook (passing focuses and setSelectedId) and remove any direct I/O
from App.
In `@src/components/PigDetail.tsx`:
- Line 115: The timer status shown by describeTimerStatus(focus.timer ?? null)
in the PigDetail component will not update because describeTimerStatus uses
Date.now() but the component doesn't re-render; add a useEffect in PigDetail
that, when focus.timer is present and running, sets up a setInterval to call a
state setter (e.g., a local tick state or forceUpdate) every 1000ms to trigger
re-renders, and ensure you clearInterval in the cleanup to avoid leaks; update
the effect dependencies to watch focus.timer (or its running state) so the
interval starts/stops correctly—apply the same pattern for the other occurrence
noted around describeTimerStatus usage (lines ~244-254).
In `@src/components/PigSprite.tsx`:
- Line 46: The bob offset is currently read from BOB_OFFSETS and applied as a
raw pixel value (const bob = BOB_OFFSETS[frame % BOB_OFFSETS.length]) which
ignores the sprite's scale; change the calculation to multiply the selected
offset by the sprite's scale (use the existing scale variable) so the bob =
BOB_OFFSETS[frame % BOB_OFFSETS.length] * scale and subsequent code that uses
bob will produce a visually consistent hop at different sizes.
In `@src/hooks/useRanchAnimalScale.test.ts`:
- Around line 4-17: Add a test to verify the non-positive duration edge case for
useRanchAnimalScale: when durationSecs is 0 or negative the hook currently
returns 3, so add an it block in the existing describe("useRanchAnimalScale")
that calls useRanchAnimalScale(1_000, 0, someNow) and useRanchAnimalScale(1_000,
-10, someNow) (or separate expectations) and asserts they each return 3; place
the new it next to the other behavior tests to document this edge case.
In `@src/hooks/useRanchAnimalScale.ts`:
- Line 7: The early-return in useRanchAnimalScale that currently returns 3 for
non-positive durations (the line checking durationSecs <= 0) is unclear and
should be changed to return 1 (default scale) for invalid/edge-case durations;
update the conditional in useRanchAnimalScale to return 1 instead of 3, add a
short inline comment explaining that non-positive durations default to normal
scale, and add/update a unit test for useRanchAnimalScale to cover durationSecs
<= 0 behavior.
- Line 13: The export uses a `use` prefix for a pure function; rename the export
to remove the hook-style prefix by exporting ranchAnimalScale directly instead
of useRanchAnimalScale (or add a non-hook alias `ranchAnimalScale` that
references the existing ranchAnimalScale function for backward compatibility),
and update any imports that relied on useRanchAnimalScale to use the new name;
locate the symbol useRanchAnimalScale in the file and change the exported
identifier to ranchAnimalScale (or export both names with the non-hook name as
primary).
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 71f3111a-3a11-4529-8c26-7ec22c4cba39
📒 Files selected for processing (29)
CONTEXT.mdPRD.mdREADME.mdissues/030-pig-growth-expired-visual-tray-list.mdissues/031-notification-settings-tray.mdissues/031-notification-source-settings.mdissues/README.mdissues/done/029-timer-expiry-and-notification-interface.mdissues/done/037-collapse-focus-reader.mdissues/done/038-collapse-proposal-reader.mdissues/done/039-unify-caps-on-polled-reader.mdissues/done/040-generic-tauri-reader-factory.mdissues/done/041-focus-writer-write-outcome.mdissues/done/043-proposal-lifecycle-atomicity-design.mdissues/done/044-domain-timer-ticker.mdissues/done/048-focus-document-mutation-module.mdissues/done/049-display-space-pig-movement-seam.mdsrc-tauri/src/app/tray.rssrc/api/pig.tssrc/components/App.test.tsxsrc/components/App.tsxsrc/components/PigDetail.test.tsxsrc/components/PigDetail.tsxsrc/components/PigSprite.tsxsrc/hooks/usePigMovement.test.tssrc/hooks/usePigMovement.tssrc/hooks/useRanchAnimalScale.test.tssrc/hooks/useRanchAnimalScale.tssrc/styles.css
💤 Files with no reviewable changes (1)
- issues/031-notification-settings-tray.md
|
|
||
| Deferred v1.3 writer: | ||
|
|
||
| 2. **In-session agent via `/checkpoint`** — HTTP POST `/proposals`, then user accepts/rejects. |
There was a problem hiding this comment.
Fix ordered-list prefix to satisfy markdownlint.
Line 157 uses 2. where the configured ordered-list style expects 1. prefixes.
Suggested patch
-2. **In-session agent via `/checkpoint`** — HTTP POST `/proposals`, then user accepts/rejects.
+1. **In-session agent via `/checkpoint`** — HTTP POST `/proposals`, then user accepts/rejects.📝 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.
| 2. **In-session agent via `/checkpoint`** — HTTP POST `/proposals`, then user accepts/rejects. | |
| 1. **In-session agent via `/checkpoint`** — HTTP POST `/proposals`, then user accepts/rejects. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 157-157: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
🤖 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 `@CONTEXT.md` at line 157, The ordered list item starting with "In-session
agent via `/checkpoint` — HTTP POST `/proposals`, then user accepts/rejects."
uses the prefix "2." but the repo's markdownlint config expects all ordered-list
prefixes to use "1."; update that list item prefix from "2." to "1." so the
entry matches the surrounding ordered-list style and satisfies markdownlint.
| ```ts | ||
| export function usePigScale(startedAt: number | null, durationSecs: number | null): number | ||
| export function useRanchAnimalScale(startedAt: number | null, durationSecs: number | null): number | ||
| // Returns 1.0 if no timer. Otherwise pig_scale(elapsed, duration) clamped 1.0–3.0. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Minor terminology inconsistency in code comment.
The function signature correctly uses the animal-neutral name useRanchAnimalScale, but the comment still references pig_scale(elapsed, duration). For consistency with the animal-neutral naming guidance added in lines 9-11, consider updating the comment to use a neutral term like animal_scale or simply scale.
📝 Suggested update for consistency
export function useRanchAnimalScale(startedAt: number | null, durationSecs: number | null): number
- // Returns 1.0 if no timer. Otherwise pig_scale(elapsed, duration) clamped 1.0–3.0.
+ // Returns 1.0 if no timer. Otherwise scale(elapsed, duration) clamped 1.0–3.0.📝 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.
| // Returns 1.0 if no timer. Otherwise pig_scale(elapsed, duration) clamped 1.0–3.0. | |
| // Returns 1.0 if no timer. Otherwise scale(elapsed, duration) clamped 1.0–3.0. |
🤖 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 `@issues/030-pig-growth-expired-visual-tray-list.md` at line 19, The comment
referencing pig-specific terminology should be made animal-neutral: update the
line that reads "Returns 1.0 if no timer. Otherwise pig_scale(elapsed, duration)
clamped 1.0–3.0." to use the neutral function/term used in the code (e.g.,
"animal_scale(elapsed, duration)" or simply "scale(elapsed, duration)") so it
matches the useRanchAnimalScale naming; keep the semantics about returning 1.0
if no timer and clamping the result to 1.0–3.0.
| ``` | ||
| ~/.adhd-ranch/ | ||
| focuses/ | ||
| <slug>/focus.md YAML frontmatter + - [ ] bullets | ||
| <slug>/timer.json optional countdown timer sidecar | ||
| proposals.jsonl pending proposals, one per line | ||
| decisions.jsonl audit log of accept/reject (with edited flag) | ||
| settings.yaml optional caps + alert config | ||
| settings.yaml optional caps + notification/widget/display config | ||
| run/port ephemeral HTTP port | ||
| ``` |
There was a problem hiding this comment.
Add a language hint to the fenced code block.
The storage layout fence is missing a language token (MD040).
Suggested patch
-```
+```text
~/.adhd-ranch/
focuses/
<slug>/focus.md YAML frontmatter + - [ ] bullets
<slug>/timer.json optional countdown timer sidecar
proposals.jsonl pending proposals, one per line
decisions.jsonl audit log of accept/reject (with edited flag)
settings.yaml optional caps + notification/widget/display config
run/port ephemeral HTTP port</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@README.md` around lines 51 - 60, The fenced code block in README.md for the
storage layout is missing a language hint; update the opening triple-backtick
for that block to include a language token (e.g., change ``` to ```text) so the
block becomes fenced as ```text, preserving the existing block contents; locate
the block showing "~/.adhd-ranch/" and update its opening fence accordingly.
| useEffect(() => { | ||
| const unsubscribe = subscribeOpenFocusDetail((focusId) => { | ||
| if (focuses.some((focus) => focus.id === focusId)) { | ||
| setSelectedId(focusId); | ||
| } | ||
| }).catch(() => () => {}); | ||
| return () => { | ||
| unsubscribe.then((fn) => fn()); | ||
| }; | ||
| }, [focuses]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move tray-event subscription out of App and into a hook.
This effect performs API/event I/O in a component; extract it to a src/hooks/ hook and keep App view-focused.
As per coding guidelines, "Frontend React components in src/components/ should be view-only with no fetch and no direct I/O." and "Frontend hooks in src/hooks/ should handle state + effects and call api/ clients."
🤖 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/components/App.tsx` around lines 46 - 55, The tray-event subscription
logic in App's useEffect should be moved into a dedicated hook (e.g.,
useOpenFocusSubscription) so App remains view-only; create a hook in src/hooks
that accepts the focuses array and a callback (or setter) for selection, calls
subscribeOpenFocusDetail(focusId) inside an effect, handles the
promise/unsubscribe correctly (catching errors and returning the unsubscribe
function), and ensures cleanup by invoking the resolved unsubscribe on unmount;
then replace the useEffect in App with a call to this new hook (passing focuses
and setSelectedId) and remove any direct I/O from App.
| onClose(); | ||
| } | ||
|
|
||
| const timerStatus = describeTimerStatus(focus.timer ?? null); |
There was a problem hiding this comment.
Timer status display won't update in real-time.
describeTimerStatus is called during render and uses Date.now(), but the component won't re-render automatically as time passes. The timer countdown will appear frozen until another state change triggers a re-render (e.g., user interaction). Users typically expect timer displays to tick down every second.
Consider adding a useEffect hook that calls setInterval to force a re-render every second when a running timer is present.
⏱️ Proposed fix to add live timer updates
+ const [, setTick] = useState(0);
+
+ useEffect(() => {
+ if (!focus.timer || focus.timer.status === "Expired") return;
+ const interval = setInterval(() => setTick((t) => t + 1), 1000);
+ return () => clearInterval(interval);
+ }, [focus.timer]);
+
const timerStatus = describeTimerStatus(focus.timer ?? null);Also applies to: 244-254
🤖 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/components/PigDetail.tsx` at line 115, The timer status shown by
describeTimerStatus(focus.timer ?? null) in the PigDetail component will not
update because describeTimerStatus uses Date.now() but the component doesn't
re-render; add a useEffect in PigDetail that, when focus.timer is present and
running, sets up a setInterval to call a state setter (e.g., a local tick state
or forceUpdate) every 1000ms to trigger re-renders, and ensure you clearInterval
in the cleanup to avoid leaks; update the effect dependencies to watch
focus.timer (or its running state) so the interval starts/stops correctly—apply
the same pattern for the other occurrence noted around describeTimerStatus usage
(lines ~244-254).
| @@ -42,15 +46,16 @@ export function PigSprite({ | |||
| const bob = BOB_OFFSETS[frame % BOB_OFFSETS.length]; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider scaling the bob offset.
The bob offset (from BOB_OFFSETS) is applied as a constant pixel value regardless of scale. When the sprite is 3× larger, the -2px vertical hop becomes visually smaller relative to the sprite size, potentially making the walk animation look less pronounced.
If a consistent visual bob is desired, multiply the offset by scale:
- const bob = BOB_OFFSETS[frame % BOB_OFFSETS.length];
+ const bob = BOB_OFFSETS[frame % BOB_OFFSETS.length] * scale;🤖 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/components/PigSprite.tsx` at line 46, The bob offset is currently read
from BOB_OFFSETS and applied as a raw pixel value (const bob = BOB_OFFSETS[frame
% BOB_OFFSETS.length]) which ignores the sprite's scale; change the calculation
to multiply the selected offset by the sprite's scale (use the existing scale
variable) so the bob = BOB_OFFSETS[frame % BOB_OFFSETS.length] * scale and
subsequent code that uses bob will produce a visually consistent hop at
different sizes.
| describe("useRanchAnimalScale", () => { | ||
| it("returns 1 for focuses without timers", () => { | ||
| expect(useRanchAnimalScale(null, null)).toBe(1); | ||
| }); | ||
|
|
||
| it("grows linearly from 1 to 3 over the timer duration", () => { | ||
| expect(useRanchAnimalScale(1_000, 120, 1_060_000)).toBe(2); | ||
| }); | ||
|
|
||
| it("clamps to 3 at or after the timer duration", () => { | ||
| expect(useRanchAnimalScale(1_000, 120, 1_120_000)).toBe(3); | ||
| expect(useRanchAnimalScale(1_000, 120, 2_000_000)).toBe(3); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test coverage for the non-positive duration edge case.
The implementation returns 3 when durationSecs <= 0, but this behavior is not tested. Add a test case to verify and document this edge case.
🧪 Suggested test case
it("clamps to 3 at or after the timer duration", () => {
expect(useRanchAnimalScale(1_000, 120, 1_120_000)).toBe(3);
expect(useRanchAnimalScale(1_000, 120, 2_000_000)).toBe(3);
});
+
+ it("returns 3 for non-positive duration", () => {
+ expect(useRanchAnimalScale(1_000, 0, 1_060_000)).toBe(3);
+ expect(useRanchAnimalScale(1_000, -10, 1_060_000)).toBe(3);
+ });
});🤖 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/useRanchAnimalScale.test.ts` around lines 4 - 17, Add a test to
verify the non-positive duration edge case for useRanchAnimalScale: when
durationSecs is 0 or negative the hook currently returns 3, so add an it block
in the existing describe("useRanchAnimalScale") that calls
useRanchAnimalScale(1_000, 0, someNow) and useRanchAnimalScale(1_000, -10,
someNow) (or separate expectations) and asserts they each return 3; place the
new it next to the other behavior tests to document this edge case.
| nowMs = Date.now(), | ||
| ): number { | ||
| if (startedAt === null || durationSecs === null) return 1; | ||
| if (durationSecs <= 0) return 3; |
There was a problem hiding this comment.
Clarify the non-positive duration edge case.
Returning 3 (max scale) when durationSecs <= 0 seems arbitrary. This edge case is untested and could cause unexpected visual results. Consider returning 1 (default scale) for invalid durations, or document why max scale is the intended fallback.
🤖 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/useRanchAnimalScale.ts` at line 7, The early-return in
useRanchAnimalScale that currently returns 3 for non-positive durations (the
line checking durationSecs <= 0) is unclear and should be changed to return 1
(default scale) for invalid/edge-case durations; update the conditional in
useRanchAnimalScale to return 1 instead of 3, add a short inline comment
explaining that non-positive durations default to normal scale, and add/update a
unit test for useRanchAnimalScale to cover durationSecs <= 0 behavior.
| return 1 + progress * 2; | ||
| } | ||
|
|
||
| export const useRanchAnimalScale = ranchAnimalScale; |
There was a problem hiding this comment.
Remove use prefix from pure function export.
useRanchAnimalScale is not a React hook—it doesn't call any hook APIs (useState, useEffect, etc.). The use prefix is a React community convention reserved exclusively for hooks. Exporting a pure function with this prefix violates the convention and may confuse linters (e.g., react-hooks/rules-of-hooks) or developers expecting hook behavior.
📛 Proposed fix
-export const useRanchAnimalScale = ranchAnimalScale;
+export { ranchAnimalScale as ranchAnimalScale };Or if an alias is required for backward compatibility, use a non-hook name:
-export const useRanchAnimalScale = ranchAnimalScale;
+export const computeRanchAnimalScale = ranchAnimalScale;As per coding guidelines, frontend hooks in src/hooks/ should handle state + effects. This is a pure function and should not carry the use prefix.
📝 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.
| export const useRanchAnimalScale = ranchAnimalScale; | |
| export { ranchAnimalScale as ranchAnimalScale }; |
🤖 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/useRanchAnimalScale.ts` at line 13, The export uses a `use` prefix
for a pure function; rename the export to remove the hook-style prefix by
exporting ranchAnimalScale directly instead of useRanchAnimalScale (or add a
non-hook alias `ranchAnimalScale` that references the existing ranchAnimalScale
function for backward compatibility), and update any imports that relied on
useRanchAnimalScale to use the new name; locate the symbol useRanchAnimalScale
in the file and change the exported identifier to ranchAnimalScale (or export
both names with the non-hook name as primary).
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/components/PigDetail.test.tsx`:
- Around line 182-192: The Date.now spy in the "shows remaining time for a
running timer" test (nowSpy created via vi.spyOn(Date, "now")) may not be
restored if the assertion fails; update the test that calls renderDetail(...)
and expects "01:30 remaining" to guarantee cleanup by either wrapping the
assertion in a try/finally and calling nowSpy.mockRestore() in the finally
block, or add a suite-level afterEach(() => vi.restoreAllMocks()) so all spies
(including Date.now) are always restored between tests.
In `@src/styles.css`:
- Around line 425-443: The expired animation on .pig-sprite--expired
.pig-sprite-frame uses expired-animal-pulse unconditionally; add a
prefers-reduced-motion media query that removes or overrides the animation
(e.g., set animation: none and optionally keep a static visual cue like
transform: none or a subtle non-animating style) so users with reduced-motion:
reduce do not see the pulse; update or add rules targeting .pig-sprite--expired
.pig-sprite-frame within `@media` (prefers-reduced-motion: reduce) and ensure the
`@keyframes` expired-animal-pulse remains for non-reduced users.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 90f14f4e-c457-4e7a-9bc9-d29699f4d528
📒 Files selected for processing (5)
src/components/PigDetail.test.tsxsrc/components/PigDetail.tsxsrc/hooks/usePigMovement.test.tssrc/hooks/usePigMovement.tssrc/styles.css
| it("shows remaining time for a running timer", () => { | ||
| const nowSpy = vi.spyOn(Date, "now").mockReturnValue(1_030_000); | ||
| renderDetail({ | ||
| focus: { | ||
| ...baseFocus, | ||
| timer: { duration_secs: 120, started_at: 1_000, status: "Running" }, | ||
| }, | ||
| }); | ||
| expect(screen.getByText("01:30 remaining")).toBeInTheDocument(); | ||
| nowSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "PigDetail.test.tsx" -type f 2>/dev/null | head -5Repository: killallgit/adhd-ranch
Length of output: 47
🏁 Script executed:
git ls-files | grep -i pigdetailRepository: killallgit/adhd-ranch
Length of output: 166
🏁 Script executed:
fd "PigDetail.test" -t fRepository: killallgit/adhd-ranch
Length of output: 99
🏁 Script executed:
rg "PigDetail.test" --type tsxRepository: killallgit/adhd-ranch
Length of output: 93
🏁 Script executed:
fd "PigDetail" -t fRepository: killallgit/adhd-ranch
Length of output: 128
🏁 Script executed:
git ls-files src/components/ | head -20Repository: killallgit/adhd-ranch
Length of output: 705
🏁 Script executed:
wc -l src/components/PigDetail.test.tsxRepository: killallgit/adhd-ranch
Length of output: 103
🏁 Script executed:
cat -n src/components/PigDetail.test.tsx | head -50Repository: killallgit/adhd-ranch
Length of output: 1816
🏁 Script executed:
cat -n src/components/PigDetail.test.tsx | sed -n '160,240p'Repository: killallgit/adhd-ranch
Length of output: 4048
🏁 Script executed:
cat -n src/components/PigDetail.test.tsx | grep -A 3 "afterEach\|beforeEach"Repository: killallgit/adhd-ranch
Length of output: 47
🏁 Script executed:
cat -n src/components/PigDetail.test.tsxRepository: killallgit/adhd-ranch
Length of output: 12372
🏁 Script executed:
cat -n src/components/PigDetail.test.tsx | sed -n '1,50p' | grep -E "afterEach|beforeEach|afterAll|beforeAll"Repository: killallgit/adhd-ranch
Length of output: 47
🏁 Script executed:
grep -n "afterEach\|beforeEach\|vi.restoreAllMocks\|clearAllMocks" src/components/PigDetail.test.tsxRepository: killallgit/adhd-ranch
Length of output: 47
🏁 Script executed:
head -20 src/components/PigDetail.test.tsx | tail -15Repository: killallgit/adhd-ranch
Length of output: 378
🏁 Script executed:
fd "\.test\.tsx$" src/components/ | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && grep -n "afterEach\|vi.restoreAllMocks" {}'Repository: killallgit/adhd-ranch
Length of output: 194
🏁 Script executed:
rg "afterEach.*vi\.(restoreAllMocks|clearAllMocks)" src/components/Repository: killallgit/adhd-ranch
Length of output: 47
🏁 Script executed:
find . -name "vitest.config.*" -o -name "vite.config.*" | head -5Repository: killallgit/adhd-ranch
Length of output: 82
🏁 Script executed:
rg "vi\.spyOn\(Date" src/components/ -A 5Repository: killallgit/adhd-ranch
Length of output: 828
🏁 Script executed:
rg "mockRestore\(\)" src/components/ -B 5 -A 1Repository: killallgit/adhd-ranch
Length of output: 945
🏁 Script executed:
git ls-files | grep -E "vitest|vite\.config|setup" | head -10Repository: killallgit/adhd-ranch
Length of output: 96
🏁 Script executed:
cat vitest.config.ts 2>/dev/null || cat vitest.config.js 2>/dev/null || echo "No vitest config found"Repository: killallgit/adhd-ranch
Length of output: 88
🏁 Script executed:
cat -n vite.config.tsRepository: killallgit/adhd-ranch
Length of output: 993
🏁 Script executed:
cat -n vitest.setup.tsRepository: killallgit/adhd-ranch
Length of output: 115
🏁 Script executed:
cat src/components/App.test.tsx | grep -B 10 "Date.*now.*spyOn" | head -20Repository: killallgit/adhd-ranch
Length of output: 47
🏁 Script executed:
rg "afterEach|beforeEach" src/ --type tsx | head -20Repository: killallgit/adhd-ranch
Length of output: 93
🏁 Script executed:
for f in $(fd "\.test\.tsx$" src/components | head -5); do
echo "=== $f ===="
grep -c "mockRestore\|restoreAllMocks" "$f" || echo "0"
doneRepository: killallgit/adhd-ranch
Length of output: 299
🏁 Script executed:
grep -n "mockRestore\|spyOn" src/components/App.test.tsxRepository: killallgit/adhd-ranch
Length of output: 168
🏁 Script executed:
grep -B 3 -A 10 "spyOn(Date" src/components/App.test.tsxRepository: killallgit/adhd-ranch
Length of output: 518
🌐 Web query:
Vitest test execution assertion failure stops execution mockRestore
💡 Result:
In Vitest, a failing assertion in a test stops execution of the current test block (it throws immediately). So any code after the failing expect won’t run, which means teardown code like mockRestore in the middle of the test won’t be reached. Vitest explicitly notes that when an expect fails in an it/test block, it “exits that block, as if it crashes”, and suggests using expect.soft if you want the test to continue after failures [1]. What this means for “mockRestore stops execution”: 1) If you call mockRestore after an expect that can fail, it may never run because the failing expect throws and stops the rest of that test function [1]. 2) The safer pattern is to put restoration in afterEach/afterAll (or use global restore via Vitest lifecycle), so it runs even if an assertion fails. (Your restoration code needs to live in a teardown hook, not after a potentially-failing assertion.) If your goal is to restore all mocks/spies, also be aware of Vitest behavior/limitations around restoration: - Vitest has had issues where mockRestore/restoreAllMocks didn’t behave as expected for certain spy setups (e.g., restoration not returning to original descriptor / implementation), so depending on how the mock/spies were created, mockRestore might not give the exact “original behavior” you expect [2][3][4]. Common workaround/verification steps: - Move mockRestore/vi.restoreAllMocks into afterEach so it always runs. - If you need multiple assertions in a single test and still want cleanup to happen, use afterEach for cleanup and optionally use expect.soft for the non-critical assertions so execution continues [1].
Citations:
- 1: it/test blocks stop after the first failed expect vitest-dev/vitest#5379
- 2: mockRestore does not restore the original implementation vitest-dev/vitest#3335
- 3: mockRestore doesn't restore spied on properties to their original values vitest-dev/vitest#3127
- 4: restoreAllMocks doesn't restore mocks vitest-dev/vitest#5748
🌐 Web query:
Vitest vi.restoreAllMocks afterEach cleanup pattern best practice
💡 Result:
Best-practice pattern: call vi.restoreAllMocks in afterEach (per test), unless you enable the equivalent test.restoreMocks option globally. Why afterEach + restoreAllMocks - vi.restoreAllMocks “calls.mockRestore on all spies”, clearing mock history and restoring original implementations and even original property descriptors [1]. - Vitest’s mocking guide warns to “clear or restore mocks before or after each test run” to undo mock state changes between runs [2]. - The official lifecycle guide shows afterEach hooks as the per-test place for cleanup logic [3][4]. Recommended code (local) - Use afterEach( => { vi.restoreAllMocks }) in the test file/suite where you create spyOn mocks. Example: afterEach( => { vi.restoreAllMocks }) Or recommended config (global) - In vitest.config.js, you can set test.restoreMocks: true to make Vitest call vi.restoreAllMocks before each test automatically [5]. - Caution: docs warn this can cause problems with async concurrent tests because one test finishing may restore spies used by another test still in progress [5]. When not to rely on restoreAllMocks - If you’re using vi.mock (module mocks), restoreAllMocks is intended to restore spies created with vi.spyOn / vi.fn; it won’t “unmock” module imports [6]. In that case you’d use the appropriate unmock API (e.g., vi.doUnmock) instead [6]. Bottom line - For typical spyOn usage: prefer afterEach( => vi.restoreAllMocks) for isolation [1][2]. - For suites without async overlap and you want less boilerplate: consider test.restoreMocks: true, but be mindful of the concurrency warning [5].
Citations:
- 1: https://v3.vitest.dev/api/vi
- 2: https://vitest.dev/guide/mocking
- 3: restoreAllMocks doesn't restore mocks vitest-dev/vitest#5748
- 4: https://vitest.dev/guide/lifecycle
- 5: https://main.vitest.dev/config/restoremocks
- 6: Mocks are not restored using resetAllMocks() or restoreAllMocks() vitest-dev/vitest#2536
Ensure Date.now spy is always restored, even on assertion failure.
Line 190's expect(screen.getByText("01:30 remaining")) can fail, which stops test execution immediately and prevents nowSpy.mockRestore() on line 191 from running. The spy will leak into subsequent tests, tainting their results. Use try/finally to guarantee cleanup:
it("shows remaining time for a running timer", () => {
const nowSpy = vi.spyOn(Date, "now").mockReturnValue(1_030_000);
- renderDetail({
- focus: {
- ...baseFocus,
- timer: { duration_secs: 120, started_at: 1_000, status: "Running" },
- },
- });
- expect(screen.getByText("01:30 remaining")).toBeInTheDocument();
- nowSpy.mockRestore();
+ try {
+ renderDetail({
+ focus: {
+ ...baseFocus,
+ timer: { duration_secs: 120, started_at: 1_000, status: "Running" },
+ },
+ });
+ expect(screen.getByText("01:30 remaining")).toBeInTheDocument();
+ } finally {
+ nowSpy.mockRestore();
+ }
});Alternatively, add afterEach(() => vi.restoreAllMocks()) once to the suite to handle all spies globally.
📝 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.
| it("shows remaining time for a running timer", () => { | |
| const nowSpy = vi.spyOn(Date, "now").mockReturnValue(1_030_000); | |
| renderDetail({ | |
| focus: { | |
| ...baseFocus, | |
| timer: { duration_secs: 120, started_at: 1_000, status: "Running" }, | |
| }, | |
| }); | |
| expect(screen.getByText("01:30 remaining")).toBeInTheDocument(); | |
| nowSpy.mockRestore(); | |
| }); | |
| it("shows remaining time for a running timer", () => { | |
| const nowSpy = vi.spyOn(Date, "now").mockReturnValue(1_030_000); | |
| try { | |
| renderDetail({ | |
| focus: { | |
| ...baseFocus, | |
| timer: { duration_secs: 120, started_at: 1_000, status: "Running" }, | |
| }, | |
| }); | |
| expect(screen.getByText("01:30 remaining")).toBeInTheDocument(); | |
| } finally { | |
| nowSpy.mockRestore(); | |
| } | |
| }); |
🤖 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/components/PigDetail.test.tsx` around lines 182 - 192, The Date.now spy
in the "shows remaining time for a running timer" test (nowSpy created via
vi.spyOn(Date, "now")) may not be restored if the assertion fails; update the
test that calls renderDetail(...) and expects "01:30 remaining" to guarantee
cleanup by either wrapping the assertion in a try/finally and calling
nowSpy.mockRestore() in the finally block, or add a suite-level afterEach(() =>
vi.restoreAllMocks()) so all spies (including Date.now) are always restored
between tests.
| .pig-sprite--expired .pig-sprite-frame { | ||
| animation: expired-animal-pulse 700ms ease-out 1; | ||
| } | ||
|
|
||
| @keyframes expired-animal-pulse { | ||
| 0%, | ||
| 100% { | ||
| transform: translate(0, 0) scale(1); | ||
| } | ||
| 20% { | ||
| transform: translate(-2px, 0) scale(1.08); | ||
| } | ||
| 40% { | ||
| transform: translate(2px, 0) scale(1.08); | ||
| } | ||
| 60% { | ||
| transform: translate(-1px, 0) scale(1.04); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Honor reduced-motion preference for expired animation.
Line 426 applies motion unconditionally. Add a reduced-motion override so the expired state remains visually distinct without forced animation.
Proposed patch
.pig-sprite--expired .pig-sprite-frame {
animation: expired-animal-pulse 700ms ease-out 1;
}
+@media (prefers-reduced-motion: reduce) {
+ .pig-sprite--expired .pig-sprite-frame {
+ animation: none;
+ }
+}
+
`@keyframes` expired-animal-pulse {📝 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.
| .pig-sprite--expired .pig-sprite-frame { | |
| animation: expired-animal-pulse 700ms ease-out 1; | |
| } | |
| @keyframes expired-animal-pulse { | |
| 0%, | |
| 100% { | |
| transform: translate(0, 0) scale(1); | |
| } | |
| 20% { | |
| transform: translate(-2px, 0) scale(1.08); | |
| } | |
| 40% { | |
| transform: translate(2px, 0) scale(1.08); | |
| } | |
| 60% { | |
| transform: translate(-1px, 0) scale(1.04); | |
| } | |
| } | |
| .pig-sprite--expired .pig-sprite-frame { | |
| animation: expired-animal-pulse 700ms ease-out 1; | |
| } | |
| `@media` (prefers-reduced-motion: reduce) { | |
| .pig-sprite--expired .pig-sprite-frame { | |
| animation: none; | |
| } | |
| } | |
| `@keyframes` expired-animal-pulse { | |
| 0%, | |
| 100% { | |
| transform: translate(0, 0) scale(1); | |
| } | |
| 20% { | |
| transform: translate(-2px, 0) scale(1.08); | |
| } | |
| 40% { | |
| transform: translate(2px, 0) scale(1.08); | |
| } | |
| 60% { | |
| transform: translate(-1px, 0) scale(1.04); | |
| } | |
| } |
🤖 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/styles.css` around lines 425 - 443, The expired animation on
.pig-sprite--expired .pig-sprite-frame uses expired-animal-pulse
unconditionally; add a prefers-reduced-motion media query that removes or
overrides the animation (e.g., set animation: none and optionally keep a static
visual cue like transform: none or a subtle non-animating style) so users with
reduced-motion: reduce do not see the pulse; update or add rules targeting
.pig-sprite--expired .pig-sprite-frame within `@media` (prefers-reduced-motion:
reduce) and ensure the `@keyframes` expired-animal-pulse remains for non-reduced
users.
1 participant
Summary
Closes #30.
Updates #31.
Validation
task checkgit diff --checkNotes
The implementation keeps new shared timer-scale naming animal-neutral (
ranchAnimalScale/useRanchAnimalScale) while leavingPigSpriteandPigDetailas the current concrete UI components.Summary by CodeRabbit
New Features
Documentation
/checkpointnotes