Skip to content

fix: Persist settings panel preferences to localStorage #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
BenGWeeks wants to merge 1 commit into from
Closed

fix: Persist settings panel preferences to localStorage #32

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/components/settings/SettingsPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { useState, useEffect } from 'react';
import { Card } from '@/components/ui';
import { bcClient } from '@/services/bc/bcClient';
import { useAuth } from '@/services/auth';
import { useSettingsStore } from '@/hooks/useSettingsStore';
import {
BuildingOffice2Icon,
EnvelopeIcon,
Expand All @@ -26,6 +27,7 @@ interface CompanyInfo {

export function SettingsPanel() {
const { account } = useAuth();
const { weeklyHoursTarget, notificationsEnabled, updateSettings } = useSettingsStore();
const [companyInfo, setCompanyInfo] = useState<CompanyInfo | null>(null);
const [isLoading, setIsLoading] = useState(true);
const [error, setError] = useState<string | null>(null);
Expand Down Expand Up @@ -136,7 +138,13 @@ export function SettingsPanel() {
</div>
<input
type="number"
defaultValue={40}
value={weeklyHoursTarget}
onChange={(e) =>
updateSettings({
weeklyHoursTarget: Math.max(0, parseInt(e.target.value) || 0),
Comment on lines +141 to +144
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.
})
}
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"
/>
Comment on lines 139 to 149
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.
</div>
Expand All @@ -146,7 +154,12 @@ export function SettingsPanel() {
<p className="text-sm text-dark-400">Get reminded to fill in your timesheet</p>
</div>
<label className="relative inline-flex cursor-pointer items-center">
<input type="checkbox" defaultChecked className="peer sr-only" />
<input
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.
/>
<div className="peer h-6 w-11 rounded-full bg-dark-600 after:absolute after:left-[2px] after:top-[2px] after:h-5 after:w-5 after:rounded-full after:bg-white after:transition-all after:content-[''] peer-checked:bg-thyme-600 peer-checked:after:translate-x-full peer-focus:outline-none peer-focus:ring-2 peer-focus:ring-thyme-500"></div>
</label>
</div>
Expand Down