Polish settings hierarchy and interaction states - #9
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR redesigns the Always On Clock display and settings window. It adds shared clock formatting, persisted save status, reusable settings controls, a development preview harness, scoped link permissions, new styling, and product documentation. ChangesAlways On Clock application
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsWindow
participant SettingsPane
participant SettingsContext
participant SettingsStore
SettingsWindow->>SettingsPane: Render selected settings section
SettingsPane->>SettingsContext: Submit setting update
SettingsContext->>SettingsStore: Persist serialized update
SettingsStore-->>SettingsContext: Return save result
SettingsContext-->>SettingsWindow: Show save timestamp or error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2766a72 to
8696f10
Compare
9a1e197 to
5db65bf
Compare
- Add sectioned Clock, Appearance, Behavior, and About panes - Share clock rendering between overlay and settings preview - Refresh window chrome, styling, controls, and preview tooling
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/App.tsx (1)
41-57: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe shared debounce timer drops one of the two saves.
handleMoveandhandleResizesharesaveTimeout. If a resize follows a move inside 500 ms, the resize clears the pending position save, so the new position is never persisted. The reverse order loses the new size. Window drag-resize emits both events, so this happens in normal use.Use one timer per concern.
🐛 Proposed fix
const win = getCurrentWindow(); - let saveTimeout: number | null = null; + let moveTimeout: number | null = null; + let resizeTimeout: number | null = null; const handleMove = async () => { - if (saveTimeout) clearTimeout(saveTimeout); - saveTimeout = window.setTimeout(async () => { + if (moveTimeout) clearTimeout(moveTimeout); + moveTimeout = window.setTimeout(async () => { const pos = await win.innerPosition(); void updateSettings({ windowPosition: { x: pos.x, y: pos.y } }).catch(() => undefined); }, 500); }; const handleResize = async () => { - if (saveTimeout) clearTimeout(saveTimeout); - saveTimeout = window.setTimeout(async () => { + if (resizeTimeout) clearTimeout(resizeTimeout); + resizeTimeout = window.setTimeout(async () => { const size = await win.innerSize(); void updateSettings({ windowSize: { width: size.width, height: size.height }, }).catch(() => undefined); }, 500); };Also clear both timers in the cleanup function:
return () => { - if (saveTimeout) clearTimeout(saveTimeout); + if (moveTimeout) clearTimeout(moveTimeout); + if (resizeTimeout) clearTimeout(resizeTimeout); unlistenMove.then((fn) => fn()); unlistenResize.then((fn) => fn()); };🤖 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/App.tsx` around lines 41 - 57, Use separate debounce timers for the position and size persistence in handleMove and handleResize, so each concern can save independently without cancelling the other. Update the cleanup logic to clear both timers, preserving the existing 500 ms delay and updateSettings calls.vite.config.ts (1)
3-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEnable JSON module resolution for the Vite config.
tsconfig.node.jsonlacksresolveJsonModule, so TypeScript 5.9 rejects thepackage.jsonimport. AddresolveJsonModule: trueand includepackage.jsonto avoid TS6307.🤖 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 `@vite.config.ts` around lines 3 - 14, Update tsconfig.node.json to enable resolveJsonModule and include package.json in its file or include configuration, so the package import used by the Vite config’s pkg symbol type-checks successfully.
🧹 Nitpick comments (6)
src/hooks/useClockDisplay.ts (1)
12-25: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider aligning the tick to the second boundary.
A fixed 1000 ms interval drifts against the wall clock. With seconds shown, the display can skip or repeat a second. Scheduling the next update at the next second boundary removes the drift. The hook also re-renders every second when
showSecondsis false, where only the minute matters.♻️ Optional refactor
useEffect(() => { - const interval = window.setInterval(() => setNow(new Date()), 1000); - return () => window.clearInterval(interval); - }, []); + let timer = 0; + const schedule = () => { + const next = 1000 - (Date.now() % 1000); + timer = window.setTimeout(() => { + setNow(new Date()); + schedule(); + }, next); + }; + schedule(); + return () => window.clearTimeout(timer); + }, []);🤖 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/useClockDisplay.ts` around lines 12 - 25, Update the timer setup in useClockDisplay to schedule each refresh at the next wall-clock second boundary instead of using a fixed 1000 ms interval, rescheduling after each update to prevent drift. When showSeconds is false, use minute-aligned scheduling so the hook does not re-render every second; preserve the existing cleanup and formatted time/date output.src/TitleBar.tsx (1)
64-67: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove the unused event listener after the promise settles.
webview.oncereturns a cleanup function. The current code discards both cleanup functions, so the listener for the event that does not occur remains registered. Store both cleanup functions and call them infinally.🤖 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/TitleBar.tsx` around lines 64 - 67, Update the promise setup in TitleBar’s webview event handling to retain both cleanup functions returned by the tauri://created and tauri://error webview.once calls, then invoke both cleanup functions in a finally block so the unused listener is removed regardless of which event settles the promise.src/components/ThemePicker.tsx (1)
67-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHide the decorative sample from assistive technology.
The stage contains the literal text
12:00. A screen reader reads it as part of each radio name, so every option is announced as "12:00 ". Addaria-hidden="true"totheme-tile-stage; the tile name already identifies the option.♻️ Proposed change
- <span className="theme-tile-stage"> + <span className="theme-tile-stage" aria-hidden="true"> <span className="theme-tile-ramp" aria-hidden="true" />🤖 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/ThemePicker.tsx` around lines 67 - 75, Add aria-hidden="true" to the theme-tile-stage element in ThemePicker so the decorative 12:00 sample is excluded from assistive technology while the existing theme name continues identifying each option.src/components/panes/ClockPane.tsx (1)
31-34: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive the time-format samples from the shared formatter.
'2:45 PM'and'14:45'are hardcoded. The overlay renders time throughsrc/utils/clockFormat.ts, which applies the runtime locale. In a locale that renders2:45 p.m.or uses a different separator, the sample does not match the clock the user sees. Build both samples with the shared formatter from a fixed reference date, asdateOptionsalready does for dates.🤖 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/panes/ClockPane.tsx` around lines 31 - 34, Replace the hardcoded samples in the ClockPane options with values generated by the shared formatter from a fixed reference date, matching the existing dateOptions pattern. Reuse the formatter and its 12-hour/24-hour configuration so samples follow the runtime locale and match the rendered clock.src/components/panes/AppearancePane.tsx (1)
82-104: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winContinuous inputs persist a write per event. Sliders and the native color picker emit an event for every intermediate value. Each event calls
updateSettings, and each call appends a store write to the serialized save queue insrc/contexts/SettingsContext.tsx. One drag can produce dozens of disk writes and can delay thelastSavedAtacknowledgement insrc/SettingsWindow.tsx. Introduce one shared debounce boundary for continuous controls: apply the value to state immediately for the live preview, and persist the last value after the interaction settles.
src/components/panes/AppearancePane.tsx#L82-L104: debounce thebackgroundOpacityandtextOpacitywrites instead of callingupdateSettingson every slider step.src/components/ui/ColorField.tsx#L48-L55: debounce thetype="color"onChangewrite, or commit the color onblur.🤖 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/panes/AppearancePane.tsx` around lines 82 - 104, The continuous appearance controls currently persist every intermediate value; add one shared debounce boundary so live state updates immediately while only the final settled value is persisted. In src/components/panes/AppearancePane.tsx lines 82-104, debounce writes from the Background and Text sliders; in src/components/ui/ColorField.tsx lines 48-55, debounce the type="color" onChange persistence or commit it on blur. Ensure each control preserves immediate preview updates while preventing one write per event.src/SettingsWindow.tsx (1)
129-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnounce save failures assertively.
The live region uses
aria-live="polite". A failed save waits for the screen reader to finish other output. Give the status element a role that matches the state:alertforsaveError,statusotherwise.♻️ Proposed status role
<p className={`settings-status ${saveError ? "is-error" : justSaved ? "is-saved" : ""}`.trim()} + role={saveError ? "alert" : "status"} > @@ - <span aria-live="polite"> + <span> {saveError ? saveError : justSaved ? "Saved" : "Changes save instantly"} </span>🤖 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/SettingsWindow.tsx` around lines 129 - 142, Update the live-region element in the settings status block to use an assertive alert role when saveError is present and a status role otherwise, while preserving the existing save message and conditional styling.
🤖 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 `@pnpm-workspace.yaml`:
- Around line 1-2: Pin the repository’s package manager to pnpm 10.26.0 or later
via the package.json packageManager field, and update the pnpm versions used by
both CI and release workflows to the same supported version. Keep the existing
allowBuilds configuration for esbuild unchanged.
In `@src/components/panes/AboutPane.tsx`:
- Around line 65-94: Update the confirmingReset toggle in AboutPane so focus
moves to the mounted Cancel button when confirmation opens and returns to the
Restore button when confirmation closes. Add stable refs and an effect or
equivalent focus management tied to the state transition, ensuring each target
is focused only after it is mounted while preserving the existing reset
behavior.
In `@src/components/ShortcutRecorder.tsx`:
- Around line 262-266: Update the reset button in ShortcutRecorder to reuse the
existing save-handling helper used by the record flow, including saveAttemptRef
invalidation and feedback/error handling, instead of calling
onChange(defaultValue) directly. Extract or reuse an applyShortcut-style
function and invoke it from both the reset handler and keydown path, with the
reset passing defaultValue so rejected saves are surfaced to feedback.
In `@src/components/ui/ColorField.tsx`:
- Around line 19-29: Update the value synchronization useEffect in ColorField so
it only calls setDraft when the incoming value differs from the current draft
after normalization, preserving the user’s original casing and caret during
uppercase hex entry. Keep handleTextChange’s lowercase onChange behavior
unchanged.
In `@src/components/ui/Field.tsx`:
- Around line 17-40: Update the Field component’s control rendering so the
generated hintId is passed to the associated control through aria-describedby.
Ensure both the control prop and children path can consume this association,
preserving the existing hintId generation and rendering behavior.
In `@src/contexts/SettingsContext.tsx`:
- Around line 226-261: Update updateSettings to roll back only the keys included
in updates when saveOperation fails, rather than restoring the entire
previousSettings object or requiring settingsRef.current === nextSettings.
Preserve unrelated settings changes applied by cross-window synchronization or
store onChange, while clearing the save error and restoring the failed keys from
their pre-update values only when this update remains the latest relevant
version.
In `@src/dev-preview.tsx`:
- Around line 26-40: Update the invoke handler in src/dev-preview.tsx so
unsupported browser commands no longer fall through to a silent undefined
result. Add explicit stubs for promised interactions such as the SettingsWindow
close command and an event-listening path that can emit events, and make the
default branch throw for any remaining unsupported command.
In `@src/globals.css`:
- Around line 10-47: Update the Stylelint value-keyword-case configuration to
ignore the case-sensitive keywords BlinkMacSystemFont and optimizeLegibility,
preserving their casing in the --font-ui and text-rendering declarations without
changing runtime behavior.
In `@src/utils/clockFormat.ts`:
- Around line 6-18: Update formatTime to derive an hour12 boolean from
timeFormat and reuse it for both the hour option and hour12 setting; use numeric
hours when hour12 is true and 2-digit hours otherwise, preserving the existing
minute and seconds formatting.
---
Outside diff comments:
In `@src/App.tsx`:
- Around line 41-57: Use separate debounce timers for the position and size
persistence in handleMove and handleResize, so each concern can save
independently without cancelling the other. Update the cleanup logic to clear
both timers, preserving the existing 500 ms delay and updateSettings calls.
In `@vite.config.ts`:
- Around line 3-14: Update tsconfig.node.json to enable resolveJsonModule and
include package.json in its file or include configuration, so the package import
used by the Vite config’s pkg symbol type-checks successfully.
---
Nitpick comments:
In `@src/components/panes/AppearancePane.tsx`:
- Around line 82-104: The continuous appearance controls currently persist every
intermediate value; add one shared debounce boundary so live state updates
immediately while only the final settled value is persisted. In
src/components/panes/AppearancePane.tsx lines 82-104, debounce writes from the
Background and Text sliders; in src/components/ui/ColorField.tsx lines 48-55,
debounce the type="color" onChange persistence or commit it on blur. Ensure each
control preserves immediate preview updates while preventing one write per
event.
In `@src/components/panes/ClockPane.tsx`:
- Around line 31-34: Replace the hardcoded samples in the ClockPane options with
values generated by the shared formatter from a fixed reference date, matching
the existing dateOptions pattern. Reuse the formatter and its 12-hour/24-hour
configuration so samples follow the runtime locale and match the rendered clock.
In `@src/components/ThemePicker.tsx`:
- Around line 67-75: Add aria-hidden="true" to the theme-tile-stage element in
ThemePicker so the decorative 12:00 sample is excluded from assistive technology
while the existing theme name continues identifying each option.
In `@src/hooks/useClockDisplay.ts`:
- Around line 12-25: Update the timer setup in useClockDisplay to schedule each
refresh at the next wall-clock second boundary instead of using a fixed 1000 ms
interval, rescheduling after each update to prevent drift. When showSeconds is
false, use minute-aligned scheduling so the hook does not re-render every
second; preserve the existing cleanup and formatted time/date output.
In `@src/SettingsWindow.tsx`:
- Around line 129-142: Update the live-region element in the settings status
block to use an assertive alert role when saveError is present and a status role
otherwise, while preserving the existing save message and conditional styling.
In `@src/TitleBar.tsx`:
- Around line 64-67: Update the promise setup in TitleBar’s webview event
handling to retain both cleanup functions returned by the tauri://created and
tauri://error webview.once calls, then invoke both cleanup functions in a
finally block so the unused listener is removed regardless of which event
settles the promise.
🪄 Autofix
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 Plus
Run ID: cb1f8369-6917-4c97-b83b-dff2cca0316f
📒 Files selected for processing (30)
PRODUCT.mdindex.htmlpnpm-workspace.yamlpreview.htmlsrc-tauri/capabilities/settings.jsonsrc/App.tsxsrc/Clock.tsxsrc/SettingsWindow.tsxsrc/TitleBar.tsxsrc/components/ClockFace.tsxsrc/components/ClockPreview.tsxsrc/components/Settings.tsxsrc/components/ShortcutRecorder.tsxsrc/components/ThemePicker.tsxsrc/components/panes/AboutPane.tsxsrc/components/panes/AppearancePane.tsxsrc/components/panes/BehaviorPane.tsxsrc/components/panes/ClockPane.tsxsrc/components/ui/ColorField.tsxsrc/components/ui/Field.tsxsrc/components/ui/Segmented.tsxsrc/components/ui/Select.tsxsrc/components/ui/Slider.tsxsrc/contexts/SettingsContext.tsxsrc/dev-preview.tsxsrc/globals.csssrc/hooks/useClockDisplay.tssrc/utils/clockFormat.tssrc/vite-env.d.tsvite.config.ts
💤 Files with no reviewable changes (1)
- src/components/Settings.tsx
| allowBuilds: | ||
| esbuild: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP '"packageManager"\s*:|pnpm@|corepack|pnpm/action-setup|pnpm-version' \
--glob 'package.json' \
--glob '*.yml' \
--glob '*.yaml' \
--glob '.nvmrc' \
--glob '.tool-versions' \
.Repository: emmsixx/always-on-clock
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate toolchain files ---'
git ls-files | rg '(^|/)(package\.json|pnpm-lock\.yaml|pnpm-workspace\.yaml|.*\.ya?ml|\.nvmrc|\.tool-versions|.*Dockerfile.*)$' | head -200
printf '%s\n' '--- package manager and pnpm references ---'
rg -n -i 'packageManager|pnpm|corepack|pnpm/action-setup|pnpm-version|strictDepBuilds|allowBuilds' . \
--glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' || trueRepository: emmsixx/always-on-clock
Length of output: 708
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package.json ---'
cat -n package.json
printf '%s\n' '--- CI workflow ---'
cat -n .github/workflows/ci.yml
printf '%s\n' '--- release workflow ---'
cat -n .github/workflows/release.ymlRepository: emmsixx/always-on-clock
Length of output: 9751
🌐 Web query:
pnpm 9 allowBuilds setting unknown setting pnpm 10.26.0 strictDepBuilds
💡 Result:
The settings allowBuilds and strictDepBuilds are part of pnpm's build script security configuration and were introduced/formalized in pnpm v10 [1][2]. allowBuilds This setting was introduced in pnpm v10.26.0 [3][1]. It is defined in pnpm-workspace.yaml and provides a centralized, granular way to manage which packages are permitted to execute build scripts (such as postinstall) [3][1]. It accepts a map of package matchers (package name patterns) mapped to boolean values: true to explicitly allow execution, and false to disallow it [3][4]. This setting replaces several legacy options, including onlyBuiltDependencies, onlyBuiltDependenciesFile, neverBuiltDependencies, and ignoredBuiltDependencies [3][1][4]. strictDepBuilds This setting controls how pnpm handles dependencies that have unreviewed build scripts [2]. - When strictDepBuilds is true (the default in later versions), pnpm will fail the installation with an error if it encounters a dependency with an unreviewed build script that is not listed in allowBuilds [3][2][4]. - When strictDepBuilds is false, pnpm will instead print a warning and continue the installation without running the script [3][5]. Compatibility - These settings are not available in pnpm v9 [3][1][6]. If you attempt to use them in a version of pnpm that does not support them, pnpm may treat them as unknown settings or emit warnings [6]. - To migrate from pnpm v9, you should upgrade to pnpm v10.26.0 or later to utilize the allowBuilds configuration [1][5]. - For pnpm v11 and later, strictDepBuilds is true by default, and the legacy build-related settings have been removed [4].
Citations:
- 1: https://pnpm.io/blog/releases/10.26
- 2: https://depsguard.com/strict-dep-builds/
- 3: https://pnpm.io/settings/build
- 4: https://github.com/pnpm/pnpm/blob/main/pnpm/CHANGELOG.md
- 5: Renaming onlyBuiltDependencies and ignoredBuiltDependencies in pnpm v11 pnpm/pnpm#10235
- 6: feat(rush-lib): support pnpm strictDepBuilds, allowBuilds (10.26.0+), and dangerouslyAllowAllBuilds microsoft/rushstack#5800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lockfile header ---'
sed -n '1,20p' pnpm-lock.yaml
printf '%s\n' '--- local-install documentation and version references ---'
rg -n -i 'pnpm|version|corepack|install' README.md package.json pnpm-lock.yaml .github/workflows \
--glob '!**/node_modules/**' | head -120Repository: emmsixx/always-on-clock
Length of output: 4813
Use pnpm 10.26.0 or later in CI, release, and local installs. Both workflows pin pnpm 9, which does not support allowBuilds. Add a packageManager pin to package.json and update both workflow pins; otherwise the esbuild approval is not applied.
🤖 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 `@pnpm-workspace.yaml` around lines 1 - 2, Pin the repository’s package manager
to pnpm 10.26.0 or later via the package.json packageManager field, and update
the pnpm versions used by both CI and release workflows to the same supported
version. Keep the existing allowBuilds configuration for esbuild unchanged.
| {confirmingReset ? ( | ||
| <div className="confirm"> | ||
| <button | ||
| type="button" | ||
| className="button button--ghost" | ||
| onClick={() => setConfirmingReset(false)} | ||
| > | ||
| Cancel | ||
| </button> | ||
| <button | ||
| type="button" | ||
| className="button button--danger" | ||
| onClick={() => { | ||
| void resetSettings().catch(() => undefined); | ||
| setConfirmingReset(false); | ||
| }} | ||
| > | ||
| Reset everything | ||
| </button> | ||
| </div> | ||
| ) : ( | ||
| <button | ||
| type="button" | ||
| className="button button--ghost" | ||
| onClick={() => setConfirmingReset(true)} | ||
| > | ||
| <RotateCcw size={13} strokeWidth={2.3} aria-hidden="true" /> | ||
| Restore | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Focus is lost when the confirm state toggles.
The "Restore" button unmounts when confirmingReset becomes true, and the confirm buttons unmount when it returns to false. The browser moves focus to <body> each time. A keyboard user must tab back through the pane to reach the destructive action, and a screen reader announces nothing about the new choice.
Move focus to the "Cancel" button when the confirm state opens, and back to "Restore" when it closes.
🤖 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/panes/AboutPane.tsx` around lines 65 - 94, Update the
confirmingReset toggle in AboutPane so focus moves to the mounted Cancel button
when confirmation opens and returns to the Restore button when confirmation
closes. Add stable refs and an effect or equivalent focus management tied to the
state transition, ensuring each target is focused only after it is mounted while
preserving the existing reset behavior.
| {!isDefault && !isRecording && ( | ||
| <button | ||
| type="button" | ||
| className="shortcut-recorder-reset" | ||
| onClick={() => onChange(defaultValue)} | ||
| > | ||
| <RotateCcw size={12} strokeWidth={2.3} aria-hidden="true" /> | ||
| Reset | ||
| <button type="button" className="button button--link" onClick={() => onChange(defaultValue)}> | ||
| <RotateCcw size={11} strokeWidth={2.4} aria-hidden="true" /> | ||
| Reset to default | ||
| </button> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the reset save like the record save.
Line 263 calls onChange(defaultValue) and drops the returned promise. BehaviorPane returns updateSettings({ globalShortcut }), which rejects when the store write fails. Two consequences follow:
- The rejection is unhandled, so it surfaces only as a console error.
feedbacknever changes, so the reset appears to do nothing and the user gets no explanation.
The reset also does not increment saveAttemptRef, so a stale record save can still write its own feedback afterwards. Reuse the save handling from lines 153-172.
🐛 Proposed reset handling
+ const applyShortcut = (shortcut: string) => {
+ const saveAttempt = ++saveAttemptRef.current;
+ setFeedback('Saving…');
+ void Promise.resolve()
+ .then(() => onChangeRef.current(shortcut))
+ .then(
+ () => {
+ if (saveAttempt === saveAttemptRef.current) {
+ setFeedback('Saved. This shortcut now toggles the clock.');
+ }
+ },
+ () => {
+ if (saveAttempt === saveAttemptRef.current) {
+ setFeedback('Could not save. The previous shortcut is still active.');
+ }
+ },
+ );
+ }; {!isDefault && !isRecording && (
- <button type="button" className="button button--link" onClick={() => onChange(defaultValue)}>
+ <button
+ type="button"
+ className="button button--link"
+ onClick={() => applyShortcut(defaultValue)}
+ >
<RotateCcw size={11} strokeWidth={2.4} aria-hidden="true" />
Reset to default
</button>
)}Then call applyShortcut(parts.join('+')) in the keydown handler instead of the inline promise chain.
🤖 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/ShortcutRecorder.tsx` around lines 262 - 266, Update the reset
button in ShortcutRecorder to reuse the existing save-handling helper used by
the record flow, including saveAttemptRef invalidation and feedback/error
handling, instead of calling onChange(defaultValue) directly. Extract or reuse
an applyShortcut-style function and invoke it from both the reset handler and
keydown path, with the reset passing defaultValue so rejected saves are surfaced
to feedback.
| useEffect(() => { | ||
| setDraft(value); | ||
| }, [value]); | ||
|
|
||
| const handleTextChange = (next: string) => { | ||
| const normalized = next === '' ? '' : next.startsWith('#') ? next : `#${next}`; | ||
| setDraft(normalized); | ||
| if (HEX.test(normalized)) { | ||
| onChange(normalized.toLowerCase()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The draft resets while the user types uppercase hex.
handleTextChange commits normalized.toLowerCase(). The parent then updates value, and the effect at lines 19-21 overwrites draft with the lowercase string. The visible text changes case mid-entry and the caret moves to the end. Sync the draft only when the incoming value differs from the normalized draft.
🐛 Proposed draft sync guard
useEffect(() => {
- setDraft(value);
+ setDraft((current) => (current.toLowerCase() === value.toLowerCase() ? current : value));
}, [value]);📝 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.
| useEffect(() => { | |
| setDraft(value); | |
| }, [value]); | |
| const handleTextChange = (next: string) => { | |
| const normalized = next === '' ? '' : next.startsWith('#') ? next : `#${next}`; | |
| setDraft(normalized); | |
| if (HEX.test(normalized)) { | |
| onChange(normalized.toLowerCase()); | |
| } | |
| }; | |
| useEffect(() => { | |
| setDraft((current) => (current.toLowerCase() === value.toLowerCase() ? current : value)); | |
| }, [value]); | |
| const handleTextChange = (next: string) => { | |
| const normalized = next === '' ? '' : next.startsWith('#') ? next : `#${next}`; | |
| setDraft(normalized); | |
| if (HEX.test(normalized)) { | |
| onChange(normalized.toLowerCase()); | |
| } | |
| }; |
🤖 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/ui/ColorField.tsx` around lines 19 - 29, Update the value
synchronization useEffect in ColorField so it only calls setDraft when the
incoming value differs from the current draft after normalization, preserving
the user’s original casing and caret during uppercase hex entry. Keep
handleTextChange’s lowercase onChange behavior unchanged.
| export const Field: React.FC<RowProps> = ({ label, hint, control, children, htmlFor }) => { | ||
| const generatedId = useId(); | ||
| const hintId = hint ? `${generatedId}-hint` : undefined; | ||
|
|
||
| return ( | ||
| <div className={`field ${children ? 'field--stacked' : ''}`.trim()}> | ||
| <div className="field-head"> | ||
| <div className="field-copy"> | ||
| {htmlFor ? ( | ||
| <label className="field-label" htmlFor={htmlFor}> | ||
| {label} | ||
| </label> | ||
| ) : ( | ||
| <span className="field-label">{label}</span> | ||
| )} | ||
| {hint && ( | ||
| <span className="field-hint" id={hintId}> | ||
| {hint} | ||
| </span> | ||
| )} | ||
| </div> | ||
| {control && <div className="field-control">{control}</div>} | ||
| </div> | ||
| {children && <div className="field-body">{children}</div>} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The hint id is generated but never associated with a control.
hintId is applied to the hint span, and no element references it through aria-describedby. Screen reader users hear the label without the hint. Expose the id so the control can consume it.
♿ Proposed fix
-export const Field: React.FC<RowProps> = ({ label, hint, control, children, htmlFor }) => {
+export const Field: React.FC<RowProps> = ({ label, hint, control, children, htmlFor }) => {
const generatedId = useId();
const hintId = hint ? `${generatedId}-hint` : undefined;Then set aria-describedby={hintId} on the rendered control. One option is to change children and control to accept a callback:
- /** Rendered full width beneath the label (segmented controls, sliders, grids). */
- children?: React.ReactNode;
+ /** Rendered full width beneath the label. Receives the hint id for `aria-describedby`. */
+ children?: React.ReactNode | ((hintId: string | undefined) => React.ReactNode);🤖 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/ui/Field.tsx` around lines 17 - 40, Update the Field
component’s control rendering so the generated hintId is passed to the
associated control through aria-describedby. Ensure both the control prop and
children path can consume this association, preserving the existing hintId
generation and rendering behavior.
|
|
||
| const updateSettings = useCallback(async (updates: Partial<Settings>) => { | ||
| const previousSettings = settingsRef.current; | ||
| const nextSettings = { ...previousSettings, ...updates }; | ||
| const updateVersion = ++settingsUpdateVersionRef.current; | ||
| settingsRef.current = nextSettings; | ||
| setSettings(nextSettings); | ||
| setSaveError(null); | ||
|
|
||
| const saveOperation = saveOperationRef.current | ||
| .catch(() => undefined) | ||
| .then(() => saveSettings(updates)); | ||
| saveOperationRef.current = saveOperation; | ||
|
|
||
| try { | ||
| await saveSettings(updates); | ||
| await saveOperation; | ||
| } catch (err) { | ||
| settingsRef.current = previousSettings; | ||
| setSettings(previousSettings); | ||
| if ( | ||
| updateVersion === settingsUpdateVersionRef.current && | ||
| settingsRef.current === nextSettings | ||
| ) { | ||
| settingsRef.current = previousSettings; | ||
| setSettings(previousSettings); | ||
| setSaveError('Could not save changes. Try again.'); | ||
| } | ||
| throw err; | ||
| } | ||
|
|
||
| setLastSavedAt(Date.now()); | ||
|
|
||
| try { | ||
| await emit(SETTINGS_UPDATED_EVENT, updates); | ||
| } catch (err) { | ||
| console.warn('Failed to synchronize settings between windows:', err); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Roll back only the keys that failed to save.
updateSettings snapshots the whole settings object and restores it on failure. If a cross-window settings-updated event or a store onChange applies unrelated keys while the save is in flight, two outcomes are possible:
- The external update renders first.
settingsRef.currentno longer equalsnextSettings, so the rollback is skipped and the failed values stay visible. - The external update has not yet reached
settingsRef(the sync effect at lines 55-57 runs after render). The rollback then discards the external keys as well.
A key-scoped rollback avoids both cases.
🐛 Proposed key-scoped rollback
} catch (err) {
- if (
- updateVersion === settingsUpdateVersionRef.current &&
- settingsRef.current === nextSettings
- ) {
- settingsRef.current = previousSettings;
- setSettings(previousSettings);
- setSaveError('Could not save changes. Try again.');
- }
+ if (updateVersion === settingsUpdateVersionRef.current) {
+ const reverted = (Object.keys(updates) as Array<keyof Settings>).reduce<Partial<Settings>>(
+ (acc, key) => Object.assign(acc, { [key]: previousSettings[key] }),
+ {},
+ );
+ settingsRef.current = { ...settingsRef.current, ...reverted };
+ setSettings((current) => ({ ...current, ...reverted }));
+ setSaveError('Could not save changes. Try again.');
+ }
throw err;
}📝 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.
| const updateSettings = useCallback(async (updates: Partial<Settings>) => { | |
| const previousSettings = settingsRef.current; | |
| const nextSettings = { ...previousSettings, ...updates }; | |
| const updateVersion = ++settingsUpdateVersionRef.current; | |
| settingsRef.current = nextSettings; | |
| setSettings(nextSettings); | |
| setSaveError(null); | |
| const saveOperation = saveOperationRef.current | |
| .catch(() => undefined) | |
| .then(() => saveSettings(updates)); | |
| saveOperationRef.current = saveOperation; | |
| try { | |
| await saveSettings(updates); | |
| await saveOperation; | |
| } catch (err) { | |
| settingsRef.current = previousSettings; | |
| setSettings(previousSettings); | |
| if ( | |
| updateVersion === settingsUpdateVersionRef.current && | |
| settingsRef.current === nextSettings | |
| ) { | |
| settingsRef.current = previousSettings; | |
| setSettings(previousSettings); | |
| setSaveError('Could not save changes. Try again.'); | |
| } | |
| throw err; | |
| } | |
| setLastSavedAt(Date.now()); | |
| try { | |
| await emit(SETTINGS_UPDATED_EVENT, updates); | |
| } catch (err) { | |
| console.warn('Failed to synchronize settings between windows:', err); | |
| } | |
| }, []); | |
| const updateSettings = useCallback(async (updates: Partial<Settings>) => { | |
| const previousSettings = settingsRef.current; | |
| const nextSettings = { ...previousSettings, ...updates }; | |
| const updateVersion = ++settingsUpdateVersionRef.current; | |
| settingsRef.current = nextSettings; | |
| setSettings(nextSettings); | |
| setSaveError(null); | |
| const saveOperation = saveOperationRef.current | |
| .catch(() => undefined) | |
| .then(() => saveSettings(updates)); | |
| saveOperationRef.current = saveOperation; | |
| try { | |
| await saveOperation; | |
| } catch (err) { | |
| if (updateVersion === settingsUpdateVersionRef.current) { | |
| const reverted = (Object.keys(updates) as Array<keyof Settings>).reduce<Partial<Settings>>( | |
| (acc, key) => Object.assign(acc, { [key]: previousSettings[key] }), | |
| {}, | |
| ); | |
| settingsRef.current = { ...settingsRef.current, ...reverted }; | |
| setSettings((current) => ({ ...current, ...reverted })); | |
| setSaveError('Could not save changes. Try again.'); | |
| } | |
| throw err; | |
| } | |
| setLastSavedAt(Date.now()); | |
| try { | |
| await emit(SETTINGS_UPDATED_EVENT, updates); | |
| } catch (err) { | |
| console.warn('Failed to synchronize settings between windows:', err); | |
| } | |
| }, []); |
🤖 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/contexts/SettingsContext.tsx` around lines 226 - 261, Update
updateSettings to roll back only the keys included in updates when saveOperation
fails, rather than restoring the entire previousSettings object or requiring
settingsRef.current === nextSettings. Preserve unrelated settings changes
applied by cross-window synchronization or store onChange, while clearing the
save error and restoring the failed keys from their pre-update values only when
this update remains the latest relevant version.
| invoke: async (cmd: string, args: Record<string, unknown> = {}) => { | ||
| switch (cmd) { | ||
| case 'plugin:store|load': | ||
| return 1; | ||
| case 'plugin:store|get': | ||
| return [store.get(args.key as string), store.has(args.key as string)]; | ||
| case 'plugin:store|set': | ||
| store.set(args.key as string, args.value); | ||
| return undefined; | ||
| case 'plugin:event|listen': | ||
| return 1; | ||
| case 'plugin:window|is_always_on_top': | ||
| return false; | ||
| default: | ||
| return undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not silently accept unsupported browser commands.
Lines 26-40 return undefined for every command outside the switch. src/SettingsWindow.tsx calls getCurrentWindow().close() for its close button, and plugin:event|listen returns 1 without an event emission path. These calls look successful while doing nothing in the browser preview. Add explicit stubs for interactions that this harness promises to drive, or throw for unsupported commands.
Expose unsupported commands
default:
- return undefined;
+ throw new Error(`Unsupported Tauri command in browser preview: ${cmd}`);🤖 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/dev-preview.tsx` around lines 26 - 40, Update the invoke handler in
src/dev-preview.tsx so unsupported browser commands no longer fall through to a
silent undefined result. Add explicit stubs for promised interactions such as
the SettingsWindow close command and an event-listening path that can emit
events, and make the default branch throw for any remaining unsupported command.
| --font-ui: ui-sans-serif, system-ui, -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif; | ||
|
|
||
| --bg-chrome: #0b0c0e; | ||
| --bg-canvas: #121316; | ||
| --bg-raised: #1a1d21; | ||
| --bg-raised-hover: #22262b; | ||
| --bg-inset: #08090b; | ||
|
|
||
| --line: rgba(255, 255, 255, 0.07); | ||
| --line-strong: rgba(255, 255, 255, 0.12); | ||
|
|
||
| --fg: #e9ebef; | ||
| --fg-2: #a0a6b0; | ||
| --fg-3: #7e8794; | ||
|
|
||
| --accent: #4f7cf7; | ||
| --accent-bright: #9bbaff; | ||
| --accent-tint: rgba(79, 124, 247, 0.15); | ||
| --accent-ring: rgba(106, 155, 255, 0.5); | ||
|
|
||
| --ok: #5fd3a0; | ||
| --danger: #f0736b; | ||
| --danger-tint: rgba(240, 115, 107, 0.13); | ||
|
|
||
| --r-xs: 6px; | ||
| --r-sm: 8px; | ||
| --r-md: 10px; | ||
| --r-lg: 13px; | ||
|
|
||
| --ease-out: cubic-bezier(0.22, 1, 0.36, 1); | ||
| --dur-fast: 120ms; | ||
| --dur: 180ms; | ||
| --dur-slow: 260ms; | ||
|
|
||
| color-scheme: dark; | ||
| font-family: var(--font-ui); | ||
| font-synthesis: none; | ||
| text-rendering: optimizelegibility; | ||
| text-rendering: optimizeLegibility; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep this casing and relax the Stylelint rule instead.
Stylelint flags BlinkMacSystemFont (line 10) and optimizeLegibility (line 47) under value-keyword-case. Both values are case-sensitive. Lowercasing them breaks the font stack and produces an invalid text-rendering keyword. Add both to ignoreKeywords so the lint job passes without a behavior change.
🔧 Proposed Stylelint config change
{
"rules": {
"value-keyword-case": [
"lower",
{ "ignoreKeywords": ["BlinkMacSystemFont", "optimizeLegibility"] }
]
}
}🧰 Tools
🪛 Stylelint (17.14.0)
[error] 10-10: Expected "BlinkMacSystemFont" to be "blinkmacsystemfont" (value-keyword-case)
(value-keyword-case)
[error] 47-47: Expected "optimizeLegibility" to be "optimizelegibility" (value-keyword-case)
(value-keyword-case)
🤖 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/globals.css` around lines 10 - 47, Update the Stylelint
value-keyword-case configuration to ignore the case-sensitive keywords
BlinkMacSystemFont and optimizeLegibility, preserving their casing in the
--font-ui and text-rendering declarations without changing runtime behavior.
Source: Linters/SAST tools
| export function formatTime(now: Date, { timeFormat, showSeconds }: TimeOptions): string { | ||
| const options: Intl.DateTimeFormatOptions = { | ||
| hour: '2-digit', | ||
| minute: '2-digit', | ||
| hour12: timeFormat === '12h', | ||
| }; | ||
|
|
||
| if (showSeconds) { | ||
| options.second = '2-digit'; | ||
| } | ||
|
|
||
| return now.toLocaleTimeString('en-US', options); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Intl.DateTimeFormat hour 2-digit hour12 true leading zero en-US
💡 Result:
When using Intl.DateTimeFormat with hour set to '2-digit' and hour12 set to true (which is the default for 'en-US'), JavaScript engines currently do not consistently apply a leading zero to the hour [1][2]. While '2-digit' is intended to force a two-digit representation (e.g., '01', '09'), many implementations ignore this requirement when combined with a 12-hour clock (hour12: true) [1][2]. This is a long-standing known issue in various environments, including Chrome, Node.js, and others, where single-digit hours are rendered without a leading zero [1][2]. If you require a leading zero for 12-hour time formats, the most reliable approach is to use formatToParts to manually construct the string or to pad the hour value yourself [2][3]. For example: const date = new Date; const parts = new Intl.DateTimeFormat('en-US', { hour: '2-digit', hour12: true }).formatToParts(date); const hour = parts.find(p => p.type === 'hour').value.padStart(2, '0'); const ampm = parts.find(p => p.type === 'dayPeriod').value; console.log(${hour} ${ampm}); This behavior works as expected (with leading zeros) when hour12 is set to false (24-hour format) [1][2].
Citations:
- 1: https://stackoverflow.com/questions/33401520/intl-datetimeformat-options-hash-getting-leading-zeros-with-2-digit
- 2: Intl.DateTimeFormat does not format 2-digit hours correctly facebook/hermes#1537
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/formatToParts
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -i 'clockFormat|ClockPane' . || true
printf '%s\n' '--- formatTime implementation ---'
format_file="$(fd -i -t f 'clockFormat' . | head -n 1)"
cat -n "$format_file"
printf '%s\n' '--- ClockPane sample references ---'
pane_file="$(fd -i -t f 'ClockPane' . | head -n 1)"
rg -n -C 4 '2:45|12h|timeFormat|formatTime' "$pane_file" || true
printf '%s\n' '--- runtime Intl behavior ---'
node - <<'JS'
const dates = [
new Date(2024, 0, 1, 2, 45, 0),
new Date(2024, 0, 1, 12, 45, 0),
];
for (const date of dates) {
for (const hour12 of [true, false]) {
const options = { hour: '2-digit', minute: '2-digit', hour12 };
console.log(JSON.stringify({
date: date.toString(),
hour12,
output: date.toLocaleTimeString('en-US', options),
numeric12: hour12
? date.toLocaleTimeString('en-US', { hour: 'numeric', minute: '2-digit', hour12 })
: undefined,
}));
}
}
JSRepository: emmsixx/always-on-clock
Length of output: 3166
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- project runtime and browser targets ---'
for f in package.json package-lock.json pnpm-lock.yaml yarn.lock vite.config.ts vite.config.js; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
rg -n -C 3 'engines|browserslist|target|browser|vite|electron|chrome|firefox|safari' "$f" || true
fi
done
printf '%s\n' '--- formatTime usage and tests ---'
rg -n -C 5 'formatTime|2:45 PM|02:45 AM' src test tests . --glob '!node_modules' --glob '!dist' --glob '!build' 2>/dev/null || trueRepository: emmsixx/always-on-clock
Length of output: 50379
Use numeric for 12-hour output.
V8/ICU formats hour: '2-digit' with hour12: true as 02:45 AM, while ClockPane.tsx advertises 2:45 PM. Set hour to hour12 ? 'numeric' : '2-digit' and reuse hour12.
🤖 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/utils/clockFormat.ts` around lines 6 - 18, Update formatTime to derive an
hour12 boolean from timeFormat and reuse it for both the hour option and hour12
setting; use numeric hours when hour12 is true and 2-digit hours otherwise,
preserving the existing minute and seconds formatting.
1 participant
What changed
Review note
This is intentionally stacked on PR #8 so the visual-only diff stays focused. Merge/rebase PR #8 first, then retarget this PR to
mainif desired.Verification
./node_modules/.bin/tsc --noEmit./node_modules/.bin/vite buildgit diff --checknpx --yes impeccable detect src --json→ no findingsSummary by CodeRabbit
New Features
Bug Fixes