Store the search bar and field toggle state in sessionStorage#430
Conversation
🦋 Changeset detectedLatest commit: d305d6a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe PR refactors state management for field filtering and row selection by introducing Zustand stores, moving from local component state to persistent store-based state. The storage layer is enhanced to support both localStorage and sessionStorage, with corresponding updates to table components and UI layout adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #430 +/- ##
============================================
Coverage 88.07% 88.07%
============================================
Files 91 91
Lines 4621 4621
Branches 1492 1492
============================================
Hits 4070 4070
Misses 551 551
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
branch:development
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-app/app/components/blocks/fields/table.tsx (1)
127-133: UpdatefuzzyFilterto handle bothsearchTermsandshowProductiveOnly.The
FieldFilterTogglecomponent is rendered in this table and updatesshowProductiveOnlyin the store, but thefuzzyFilterfunction only destructuressearchTermsand ignores theshowProductiveOnlyparameter. The toggle has no effect on table filtering. Compare with the rotation table'sfuzzySearchAndProductivityFilter(lines 211-231), which implements filtering logic for both parameters. Apply similar filtering to maintain consistency and ensure the toggle works as expected.
🤖 Fix all issues with AI agents
In `@fdm-app/app/components/blocks/fields/table.tsx`:
- Line 144: The onGlobalFilterChange handler currently calls
useFieldFilterStore.setState(state) which can overwrite unrelated store keys;
change it to extract/validate the expected filter value (e.g. ensure the
incoming state has the expected globalFilter shape or value) and then update the
store using a partial/merge update (call useFieldFilterStore.setState(prev => ({
...prev, globalFilter: validatedValue }))). Update the handler referenced as
onGlobalFilterChange to perform the validation/extraction and only set the
specific store key instead of replacing the whole state.
In `@fdm-app/app/components/blocks/rotation/table.tsx`:
- Around line 246-249: The condition in the onGlobalFilterChange handler uses
incorrect operator precedence so the comparison against fieldFilter.searchTerms
is not performed; change the expression to first resolve the possibly-null
searchTerms (e.g., compute const newTerms = globalFilter?.searchTerms ?? "" or
wrap the coalescing in parentheses) and then compare newTerms !==
fieldFilter.searchTerms, then call fieldFilter.setSearchTerms(newTerms) when
they differ; update the onGlobalFilterChange block (referencing
onGlobalFilterChange, globalFilter?.searchTerms, fieldFilter.searchTerms, and
fieldFilter.setSearchTerms) accordingly.
🧹 Nitpick comments (1)
fdm-app/app/store/storage.ts (1)
3-5: Consider narrowing the type for better type safety.The parameter type
keyof Windowis overly broad and doesn't constrain to storage-compatible properties. This allows passing invalid keys like"document"or"navigator"that would fail at runtime.✨ Suggested improvement
-const createSSRStorage = (name: keyof Window): StateStorage => { +const createSSRStorage = (name: "localStorage" | "sessionStorage"): StateStorage => { if (typeof window !== "undefined") { - return window[name] + return window[name] as StateStorage }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@fdm-app/app/store/rotation-selection.tsx`:
- Around line 28-30: Remove the debug logging in the store: delete or guard the
console.log calls inside setSelectionFrom (and the other console.log at the
later occurrence) so internal row data is not leaked to production; either
remove the console.log(rows, selection) entirely from setSelectionFrom or wrap
it behind a development-only check (e.g., process.env.NODE_ENV ===
"development") so logs only occur in dev environments.
- Around line 69-71: The persistence key for the rotation-selection store is
reusing "field-filter-storage" and will clash with the field-filter store;
update the store's config to use a unique name like "rotation-selection-storage"
(change the name property in the rotation-selection store definition where
storage: createJSONStorage(() => ssrSafeSessionJSONStorage) is used) so the
rotation-selection state ({ selection: Record<string, Record<string, boolean>>
}) won’t be mixed with the field-filter state.
🧹 Nitpick comments (1)
fdm-app/app/store/rotation-selection.tsx (1)
35-40: Coerce selection values to booleans for a stable persisted shape.
selection[fieldRow.id]can beundefined, which yields non-boolean values inselectionand inconsistent JSON output. Coercing to!!keeps the store shape stable and matches the type.♻️ Suggested tweak
- subSelection[(fieldRow.original as FieldRow).b_id] = - selection[fieldRow.id] + subSelection[(fieldRow.original as FieldRow).b_id] = + !!selection[fieldRow.id]
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@fdm-app/app/components/blocks/rotation/table.tsx`:
- Around line 280-290: The first effect currently depends on the stable
fieldFilter reference so it won’t re-run when its internals change; change the
dependency to the specific properties (e.g., fieldFilter.searchTerms and
fieldFilter.showProductiveOnly) and keep the body calling
rotationSelectionStore.clear(); the second effect should depend on actual row
data and the store selection rather than method/store references—inside the
effect call const rows = table.getCoreRowModel().rows and then
setRowSelection(rotationSelectionStore.getSelectionFor(rows)), and use a
dependency array like [rows.map(r => r.id).join(','),
rotationSelectionStore.getState().selection] or subscribe to
rotationSelectionStore and use its selection value to trigger the effect so
rowSelection stays in sync (referencing table.getCoreRowModel,
rotationSelectionStore.getSelectionFor, setRowSelection).
♻️ Duplicate comments (1)
fdm-app/app/store/rotation-selection.ts (1)
61-64: Use a unique persist key for the rotation selection store.This store uses
"field-filter-storage", which is already used by the field-filter store (fdm-app/app/store/field-filter.ts). Since they have incompatible state shapes—field-filter stores{ showProductiveOnly: boolean; searchTerms: string }while rotation-selection stores{ selection: Record<string, Record<string, boolean>> }—they will corrupt each other's data during sessionStorage hydration.Suggested fix
{ - name: "field-filter-storage", // unique name + name: "rotation-selection-storage", storage: createJSONStorage(() => ssrSafeSessionJSONStorage), // Use SSR-safe storage },
🧹 Nitpick comments (1)
fdm-app/app/store/rotation-selection.ts (1)
11-19: Consider renaming the interface to match its purpose.The interface is named
FieldFilterStatebut this store manages rotation row selections, not field filtering. This naming inconsistency could cause confusion, especially since there's already aFieldFilterStateconcept infield-filter.ts.Suggested rename
-interface FieldFilterState { +interface RotationSelectionState { selection: Record<string, Record<string, boolean>> clear(): void getSelectionFor(rows: Row<RotationExtended>[]): Record<string, boolean> setSelectionFrom( rows: Row<RotationExtended>[], rowSelection: Record<string, boolean>, ): void } -export const useRotationSelectionStore = create<FieldFilterState>()( +export const useRotationSelectionStore = create<RotationSelectionState>()(
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@fdm-app/app/components/blocks/fields/table.tsx`:
- Around line 172-183: The effect that sets rowSelection (using setRowSelection
and table.getCoreRowModel) currently only depends on fieldSelection and can
become stale when table data changes; update the dependency list to include a
stable representation of the table rows or the data (e.g., include
table.getCoreRowModel().rows or the memoized data used to create the table) so
the effect re-runs when rows change, and ensure you derive the selection from
table.getCoreRowModel().rows and fieldSelection.fieldIds inside the effect to
recompute mapping for new/removed rows.
- Around line 147-150: The onGlobalFilterChange handler currently treats the
incoming globalFilter as a direct value and ignores the Updater<T> function
form; update the handler (onGlobalFilterChange) to accept either a value or
updater function like handleRowSelection does: detect if globalFilter is a
function and call it with the previous fieldFilter.searchTerms to derive the new
value, otherwise use globalFilter directly, then call fieldFilter.setSearchTerms
with that resolved string (falling back to "" if undefined).
🧹 Nitpick comments (1)
fdm-app/app/store/field-selection.ts (1)
5-8: Rename the state type to match field selection.
FieldFilterStateis easy to confuse with the field-filter store. ConsiderFieldSelectionStatefor clarity and consistency.♻️ Suggested rename
-interface FieldFilterState { +interface FieldSelectionState { fieldIds: string[] setFieldIds: (fieldIds: string[]) => void } -export const useFieldSelectionStore = create<FieldFilterState>()( +export const useFieldSelectionStore = create<FieldSelectionState>()(
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@fdm-app/app/components/blocks/rotation/table.tsx`:
- Around line 244-247: The onGlobalFilterChange handler incorrectly assumes its
argument is a value; update it to handle an updater-or-value by detecting if
globalFilter is a function and, if so, calling it with the current filter state
(fieldFilter.searchTerms) to compute the new search terms before calling
fieldFilter.setSearchTerms; mirror the pattern used in
./fdm-app/app/components/blocks/fields/table.tsx so both direct values and
updater functions are supported.
|
Some users have an issue on the rotation table where they are not able to select the crop rows using the checkbox. Might also be clicking the row I am not very sure, but the former is more likely. |
|
@coderabbitai Some users have an issue on the rotation table where they are not able to select the crop rows using the checkbox. Might also be clicking the row I am not very sure, but the former is more likely. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 25371 🏁 Script executed: Length of output: 15574 🏁 Script executed: Length of output: 2608
The BugWhen a user clicks a checkbox, the selection flow is:
The current code (Line 80-84): function handleRowSelection(selection: Record<string, boolean>) {
rotationSelectionStore.setSelectionFrom(
table.getCoreRowModel().rows,
selection,
)
}The FixThe function needs to update the local state immediately: function handleRowSelection(selection: Record<string, boolean>) {
setRowSelection(selection) // Add this line to update local state
rotationSelectionStore.setSelectionFrom(
table.getCoreRowModel().rows,
selection,
)
}This ensures that:
The 🧠 Learnings used |
|
@coderabbitai why doesn't the useeffect help here? The rotationselectionstate changes which is synched through userotationselectionstore. Am I wrong possibly? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2129 🏁 Script executed: Length of output: 1027 🏁 Script executed: Length of output: 2083 🏁 Script executed: Length of output: 2608
Why the useEffect Doesn't HelpWhen you click a checkbox, here's what happens:
The ProblemLook at the useEffect dependencies: useEffect(() => {
setRowSelection(
rotationSelectionStore.getSelectionFor(table.getCoreRowModel().rows),
)
}, [memoizedData, table.getCoreRowModel, rotationSelectionStore])The issue is that So during the re-render triggered by the store update:
None of the dependencies changed, so the useEffect is skipped! The SolutionYou need to update the local state immediately in function handleRowSelection(selection: Record<string, boolean>) {
setRowSelection(selection) // ← Update local state immediately
rotationSelectionStore.setSelectionFrom(
table.getCoreRowModel().rows,
selection,
)
}This way:
Great catch on questioning this! 🎯 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes.
Zustand Subscription vs useEffect DependenciesYou're correct that when The useEffect only runs when one of its dependencies changes: useEffect(() => {
setRowSelection(
rotationSelectionStore.getSelectionFor(table.getCoreRowModel().rows),
)
}, [memoizedData, table.getCoreRowModel, rotationSelectionStore])The issue: Two Possible SolutionsOption 1 (Recommended): Update local state immediately in function handleRowSelection(selection: Record<string, boolean>) {
setRowSelection(selection) // Immediate update
rotationSelectionStore.setSelectionFrom(table.getCoreRowModel().rows, selection)
}Option 2: Fix the useEffect dependency to include the actual state: }, [memoizedData, table.getCoreRowModel, rotationSelectionStore.selection])Option 1 is cleaner because it updates immediately without waiting for the next render cycle. Does this clarify the distinction? 🧠 Learnings used |
…e using a single useFieldSelectionStore, implemented syncFarm to clear selection on farm switch, and removed legacy rotation-selection store.
2 participants
Enhancements
Summary by CodeRabbit
New Features
UI/UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.