Make productive filter work on rotation tables#590
Conversation
… with rotation row IDs
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughConsolidates rotation filtering into the shared Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as RotationTable
participant Store as FieldFilterStore
participant Loader as RouteLoader
rect rgba(200,230,255,0.5)
User->>UI: enter search / toggle productive-only
UI->>Store: set searchTerms / showProductiveOnly
Store-->>UI: current searchTerms & showProductiveOnly
UI->>Loader: request transformed rows (includes b_lu_catalogue)
Loader-->>UI: rows (fields + crop groups)
UI->>UI: globalFilterFn(row.original, searchTerms, showProductiveOnly)
UI-->>User: render filtered rows
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
fdm-app/app/components/blocks/rotation/table.tsx (3)
382-382: Nit: unnecessary optional chaining onfieldFilter.
useFieldFilterStore()always returns the store state, sofieldFilter?.searchTermscan befieldFilter.searchTerms. Harmless, but slightly misleading about nullability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/rotation/table.tsx` at line 382, The value prop uses unnecessary optional chaining on fieldFilter (value={fieldFilter?.searchTerms ?? ""}) even though useFieldFilterStore() always returns a state object; update the expression to use fieldFilter.searchTerms with the same fallback (e.g., value = fieldFilter.searchTerms ?? "") to remove the misleading nullability. Locate the usage in the component where useFieldFilterStore() is called and change the value reference for the search input accordingly.
560-571: RedundantRow<MemoizedTData>casts.
row.getParentRow()already returnsRow<MemoizedTData> | undefinedgivenuseReactTable<MemoizedTData>on Line 273, so the twoas Row<MemoizedTData>casts are unnecessary and can be dropped.♻️ Proposed cleanup
- (( - row.getParentRow() as Row<MemoizedTData> - )?.subRows.length === 1 + (row.getParentRow()?.subRows.length === 1 ? "shadow-[inset_0_1em_2em_-2em_#00000088,inset_0_-1em_2em_-2em_#00000088]" : row.index === 0 ? "shadow-[inset_0_1em_2em_-2em_#00000088]" : row.index === - ( - row.getParentRow() as Row<MemoizedTData> - )?.subRows.length - + (row.getParentRow()?.subRows.length ?? 0) - 1 && "shadow-[inset_0_-1em_2em_-2em_#00000088]"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/rotation/table.tsx` around lines 560 - 571, Remove redundant type assertions for Row<MemoizedTData> in the conditional that builds the shadow class string: `row.getParentRow() as Row<MemoizedTData>` should be replaced with `row.getParentRow()` (no cast) in both occurrences; update the expression that checks `?.subRows.length` and the subtraction expression so they operate directly on `row.getParentRow()` to avoid unnecessary `as Row<MemoizedTData>` casts—no other logic changes required (this is safe because `useReactTable<MemoizedTData>` already types `getParentRow()` as `Row<MemoizedTData> | undefined`).
67-100: Subscribe to individual store slices instead of the whole store.
useFieldFilterStore()with no selector subscribes this component to every field-filter state change (includingfarmIdupdates fromsyncFarm), causing unnecessary re-renders and re-running therowSelectionmemo whenever any unrelated slice changes. It also meansfieldFilterbecomes a fresh object reference on every store update, which propagates intoglobalFilter: fieldFilter(line 315) and theuseEffectdepfieldFilter.syncFarm(line 100).Consider selecting only what's needed:
♻️ Proposed refactor
- const fieldFilter = useFieldFilterStore() + const searchTerms = useFieldFilterStore((s) => s.searchTerms) + const showProductiveOnly = useFieldFilterStore((s) => s.showProductiveOnly) + const setSearchTerms = useFieldFilterStore((s) => s.setSearchTerms) + const syncFieldFilterFarm = useFieldFilterStore((s) => s.syncFarm)Then use
searchTerms/showProductiveOnlydirectly in the filter call and inglobalFilterstate, and passsyncFieldFilterFarminto the effect dep array (stable reference).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/rotation/table.tsx` around lines 67 - 100, The component currently calls useFieldFilterStore() with no selector, causing full-store subscriptions and a new object reference on every update which triggers unnecessary re-renders and stale deps; change to selecting only the needed slices (e.g. select searchTerms and showProductiveOnly and the syncFarm function) via useFieldFilterStore(selector) so you get stable references for the sync function and primitive values for filters, then update usages: replace globalFilter: fieldFilter with the selected searchTerms/showProductiveOnly values and use the selected sync function (e.g. syncFieldFilterFarm) in the useEffect dependency array instead of fieldFilter.syncFarm, and ensure rowSelection memo only depends on those selected values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-app/app/components/blocks/rotation/table.tsx`:
- Line 382: The value prop uses unnecessary optional chaining on fieldFilter
(value={fieldFilter?.searchTerms ?? ""}) even though useFieldFilterStore()
always returns a state object; update the expression to use
fieldFilter.searchTerms with the same fallback (e.g., value =
fieldFilter.searchTerms ?? "") to remove the misleading nullability. Locate the
usage in the component where useFieldFilterStore() is called and change the
value reference for the search input accordingly.
- Around line 560-571: Remove redundant type assertions for Row<MemoizedTData>
in the conditional that builds the shadow class string: `row.getParentRow() as
Row<MemoizedTData>` should be replaced with `row.getParentRow()` (no cast) in
both occurrences; update the expression that checks `?.subRows.length` and the
subtraction expression so they operate directly on `row.getParentRow()` to avoid
unnecessary `as Row<MemoizedTData>` casts—no other logic changes required (this
is safe because `useReactTable<MemoizedTData>` already types `getParentRow()` as
`Row<MemoizedTData> | undefined`).
- Around line 67-100: The component currently calls useFieldFilterStore() with
no selector, causing full-store subscriptions and a new object reference on
every update which triggers unnecessary re-renders and stale deps; change to
selecting only the needed slices (e.g. select searchTerms and showProductiveOnly
and the syncFarm function) via useFieldFilterStore(selector) so you get stable
references for the sync function and primitive values for filters, then update
usages: replace globalFilter: fieldFilter with the selected
searchTerms/showProductiveOnly values and use the selected sync function (e.g.
syncFieldFilterFarm) in the useEffect dependency array instead of
fieldFilter.syncFarm, and ensure rowSelection memo only depends on those
selected values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a5ae35d8-9f96-4418-89f5-3d030bb7a9ef
📒 Files selected for processing (1)
fdm-app/app/components/blocks/rotation/table.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fdm-app/app/components/blocks/rotation/table.tsx (1)
185-226: Tighten the component types instead of casting around them.
DataTablestill advertises a genericTData, but this implementation now hard-depends on crop rows with nested field rows and needs both(data as CropRow[])andcolumns as ColumnDef<MemoizedTData>[]to compile. Narrowing the props to the concrete rotation row shape would bring the new typing work back under compiler enforcement and make column/data drift fail earlier.Also applies to: 273-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/rotation/table.tsx` around lines 185 - 226, The DataTable usage is relying on casts like (data as CropRow[]) and columns as ColumnDef<MemoizedTData>[]; fix by tightening the component/prop typings so DataTable's generic TData is the concrete rotation row shape instead of any — update the component props (where memoizedData, data, and columns are declared) to use CropRow and FieldRow (or a union type identical to MemoizedTData) as the TData generic for DataTable and change ColumnDef types to ColumnDef<CropRow | CropFieldRow> (or the exact MemoizedTData type) so you can remove the casts around memoizedData, columns, and data; apply the same type tightening at the other occurrence referenced (the block around memoizedData usage at lines ~273-276).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-app/app/components/blocks/rotation/table.tsx`:
- Around line 185-226: The DataTable usage is relying on casts like (data as
CropRow[]) and columns as ColumnDef<MemoizedTData>[]; fix by tightening the
component/prop typings so DataTable's generic TData is the concrete rotation row
shape instead of any — update the component props (where memoizedData, data, and
columns are declared) to use CropRow and FieldRow (or a union type identical to
MemoizedTData) as the TData generic for DataTable and change ColumnDef types to
ColumnDef<CropRow | CropFieldRow> (or the exact MemoizedTData type) so you can
remove the casts around memoizedData, columns, and data; apply the same type
tightening at the other occurrence referenced (the block around memoizedData
usage at lines ~273-276).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: abb87d5e-c847-4e1b-b5e7-5b9e9ec4e54c
📒 Files selected for processing (1)
fdm-app/app/components/blocks/rotation/table.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
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/rotation/variety-selector.tsx (1)
53-80:⚠️ Potential issue | 🟠 MajorKeep crop-row variety updates scoped to the visible field subset.
This submit path still uses
row.original.fieldsfor crop rows, while the displayed counts are now aggregated fromrow.subRows. In a filtered crop row, selecting a variety here will update hidden fields as well.Suggested fix
- const fieldIds = ( - row.original.type === "crop" - ? row.original.fields - : [row.original] - ) + const fieldIds = ( + row.original.type === "crop" + ? (row.subRows ?? []).map( + (fieldRow) => + fieldRow.original as FieldRow, + ) + : [row.original] + ) .map((field) => encodeURIComponent(field.b_id), ) .join(",")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/rotation/variety-selector.tsx` around lines 53 - 80, The submit currently builds fieldIds from row.original.fields which updates hidden fields when the table is filtered; change the logic inside onValueChange to use row.subRows (mapping each subRow.original.b_id) as the source for crop-type rows instead of row.original.fields, and keep the fallback for non-crop rows as [row.original]; then URL-encode and join those IDs exactly as before before calling fetcher.submit (leave cultivationIds computation and the fetcher.submit call unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/components/blocks/rotation/fertilizer-display.tsx`:
- Around line 52-65: The bug is that fieldIds is built from row.original.fields
instead of the already-filtered fields variable used for badges, causing edit
links to include hidden fields; fix by deriving fieldIds from fields (the
variable computed from row.original/type and row.subRows) — e.g., map the same
fields array to each field.b_id (keeping the same order/uniqueness logic as
fertilizers/uniqueFertilizers) so the fertilizer edit link only targets the
visible/filtered field subset.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/rotation/variety-selector.tsx`:
- Around line 53-80: The submit currently builds fieldIds from
row.original.fields which updates hidden fields when the table is filtered;
change the logic inside onValueChange to use row.subRows (mapping each
subRow.original.b_id) as the source for crop-type rows instead of
row.original.fields, and keep the fallback for non-crop rows as [row.original];
then URL-encode and join those IDs exactly as before before calling
fetcher.submit (leave cultivationIds computation and the fetcher.submit call
unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 32a4421a-01fb-4842-85ef-8189d6284e34
📒 Files selected for processing (8)
fdm-app/app/components/blocks/rotation/columns.tsxfdm-app/app/components/blocks/rotation/crop-residue-checkbox.tsxfdm-app/app/components/blocks/rotation/date-selector.tsxfdm-app/app/components/blocks/rotation/fertilizer-display.tsxfdm-app/app/components/blocks/rotation/harvest-dates-display.tsxfdm-app/app/components/blocks/rotation/variety-selector.tsxfdm-app/app/components/custom/date-picker-v2.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/components/blocks/rotation/columns.tsx
2 participants
Bug fixes
Summary by CodeRabbit