Skip to content

fix(OneFilterPicker): track nested filter keys when options load async - #5270

Open
factmarc10010 wants to merge 3 commits into
mainfrom
fix/nested-filter-keys-async-options
Open

fix(OneFilterPicker): track nested filter keys when options load async#5270
factmarc10010 wants to merge 3 commits into
mainfrom
fix/nested-filter-keys-async-options

Conversation

@factmarc10010

@factmarc10010 factmarc10010 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Problem

A nested in filter exposed its child filter keys only when its options were a literal array. Any filter that loads its options from a function (or a source) resolved to no nested keys, so the picker treated the parent as empty everywhere it is summarised outside the option list:

Where Symptom
FilterList Parent gets no active dot when only a child is selected — the reported bug
InFilter Child selections missing from the "N selected" count and the select-all indeterminate state
InFilter Parent's clear / select-all toggle leaves the child values behind
OneFilterPicker.removeFilterValue Removing the parent chip orphans the child values: chip gone, list still filtered
getClearedFiltersValue Same call — masked today only when the child key is also declared as its own filter

Repro in the product: People → Filters → Workplace, select only a work area under an office. The office row gets its dot (that path reads the already-loaded options), the Workplace entry in the filter list does not.

Screenshot 2026-08-26 at 16 57 44

Fix

The nested keys have to be knowable without the options at hand, because the filter list and the chips render before the filter is ever opened — and still render that way after a reload with filters in the URL.

  • New optional nestedFilterKeys on the in-filter options, declaring the child keys up front. Authoritative and always available, including for source-backed options.
  • collectNestedFilterKeys now takes the whole schema, so when options is not a literal array it can also fall back to the options a previous load already resolved.
  • Inside InFilter the keys additionally union with the currently loaded options, so they pick up children that were still unresolved when the schema was read.

Consumers with literal arrays keep working untouched; nestedFilterKeys is only needed for async/source options.

options: {
  cache: true,
  nestedFilterKeys: ["workArea"],
  options: loadWorkplaces,
}
Screenshot 2026-08-26 at 17 04 27 Screenshot 2026-08-26 at 17 03 09

Testing

packages/react/src/patterns/OneFilterPicker/__test__/nestedAsyncOptions.test.tsx — nested filter with async options, covering the four behaviours above end to end. All four fail on main.

.../InFilter/components/__tests__/option-utils.test.ts — unit coverage for the three key sources (declared, literal, resolved-from-cache) and their merge.

The existing nested tests all use literal arrays, which is why this slipped through.

Ref: https://factorialmakers.atlassian.net/browse/FCT-61539

A nested `in` filter only exposed its child filter keys when its options
were a literal array, so a filter whose options arrive from a loader
resolved to no nested keys at all. The parent then read as empty
everywhere it is summarised outside the option list: no active dot in the
filter list, no contribution to the "N selected" count or the select-all
indeterminate state, nothing cleared by its clear/select-all toggle, and
an orphaned child value left behind when its chip is removed.

The keys have to be knowable without the options at hand, since the
filter list and the chips render before the filter is ever opened. Adds
an optional `nestedFilterKeys` to the in-filter options for that, and
falls back to the options a previous load already resolved.
@github-actions github-actions Bot added fix react Changes affect packages/react labels Aug 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Review policy: Code change

Default rule: any other change needs one approval from f0-devs (rule 4).

Required approvals

Team Why Status
@factorialco/f0-devs Every code change needs a dev approval ⏳ pending
How this was decided
  • PRs touching only sds/ modules require their owners and nothing else.
  • Otherwise, docs-only changes (*.md, *.mdx, *.stories.tsx, anything in __stories__/) → one f0-general approval.
  • Otherwise, feat: titles → one f0-devs and one f0-designers approval. Not a feature? Fix the title prefix.
  • Anything else → one f0-devs approval.
  • Add the needs-design-review label to also request a design approval on any PR.
  • Creating a new sds/ module (new package.yml) additionally requires an f0-general approval.

Policy source: ownership/review-policy.ts · Team members: ownership/teams.yml

@github-actions

Copy link
Copy Markdown
Contributor

✅ No untranslated copy added

Every user-visible string in this PR comes from the i18n layer. Codebase total unchanged at 133.

@github-actions

Copy link
Copy Markdown
Contributor

✅ No New Circular Dependencies

No new circular dependencies detected. Current count: 0

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

✅ Storybook docs — no pages lost

Every page reachable on main is still reachable here.

Links point at this PR's Storybook build — browse the full Storybook.

Snapshot of the Storybook index (docs pages + stories) compared against main. Non-blocking.

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

📦 Alpha Package Version Published

Use pnpm i github:factorialco/f0#npm/alpha-pr-5270 to install the package

Use pnpm i github:factorialco/f0#e0daadf69001b577eb01cb31bcd99e0d71f2aeeb to install this specific commit

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🔍 Visual review for your branch is published 🔍

Here are the links to:

@github-actions

Copy link
Copy Markdown
Contributor

♿ Accessibility (axe) — components changed in this PR

✅ No a11y issues in the stories this PR changed.

Scope: only stories in the files/component folders this PR changed. It can't yet flag downstream ripple from shared-code/token changes, or diff against main (planned: base-vs-head delta).

@github-actions

Copy link
Copy Markdown
Contributor

✅ No breaking public API changes

No public exports were removed, renamed, or had existing props/types changed in a breaking way compared to main.

Comparing f0, experimental and ai against main. Adding components, types, or optional props is safe. This check is non-blocking.

⚠️ Could not analyze component-status (no-base) — a build may have failed; results may be incomplete.

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Coverage Report for packages/react

Status Category Percentage Covered / Total
🔵 Lines 69.02% 29130 / 42205
🔵 Statements 67.97% 30815 / 45332
🔵 Functions 61.87% 6955 / 11240
🔵 Branches 61.66% 21816 / 35376
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/react/src/patterns/OneFilterPicker/OneFilterPicker.tsx 97.22% 89.36% 95.23% 98.5% 235, 355
packages/react/src/patterns/OneFilterPicker/components/FilterList.tsx 100% 94.44% 100% 100%
packages/react/src/patterns/OneFilterPicker/filterTypes/InFilter/InFilter.tsx 91.3% 84% 88.88% 92.13% 130, 146, 201-210, 264, 269-270
packages/react/src/patterns/OneFilterPicker/filterTypes/InFilter/types.ts 100% 100% 100% 100%
packages/react/src/patterns/OneFilterPicker/filterTypes/InFilter/useLoadOptions.ts 72.41% 43.9% 86.66% 73.21% 77-83, 95, 151, 155, 167-169, 180-190
packages/react/src/patterns/OneFilterPicker/filterTypes/InFilter/components/option-utils.ts 82.75% 83.33% 83.33% 84% 14-19, 35
packages/react/src/patterns/OneFilterPicker/internal/getClearedFiltersValue.ts 0% 0% 0% 0% 8-27
Generated in workflow #17540 for commit 5a088df by the Vitest Coverage Report Action

@factmarc10010
factmarc10010 marked this pull request as ready for review August 27, 2026 06:51
@factmarc10010
factmarc10010 requested a review from a team as a code owner August 27, 2026 06:51

@albertpmz albertpmz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, let's check this issue below

Comment on lines +72 to +75
const resolvedOptions =
"options" in filterOptions && Array.isArray(filterOptions.options)
? filterOptions.options
: getCachedOptions<T>(getCacheKey(schema))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fallback doesn't fire where it's needed. With async options and no nestedFilterKeys, the dot stays off after the options load and only appears on a later mount: nestedKeysMap in FilterList.tsx is a useMemo keyed on definition, so the cache filling later never invalidates it. Same filter, two behaviors, depending on history the user can't see. It also makes render impure, since this reads a mutable module-level Map during render.

Suggest dropping the fallback and letting nestedFilterKeys be the single answer for non-literal options. It's deterministic and already carries the fix on its own: all four integration tests pass on the declared keys alone.

Heads up that option-utils.test.ts:76 can't catch this. It calls loadOptions(getCacheKey(schema), ...) with the same object and asserts the read back, so it covers the Map round-trip, not the component path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix react Changes affect packages/react

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants