Skip to content

fix: Persist settings panel preferences to localStorage #31

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 #31

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
18 changes: 16 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,14 @@ export function SettingsPanel() {
</div>
<input
type="number"
defaultValue={40}
value={weeklyHoursTarget}
onChange={(e) =>
updateSettings({
weeklyHoursTarget: parseInt(e.target.value, 10) || 0,
})
}
Comment on lines +142 to +146

Copilot AI Jan 6, 2026

Copy link

Choose a reason for hiding this comment

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

The fallback value of 0 when parsing fails could result in an invalid weeklyHoursTarget. When a user clears the input field or enters invalid text, parseInt returns NaN, which would then be replaced with 0. This means users cannot intentionally clear the field, and it will immediately snap to 0, which may not be the desired behavior. Consider either maintaining the previous valid value when parsing fails, or using an empty string check before parsing to preserve user input during editing.

Suggested change
onChange={(e) =>
updateSettings({
weeklyHoursTarget: parseInt(e.target.value, 10) || 0,
})
}
onChange={(e) => {
const parsed = parseInt(e.target.value, 10);
updateSettings({
weeklyHoursTarget: Number.isNaN(parsed) ? weeklyHoursTarget : parsed,
});
}}

Copilot uses AI. Check for mistakes.
min={0}
max={168}
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 150

Copilot AI Jan 6, 2026

Copy link

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 nearby text "Weekly Hours Target" should be associated with this input using either an id and htmlFor pairing or by wrapping the input in a <label> element. Screen readers currently won't announce the purpose of this input when users focus on it.

Copilot uses AI. Check for mistakes.
</div>
Expand All @@ -146,7 +155,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"
/>
<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>
Comment on lines 157 to 165

Copilot AI Jan 6, 2026

Copy link

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 input is wrapped in a <label> element, it doesn't contain any text content that screen readers can announce. The nearby text "Timesheet Reminders" should be included within the label element or associated with the input using id and htmlFor attributes so screen readers can properly announce the checkbox's purpose.

Copilot uses AI. Check for mistakes.
</div>
Expand Down