fix(settings): details and visiting date field validation #4615
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds nullable DatePicker value handling plus Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant WeeksList
participant WeekItem
participant WeekHook as useWeekItem
participant WeeksHook as useWeeksList
User->>WeekItem: change DatePicker (Date|null)
WeekItem->>WeekHook: handleDateChange(Date|null)
Note over WeekHook: compute nextWeek / prevWeek<br/>guard no-op if unchanged
WeekHook->>WeekHook: conditional reset of prev week type (if unused)
WeekHook->>WeekHook: set next week type -> CO_VISIT (if set)
WeekHook-->>WeekItem: resolve (success) / throw (error)
WeekItem->>WeeksHook: onWeekChange(id, nextWeekOf)
WeeksHook->>WeeksHook: updateWeeks() & computeErrors()
WeeksHook-->>WeeksList: new weeks + errors
WeeksList-->>WeekItem: pass `error` / `helperText`
sequenceDiagram
autonumber
actor User
participant UI
participant Hook as useCircuitOverseer
participant Timer as saveTimers[field]
participant DB as dbAppSettingsUpdate
User->>UI: edit firstname/lastname/displayname
UI->>Hook: onChange(field, value)
Hook->>Hook: markEditing(field)
Hook->>Timer: scheduleSave(field, debounce)
Note over Timer: debounce timer per-field
Timer-->>Hook: timer fires
Hook->>DB: update nested field path (specific key)
DB-->>Hook: success
Hook->>Hook: clearTimer(field), update local state
Hook-->>UI: reflect saved value
Note over Hook: external updates sync only for non-editing fields
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Files/areas to inspect closely:
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/features/congregation/settings/circuit_overseer/useCircuitOverseer.tsx (1)
37-51: Handle save errors to reset editing state.If the async
fn()on Line 44 rejects (Dexie hiccup, offline, etc.), the timeout exits before reaching thesetEditingblock, leaving the field stuck in “editing” mode and surfacing an unhandled rejection. Wrapping the await in atry/finallykeeps the UI state consistent even on failures.- saveTimers.current[key] = setTimeout(async () => { - saveTimers.current[key] = undefined; - await fn(); - setEditing((prev) => { - const next = { ...prev }; - for (const field of markCompleteKeys) { - next[field] = false; - } - return next; - }); - }, 1000); + saveTimers.current[key] = setTimeout(async () => { + saveTimers.current[key] = undefined; + try { + await fn(); + } finally { + setEditing((prev) => { + const next = { ...prev }; + for (const field of markCompleteKeys) { + next[field] = false; + } + return next; + }); + } + }, 1000);src/components/date_picker/slots/toolbar.tsx (1)
7-7: Null-safe toolbar formattingHandling null with a guard is correct and avoids invalid-date formatting.
Consider replacing '***' with a localized placeholder via t(...) for consistency.
Also applies to: 13-17
src/components/date_picker/index.tsx (1)
34-36: Null-aware DatePicker and error forwarding look solidState syncing, null handling, and text field error/helperText propagation are correct.
Type onMonthChange param and avoid re-wrapping:
- const changeHeight = (event) => { - if (getWeeksInMonth(new Date(event), { weekStartsOn: 0 }) === 6) { + const changeHeight = (date: Date) => { + if (getWeeksInMonth(date, { weekStartsOn: 0 }) === 6) { setHeight(290); } else { setHeight(240); } };Also applies to: 45-46, 61-67, 69-79, 81-96, 119-121, 161-171
src/features/congregation/settings/circuit_overseer/weeks_list/useWeeksList.tsx (1)
16-46: Edge case: multiple empties via edits are not flagged consistentlycomputeErrors only preserves previously flagged 'empty'. Clearing another week to empty won’t flag it, allowing multiple empties without warning. To consistently “allow only one empty,” mark all additional empties as errors and prefer keeping the previously flagged one (or first) as allowed.
Apply this refactor:
- const computeErrors = ( - list: WeekRecord[], - previous: Record<string, WeekErrorType> - ) => { - const nextErrors: Record<string, WeekErrorType> = {}; - - for (const record of list) { - if (record.weekOf.length === 0 && previous?.[record.id] === 'empty') { - nextErrors[record.id] = 'empty'; - } - } - - const byWeek = new Map<string, string[]>(); - - for (const record of list) { - if (record.weekOf.length === 0) continue; - const collection = byWeek.get(record.weekOf) ?? []; - collection.push(record.id); - byWeek.set(record.weekOf, collection); - } - - byWeek.forEach((ids) => { - if (ids.length > 1) { - ids.forEach((id) => { - nextErrors[id] = 'duplicate'; - }); - } - }); - - return nextErrors; - }; + const computeErrors = ( + list: WeekRecord[], + previous: Record<string, WeekErrorType> + ) => { + const nextErrors: Record<string, WeekErrorType> = {}; + + // Determine which empty (if any) is "allowed" + const emptyIds = list.filter((r) => r.weekOf.length === 0).map((r) => r.id); + const preserved = emptyIds.find((id) => previous?.[id] === 'empty'); + const allowedEmptyId = preserved ?? emptyIds[0]; + // Flag all additional empties beyond the allowed one + for (const id of emptyIds) { + if (id !== allowedEmptyId) nextErrors[id] = 'empty'; + } + + // Flag duplicates (non-empty) + const byWeek = new Map<string, string[]>(); + for (const record of list) { + if (record.weekOf.length === 0) continue; + const collection = byWeek.get(record.weekOf) ?? []; + collection.push(record.id); + byWeek.set(record.weekOf, collection); + } + byWeek.forEach((ids) => { + if (ids.length > 1) { + ids.forEach((id) => { + nextErrors[id] = 'duplicate'; + }); + } + }); + + return nextErrors; + };Does the product requirement intend to block creating a second empty via edits, or only warn? If blocking is desired, we can add a guard in handleWeekChange to revert clears when another empty exists.
Also applies to: 59-76, 78-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/locales/en/general.jsonis excluded by!**/*.json
📒 Files selected for processing (11)
.env.example(1 hunks)src/components/date_picker/index.tsx(5 hunks)src/components/date_picker/index.types.ts(3 hunks)src/components/date_picker/slots/toolbar.tsx(1 hunks)src/features/congregation/settings/circuit_overseer/useCircuitOverseer.tsx(3 hunks)src/features/congregation/settings/circuit_overseer/week_item/index.tsx(2 hunks)src/features/congregation/settings/circuit_overseer/week_item/index.types.ts(1 hunks)src/features/congregation/settings/circuit_overseer/week_item/useWeekItem.tsx(4 hunks)src/features/congregation/settings/circuit_overseer/weeks_list/index.tsx(2 hunks)src/features/congregation/settings/circuit_overseer/weeks_list/useWeeksList.tsx(1 hunks)vite.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/features/congregation/settings/circuit_overseer/week_item/useWeekItem.tsx (2)
src/definition/settings.ts (1)
CircuitOverseerVisitType(28-33)src/services/dexie/settings.ts (1)
dbAppSettingsUpdate(24-58)
src/components/date_picker/index.tsx (1)
src/utils/date.ts (1)
isValidDate(411-413)
src/features/congregation/settings/circuit_overseer/useCircuitOverseer.tsx (1)
src/services/dexie/settings.ts (1)
dbAppSettingsUpdate(24-58)
src/features/congregation/settings/circuit_overseer/weeks_list/useWeeksList.tsx (3)
src/definition/settings.ts (1)
CircuitOverseerVisitType(28-33)src/states/settings.ts (1)
settingsState(19-19)src/utils/date.ts (2)
formatDate(20-22)getWeekDate(73-79)
src/features/congregation/settings/circuit_overseer/week_item/index.tsx (2)
src/features/congregation/settings/circuit_overseer/week_item/index.types.ts (1)
WeekItemType(3-9)src/utils/date.ts (2)
formatDate(20-22)getWeekDate(73-79)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 5-5: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 5-5: [UnorderedKey] The VITE_APP_MODE key should go before the VITE_FIREBASE_APIKEY key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
src/features/congregation/settings/circuit_overseer/week_item/useWeekItem.tsx (1)
79-93: Nice guard against stale week types.The
previousWeekcheck plushasAnotherVisitWithWeekkeeps us from downgrading schedules when another visit still references the same week. This closes the gap where a single edit could reset the wrong week type.src/features/congregation/settings/circuit_overseer/week_item/index.types.ts (1)
5-8: Props surface extension looks goodNew props (error, helperText, onWeekChange, onDelete) are coherent with the updated flows.
src/features/congregation/settings/circuit_overseer/weeks_list/index.tsx (1)
13-18: Per-item error wiring is correctGood mapping of errors to WeekItem’s error/helperText and forwarding onWeekChange/onDelete.
Confirm translation keys exist: tr_fillRequiredField and tr_dateAlreadyExists.
Also applies to: 28-47
OneChanceLance
changed the title
OneChanceLance
changed the title
# Description Fixed the C.O. name resetting when out of focus or changing focus. Fixed the UI flickering when swapping dates. Please delete options that are not relevant. - [X] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update # Checklist: - [X] My code follows the style guidelines of this project - [X] I have performed a self-review of my own code - [X] I have commented my code, particularly in hard-to-understand areas - [X] I have made corresponding changes to the documentation - [X] My changes generate no new warnings - [X] Any dependent changes have been merged and published in downstream modules
rhahao
force-pushed
the
CU-86c1dwh1e_FIX-Allow-only-one-empty-CO-visit-field-Congregation-settings_Jake-Maragos
branch
from
e3a7265 to
183c261
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/features/congregation/settings/circuit_overseer/week_item/index.tsx (2)
17-23: Fix: Clone date before passing to getWeekDate to prevent mutation
getWeekDatemutates the passed Date object viasetDate. Passing the picker's date directly can cause unintended side effects and incorrect week calculations.Apply this diff:
const computeWeekOf = (date: Date | null) => { if (date === null) { return ''; } - return formatDate(getWeekDate(date), 'yyyy/MM/dd'); + // Clone to avoid mutating the original Date via getWeekDate + return formatDate(getWeekDate(new Date(date)), 'yyyy/MM/dd'); };
60-60: Fix: Use format-aware date parser for Safari compatibility
new Date('yyyy/MM/dd')is implementation-dependent and fails on Safari. The date string must be parsed with an explicit format parser.Apply this diff:
+import { parse } from 'date-fns'; +import { isValidDate } from '@utils/date'; // ... inside component ... <DatePicker disablePast label={t('tr_coNextVisitWeek')} - value={visit.weekOf === '' ? null : new Date(visit.weekOf)} + value={ + visit.weekOf === '' + ? null + : (() => { + const parsed = parse(visit.weekOf, 'yyyy/MM/dd', new Date()); + return isValidDate(parsed) ? parsed : null; + })() + } onChange={handlePickerChange} readOnly={!isAdmin} error={error} helperText={helperText} />
🧹 Nitpick comments (2)
.env.example (1)
5-5: Remove unnecessary quotes and consider reordering for linter compliance.The static analysis tools flag two style issues:
- The value
"TEST"has unnecessary quotes—simple alphanumeric values in.envfiles don't need quoting.- The key should be ordered alphabetically before
VITE_FIREBASE_APIKEYper dotenv-linter rules.Apply this diff if your project enforces dotenv-linter:
-VITE_FIREBASE_APIKEY=organized-local-api-key -VITE_FIREBASE_AUTHDOMAIN=localhost -VITE_FIREBASE_PROJECTID=organized-local -VITE_FIREBASE_APPID=organized-local-app-id -VITE_APP_MODE="TEST" +VITE_APP_MODE=TEST +VITE_FIREBASE_APIKEY=organized-local-api-key +VITE_FIREBASE_AUTHDOMAIN=localhost +VITE_FIREBASE_PROJECTID=organized-local +VITE_FIREBASE_APPID=organized-local-app-idIf grouping Firebase variables together is intentional, you may suppress the linter warnings instead.
vite.config.ts (1)
49-59: LGTM! Proxy configuration looks good for local development.The proxy setup correctly routes
/apicalls to the backend service with appropriate CORS handling. Thesecure: falsesetting is fine for local development.Optional improvement: Use environment variables for flexibility.
Consider making the backend target configurable via environment variables to support different development environments:
server: { port: 4050, host: true, proxy: { '/api': { - target: 'http://localhost:8000', + target: process.env.VITE_API_TARGET || 'http://localhost:8000', changeOrigin: true, secure: false, }, }, },Additionally, a brief inline comment explaining the proxy purpose would help future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/locales/en/general.jsonis excluded by!**/*.json
📒 Files selected for processing (11)
.env.example(1 hunks)src/components/date_picker/index.tsx(5 hunks)src/components/date_picker/index.types.ts(3 hunks)src/components/date_picker/slots/toolbar.tsx(1 hunks)src/features/congregation/settings/circuit_overseer/useCircuitOverseer.tsx(3 hunks)src/features/congregation/settings/circuit_overseer/week_item/index.tsx(2 hunks)src/features/congregation/settings/circuit_overseer/week_item/index.types.ts(1 hunks)src/features/congregation/settings/circuit_overseer/week_item/useWeekItem.tsx(4 hunks)src/features/congregation/settings/circuit_overseer/weeks_list/index.tsx(2 hunks)src/features/congregation/settings/circuit_overseer/weeks_list/useWeeksList.tsx(1 hunks)vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/date_picker/slots/toolbar.tsx
- src/components/date_picker/index.types.ts
- src/components/date_picker/index.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-09T14:24:33.861Z
Learnt from: nobodyzero1
Repo: sws2apps/organized-app PR: 4326
File: src/utils/enrollments.ts:20-25
Timestamp: 2025-07-09T14:24:33.861Z
Learning: In src/utils/enrollments.ts, the enrollmentStartDateChange function already includes a null check for the current variable to handle cases where the find operation returns undefined.
Applied to files:
src/features/congregation/settings/circuit_overseer/week_item/useWeekItem.tsx
🧬 Code graph analysis (4)
src/features/congregation/settings/circuit_overseer/week_item/useWeekItem.tsx (2)
src/definition/settings.ts (1)
CircuitOverseerVisitType(28-33)src/services/dexie/settings.ts (1)
dbAppSettingsUpdate(24-58)
src/features/congregation/settings/circuit_overseer/weeks_list/useWeeksList.tsx (3)
src/definition/settings.ts (1)
CircuitOverseerVisitType(28-33)src/states/settings.ts (1)
settingsState(19-19)src/utils/date.ts (2)
formatDate(20-22)getWeekDate(73-79)
src/features/congregation/settings/circuit_overseer/useCircuitOverseer.tsx (1)
src/services/dexie/settings.ts (1)
dbAppSettingsUpdate(24-58)
src/features/congregation/settings/circuit_overseer/week_item/index.tsx (2)
src/features/congregation/settings/circuit_overseer/week_item/index.types.ts (1)
WeekItemType(3-9)src/utils/date.ts (2)
formatDate(20-22)getWeekDate(73-79)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 5-5: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 5-5: [UnorderedKey] The VITE_APP_MODE key should go before the VITE_FIREBASE_APIKEY key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: dependency-review
- GitHub Check: Code QL
- GitHub Check: Summary
🔇 Additional comments (16)
src/features/congregation/settings/circuit_overseer/useCircuitOverseer.tsx (2)
149-168: LGTM: Effects handle syncing and cleanup correctly.The sync effect properly respects the editing state to prevent external updates from overwriting user input. The cleanup effect correctly clears all pending timers on unmount, preventing memory leaks and stale updates.
96-114: No polyfill needed—the project targets ESNext with native structuredClone support.The codebase explicitly targets
ESNextintsconfig.json(target: "ESNext", lib: ["ESNext", "DOM"]) and uses Vite without explicit legacy browser targeting. This means structuredClone is fully supported in all target browsers (Chrome 98+, Firefox 94+, Safari 15.4+).However, if future requirements expand browser support to legacy environments, core-js's structured-clone polyfill or MessageChannel-based fallback patterns should be added.
src/features/congregation/settings/circuit_overseer/week_item/index.types.ts (1)
5-8: LGTM!The type extensions cleanly support error display and per-item event handling, aligning well with the enhanced validation flow in the parent components.
src/features/congregation/settings/circuit_overseer/week_item/useWeekItem.tsx (4)
14-16: LGTM!The early return guard prevents unnecessary processing and database updates when
weekOfis empty or null.
41-51: LGTM!The helper correctly prevents resetting week types when multiple visits share the same week, avoiding data corruption.
53-100: LGTM!The enhanced date change logic correctly:
- Handles nullable dates
- Preserves previous week types when other visits reference them
- Only updates week types when appropriate
- Returns the computed date for potential chaining
The conditional cleanup using
hasAnotherVisitWithWeekis a critical improvement that prevents data corruption.
102-131: LGTM!The deletion logic mirrors the date change improvements, ensuring week types are only reset to NORMAL when no other visits reference the same week. This maintains data consistency across deletions.
src/features/congregation/settings/circuit_overseer/week_item/index.tsx (2)
25-45: Good implementation of optimistic updatesThe optimistic update pattern improves perceived responsiveness. The error handling correctly reverts to the original value if the async operation fails, and reconciles any mismatch between optimistic and actual values.
47-53: LGTM!The delete handler correctly awaits the async operation and invokes the callback, maintaining proper control flow.
src/features/congregation/settings/circuit_overseer/weeks_list/index.tsx (2)
13-17: LGTM!Clean integration with the expanded hook API and appropriate translation strings for error messaging.
28-47: LGTM!The per-item error handling correctly maps error types to appropriate messages and passes validation state to each WeekItem. The logic cleanly handles undefined error states.
src/features/congregation/settings/circuit_overseer/weeks_list/useWeeksList.tsx (5)
7-8: LGTM!The type definitions appropriately model draft state and validation error types for the week management flow.
16-46: Clever validation pattern for progressive error disclosureThe
computeErrorslogic implements a progressive error pattern:
- 'empty' errors persist only after being explicitly set (via
handleAddVisit), preventing false positives on initial render- Duplicate detection correctly groups by
weekOfand flags all conflicting entries- Empty weeks are appropriately excluded from duplicate checks
48-57: LGTM!The
updateWeekshelper cleanly couples state updates with error recomputation, ensuring validation stays synchronized with data changes.
59-94: LGTM!The handlers implement the correct flow:
handleAddVisitprevents multiple empty drafts by validating before addinghandleWeekChangeappropriately clears draft status when a date is sethandleVisitDeletedcleanly removes items from the listThe use of
crypto.randomUUID()for client-side IDs is appropriate.
96-117: LGTM!The synchronization effect correctly:
- Filters upcoming visits from persisted settings
- Preserves local drafts that haven't been persisted yet
- Combines both sources into the working list
- Recomputes errors to maintain validation consistency
The draft preservation logic ensures a smooth editing experience when settings update from external sources.
| const scheduleSave = (key: FieldKey, fn: () => Promise<void>, markCompleteKeys: FieldKey[]) => { | ||
| clearTimer(key); | ||
|
|
||
| saveTimers.current[key] = setTimeout(async () => { | ||
| saveTimers.current[key] = undefined; | ||
| await fn(); | ||
| setEditing((prev) => { | ||
| const next = { ...prev }; | ||
| for (const field of markCompleteKeys) { | ||
| next[field] = false; | ||
| } | ||
| return next; | ||
| }); | ||
| }, 1000); | ||
| }; |
There was a problem hiding this comment.
Race condition when firstname and lastname are edited in quick succession.
When a user edits firstname and then lastname before the firstname timer fires:
- Both fields mark displayname as editing
- Firstname timer completes after 1s, marks firstname and displayname as not editing
- At this point, displayname is no longer protected from external updates (line 154), even though lastname is still pending
- The useEffect may sync displayname from settings, potentially overwriting the lastname-derived value
- Lastname timer completes, but the derived displayname may have been lost
Consider tracking whether any name field is being edited before clearing displayname's editing state:
- const scheduleSave = (key: FieldKey, fn: () => Promise<void>, markCompleteKeys: FieldKey[]) => {
+ const scheduleSave = (key: FieldKey, fn: () => Promise<void>) => {
clearTimer(key);
saveTimers.current[key] = setTimeout(async () => {
saveTimers.current[key] = undefined;
await fn();
setEditing((prev) => {
const next = { ...prev };
- for (const field of markCompleteKeys) {
- next[field] = false;
- }
+ next[key] = false;
+ // Only mark displayname as not editing if neither firstname nor lastname is still editing
+ if (key === 'firstname' || key === 'lastname') {
+ if (!next.firstname && !next.lastname) {
+ next.displayname = false;
+ }
+ }
return next;
});
}, 1000);
};Then update the call sites to remove the markCompleteKeys parameter (lines 85, 89, 93).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/features/congregation/settings/circuit_overseer/useCircuitOverseer.tsx
around lines 37 to 51, the scheduleSave helper currently accepts
markCompleteKeys and unconditionally clears those editing flags when each timer
fires, which causes a race where firstname finishing can clear displayname while
lastname is still pending; change scheduleSave to only clear individual field
keys it directly saved and to avoid clearing shared/derived keys like
displayname unless no other related name timers remain (e.g., track active
name-field timers or an activeNames set/count and check it before clearing
displayname), and remove the now-unused markCompleteKeys parameter from the call
sites on lines 85, 89, and 93 so callers no longer rely on scheduleSave to clear
derived editing state.
| server: { | ||
| port: 4050, | ||
| host: true, | ||
| proxy: { |
There was a problem hiding this comment.
@OneChanceLance: since accessing the backend host is handled differently, can we please remove this? Thanks
2 participants
Description
Fixed the name edits with per-field debounced saves, overhauled the visit list to track drafts and flag blanks or duplicate weeks with warning messages, and fixed week changing so the picker no longer flickers.
Type of change
Please delete options that are not relevant.
Checklist: