[timer-picker-ui]: Preset picker in NewFocusWindow + PigDetail - #55
Merged
Conversation
Two timer-setting surfaces. The dedicated Tauri new-focus window now exposes a preset dropdown (2/4/8/16/32m + custom). The pig-detail popover does the same, plus a Start/Restart button that fires a new start_timer command against the existing focus. Shared component: - TimerPresetPicker (select + custom-minutes input) extracted from the orphaned NewFocusForm; orphan left in place per CLAUDE.md scope rule. - resolvePreset() / isCustomValid() are pure helpers used by both surfaces. NewFocusWindow / useNewFocusWindow: - Picker state lifted into the hook. Submit forwards the resolved preset to focusWriter.createFocus. Custom-minutes validation matches the inline form (reject < 1). Backend (crates/commands): - Commands::start_timer(focus_id, preset) builds a Running FocusTimer with started_at = clock_secs and calls store.update_timer. - Tests cover preset-duration mapping and not-found from store. Tauri + frontend transport: - ui_bridge::start_timer registered; FocusWriter gains startTimer (mirrors the WriteOutcome pattern). Fixture writer + App test mock updated. PigDetail: - Renders TimerPresetPicker + Start button. Button reads "Restart" when focus.timer.status === "Running" to flag the overwrite.
📝 WalkthroughWalkthroughThis PR extends timer functionality across the full stack: a backend ChangesTimer Start Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useNewFocusWindow.ts (1)
47-55: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
hideWindow()in the Escape path to avoid reset drift.Escape currently reimplements reset logic inline. Reusing
hideWindow()keeps one reset source of truth.Suggested patch
useEffect(() => { const onKey = (e: KeyboardEvent) => { if (e.key === "Escape") { - setTitle(""); - setDescription(""); - setTimerSelection("none"); - setCustomMinutes(10); - setError(null); - void getCurrentWindow().hide(); + void hideWindow(); } };🤖 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/useNewFocusWindow.ts` around lines 47 - 55, Replace the inline reset logic in the onKey handler (the Escape branch inside function onKey) with a call to the existing hideWindow() helper so the reset behavior is centralized; locate onKey and remove the manual setTitle, setDescription, setTimerSelection, setCustomMinutes, setError and getCurrentWindow().hide() calls and invoke hideWindow() instead to preserve single source of truth for resetting/hiding.
🤖 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.tsx`:
- Around line 52-55: The timer state (timerSelection, customMinutes, timerError)
in PigDetail is only initialized once and will persist when the component
receives a new focus prop; add a useEffect inside PigDetail that watches the
focus prop and calls setTimerSelection("Eight"), setCustomMinutes(10) and
setTimerError(null) to reset the picker whenever focus changes so state doesn't
leak between focuses.
In `@src/hooks/useNewFocusWindow.ts`:
- Around line 4-8: The hook useNewFocusWindow imports UI component logic
(PresetSelection type and helper functions isCustomValid, resolvePreset) from
src/components; extract these three symbols into a new shared non-UI module (for
example src/timer/presets or src/timer/index) and update imports so both
useNewFocusWindow and TimerPresetPicker import from that shared module instead
of src/components; ensure the exported names remain PresetSelection,
isCustomValid, and resolvePreset and update any relative import paths in
useNewFocusWindow and the component to reference the new module.
---
Outside diff comments:
In `@src/hooks/useNewFocusWindow.ts`:
- Around line 47-55: Replace the inline reset logic in the onKey handler (the
Escape branch inside function onKey) with a call to the existing hideWindow()
helper so the reset behavior is centralized; locate onKey and remove the manual
setTitle, setDescription, setTimerSelection, setCustomMinutes, setError and
getCurrentWindow().hide() calls and invoke hideWindow() instead to preserve
single source of truth for resetting/hiding.
🪄 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: ec656ff6-ee95-4986-a61e-b52efb7f5ddb
📒 Files selected for processing (15)
crates/commands/src/focus.rssrc-tauri/src/app/mod.rssrc-tauri/src/ui_bridge/mod.rssrc/api/fixtureFocusWriter.tssrc/api/focusWriter.test.tssrc/api/focusWriter.tssrc/components/App.test.tsxsrc/components/App.tsxsrc/components/NewFocusWindow.tsxsrc/components/PigDetail.test.tsxsrc/components/PigDetail.tsxsrc/components/TimerPresetPicker.test.tsxsrc/components/TimerPresetPicker.tsxsrc/hooks/useNewFocusWindow.tssrc/new-focus.tsx
- Move PresetSelection / resolvePreset / isCustomValid out of src/components/ into src/lib/timerPreset.ts. The hook useNewFocusWindow was importing domain helpers from a component module; per CLAUDE.md frontend rules, hooks/ should depend on lib/ or api/, not on components/. TimerPresetPicker stays the UI piece and now just imports PresetSelection from the lib module. - Reset PigDetail's timer-picker state when focus.id changes. Previously the same component instance could leak selection and custom-minutes across focuses when its focus prop swapped. Test coverage: - Move resolvePreset + isCustomValid unit tests to lib/timerPreset.test.ts. - New PigDetail test renders a Harness that swaps the focus.id mid-flight and asserts the picker resets to the default selection.
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 (1)
src/hooks/useNewFocusWindow.ts (1)
69-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle thrown errors from
createFocusto avoid unhandled submit failures.Line 70 assumes
focusWriter.createFocusalways resolves toWriteOutcome. If it throws, submit fails without a controlled error state. Add acatchpath and set a fallback error.Suggested patch
try { const outcome = await focusWriter.createFocus({ title: title.trim(), description: description.trim(), timer_preset: resolvePreset(timerSelection, customMinutes), }); if (outcome.ok) { await hideWindow(); } else { setError(outcome.message); } + } catch (error) { + setError(error instanceof Error ? error.message : "failed to create focus"); } finally { setSubmitting(false); }🤖 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/useNewFocusWindow.ts` around lines 69 - 82, The submit block assumes focusWriter.createFocus always resolves; wrap the await focusWriter.createFocus(...) call in a try/catch (inside the existing try/finally) or add a nested try/catch so thrown exceptions are caught, call setError with a sensible fallback message (e.g., "Failed to create focus" plus the caught error message), and ensure you still call setSubmitting(false) in the finally; also guard against outcome being undefined before checking outcome.ok and calling hideWindow or setError. Reference: useNewFocusWindow.ts, focusWriter.createFocus, setError, setSubmitting, hideWindow, resolvePreset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/hooks/useNewFocusWindow.ts`:
- Around line 69-82: The submit block assumes focusWriter.createFocus always
resolves; wrap the await focusWriter.createFocus(...) call in a try/catch
(inside the existing try/finally) or add a nested try/catch so thrown exceptions
are caught, call setError with a sensible fallback message (e.g., "Failed to
create focus" plus the caught error message), and ensure you still call
setSubmitting(false) in the finally; also guard against outcome being undefined
before checking outcome.ok and calling hideWindow or setError. Reference:
useNewFocusWindow.ts, focusWriter.createFocus, setError, setSubmitting,
hideWindow, resolvePreset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 277fcc4b-a210-4d2f-8ede-9c69e1b54c26
📒 Files selected for processing (8)
src/components/NewFocusWindow.tsxsrc/components/PigDetail.test.tsxsrc/components/PigDetail.tsxsrc/components/TimerPresetPicker.test.tsxsrc/components/TimerPresetPicker.tsxsrc/hooks/useNewFocusWindow.tssrc/lib/timerPreset.test.tssrc/lib/timerPreset.ts
Contributor
Author
|
Declining the latest outside-diff CodeRabbit finding ( |
This was referenced May 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces #54 (auto-closed when its base branch was deleted after #53 merged).
Adds the two surfaces where users actually set a timer:
create_focus.start_timercommand against an existing focus. Button label flips to "Restart" whenfocus.timer.status === "Running"to flag the overwrite.Shared bits:
TimerPresetPickercomponent extracted from the orphanedNewFocusForm. Orphan left in place per CLAUDE.md scope rule.resolvePreset/isCustomValidpure helpers; rejects custom < 1 minute.Backend:
Commands::start_timer(focus_id, preset)builds a RunningFocusTimer(started_at = clock_secs, duration from preset) and callsstore.update_timer.ui_bridge::start_timerregistered;FocusWriter.startTimerfollows the existingWriteOutcomepattern. Fixture +App.test.tsxmock updated.Test plan
task checkgreen (lint + typecheck + cargo tests + vitest)start_timertests (preset-duration mapping, not-found)TimerPresetPickertests + 2FocusWriter.startTimertests + 4PigDetailtimer-picker testsSummary by CodeRabbit
New Features
Bug Fixes / Validation
Tests