-
Notifications
You must be signed in to change notification settings - Fork 1
refactor(calendar): move filters state mgmt to zustand store #331
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.
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
Conversation
Moved calendar filter state management from local component state to a global store via `calendarFilters` and related actions. Removed MonthCalendarHeader and integrated filter visibility logic into MonthCalendarNavigation and MonthCalendarFilters. Updated filtering logic to use arrays instead of Sets, and ensured filters are updated when habits or traits change. This improves consistency and enables filter state sharing across components.
📝 WalkthroughWalkthroughThis PR extracts MonthCalendarHeader into MonthCalendarNavigation and MonthCalendarFilters, moves calendar filter state into the UI store (arrays, new hooks), removes user-driven initialization from month/week calendars, and changes MonthCalendarGrid to consume filters via hooks instead of props. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
#176 Bundle Size — 1.82MiB (-0.02%).973e080(current) vs ed681fa main#174(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch refactor/global-filters-store Project dashboard Generated by RelativeCI Documentation Report issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/calendar/MonthCalendarGrid.tsx (1)
214-225: Remove the defensive fallback|| ''or use a dedicated sentinel that won't collide with data values.The filter
filters.traitIds.includes(o.habit.trait.id || '')creates a logic bug: whentraitIds = [''](nothing selected), occurrences with undefinedtrait.idwould incorrectly match because the fallback''equals the sentinel value, showing them when they should be hidden. SinceOccurrence.habit.trait.idis typed as required, either ensure this data is never falsy, or replace the sentinel with a value that can't occur in real trait IDs (e.g.,'__NONE__').
🤖 Fix all issues with AI agents
In `@src/components/calendar/MonthCalendarFilters.tsx`:
- Around line 240-254: The img inside SelectItem is using an invalid ARIA role
("role=\"habit-icon\"") which will be ignored; remove the role attribute and if
the attribute was only for tests replace it with a data-testid (e.g., on the img
element), otherwise rely on the existing meaningful alt text (habit.name) for
accessibility; locate the img in the SelectItem rendering where
getPublicUrl(StorageBuckets.HABIT_ICONS, habit.iconPath) is used and update
accordingly.
In `@src/stores/habits.store.ts`:
- Around line 48-55: The current set(...) call blindly replaces
state.calendarFilters.habitIds with all fetched habit ids, which clears any user
selection; modify the update in the set block (around state.habits and
state.calendarFilters) to compute the intersection of existing
state.calendarFilters.habitIds and the newly fetched habits' ids (habits.map(h
=> h.id)), and only use the full list when there is no prior selection (e.g.,
undefined/empty). Concretely, read the prior selection from
state.calendarFilters.habitIds, intersect it with the fetched ids to produce
newHabitIds (falling back to fetched ids if intersection is
empty/uninitialized), and assign that to calendarFilters.habitIds while still
setting state.habits = keyBy(habits, 'id').
In `@src/stores/traits.store.ts`:
- Around line 39-46: The current set((state) => { ... }) block overwrites
state.calendarFilters.traitIds with all fetched trait IDs which erases any user
selections; change the assignment to preserve existing selections by computing
the intersection between the existing state.calendarFilters.traitIds and the
newly fetched traits' ids (traits.map(t => t.id)), and only default to the full
list of fetched ids when state.calendarFilters.traitIds is undefined or empty;
update the logic around state.traits = keyBy(traits, 'id') and
state.calendarFilters to use this intersection so user-selected filters survive
a refresh.
🧹 Nitpick comments (3)
src/components/calendar/MonthCalendar.tsx (1)
17-21: Use@componentsalias (and barrel export) for the new subcomponent imports.This keeps imports consistent with the repo’s alias + barrel conventions.
As per coding guidelines, “Use path aliases throughout the codebase: `@components`, `@pages`, `@stores`, `@services`, `@hooks`, `@models`, `@utils`, `@db-types`, `@tests`” and “Export new components, hooks, models, services, stores, and utils from their respective barrel files and import using corresponding path aliases”.♻️ Suggested import adjustment
-import MonthCalendarFilters from './MonthCalendarFilters'; -import MonthCalendarNavigation from './MonthCalendarNavigation'; +import MonthCalendarFilters from '@components/calendar/MonthCalendarFilters'; +import MonthCalendarNavigation from '@components/calendar/MonthCalendarNavigation';src/components/calendar/MonthCalendarGrid.tsx (1)
24-27: Remove commented-out code.The commented prop
// filters: OccurrenceFilters;is dead code and should be removed for cleanliness.Suggested fix
type CalendarGridProps = { - // filters: OccurrenceFilters; state: CalendarState; };src/components/calendar/MonthCalendarFilters.tsx (1)
42-56: Consider extracting sentinel value to a constant.The empty string
''is used as a sentinel value across multiple checks (lines 43, 51, 71, 102). Extracting this to a named constant would improve readability and reduce magic value usage.Suggested refactor
+const EMPTY_FILTER_SENTINEL = ''; + const MonthCalendarFilters = () => { // ... const areAllHabitsSelected = React.useMemo(() => { - if (filters.habitIds.length === 1 && filters.habitIds.includes('')) { + if (filters.habitIds.length === 1 && filters.habitIds.includes(EMPTY_FILTER_SENTINEL)) { return false; } // ... }, [filters.habitIds, habits]);This constant could be defined in the store and exported for consistent usage across components.
Moved calendar filter state management from local component state to a global store via
calendarFiltersand related actions. Removed MonthCalendarHeader and integrated filter visibility logic into MonthCalendarNavigation and MonthCalendarFilters. Updated filtering logic to use arrays instead of Sets, and ensured filters are updated when habits or traits change. This improves consistency and enables filter state sharing across components.Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.