Skip to content

fix: Persist settings panel preferences to localStorage#32

Closed
BenGWeeks wants to merge 1 commit intomainfrom
claude/review-mvp-issues-ZWVwA
Closed

fix: Persist settings panel preferences to localStorage#32
BenGWeeks wants to merge 1 commit intomainfrom
claude/review-mvp-issues-ZWVwA

Conversation

@BenGWeeks
Copy link
Contributor

Connect SettingsPanel UI controls to useSettingsStore hook so that
Weekly Hours Target and Timesheet Reminders preferences are saved
to localStorage and persist across browser sessions.

Fixes #10

Connect SettingsPanel UI controls to useSettingsStore hook so that
Weekly Hours Target and Timesheet Reminders preferences are saved
to localStorage and persist across browser sessions.

Fixes #10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR connects the SettingsPanel UI controls to the useSettingsStore hook, enabling persistence of Weekly Hours Target and Timesheet Reminders preferences to localStorage via Zustand's persist middleware. Previously, these settings used defaultValue and defaultChecked attributes which did not persist across sessions.

  • Changed Weekly Hours Target input from uncontrolled (defaultValue) to controlled (value) with store integration
  • Changed Timesheet Reminders checkbox from uncontrolled (defaultChecked) to controlled (checked) with store integration
  • Added useSettingsStore hook import and integration to manage state persistence

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 139 to 149
<input
type="number"
defaultValue={40}
value={weeklyHoursTarget}
onChange={(e) =>
updateSettings({
weeklyHoursTarget: Math.max(0, parseInt(e.target.value) || 0),
})
}
min={0}
className="w-20 rounded-lg border border-dark-600 bg-dark-800 px-3 py-2 text-dark-100 focus:outline-none focus:ring-2 focus:ring-thyme-500"
/>
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number input lacks an accessible label. The input should have either an explicit label association via htmlFor/id or an aria-label attribute to ensure screen reader users can understand the purpose of this input field. The visible text "Weekly Hours Target" above the input is not programmatically associated with the input element.

Copilot uses AI. Check for mistakes.
Comment on lines +141 to +144
value={weeklyHoursTarget}
onChange={(e) =>
updateSettings({
weeklyHoursTarget: Math.max(0, parseInt(e.target.value) || 0),
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the input is cleared completely (empty string), parseInt will return NaN, and the fallback to 0 will set weeklyHoursTarget to 0. This means users cannot backspace to clear the field and type a new value without the field immediately resetting to 0. Consider using a controlled input pattern that allows temporary invalid states while typing, or use a separate local state to handle the input value and only update the store onBlur.

Suggested change
value={weeklyHoursTarget}
onChange={(e) =>
updateSettings({
weeklyHoursTarget: Math.max(0, parseInt(e.target.value) || 0),
defaultValue={weeklyHoursTarget}
onBlur={(e) =>
updateSettings({
weeklyHoursTarget: Math.max(
0,
Number.isNaN(parseInt(e.target.value, 10)) ? 0 : parseInt(e.target.value, 10)
),

Copilot uses AI. Check for mistakes.
type="checkbox"
checked={notificationsEnabled}
onChange={(e) => updateSettings({ notificationsEnabled: e.target.checked })}
className="peer sr-only"
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checkbox input lacks an accessible label. While the checkbox has a visible toggle switch, it should have either an aria-label attribute or be wrapped in a label element that includes the text "Timesheet Reminders" to ensure screen reader users can understand what the checkbox controls. The current label element only contains the visual toggle, not the descriptive text.

Suggested change
className="peer sr-only"
className="peer sr-only"
aria-label="Timesheet Reminders"

Copilot uses AI. Check for mistakes.
@BenGWeeks
Copy link
Contributor Author

Closing in favor of PR #30, which fully addresses issue #10 by also integrating weeklyHoursTarget into TeamList calculations (required by acceptance criteria).

@BenGWeeks BenGWeeks closed this Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: Settings Panel preferences do not persist

3 participants