Conversation
…um (#371) Phase 3 (ADVANCED-FILTERS-BRIEF): Notion/Twenty-style filter bar (AdvancedFilterBar) wired into leads-table.tsx behind NEXT_PUBLIC_ADVANCED_FILTERS, URL-backed via ?f= (use-advanced-filters.ts), reusing FilterOptionList verbatim for multi-select. compileAssignees now drops no-op conditions instead of emitting a tautology inside OR groups. Phase 3 addendum — facet counts (regression fix + server-side move): - A/B: restored counts + zero-count hiding on Assigned To and Collaborators in advancedFilterOptionOverrides (parity with the legacy filterDefs). - C: server-side Assigned-To facet via lead_aggregates()'s existing `counselor` dimension (getAssigneeFacet, aggregates.ts) — exact, tenant-wide counts instead of the 25-row-page client computation. ?facets= now accepts a comma-separated dimension list (?facets=source,assignee) in one client round-trip; ?facets=source alone stays byte-identical (KanbanBoard.tsx depends on it). Collaborator counts stay client-side/page-scoped, labelled as such — no lead_aggregates() dimension exists for that join table yet. - D: no industry gate — assignee counts gate on isAdmin/isTeamScoped only. - E: tree-to-aggregate-params.ts pulled forward from Phase 5 — a pure-AND, fully-expressible ?f= tree now drives real facet counts instead of a blanket counts:null; anything inexpressible (OR groups, contains, unknown fields/ops) still falls back to counts:null, pino-logged with the reason. - G: Apply bug root-caused via browser repro (Sadin) — two fixes: 1. serialize.ts used Buffer.from(...).toString("base64url"), which throws "Unknown encoding: base64url" in a real browser bundle (the `buffer` shim doesn't support it, even though Node's real Buffer does) — handleApply threw before setOpen(false), so the popover never closed and the URL never gained ?f=. Replaced with an isomorphic TextEncoder/TextDecoder + btoa/atob codec. Added serialize.browser.test.ts (jsdom + Buffer deleted) so a Node-only API can't pass CI silently again — plain jsdom alone doesn't catch this, since jsdom still runs on Node's real Buffer. 2. filter-value-editor.tsx: selecting a multi-select option reveals FilterOptionList's "Clear" row, pushing Apply down ~45px under the cursor. Reserved that row's height with a spacer whenever Clear isn't shown, so the container's total height never changes. Verified live on local dev (browser): Assigned To shows exact tenant-wide counts (Unassigned 27 + counselor 6 = 33, zero-count members absent); Apply button position stable across the Clear-row transition; Apply correctly gains ?f=, closes the popover, and filters the table; two stacked chips (Assigned To + Status) compose correctly; a copied ?f= URL reproduces the same filtered view in a fresh tab. npm run test: 1326/1326 green. npm run build clean. npx eslint --max-warnings 50: 0 errors, 46 pre-existing warnings. Co-authored-by: Anish Balami <anishbalami38@gmail.com>
…ge build (#372) NEXT_PUBLIC_* is inlined at build time, so the stage image never actually picked up the flag despite Phases 0-3 being merged (d3841a6) — the deployed bundle rendered the legacy toolbar. - Dockerfile: ARG + ENV NEXT_PUBLIC_ADVANCED_FILTERS alongside the existing NEXT_PUBLIC_* pairs. - deploy-staging.yml: NEXT_PUBLIC_ADVANCED_FILTERS=1 literal in build-args, matching how NEXT_PUBLIC_SENTRY_ENVIRONMENT=staging is done. - deploy.yml (prod) untouched — prod stays off; with no ARG value passed the Dockerfile ARG resolves empty and the flag is undefined there. Also commits docs/ADVANCED-FILTERS-BRIEF.md Phase 3.5 addendum (was already in the working tree at session start).
Search box matched name/email/phone only, so searching by lead ID (e.g. ADM-6038) silently returned 0 results — client hit this in prod. Add display_id to both the server field registry (leads.ts) and the client-side legacy fallback matcher (leads-table.tsx) so ID search works the same way global search already does. Co-authored-by: Anish Balami <anishbalami38@gmail.com>
📝 WalkthroughWalkthroughThe PR adds a feature-flagged advanced filter bar for leads. It adds URL-backed filter trees, browser-safe serialization, aggregate facet support for source and assignee, compiler no-op handling, leads-table integration, reusable UI primitives, tests, and staging-only deployment configuration. ChangesAdvanced leads filtering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LeadUser
participant LeadsTable
participant AdvancedFilterBar
participant LeadsRoute
participant LeadAggregates
LeadUser->>AdvancedFilterBar: edit and apply filter tree
AdvancedFilterBar->>LeadsTable: update controlled filter state
LeadsTable->>LeadsRoute: request encoded filter tree and facets
LeadsRoute->>LeadAggregates: query filtered source and assignee aggregates
LeadAggregates-->>LeadsRoute: return facet counts
LeadsRoute-->>LeadsTable: return filtered leads and facets
LeadsTable-->>LeadUser: render filtered results and chips
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.72.0)Trivy execution failed: 2026-08-10T14:54:13Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: cloudformation scan error: fs filter error: fs filter error: walk error open .coderabbit-opengrep-fallback.8bf81df4-434d-4f7d-afdb-ae4257a2883b.yml: no such file or directory: open .coderabbit-opengrep-fallback.8bf81df4-434d-4f7d-afdb-ae4257a2883b.yml: no such file or directory 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 |
sthasadin
approved these changes
Aug 10, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ADVANCED-FILTERS-BRIEF.md`:
- Around line 544-556: Label the component-tree fenced code block in the
documentation with the text language identifier by changing its opening fence;
leave the diagram content unchanged.
In `@src/app/`(main)/api/v1/leads/route.ts:
- Around line 511-559: Update the aggregate facet flow around baseFacetParams in
src/app/(main)/api/v1/leads/route.ts:511-559 to preserve parity with the page
query by representing legacy assigned_to, stage, and source filters in the
aggregate RPC, or return the existing counts:null fallback whenever any such
filter cannot be represented exactly. Add regression cases in
src/app/(main)/api/v1/leads/route.test.ts:817-900 covering stage, assigned_to,
and legacy source filters, asserting either the correct RPC constraints or the
no-count fallback.
In `@src/components/dashboard/leads-table.tsx`:
- Around line 1928-1970: The advanced filter registry currently exposes the
destinations field without selectable options. Update the
advancedFilterOptionOverrides or the corresponding useFilterOptions
configuration to supply destinations values through an existing override or
async loader, and ensure advancedVisibleFields does not include destinations
while those values are unavailable.
- Around line 711-738: Update the fetch chain in the facet-loading flow to check
res.ok immediately after fetch and throw for non-2xx responses before calling
res.json(). Keep the existing facet mapping in the body handler and let the
catch block preserve prior facet state for HTTP and network failures.
- Around line 366-372: Sanitize the advanced filter tree against the visible
field registry before determining activity, fetching leads, or rendering filter
controls. Update the useAdvancedFilters flow and the related leads fetch/render
logic to use the same advancedVisibleFields-filtered registry passed to the bar,
removing role- and industry-restricted conditions from decoded URL state so
hidden filters cannot affect requests or remain active without a visible chip.
- Around line 371-372: Restrict advanced filter activation in LeadsTable to
supported server-paginated data paths by incorporating serverPaginated into
advancedFiltersEnabled or the equivalent AdvancedFilterBar/render gating. Ensure
non-server-paginated consumers do not display active advanced filtering controls
while preserving existing behavior for server-paginated tables and the
fetch/filter flows around advancedFilters.tree.
- Around line 510-525: Update the advanced-filter facet aggregation flow around
treeToAggregateParams to remove the requested facet dimension, including source
and assignees, from the encoded filter tree before computing counts. Establish
one shared client/server contract for stripping the requested dimension, while
preserving all other advanced conditions and existing legacy-filter behavior.
- Around line 386-390: Remove synchronous setters from the effects in
LeadsTable: at src/components/dashboard/leads-table.tsx lines 386-390, derive
the SSR-prop values for localLeads and total during render while preserving
advancedFilterActive behavior; at lines 704-708, derive empty facet values when
facetFetchParams is null instead of clearing facet state in the effect. Update
the relevant reads to use these derived values and retain effect-driven updates
only where asynchronous side effects are required.
In `@src/components/filters/condition-defaults.ts`:
- Around line 79-84: Replace RELATIVE_DATE_PRESETS with direction-specific
preset maps for within_last and within_next, using the documented values 7d,
30d, 3m, and 1y from tree-to-aggregate-params.ts. Update filter-value-editor.tsx
to pass the appropriate map per operator and update chip-label.ts to use
direction-correct labels, avoiding undocumented 1d and 1m presets.
In `@src/components/filters/filter-value-editor.tsx`:
- Around line 114-129: Update the numeric onChange handlers in the between-range
and field.type === "number" branches of the filter value editor so clearing an
input preserves an empty draft value instead of converting it to 0. Pass the raw
input value or the component’s established empty-value representation through
onChange, while retaining numeric values for non-empty input and existing range
behavior.
In `@src/lib/filters/serialize.browser.test.ts`:
- Around line 63-72: Update the test case around encodeFilterTree so the encoded
input is produced directly with originalBuffer using the legacy server-side
Buffer base64url encoding, rather than calling encodeFilterTree. Keep the
subsequent deletion of globalThis.Buffer and decodeFilterTree assertion
unchanged.
In `@src/lib/filters/tree-to-aggregate-params.ts`:
- Around line 30-34: Update the date subtraction helper containing the `unit`
branch so month (`"m"`) and year (`"y"`) subtraction clamps to the target
calendar month’s last valid day instead of allowing `setMonth()` or
`setFullYear()` rollover; preserve day subtraction and time components, and
cover leap-day and month-end cases in tests.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 96c5f8a4-2e91-4ac7-ac50-33f84bb4fac3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.env.example.github/workflows/deploy-staging.ymlDockerfiledocs/ADVANCED-FILTERS-BRIEF.mdpackage.jsonsrc/app/(main)/api/v1/leads/route.test.tssrc/app/(main)/api/v1/leads/route.tssrc/components/dashboard/leads-table.tsxsrc/components/filters/add-filter-button.tsxsrc/components/filters/advanced-filter-bar.tsxsrc/components/filters/chip-label.tssrc/components/filters/condition-defaults.tssrc/components/filters/conjunction-toggle.tsxsrc/components/filters/filter-chip-row.tsxsrc/components/filters/filter-chip.tsxsrc/components/filters/filter-condition-editor.tsxsrc/components/filters/filter-field-picker.tsxsrc/components/filters/filter-operator-picker.tsxsrc/components/filters/filter-value-editor.tsxsrc/components/filters/types.tssrc/components/filters/use-filter-options.tssrc/components/ui/combobox.tsxsrc/components/ui/filter-menu.tsxsrc/components/ui/scroll-area.tsxsrc/lib/filters/compile.test.tssrc/lib/filters/compile.tssrc/lib/filters/registry/leads.test.tssrc/lib/filters/registry/leads.tssrc/lib/filters/serialize.browser.test.tssrc/lib/filters/serialize.tssrc/lib/filters/tree-to-aggregate-params.test.tssrc/lib/filters/tree-to-aggregate-params.tssrc/lib/filters/types.tssrc/lib/filters/use-advanced-filters.tssrc/lib/leads/aggregates.ts
Comment on lines
+544
to
+556
| ``` | ||
| AdvancedFilterBar advanced-filter-bar.tsx | ||
| ├── FilterChipRow → FilterChip click a chip to edit it in place | ||
| ├── ConjunctionToggle "Where / and / or" — hand-rolled 2-state, no toggle-group primitive | ||
| └── AddFilterButton "+ Add filter" | ||
| └── FilterFieldPicker Command + CommandInput + grouped items (reference screenshot 1) | ||
| └── FilterConditionEditor | ||
| ├── FilterOperatorPicker options from isOperatorAllowed() (reference screenshot 2) | ||
| └── FilterValueEditor dispatch on field.type + operator arity | ||
| ├── text · number (1 or 2 inputs for `between`) · date | ||
| ├── select · boolean | ||
| └── multi-select ← WRAPS the existing FilterOptionList | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Set a language for the component-tree code fence.
Line 544 opens an unlabeled fenced block. Markdownlint reports MD040. Use text for this component-tree diagram.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 544-544: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/ADVANCED-FILTERS-BRIEF.md` around lines 544 - 556, Label the
component-tree fenced code block in the documentation with the text language
identifier by changing its opening fence; leave the diagram content unchanged.
Source: Linters/SAST tools
Comment on lines
+511
to
+559
| // Effective per-axis filter values: from the ?f= tree (via treeToAggregateParams) | ||
| // when ?f= is present, else from the legacy toolbar params — exactly like the page | ||
| // query's own filterTree/legacyLeadsParamsToTree split above. `search` and the | ||
| // industry "__none__" sentinel have no tree-translation path (search is always | ||
| // `contains`, rejected above; is_empty is rejected above too) — both are legacy-only. | ||
| const effectiveStatus = rawFilterParam !== null ? (aggParams?.status ?? null) : status || null; | ||
| const assigneesIdsLegacy = assigneesTokens.filter((t) => t !== "unassigned" && UUID_RE.test(t)); | ||
| const wantsUnassignedLegacy = assigneesTokens.includes("unassigned"); | ||
| const effectiveAssigneesAny = rawFilterParam !== null ? (aggParams?.assigneesAny ?? null) : (assigneesIdsLegacy.length > 0 ? assigneesIdsLegacy : null); | ||
| const effectiveIncludeUnassigned = rawFilterParam !== null ? !!aggParams?.includeUnassigned : wantsUnassignedLegacy; | ||
| const validCollaboratorIdsLegacy = collaboratorIds.filter((id) => UUID_RE.test(id)); | ||
| const effectiveCollaboratorIds = rawFilterParam !== null ? (aggParams?.collaboratorIds ?? null) : (validCollaboratorIdsLegacy.length > 0 ? validCollaboratorIdsLegacy : null); | ||
| const effectiveTag = rawFilterParam !== null ? (aggParams?.tag ?? null) : (tagFilter && tagFilter !== "all" ? tagFilter : null); | ||
| const effectiveProspectIndustry = rawFilterParam !== null ? (aggParams?.prospectIndustry ?? null) : (industryFilter && industryFilter !== "all" && industryFilter !== "__none__" ? industryFilter : null); | ||
| const effectiveProspectIndustryNone = rawFilterParam !== null ? false : industryFilter === "__none__"; | ||
| const effectiveFormConfigId = rawFilterParam !== null ? (aggParams?.formConfigId ?? null) : (formFilter && formFilter !== "all" && UUID_RE.test(formFilter) ? formFilter : null); | ||
| const effectiveCreatedAfter = rawFilterParam !== null ? (aggParams?.createdAfter ?? null) : createdAfter; | ||
| const effectiveSearch = rawFilterParam !== null ? null : (search ? search.replace(/[,().]/g, "") : null); | ||
|
|
||
| const baseFacetParams = { | ||
| tenantId: auth.tenantId, | ||
| scope: facetScope, | ||
| user: facetScope === "own" ? scope.userId : null, | ||
| userBranchId: scope.userBranchId, | ||
| crossPoolSlug: scope.crossBranchPoolListSlug, | ||
| branchId: facetScope === "branch" ? scope.branchId : null, | ||
| sharedPoolAssignedToAny, | ||
| pipelineIds: auth.permissions.pipelineAccess !== "all" ? [...auth.permissions.pipelineAccess.ids] : null, | ||
| status: effectiveStatus, | ||
| collaboratorIds: effectiveCollaboratorIds, | ||
| tag: effectiveTag, | ||
| prospectIndustry: effectiveProspectIndustry, | ||
| prospectIndustryNone: effectiveProspectIndustryNone, | ||
| formConfigId: effectiveFormConfigId, | ||
| createdAfter: effectiveCreatedAfter, | ||
| // Mirror the page query's either/or (route.ts:309-316) exactly: an explicit list | ||
| // wins outright, a funnel's list set wins next, and the archive/staging exclusion | ||
| // only applies when neither is present. Passing more than one of these unconditionally | ||
| // ANDs them in lead_aggregates — for a staging list that becomes `list_id = X AND | ||
| // list_id NOT IN (…X…)`, an unsatisfiable predicate that zeroed the facet. | ||
| listIdEq: resolvedListId, | ||
| listIdAny: !resolvedListId && funnelListIds.length > 0 ? funnelListIds : null, | ||
| excludeListIds: | ||
| !resolvedListId && funnelListIds.length === 0 && excludeListIds.length > 0 | ||
| ? excludeListIds | ||
| : null, | ||
| search: effectiveSearch, | ||
| includeConverted, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve page-filter parity before returning legacy facet counts.
The page query applies assigned_to, stage, and legacy source filters. The aggregate parameters do not represent these filters. In particular, Lines 444-448 confirm that stage cannot reach lead_aggregates(). Facet options can therefore describe a different lead set than the table.
src/app/(main)/api/v1/leads/route.ts#L511-L559: Add equivalent aggregate support, or return the existingcounts: nullfallback when any legacy filter cannot be represented exactly.src/app/(main)/api/v1/leads/route.test.ts#L817-L900: Add regression cases forstage,assigned_to, and legacy source filters. Assert that each case either constrains the RPC correctly or returns the no-count fallback.
📍 Affects 2 files
src/app/(main)/api/v1/leads/route.ts#L511-L559(this comment)src/app/(main)/api/v1/leads/route.test.ts#L817-L900
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/`(main)/api/v1/leads/route.ts around lines 511 - 559, Update the
aggregate facet flow around baseFacetParams in
src/app/(main)/api/v1/leads/route.ts:511-559 to preserve parity with the page
query by representing legacy assigned_to, stage, and source filters in the
aggregate RPC, or return the existing counts:null fallback whenever any such
filter cannot be represented exactly. Add regression cases in
src/app/(main)/api/v1/leads/route.test.ts:817-900 covering stage, assigned_to,
and legacy source filters, asserting either the correct RPC constraints or the
no-count fallback.
Comment on lines
+366
to
+372
| const advancedFilterRegistry = useMemo( | ||
| () => leadFields({ tz: "UTC", now: new Date(0), industryId: industryId ?? null, permissions: {} } satisfies CompileCtx), | ||
| [industryId] | ||
| ); | ||
| const advancedFilters = useAdvancedFilters(advancedFilterRegistry); | ||
| const advancedFiltersEnabled = process.env.NEXT_PUBLIC_ADVANCED_FILTERS === "1"; | ||
| const advancedFilterActive = advancedFiltersEnabled && !isEmptyTree(advancedFilters.tree); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove hidden conditions before fetching and rendering.
useAdvancedFilters receives the full registry. advancedVisibleFields later removes role- and industry-restricted fields. A shared URL can retain a known but hidden condition, send it to /api/v1/leads, and omit its chip. The user cannot see or remove the active condition.
Decode and sanitize the tree against the same visible field registry that the bar receives.
Also applies to: 1916-1931
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/dashboard/leads-table.tsx` around lines 366 - 372, Sanitize
the advanced filter tree against the visible field registry before determining
activity, fetching leads, or rendering filter controls. Update the
useAdvancedFilters flow and the related leads fetch/render logic to use the same
advancedVisibleFields-filtered registry passed to the bar, removing role- and
industry-restricted conditions from decoded URL state so hidden filters cannot
affect requests or remain active without a visible chip.
Comment on lines
+371
to
+372
| const advancedFiltersEnabled = process.env.NEXT_PUBLIC_ADVANCED_FILTERS === "1"; | ||
| const advancedFilterActive = advancedFiltersEnabled && !isEmptyTree(advancedFilters.tree); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Limit advanced mode to supported data paths.
NEXT_PUBLIC_ADVANCED_FILTERS enables AdvancedFilterBar for every LeadsTable consumer. When serverPaginated is false, the fetch effect returns at Line 552 and filtered does not apply advancedFilters.tree. Contacts and leads-organise can show the new bar while its conditions do nothing.
Gate advanced mode with serverPaginated, or add local tree evaluation for those consumers.
Also applies to: 551-563, 895-945, 2037-2050
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/dashboard/leads-table.tsx` around lines 371 - 372, Restrict
advanced filter activation in LeadsTable to supported server-paginated data
paths by incorporating serverPaginated into advancedFiltersEnabled or the
equivalent AdvancedFilterBar/render gating. Ensure non-server-paginated
consumers do not display active advanced filtering controls while preserving
existing behavior for server-paginated tables and the fetch/filter flows around
advancedFilters.tree.
Comment on lines
+386
to
+390
| useEffect(() => { | ||
| if (advancedFilterActive) return; | ||
| setLocalLeads(leads); | ||
| setTotal(initialTotal ?? leads.length); | ||
| }, [leads, initialTotal, advancedFilterActive]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant component and project lint configs/rules.
printf 'Files:\n'
git ls-files | rg '(^|/)leads-table\.tsx$|(^|/)(eslint\.config\.(js|mjs|cjs)|package\.json|\.eslintrc|tsconfig\.json)$' || true
printf '\nRelevant component outline/sections:\n'
if [ -f src/components/dashboard/leads-table.tsx ]; then
wc -l src/components/dashboard/leads-table.tsx
sed -n '350,410p' src/components/dashboard/leads-table.tsx
sed -n '680,735p' src/components/dashboard/leads-table.tsx
fi
printf '\nLint config references:\n'
for f in $(git ls-files | rg '(^|/)(eslint\.config\.(js|mjs|cjs)|\.eslintrc.*|package\.json)$'); do
echo "--- $f"
rg -n "react-hooks|react-hooks\/set-state-in-effect|eslint-plugin|react" "$f" || true
doneRepository: Zunkireelabs/edgexcrm
Length of output: 7009
🌐 Web query:
eslint-plugin-react-hooks set-state-in-effect synchronous state updates in useEffect lint error
💡 Result:
The eslint-plugin-react-hooks rule react-hooks/set-state-in-effect is designed to prevent performance issues caused by synchronous state updates inside useEffect [1][2]. When you call setState synchronously within an effect, React is forced to perform an extra render pass because the component must re-render to apply the new state, followed by another potential effect execution [1][3]. This pattern often results in visual jank and inefficient double-rendering [1][2]. Key points regarding this lint error: Why it occurs: Synchronous updates in an effect happen after the initial render [1][2]. React must then re-render, apply DOM changes, and re-run effects, which is wasteful if the data could have been derived during the initial render or calculated from props [1][3]. Recommended alternatives: - Derive state during render: If the state is based on props or existing state, calculate the value directly in the component body instead of storing it in state [1][3]. - Use useReducer: For complex state transitions or when multiple state updates are related, useReducer can consolidate these into a single dispatch, avoiding multiple render triggers [4]. - Use initial state: If you are setting state based on a prop, consider initializing the state with that value or ensuring the component design does not require synchronization [1][2]. Exceptions and limitations: - Asynchronous updates: Calling setState inside an asynchronous function (e.g., after an await) is generally permitted because it does not trigger a synchronous cascading render [5][3]. - Ref-guarded updates: Updating state based on measurements (like using refs with useLayoutEffect) is often valid [6][2]. Note that the rule has faced challenges with false positives [6][5], and maintainers have been working to improve its ability to distinguish between problematic synchronous patterns and necessary use cases [6][5]. If you encounter this error, evaluate if the state update is truly necessary or if it can be achieved by refactoring your component to derive the value during render [1][4][2]. If you believe the warning is a false positive, verify that your state update is indeed asynchronous or necessary for a legitimate side effect that cannot be handled via derivation [6][5][3].
Citations:
- 1: https://react.dev/reference/eslint-plugin-react-hooks/lints/set-state-in-effect
- 2: https://uk.react.dev/reference/eslint-plugin-react-hooks/lints/set-state-in-effect
- 3: https://www.eslint-react.xyz/docs/rules/set-state-in-effect
- 4: https://stackoverflow.com/questions/79856121/calling-setstate-synchronously-within-an-effect-can-trigger-cascading-renders
- 5: [Compiler Bug]: react-hooks/set-state-in-effect false positive on async function react/react#34905
- 6: Bug: react-hooks/set-state-in-effect false positive on valid example in docs react/react#34858
Remove synchronous state updates from these effects. Both effects call setters synchronously, which is flagged by react-hooks/set-state-in-effect.
src/components/dashboard/leads-table.tsx#L386-L390: derive the SSR-prop sync values instead of updatinglocalLeadsandtotalin this effect.src/components/dashboard/leads-table.tsx#L704-L708: derive cleared facet values whenfacetFetchParamsis null instead of clearing both facet states in the effect.
🧰 Tools
🪛 ESLint
[error] 388-388: Error: Calling setState synchronously within an effect can trigger cascading renders
Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:
- Update external systems with the latest state from React.
- Subscribe for updates from some external system, calling setState in a callback function when external state changes.
Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect).
/home/jailuser/git/src/components/dashboard/leads-table.tsx:388:5
386 | useEffect(() => {
387 | if (advancedFilterActive) return;
388 | setLocalLeads(leads);
| ^^^^^^^^^^^^^ Avoid calling setState() directly within an effect
389 | setTotal(initialTotal ?? leads.length);
390 | }, [leads, initialTotal, advancedFilterActive]);
391 |
(react-hooks/set-state-in-effect)
📍 Affects 1 file
src/components/dashboard/leads-table.tsx#L386-L390(this comment)src/components/dashboard/leads-table.tsx#L704-L708
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/dashboard/leads-table.tsx` around lines 386 - 390, Remove
synchronous setters from the effects in LeadsTable: at
src/components/dashboard/leads-table.tsx lines 386-390, derive the SSR-prop
values for localLeads and total during render while preserving
advancedFilterActive behavior; at lines 704-708, derive empty facet values when
facetFetchParams is null instead of clearing facet state in the effect. Update
the relevant reads to use these derived values and retain effect-driven updates
only where asynchronous side effects are required.
Source: Linters/SAST tools
Comment on lines
+1928
to
+1970
| const advancedVisibleFields = useMemo( | ||
| () => Object.values(advancedFilterRegistry).filter((f) => !advancedVisibleFieldKeys.has(f.key)), | ||
| [advancedFilterRegistry, advancedVisibleFieldKeys] | ||
| ); | ||
|
|
||
| // Reuses the exact option arrays the legacy filterDefs above compute — same | ||
| // counselors/sources/forms/tags lists, just fed to a different UI. ADVANCED-FILTERS- | ||
| // BRIEF Phase 3 addendum §A: assignees/collaborators must carry counts AND hide | ||
| // zero-count people, matching the legacy filterDefs' "counselor"/"collaborator" | ||
| // entries above byte-for-byte — the bar must not ship with a visible count | ||
| // regression against the toolbar it replaces. | ||
| const advancedFilterOptionOverrides: Partial<Record<string, FilterOption[]>> = useMemo( | ||
| () => ({ | ||
| status: statusFilterOptions, | ||
| source: sources.map((s) => ({ value: s, label: `${s} (${(sourceCounts.get(s) ?? 0).toLocaleString()})` })), | ||
| assignees: [ | ||
| ...((counselorCounts.get("unassigned") ?? 0) > 0 | ||
| ? [{ value: "unassigned", label: `Unassigned (${(counselorCounts.get("unassigned") ?? 0).toLocaleString()})` }] | ||
| : []), | ||
| ...counselors | ||
| .filter(([userId]) => (counselorCounts.get(userId) ?? 0) > 0) | ||
| .map(([userId, email]) => ({ | ||
| value: userId, | ||
| label: `${memberNames[userId] || email.split("@")[0]} (${(counselorCounts.get(userId) ?? 0).toLocaleString()})`, | ||
| })), | ||
| ], | ||
| // Collaborator counts stay client-side, computed from `localLeads` (the current | ||
| // server page only) — deliberately, not silently: `lead_collaborators` is a join | ||
| // table with no existing `lead_aggregates()` dimension, and adding one is a | ||
| // bigger change than this addendum covers (ADVANCED-FILTERS-BRIEF Phase 3 | ||
| // addendum §C). Being explicitly page-scoped-and-labelled beats shipping a | ||
| // second, silently different meaning of the same-looking number. | ||
| collaborators: counselors | ||
| .filter(([userId]) => (collaboratorCounts.get(userId) ?? 0) > 0 && memberRoleMap[userId] !== "owner" && memberRoleMap[userId] !== "admin") | ||
| .map(([userId, email]) => ({ | ||
| value: userId, | ||
| label: `${memberNames[userId] || email.split("@")[0]} (${(collaboratorCounts.get(userId) ?? 0).toLocaleString()})`, | ||
| })), | ||
| tags: [{ value: "student", label: "Student" }], | ||
| industry: PROSPECT_INDUSTRIES.map((ind) => ({ value: ind.value, label: ind.label })), | ||
| form: formEntries.map(([id, name]) => ({ value: id, label: name })), | ||
| }), | ||
| [statusFilterOptions, sources, sourceCounts, counselorCounts, counselors, memberNames, collaboratorCounts, memberRoleMap, formEntries] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not expose destinations without option data.
advancedVisibleFields includes the registry's destinations multiselect field. It has no static options, and advancedFilterOptionOverrides does not provide one. useFilterOptions therefore returns an empty list and the value editor has no selectable values.
Provide an override or async loader for destinations, or hide the field until its values are available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/dashboard/leads-table.tsx` around lines 1928 - 1970, The
advanced filter registry currently exposes the destinations field without
selectable options. Update the advancedFilterOptionOverrides or the
corresponding useFilterOptions configuration to supply destinations values
through an existing override or async loader, and ensure advancedVisibleFields
does not include destinations while those values are unavailable.
Comment on lines
+79
to
+84
| export const RELATIVE_DATE_PRESETS = [ | ||
| { value: "1d", label: "Today" }, | ||
| { value: "7d", label: "Last 7 days" }, | ||
| { value: "30d", label: "Last 30 days" }, | ||
| { value: "1m", label: "This month" }, | ||
| ]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/lib/filters --items all --type function
rg -n -C 3 'within_last|within_next|"1d"|"1m"|"3m"|"1y"|RELATIVE_DATE' \
src/lib/filters src/components/filtersRepository: Zunkireelabs/edgexcrm
Length of output: 18362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/components/filters/condition-defaults.ts --items all --type function
sed -n '1,120p' src/components/filters/condition-defaults.ts
sed -n '1,70p' src/components/filters/filter-value-editor.tsx
sed -n '1,35p' src/components/filters/chip-label.ts
sed -n '220,250p' src/lib/filters/compile.ts
python3 - <<'PY'
from pathlib import Path
p = Path("src/components/filters/condition-defaults.ts").read_text()
start = p.index("export const RELATIVE_DATE_PRESETS = [")
end = p.index("];", start) + 2
block = p[start:end]
print(block)
# Simple parse objects { value: "x", label: "y" }
import re
values = []
for m in re.finditer(r'\{\s*value:\s*"([^"]+)"', block):
label = next((x.group(1) for x in re.finditer(r'label:\s*"([^"]+)"', block[m.start():]), None))
label = re.search(r'label:\s*"([^"]+)"', block[m.start():]).group(1) if label is None else label
values.append((m.group(1), label))
print("present_presets=", values)
compile = Path("src/lib/filters/compile.ts").read_text()
start = compile.index('case "within_last":')
end = compile.index('case "within_next":', start)
last = compile[start:end]
next_ = compile[end:end+200]
print("within_last_branch_contains=", f'subtractRelativeWindow({last})' in last)
print("within_next_branch_contains=", f'addRelativeWindow({next_})' in next_)
PYRepository: Zunkireelabs/edgexcrm
Length of output: 10423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- condition-defaults.ts ---\n'
sed -n '1,110p' src/components/filters/condition-defaults.ts
printf '\n--- filter-value-editor.tsx ---\n'
sed -n '1,75p' src/components/filters/filter-value-editor.tsx
printf '\n--- chip-label.ts ---\n'
sed -n '1,40p' src/components/filters/chip-label.ts
printf '\n--- compile.ts relative window helpers ---\n'
rg -n -C 4 'function (add|subtract)RelativeWindow|dayBoundsInTz|localMidnightUtc|nextCalendarDate' src/lib/filters/compile.ts
printf '\n--- parse preset values and compute window bounds from source definitions ---\n'
python3 - <<'PY'
from pathlib import Path
import re
compile = Path("src/lib/filters/compile.ts").read_text()
# Extract window branch implementations as readable source snippets
m = re.search(r'function (add|subtract)RelativeWindow\(.*?\n\}[ \n]*\n\}', compile, re.S)
if not m:
raise SystemExit("no window function found")
fn = m.group(0)
print(fn)
start = compile.index('function addRelativeWindow')
end = compile.index('\n}', start) + 2
snippet = compile[start:end]
print("uses_addLocalMidnight=", "addLocalMidnight" in snippet)
print("uses_addRelativeWindow=", "addRelativeWindow" in snippet)
print("adds_duration_units=", bool(re.search(r'(?:milliseconds|minutes|hours|days|weeks|months|years)\s*%', snippet)))
# Locate relative date presets
value_label = {}
for m in re.finditer(r'\{\s*value:\s*"([^"]+)"[\s\S]{0,200}?label:\s*"([^"]+)"', compile[compile.index('RELATIVE_DATE_PRESETS'):]):
value_label[m.group(1)] = m.group(2)
print("value_label_in_compile=", value_label)
PYRepository: Zunkireelabs/edgexcrm
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- condition-defaults.ts ---'
sed -n '1,110p' src/components/filters/condition-defaults.ts
echo
echo '--- filter-value-editor.tsx ---'
sed -n '1,75p' src/components/filters/filter-value-editor.tsx
echo
echo '--- chip-label.ts ---'
sed -n '1,40p' src/components/filters/chip-label.ts
echo
echo '--- compile.ts window helpers ---'
rg -n -C 4 'function (add|subtract)RelativeWindow|dayBoundsInTz|localMidnightUtc|nextCalendarDate' src/lib/filters/compile.ts
echo
echo '--- window implementation source ---'
python3 - <<'PY'
from pathlib import Path
import re
text = Path("src/lib/filters/compile.ts").read_text()
fn_name = "addRelativeWindow"
start = text.index(f"function {fn_name}")
brace = text.index("{", start)
depth = 0
end = None
for i in range(brace, len(text)):
if text[i] == "{":
depth += 1
elif text[i] == "}":
depth -= 1
if depth == 0:
end = i + 1
break
print(text[start:end])
PYRepository: Zunkireelabs/edgexcrm
Length of output: 12847
Use direction-specific presets that match the documented relative-date values.
filter-value-editor.tsx passes the same presets into both within_last and within_next, while chip-label.ts applies Last ... / This month labels to within_next. Split the preset maps by operator direction and include the documented values from src/lib/filters/tree-to-aggregate-params.ts (7d, 30d, 3m, 1y) instead of adding undocumented 1d and 1m values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/filters/condition-defaults.ts` around lines 79 - 84, Replace
RELATIVE_DATE_PRESETS with direction-specific preset maps for within_last and
within_next, using the documented values 7d, 30d, 3m, and 1y from
tree-to-aggregate-params.ts. Update filter-value-editor.tsx to pass the
appropriate map per operator and update chip-label.ts to use direction-correct
labels, avoiding undocumented 1d and 1m presets.
Comment on lines
+114
to
+129
| if (op === "between") { | ||
| const [min, max] = (Array.isArray(value) ? value : [0, 0]) as [number, number]; | ||
| return ( | ||
| <div className="flex items-center gap-1.5 p-1"> | ||
| <Input type="number" className="h-8 w-20 text-xs" value={min} onChange={(e) => onChange([Number(e.target.value), max])} /> | ||
| <span className="text-xs text-muted-foreground">and</span> | ||
| <Input type="number" className="h-8 w-20 text-xs" value={max} onChange={(e) => onChange([min, Number(e.target.value)])} /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (field.type === "number") { | ||
| return ( | ||
| <div className="p-1"> | ||
| <Input type="number" className="h-8 w-28 text-xs" value={typeof value === "number" ? value : ""} onChange={(e) => onChange(Number(e.target.value))} /> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not coerce an empty numeric input to zero.
Number(e.target.value) converts an empty input to 0. If a user clears a numeric condition, the controlled value immediately becomes zero and applying the condition filters for zero instead of an empty value.
Preserve the empty draft value until validation or apply handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/filters/filter-value-editor.tsx` around lines 114 - 129,
Update the numeric onChange handlers in the between-range and field.type ===
"number" branches of the filter value editor so clearing an input preserves an
empty draft value instead of converting it to 0. Pass the raw input value or the
component’s established empty-value representation through onChange, while
retaining numeric values for non-empty input and existing range behavior.
Comment on lines
+63
to
+72
| it("decodeFilterTree also works with no global Buffer, server-encoded input included", () => { | ||
| // Encode with Buffer present (server-side path), then decode with it absent — | ||
| // both directions of the "browser encodes, server decodes" contract must hold, | ||
| // and the codec must be identical either way (not two divergent implementations). | ||
| globalThis.Buffer = originalBuffer; | ||
| const tree: FilterTree = { conjunction: "and", conditions: [{ id: "c1", field: "status", op: "is", value: "new" }] }; | ||
| const encoded = encodeFilterTree(tree); | ||
| // @ts-expect-error — re-simulate the browser gap for the decode half. | ||
| delete globalThis.Buffer; | ||
| expect(decodeFilterTree(encoded)).toEqual({ ok: true, tree }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test actual legacy server encoding.
Line 69 still calls encodeFilterTree, which always uses the new browser-safe codec. Restoring Buffer does not test decoding URLs produced by the previous Buffer.from(...).toString("base64url") implementation.
Build the encoded value with originalBuffer before deleting globalThis.Buffer.
Proposed test correction
- globalThis.Buffer = originalBuffer;
const tree: FilterTree = { conjunction: "and", conditions: [{ id: "c1", field: "status", op: "is", value: "new" }] };
- const encoded = encodeFilterTree(tree);
+ const encoded = originalBuffer
+ .from(JSON.stringify(tree), "utf8")
+ .toString("base64url");
// `@ts-expect-error` — re-simulate the browser gap for the decode half.
delete globalThis.Buffer;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("decodeFilterTree also works with no global Buffer, server-encoded input included", () => { | |
| // Encode with Buffer present (server-side path), then decode with it absent — | |
| // both directions of the "browser encodes, server decodes" contract must hold, | |
| // and the codec must be identical either way (not two divergent implementations). | |
| globalThis.Buffer = originalBuffer; | |
| const tree: FilterTree = { conjunction: "and", conditions: [{ id: "c1", field: "status", op: "is", value: "new" }] }; | |
| const encoded = encodeFilterTree(tree); | |
| // @ts-expect-error — re-simulate the browser gap for the decode half. | |
| delete globalThis.Buffer; | |
| expect(decodeFilterTree(encoded)).toEqual({ ok: true, tree }); | |
| it("decodeFilterTree also works with no global Buffer, server-encoded input included", () => { | |
| // Encode with Buffer present (server-side path), then decode with it absent — | |
| // both directions of the "browser encodes, server decodes" contract must hold, | |
| // and the codec must be identical either way (not two divergent implementations). | |
| const tree: FilterTree = { conjunction: "and", conditions: [{ id: "c1", field: "status", op: "is", value: "new" }] }; | |
| const encoded = originalBuffer | |
| .from(JSON.stringify(tree), "utf8") | |
| .toString("base64url"); | |
| // `@ts-expect-error` — re-simulate the browser gap for the decode half. | |
| delete globalThis.Buffer; | |
| expect(decodeFilterTree(encoded)).toEqual({ ok: true, tree }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/filters/serialize.browser.test.ts` around lines 63 - 72, Update the
test case around encodeFilterTree so the encoded input is produced directly with
originalBuffer using the legacy server-side Buffer base64url encoding, rather
than calling encodeFilterTree. Keep the subsequent deletion of globalThis.Buffer
and decodeFilterTree assertion unchanged.
Comment on lines
+30
to
+34
| const result = new Date(now); | ||
| if (unit === "d") result.setDate(result.getDate() - amount); | ||
| else if (unit === "m") result.setMonth(result.getMonth() - amount); | ||
| else result.setFullYear(result.getFullYear() - amount); | ||
| return result; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '^src/lib/filters/tree-to-aggregate-params\.ts$|^src/app/\(main\)/api/v1/leads/route\.ts$' || true
echo "== file outline =="
ast-grep outline src/lib/filters/tree-to-aggregate-params.ts || true
echo "== file contents =="
cat -n src/lib/filters/tree-to-aggregate-params.ts
echo "== JS Date behavior probe =="
node - <<'JS'
const cases = [
["March 31 minus 1m", "2020-03-31", "d", 1],
["Feb 29 minus 1y", "2020-02-29", "y", 1],
["March 31 minus 31d", "2020-03-31", "d", 31],
];
for (const [label, iso, unit, amount] of cases) {
const now = new Date(iso);
const result = new Date(now);
if (unit === "d") result.setDate(result.getDate() - amount);
else if (unit === "m") result.setMonth(result.getMonth() - amount);
else result.setFullYear(result.getFullYear() - amount);
console.log(label, iso, "=>", result.toISOString().slice(0,10), result.toString());
}
JS
echo "== usages of tree-to-aggregate-params =="
rg -n "tree-to-aggregate-params|within_last|createdAfter" src/lib src/app -S || trueRepository: Zunkireelabs/edgexcrm
Length of output: 15063
Clamp calendar month and year subtraction.
When now is a date that does not exist in the target month, setMonth() and setFullYear() roll forward instead of keeping the target calendar boundary. For example, 2020-03-31T... 1m becomes 2020-03-30T..., so createdAfter excludes a valid last-day record. Use clamped calendar subtraction for m and y, and cover leap-day and last-day edge cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/filters/tree-to-aggregate-params.ts` around lines 30 - 34, Update the
date subtraction helper containing the `unit` branch so month (`"m"`) and year
(`"y"`) subtraction clamps to the target calendar month’s last valid day instead
of allowing `setMonth()` or `setFullYear()` rollover; preserve day subtraction
and time components, and cover leap-day and month-end cases in tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
No migrations in this promote.
Test plan
Summary by CodeRabbit
New Features
Bug Fixes