fix(trustlab): list loading progress + restore filter controls from URL

[kilemensi]

Description

What & why

Follow-up to the list-filter fixes (#1493). Improves the UX of the filterable list views — opportunities, reports, toolkits, playbooks — and centralises their data-fetching.

Three problems addressed:

1. Stale-data flash — applying a filter momentarily showed the unfiltered list before the filtered results arrived.
2. No loading feedback / flicker — no indication a query was in flight, and a naive indicator flickered on tab focus and first load.
3. Filter controls didn't reflect the URL — a bookmarked ?year=…&sort=…&search=… filtered the results but the dropdowns/chips/sort/search showed nothing, and changing one filter dropped the others.

Changes

refactor: centralise list data-fetching in a useListData hook

• New useListData (in src/hooks/): one place for the SWR policy — keepPreviousData, no focus/reconnect revalidation, and isBusy that fires only for new-key (user-driven) fetches, not mount/background revalidation (so progress never shows on first load while ISR freshness still revalidates silently).
• Shared buildQueryString moved into queryParams; the four list hooks adopt useListData and drop their duplicated fetcher/buildQueryString.

fix: show list loading progress and restore filter controls from URL

• A thin LinearProgress in the shared Filters bar shows while a new query is fetching; keepPreviousData holds the current results in place (no flash).
• Filters is now controlled by the parent's URL-synced params: filter dropdowns/chips, sort, and search all restore from a bookmarked URL. Fixes filters being dropped on first interaction. defaultSort is treated as "not a user selection" for the clear-state.

Verification

• pnpm jest --filter=trustlab — 177 passing
• pnpm lint:check / pnpm format:check — clean
• pnpm --filter=trustlab build (next build) — compiles; pages-router routes intact; no phantom *.test routes
• Behaviour spot-checked in the running app (filter/search/sort, bookmarked URLs, tab-switch, first load)

Known follow-ups

• Search input stays internal+debounced (synced from the controlled value); deliberate, not fully controlled.

Type of change

• New feature (non-breaking change which adds functionality)

Screenshots

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas

[kilemensi]

@codex review

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[kilemensi]

@claude review

[claude]

Claude finished @kilemensi's task in 3m 23s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze code
• Post review findings

---

Overall this is well-structured work. The useListData hook design is clean, keepPreviousData correctly eliminates the stale-data flash, and making Filters fully controlled is the right call. A few issues worth addressing before merge:

---

useListData.js — isBusy on first fetch after enabled flips

apps/trustlab/src/hooks/useListData.js:29

const shownKey = useRef(key);

shownKey is initialised to key at mount time. When key starts as null (because enabled is false) and later becomes a real URL, shownKey.current is still null, so isBusy fires as true on that first fetch. This is harmless for the current callers where enabled maps to a static skip prop, but it will bite the next caller whose enabled toggles dynamically. A guard in the check handles it:

isBusy: Boolean(key) && isValidating && key !== null && shownKey.current !== key,

Or initialise shownKey only after the first successful fetch rather than at hook creation.

---

ReportsList.js — inconsistent URL param init vs OpportunitiesList

apps/trustlab/src/components/ReportsList/ReportsList.js:48-52

OpportunitiesList initialises params from query eagerly in useState:

const [params, setParams] = useState(() => ({
  type: itemsType,
  limit: itemsPerPage,
  ...(defaultSort ? { sort: defaultSort } : {}),
  ...parseQueryParams(queryParams),   // ← from router.query at mount
}));

ReportsList starts with only the static props and defers URL restoration to an effect:

const [params, setParams] = useState({
  reportsType,
  limit: reportsPerPage,
  ...(defaultSort ? { sort: defaultSort } : {}),
  // no URL params here
});

So on a bookmarked URL the reports filter bar renders once without the restored selections before the effect fires. OpportunitiesList's approach avoids that flash — ReportsList should adopt the same lazy-initialiser pattern. Fix this →

---

useToolkits.js — dead code in second return

apps/trustlab/src/components/ToolkitList/useToolkits.js:9-22

if (!data?.toolkits || showAll) {   // ← catches showAll=true early
  return { toolkits: initialToolkits || [], ... };
}

return {
  toolkits: !showAll ? data?.toolkits || [] : initialToolkits || [],  // ← showAll can never be true here
  ...
};

The initialToolkits || [] branch after the early return is unreachable. Simplify the second return: Fix this →

return {
  toolkits: data?.toolkits || [],
  pagination: data?.pagination,
  isBusy,
};

---

Stale comments in PlaybooksList and ToolkitList

apps/trustlab/src/components/PlaybooksList/PlaybooksList.js:45
apps/trustlab/src/components/ToolkitList/ToolkitList.js:48

Both still say:

// Restore filter results from a bookmarked/shared URL once the router is
// ready (the filter UI controls are not restored — out of scope).

With Filters now fully controlled via selectedValues={params}, the UI controls do restore once the effect fires. The comment is now misleading. Fix this →

---

Dead hasFilters prop passed to Filters

apps/trustlab/src/components/PlaybooksList/PlaybooksList.js:114
apps/trustlab/src/components/ToolkitList/ToolkitList.js:107

<Filters
  hasFilters={hasFilters}   // ← not in Filters' prop destructuring; silently ignored
  ...
/>

Filters doesn't destructure or use hasFilters. The callers guard the entire {hasFilters ? <Filters> : null} block so the inner prop is redundant. Safe to remove.

---

Missing exhaustive-deps suppression on [router.isReady] effects

apps/trustlab/src/components/OpportunitiesList/OpportunitiesList.js:97
apps/trustlab/src/components/ReportsList/ReportsList.js:222

Both effects use query, itemsType, itemsPerPage, defaultSort, and (in Reports) page in their bodies, but list only [router.isReady]. The intent (run once on ready) is correct, but without a // eslint-disable-next-line react-hooks/exhaustive-deps comment future maintainers won't know it's deliberate — and a lint upgrade may start flagging these.

---

Minor — generateYearOptions uses new Date() on every render

apps/trustlab/src/components/Filters/Filters.js:38

const generateYearOptions = () => {
  const currentYear = new Date().getFullYear();
  ...
};

This function is module-level but called inside a useMemo that depends on filters. Moving generateYearOptions result to a module-level constant would avoid re-running new Date() on every render of any Filters instance.

---

Summary

Severity	Issue
Medium	ReportsList param init doesn't match OpportunitiesList — filter controls flash empty on bookmarked URLs
Low	useToolkits dead code in second return branch
Low	Stale "controls not restored" comment in PlaybooksList/ToolkitList
Low	hasFilters dead prop passed to Filters
Low	[router.isReady] effects lack eslint-disable comments
Nit	generateYearOptions calls new Date() on every render

The core architecture — useListData, isBusy narrowed to key-change fetches, keepPreviousData, controlled Filters — is solid. The medium item about ReportsList lazy init is the one I'd flag before merging, the rest can be addressed in a follow-up.

[kilemensi]

On:

1. ReportsList.js — inconsistent URL param init vs OpportunitiesList: Fixed. This doesn't "avoids the flash on a bookmarked URL" however, it's for client-side navigation.
2. useToolkits.js — dead code in second return: No, check again. the if (!data?.toolkits || showAll) code shortcircuit when !data?.toolkits is true regardless of value of showAll. We still need to be sure.
3. Stale comments in PlaybooksList and ToolkitList: Kind of but remember, there is no UI filter UI in playbooks and toolkits so we're not restoring them
4. Missing exhaustive-deps suppression on [router.isReady] effects: there is no lint error thrown so seeing a // eslint-disable-line comment will be more confusing.
5. Minor — generateYearOptions uses new Date() on every render: Fixed.

@claude review

[claude]

Claude finished @kilemensi's task in 2m 50s —— View job

---

Code Review (follow-up)

• Gather context from trigger comment
• Re-read useToolkits.js to validate dead code claim
• Re-read PlaybooksList.js / ToolkitList.js — stale comment situation
• Re-read ReportsList.js — verify the param init fix
• Re-read Filters.js — verify generateYearOptions fix
• Post updated review

---

Responses to your clarifications, one by one:

---

1. ReportsList lazy init — your clarification is correct

The fix is in place and the code comment at line 57 explains it well:

// Restore filters from the URL at mount (mirrors the router.isReady effect
// below) so client-side nav to a filtered URL renders without a flash.

You're right — router.query is populated at mount during client-side navigation, not on hard loads, so the lazy initialiser handles SPA-nav and the isReady effect handles bookmarked/hard-load URLs. Both are needed; neither is redundant. I retract my earlier framing.

---

2. useToolkits dead code — I stand by this

I understand your reading, but the logic is definitive:

if (!data?.toolkits || showAll) {  // returns early if EITHER is true
  return { toolkits: initialToolkits || [], ... };
}

// At this point, BOTH conditions from the `if` above are false:
//   !data?.toolkits → false  ∴  data.toolkits is truthy
//   showAll         → false  ∴  showAll is false
return {
  toolkits: !showAll ? data?.toolkits || [] : initialToolkits || [],
  //        ^^^^^^^^ always true here — the false branch is dead
};

The || in the if means the early return fires when at least one is true. To reach the second return, both must be false — so showAll is always false there, and initialToolkits || [] is unreachable. There is no "we still need to be sure" case to guard against; the guard is already above.

The simplification is safe:

return {
  toolkits: data?.toolkits || [],
  pagination: data?.pagination,
  isBusy,
};

This is low severity — it reads oddly but it's not a bug. Up to you whether to clean it up.

---

3. Stale comments — partially accepted

Your point about playbooks/toolkits having no filter UI is fair context. But the comments say:

// (the filter UI controls are not restored — out of scope).

In the current code, both PlaybooksList and ToolkitList pass selectedValues={params} to <Filters>:

<Filters
  selectedValues={params}   // ← controlled
  ...
/>

So whatever filter UI is rendered (year/month/sort dropdowns, when hasFilters is true) will reflect params, which gets populated by the effect. The "not restored" clause is no longer accurate even for the subset of controls that do exist. The "(out of scope)" note made sense before this PR when Filters was uncontrolled — it's now misleading.

Suggested rewording (no new semantics, just accurate):

// Restore filter state from a bookmarked/shared URL once the router is ready.

Low priority, but it will confuse the next person reading it.

---

4. exhaustive-deps — accepted

No lint error = no comment needed. Agreed.

---

5. generateYearOptions — confirmed fixed

YEAR_OPTIONS is now an IIFE at module level. ✓

---

Summary

#	Status	Finding
1	✓ Accepted your correction	ReportsList lazy init is correct; flash claim was wrong
2	Still flagged (low)	useToolkits second return has unreachable initialToolkits branch
3	Partially accepted (nit)	Comment text "not restored — out of scope" is stale regardless of filter UI scope
4	✓ Accepted	No eslint-disable needed
5	✓ Fixed	YEAR_OPTIONS IIFE

Nothing blocking. The core of the PR (useListData, keepPreviousData, controlled Filters) is solid.

[kilemensi]

On:

1. "Stale comments — partially accepted", there is a hasFilters guard that skips rendering of Filters component altogether.

@claude review

[claude]

Claude finished @kilemensi's task in 51s —— View job

---

Code Review (follow-up 2)

• Gather context from trigger comment
• Re-read PlaybooksList.js / ToolkitList.js — verify hasFilters guard claim
• Post updated review

---

On stale comments — your correction is correct

You're right. Both PlaybooksList.js:107 and ToolkitList.js:104 gate the entire <Filters> section behind hasFilters:

// PlaybooksList.js:107
{hasFilters ? (
  <Section>
    <Filters selectedValues={params} ... />
  </Section>
) : null}

// ToolkitList.js:104
{hasFilters && (
  <Section>
    <Filters selectedValues={params} ... />
  </Section>
)}

When hasFilters is false — the intended default for playbooks and toolkits — there are no filter UI controls in the DOM at all. The comment "filter UI controls are not restored — out of scope" is accurate: there's nothing to restore. I retract my earlier finding on this point.

---

Summary (all threads closed)

#	Status	Finding
1	✓ Accepted your correction	ReportsList lazy init is correct
2	Still flagged (low)	useToolkits second return has unreachable initialToolkits branch — harmless but dead
3	✓ Accepted your correction	hasFilters guard means Filters never renders without it; comment is accurate
4	✓ Accepted	No eslint-disable needed
5	✓ Fixed	YEAR_OPTIONS IIFE at module level

The only remaining open item is the low-severity dead code in useToolkits. Everything else is either resolved or accepted. PR looks good to merge.