fix(views): resolve useEffect anti-patterns across Views and Audit-Report components

[Copilot]

• Anti-pattern 1 & 2 (ViewPage.tsx): Add AbortController cleanup; remove redundant viewId state
• Anti-pattern 3 (ViewsPage.tsx): Replace Effect that clears editedRow with inline close handlers
• Anti-pattern 4 (TimeseriesPanel.tsx): Replace useState+useEffect with useMemo
• Anti-pattern 5 (GlobalFiltersForm.tsx & ViewTableFilterForm.tsx): Initialise Formik with URL-derived initialValues
• Fix build error: restore useEffect import in TimeseriesPanel.tsx
• Resolve PR conflict: restore EMPTY_VARIABLES
• Review fix: clear stale lookup state (fetchedId/isLoading/error) in ViewPage.tsx when direct id param is present
• Review fix: add filterFields to ViewTableFilterForm useEffect dependency array (remove eslint-disable)
• Review fix: revert adding variables to GlobalFiltersForm useEffect dep array – maintainer confirmed this cannot be done (would cause infinite re-renders if caller passes a new array ref each render)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Views | useEffect() anti-patterns</issue_title>
<issue_description>High-confidence anti-patterns (per the article)

1. Fetching in useEffect without cleanup (race condition risk)
   • File: src/pages/views/ViewPage.tsx:28
   • The async fetch for view ID (getViewIdByName / getViewIdByNamespaceAndName) has no ignore/cancel cleanup.
   • Matches the docs’ “Fetching data without cleanup logic” anti-pattern.
2. Redundant derived state from route params
   • File: src/pages/views/ViewPage.tsx:28 (setViewId(id))
   • viewId is being synced from id in an Effect.
   • Matches “Updating state based on props or state” (redundant state + extra render).
3. Resetting related state in an Effect instead of in the event
   • File: src/pages/views/ViewsPage.tsx:110
   • editedRow is cleared when isOpen becomes false via Effect.
   • Matches “Adjusting state when prop/state changes in Effect” / event logic better handled in close handlers.
4. Derived render data stored in state via Effects
   • File: src/pages/audit-report/components/View/panels/TimeseriesPanel.tsx:163,169
   • cachedDomain and cachedTicks are derived from timeRange/containerWidth but set via Effects.
   • Matches “If you can calculate during render, don’t use Effect/state” and “useMemo for caching calculations”.
5. Effect chain syncing form state ↔ URL state
   • Files:
     - src/pages/audit-report/components/View/GlobalFiltersForm.tsx:24,41
     - src/pages/audit-report/components/View/ViewTableFilterForm.tsx:25,60
   • One Effect writes URL params from form values; another reads URL params back into form values.
   • This is close to the docs’ “Chains of computations” smell (extra render churn, harder reasoning).

Generated by Codex5.3 using https://react.dev/learn/you-might-not-need-an-effect#subscribing-to-an-external-store as a reference</issue_description>

<agent_instructions>Go through the 5 anti-patterns mentioned in the Github issue and create a pull request. Make sure to make one commit per bullet point</agent_instructions>

Comments on the Issue (you are @copilot in this section)

• Fixes Views | useEffect() anti-patterns #2946

---

💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

Summary by CodeRabbit

• Refactor
  • Improved form and filter state synchronization with URL parameters for more reliable consistency.
  • Optimized rendering performance in data visualizations through memoization.
  • Enhanced modal dialog state handling to prevent stale UI states.
• Bug Fixes
  • Made lookups and remote requests more robust by adding cancellation and better stale-state handling.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
aws-preview	Ready	Preview	Mar 24, 2026 10:08am
flanksource-ui	Ready	Preview	Mar 24, 2026 10:08am

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[adityathebe]

@copilot fix the build error

[Copilot]

@copilot fix the build error

Fixed in c2dd268. When I replaced the two cachedDomain/cachedTicks useEffect calls with useMemo, I also accidentally removed useEffect from the import — but the container-width measurement effect still needs it. The import is now restored.

[adityathebe]

@copilot rebase with main and resolve conflicts

[Copilot]

@copilot rebase with main and resolve conflicts

Done in 5154ccd. Merged origin/main (v1.4.217) into the branch and resolved the conflict in GlobalFiltersForm.tsx by keeping our anti-pattern fix (removing currentVariables from GlobalFiltersListener since the outer component now passes correct initialValues to Formik).

[adityathebe]

@copilot I'm still seeing conflicts

[Copilot]

@copilot I'm still seeing conflicts

Fixed in 2a9efa5. The conflict was a true 3-way merge issue: main changed currentVariables = {} → currentVariables = EMPTY_VARIABLES, while our branch had deleted that line. I've restored EMPTY_VARIABLES and kept currentVariables = EMPTY_VARIABLES in the GlobalFiltersListener props (matching main's change exactly), so GitHub's 3-way merge now sees both sides making the same change. The variable is accepted but not used in the body — the parent GlobalFiltersForm already handles URL→form initialisation via initialValues. The PR is no longer dirty.

[adityathebe]

@coderabbitai review one more time

[coderabbitai]

@adityathebe Sure! I'll re-review the entire PR right away.

✅ Actions performed

Full review triggered.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/pages/audit-report/components/View/ViewTableFilterForm.tsx (1)

68-97: ⚠️ Potential issue | 🟠 Major

Hoist the empty defaultFieldValues fallback to a module-level constant.

defaultFieldValues is included in the useMemo dependency array (line 89). Both call sites (View.tsx:297 and the unit test) pass inline objects like defaultFieldValues={{}}, which are new references on every parent render. This causes the memo to recalculate unnecessarily on each parent render. With enableReinitialize=true, Formik may reset the form state even when values haven't changed. Move the empty object to a module-level constant to prevent this.

🔧 Suggested fix

+const EMPTY_FIELD_VALUES: Record<string, string> = {};
+
 export default function ViewTableFilterForm({
   children,
   filterFields,
-  defaultFieldValues = {},
+  defaultFieldValues = EMPTY_FIELD_VALUES,
   tablePrefix
 }: ViewTableFilterFormProps) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/audit-report/components/View/ViewTableFilterForm.tsx` around lines
68 - 97, The memo is recalculating because callers pass inline empty objects for
the defaultFieldValues prop; hoist the empty fallback to a module-level constant
and use that constant as the default value for the defaultFieldValues param in
ViewTableFilterForm so the reference is stable. Update the component signature
to default defaultFieldValues to the new constant, keep useMemo([tableParams,
filterFields, defaultFieldValues]) as-is, and ensure tests/call sites can still
pass overrides; this prevents unnecessary recomputation and Formik
reinitialization while preserving current logic in useMemo and
enableReinitialize.

src/pages/audit-report/components/View/GlobalFiltersForm.tsx (1)

58-93: ⚠️ Potential issue | 🟠 Major

Stabilize the params snapshot before feeding enableReinitialize.

usePrefixedSearchParams creates a new URLSearchParams reference on every router search params change (via filterPrefixedParams). Because initialValues depends on that object directly, every form→URL write rebuilds the Formik input and causes enableReinitialize to recalculate unnecessarily. While Formik's deep-equality check prevents spurious resets when the URL reflects the current form values, this approach is fragile and inefficient. Use a stable value-based dependency (globalParams.toString()) instead of the URLSearchParams reference.

🔧 Proposed fix

   const [globalParams] = usePrefixedSearchParams(globalVarPrefix);
+  const globalParamsKey = globalParams.toString();
 
   const initialValues = useMemo<Record<string, string>>(() => {
+    const params = new URLSearchParams(globalParamsKey);
     const values: Record<string, string> = {};
     variables.forEach((variable) => {
-      const urlValue = globalParams.get(variable.key);
+      const urlValue = params.get(variable.key);
       const currentValue = currentVariables?.[variable.key];
       const optionItems = variable.optionItems ?? [];
       const defaultValue =
@@
     });
     return values;
-  }, [globalParams, variables, currentVariables]);
+  }, [globalParamsKey, variables, currentVariables]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/audit-report/components/View/GlobalFiltersForm.tsx` around lines 58
- 93, The initialValues useMemo currently depends on the URLSearchParams
reference returned by usePrefixedSearchParams (globalParams), which changes
identity on every router update and forces unnecessary re-evaluation; change the
dependency to a stable, value-based snapshot by using globalParams.toString()
(or an equivalent stringified value) in the useMemo dependency array and use
that snapshot when computing values so that initialValues only recomputes when
the actual query string content changes, keeping enableReinitialize stable;
update the dependency list from [globalParams, variables, currentVariables] to
[globalParams.toString(), variables, currentVariables] and reference the
stringified snapshot when needed.

♻️ Duplicate comments (1)

src/pages/views/ViewPage.tsx (1)

39-45: ⚠️ Potential issue | 🟡 Minor

Scope error and the first-paint loading state to lookupKey too.

fetchedId is route-scoped now, but the render still reads the previous route's raw error/isLoading. That means /view/:name can briefly hit the "No view identifier provided" branch before the effect flips loading, and a failed lookup can still flash its stale error during the next /view/:id navigation. Please key those render guards to the current route as well.

Possible direction

   const scopedFetchedId =
     lookupKey && fetchedLookupKey === lookupKey ? fetchedId : undefined;
+  const routeError =
+    id || !error
+      ? null
+      : lookupKey
+        ? fetchedLookupKey === lookupKey
+          ? error
+          : null
+        : error;
+  const lookupPending = !id && !!lookupKey && fetchedLookupKey !== lookupKey;
 
   // Derived: prefer the direct `id` param; fall back to whatever we fetched.
   const viewId = id ?? scopedFetchedId;

Then render from isLoading || lookupPending and routeError below instead of the raw state.

Please verify by navigating /view/:id -> /view/:name and /view/missing -> /view/:id; neither flow should flash the empty/error state anymore.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/views/ViewPage.tsx` around lines 39 - 45, Scope the raw lookup
state to the current route like you did for fetchedId: derive scopedError and
scopedIsLoading using the same check (e.g. scopedError = lookupKey &&
fetchedLookupKey === lookupKey ? error : undefined; scopedIsLoading = lookupKey
&& fetchedLookupKey === lookupKey ? isLoading : false), then compute
routeError/routeIsLoading (or reuse those names) and use routeIsLoading ||
lookupPending and routeError in the render guards instead of the raw
isLoading/error; keep viewId as id ?? scopedFetchedId so the identifier logic
remains unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/pages/audit-report/components/View/GlobalFiltersForm.tsx`:
- Around line 58-93: The initialValues useMemo currently depends on the
URLSearchParams reference returned by usePrefixedSearchParams (globalParams),
which changes identity on every router update and forces unnecessary
re-evaluation; change the dependency to a stable, value-based snapshot by using
globalParams.toString() (or an equivalent stringified value) in the useMemo
dependency array and use that snapshot when computing values so that
initialValues only recomputes when the actual query string content changes,
keeping enableReinitialize stable; update the dependency list from
[globalParams, variables, currentVariables] to [globalParams.toString(),
variables, currentVariables] and reference the stringified snapshot when needed.

In `@src/pages/audit-report/components/View/ViewTableFilterForm.tsx`:
- Around line 68-97: The memo is recalculating because callers pass inline empty
objects for the defaultFieldValues prop; hoist the empty fallback to a
module-level constant and use that constant as the default value for the
defaultFieldValues param in ViewTableFilterForm so the reference is stable.
Update the component signature to default defaultFieldValues to the new
constant, keep useMemo([tableParams, filterFields, defaultFieldValues]) as-is,
and ensure tests/call sites can still pass overrides; this prevents unnecessary
recomputation and Formik reinitialization while preserving current logic in
useMemo and enableReinitialize.

---

Duplicate comments:
In `@src/pages/views/ViewPage.tsx`:
- Around line 39-45: Scope the raw lookup state to the current route like you
did for fetchedId: derive scopedError and scopedIsLoading using the same check
(e.g. scopedError = lookupKey && fetchedLookupKey === lookupKey ? error :
undefined; scopedIsLoading = lookupKey && fetchedLookupKey === lookupKey ?
isLoading : false), then compute routeError/routeIsLoading (or reuse those
names) and use routeIsLoading || lookupPending and routeError in the render
guards instead of the raw isLoading/error; keep viewId as id ?? scopedFetchedId
so the identifier logic remains unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8eab1178-4f32-4ff6-a000-f427381f43a0

📥 Commits

Reviewing files that changed from the base of the PR and between c27d7ed and db6738f.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (6)

• src/api/services/views.ts
• src/pages/audit-report/components/View/GlobalFiltersForm.tsx
• src/pages/audit-report/components/View/ViewTableFilterForm.tsx
• src/pages/audit-report/components/View/panels/TimeseriesPanel.tsx
• src/pages/views/ViewPage.tsx
• src/pages/views/ViewsPage.tsx