Fix filter operator switching behavior#7304
Open
sk10727-a11y wants to merge 6 commits intoactualbudget:masterfrom
Open
Fix filter operator switching behavior#7304sk10727-a11y wants to merge 6 commits intoactualbudget:masterfrom
sk10727-a11y wants to merge 6 commits intoactualbudget:masterfrom
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
👋 Hello contributor! We would love to review your PR! Before we can do that, please make sure:
We do this to reduce the TOIL the core contributor team has to go through for each PR and to allow for speedy reviews and merges. For more information, please see our Contributing Guide. |
sk10727-a11y
changed the title
✅ Deploy Preview for actualbudget-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
updated to allow conversion from multi-ID to text if only one array value and to handle no-value operators for account filter final fixes before upstream sync Fix UUID leakage when switching filter operators Fix UUID leakage when switching filter operators#
sk10727-a11y
force-pushed
the
filter-fix
branch
from
2429bcc to
c777c68
Compare
Contributor
📝 WalkthroughWalkthroughFiltersMenu.tsx now loads accounts, categories, and payees to map stored IDs to display text, replaces hyphenated op tokens with camelCase variants, and centralizes operator switching in a local setOp(nextOp) that translates and preserves values across op type transitions. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant FM as FiltersMenu
participant Acc as AccountsService
participant Cat as CategoriesService
participant Pay as PayeesService
participant FR as FilterReducer
User->>FM: change operator / press op button
FM->>FM: setOp(nextOp) (determine op type transition)
alt needs ID↔text or single↔multi translation
FM->>Acc: lookup account names/IDs
FM->>Cat: lookup category names/IDs
FM->>Pay: lookup payee names/IDs
Acc-->>FM: account mappings
Cat-->>FM: category mappings
Pay-->>FM: payee mappings
FM->>FM: convert/preserve value(s)
end
FM->>FR: dispatch set-value? / set-op
FR-->>FM: new filter state
FM-->>User: updated UI (inputs / tokens)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/desktop-client/src/components/filters/FiltersMenu.tsx`:
- Around line 227-321: In setOp, normalize value shapes for all exact-ID
operator transitions: when switching to multi-ID ops (use isMultiIdOp(nextOp)),
always dispatch set-value with an array (e.g., [id] or []), and when switching
to single-ID ops (isSingleIdOp(nextOp)), unwrap single-element arrays into a
bare string or '' if none; update the Text→Multi and Text→Single branches that
call resolveTextToId (use resolvedValue ? [resolvedValue] : [] for multi, and
resolvedValue || '' for single) and the Multi→Text/Single→Text branches to
unwrap arrays (use value[0] when Array.isArray(value)). Also ensure transitions
from contains/matches/doesNotContain → oneOf/notOneOf set [] on failed lookup
instead of leaving the old string, and in the account no-value branches (field
=== 'account' && isNoValueAccountOp(op)) preserve existing ID arrays when
returning to single/multi ID ops (if value is already an array, keep or unwrap
it appropriately) — use the helper functions resolveTextToId and resolveIdToText
and always call dispatch({type: 'set-value', value: ...}) with the normalized
shape before dispatch({type: 'set-op', op: nextOp}).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d39d12fe-c9df-4294-a7d4-0ebd3b368ccf
📒 Files selected for processing (2)
packages/desktop-client/src/components/filters/FiltersMenu.tsxupcoming-release-notes/7304.md
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/desktop-client/src/components/filters/FiltersMenu.tsx (2)
236-267:⚠️ Potential issue | 🟠 MajorClear unmatched names before entering exact-ID operators.
If lookup fails here, the exact-match branches keep free text in state (
is/isNotkeep the raw string,oneOf/notOneOfcan end up with raw text, and the no-value account path falls back tovalue/[value]). That breaks the new “exact ops store IDs only” contract and can hand invalid values to the exact-ID inputs or validator.Suggested fix
if (isIdField && isTextOp(op) && isSingleIdOp(nextOp)) { const resolvedValue = resolveTextToId(field, subfield, value); - if (resolvedValue) { - dispatch({ - type: 'set-value', - value: resolvedValue, - }); - } + dispatch({ + type: 'set-value', + value: resolvedValue ?? '', + }); } @@ if (isIdField && isTextOp(op) && isMultiIdOp(nextOp)) { const resolvedValue = resolveTextToId(field, subfield, value); - if (resolvedValue) { - dispatch({ - type: 'set-value', - value: resolvedValue ? [resolvedValue] : [], - }); - } + dispatch({ + type: 'set-value', + value: resolvedValue ? [resolvedValue] : [], + }); } @@ if (field === 'account' && isNoValueAccountOp(op) && isSingleIdOp(nextOp)) { if (typeof value === 'string') { - const resolvedValue = resolveTextToId(field, subfield, value); + const resolvedValue = + resolveIdToText(field, subfield, value) !== '' + ? value + : resolveTextToId(field, subfield, value); dispatch({ type: 'set-value', - value: resolvedValue || value, + value: resolvedValue ?? '', }); } else { dispatch({ type: 'set-value', value: '', @@ if (field === 'account' && isNoValueAccountOp(op) && isMultiIdOp(nextOp)) { if (typeof value === 'string') { - const resolvedValue = resolveTextToId(field, subfield, value); + const resolvedValue = + resolveIdToText(field, subfield, value) !== '' + ? value + : resolveTextToId(field, subfield, value); dispatch({ type: 'set-value', - value: resolvedValue ? [resolvedValue] : [value], + value: resolvedValue ? [resolvedValue] : [], }); } else { dispatch({ type: 'set-value', value: [],Also applies to: 289-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/filters/FiltersMenu.tsx` around lines 236 - 267, When converting text -> exact-ID operators in FiltersMenu (the branches that call resolveTextToId and then dispatch 'set-value'), ensure that a failed lookup clears the value instead of leaving raw free-text: if resolveTextToId(field, subfield, value) returns falsy, dispatch set-value with '' for single-ID operators (is/isNot) and [] for multi-ID operators (oneOf/notOneOf); apply the same clearing logic to the corresponding mirrored branches (the other text->ID and multi-ID->text conversion blocks) so no raw string can be kept when entering exact-ID operators.
227-321:⚠️ Potential issue | 🟠 MajorPreserve multi-account selections when switching into
onBudget/offBudget.There’s still no
isMultiIdOp(op) && isNoValueAccountOp(nextOp)path, so this falls through topackages/desktop-client/src/components/filters/updateFilterReducer.tsand unwraps the array to its first ID. Toggling back from a no-value account operator then cannot recover the original multi-selection.Suggested fix
const setOp = (nextOp: T['op']) => { + if (field === 'account' && isMultiIdOp(op) && isNoValueAccountOp(nextOp)) { + dispatch({ type: 'set-op', op: nextOp }); + dispatch({ type: 'set-value', value }); + return; + } + // Single ID -> Text: Convert stored ID to text with one to one mapping if (isIdField && isSingleIdOp(op) && isTextOp(nextOp)) { dispatch({ type: 'set-value', value: resolveIdToText(field, subfield, value), @@ // No-value Account -> Single-ID: If preserved value is text, resolve to an ID; // If it is already a single ID string, keep as-is if (field === 'account' && isNoValueAccountOp(op) && isSingleIdOp(nextOp)) { - if (typeof value === 'string') { + if (Array.isArray(value)) { + dispatch({ + type: 'set-value', + value: value[0] ?? '', + }); + } else if (typeof value === 'string') { const resolvedValue = resolveTextToId(field, subfield, value); dispatch({ type: 'set-value', value: resolvedValue || value, }); @@ // No-value Account -> Multi-ID: If the preserved value is text, resolve to a single ID and wrap; // if the preserved value is already a single ID string, wrap it directly; // otherwise clear if (field === 'account' && isNoValueAccountOp(op) && isMultiIdOp(nextOp)) { - if (typeof value === 'string') { + if (Array.isArray(value)) { + dispatch({ + type: 'set-value', + value, + }); + } else if (typeof value === 'string') { const resolvedValue = resolveTextToId(field, subfield, value); dispatch({ type: 'set-value', value: resolvedValue ? [resolvedValue] : [value],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/filters/FiltersMenu.tsx` around lines 227 - 321, The setOp handler is missing a branch for when switching from a multi-ID operator into a no-value account operator, causing the array to be unwrapped later; add a case in setOp that checks if field === 'account' && isMultiIdOp(op) && isNoValueAccountOp(nextOp) and dispatches { type: 'set-value', value: Array.isArray(value) ? value : [] } (preserving the multi-selection) before the final dispatch({ type: 'set-op', op: nextOp }); reference setOp, isMultiIdOp, isNoValueAccountOp, dispatch, field, and value.
🧹 Nitpick comments (1)
packages/desktop-client/src/components/filters/FiltersMenu.tsx (1)
147-158: Preferfunctiondeclarations for these op classifiers.These helpers are pure, and the repo rule prefers
functionoverconstarrows here.As per coding guidelines, "packages//src/**/.{ts,tsx,js}: Use the
functionkeyword for pure functions instead of arrow functions or const declarations"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/filters/FiltersMenu.tsx` around lines 147 - 158, Change the op-classifier helpers from const arrow assignments to named function declarations: replace isPayeeIdOp, isSingleIdOp, isMultiIdOp, isTextOp, and isNoValueAccountOp (currently defined as const <name> = (op: T['op']) => ...) with function <name>(op: T['op']) { ... } forms so they follow the repo guideline to use the `function` keyword for pure helpers; keep the same logic and return values and ensure exported/local references remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/desktop-client/src/components/filters/FiltersMenu.tsx`:
- Around line 236-267: When converting text -> exact-ID operators in FiltersMenu
(the branches that call resolveTextToId and then dispatch 'set-value'), ensure
that a failed lookup clears the value instead of leaving raw free-text: if
resolveTextToId(field, subfield, value) returns falsy, dispatch set-value with
'' for single-ID operators (is/isNot) and [] for multi-ID operators
(oneOf/notOneOf); apply the same clearing logic to the corresponding mirrored
branches (the other text->ID and multi-ID->text conversion blocks) so no raw
string can be kept when entering exact-ID operators.
- Around line 227-321: The setOp handler is missing a branch for when switching
from a multi-ID operator into a no-value account operator, causing the array to
be unwrapped later; add a case in setOp that checks if field === 'account' &&
isMultiIdOp(op) && isNoValueAccountOp(nextOp) and dispatches { type:
'set-value', value: Array.isArray(value) ? value : [] } (preserving the
multi-selection) before the final dispatch({ type: 'set-op', op: nextOp });
reference setOp, isMultiIdOp, isNoValueAccountOp, dispatch, field, and value.
---
Nitpick comments:
In `@packages/desktop-client/src/components/filters/FiltersMenu.tsx`:
- Around line 147-158: Change the op-classifier helpers from const arrow
assignments to named function declarations: replace isPayeeIdOp, isSingleIdOp,
isMultiIdOp, isTextOp, and isNoValueAccountOp (currently defined as const <name>
= (op: T['op']) => ...) with function <name>(op: T['op']) { ... } forms so they
follow the repo guideline to use the `function` keyword for pure helpers; keep
the same logic and return values and ensure exported/local references remain
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14f02f09-9109-4b0b-a59e-34113755346e
📒 Files selected for processing (2)
packages/desktop-client/src/components/filters/FiltersMenu.tsxupcoming-release-notes/7304.md
🚧 Files skipped from review as they are similar to previous changes (1)
- upcoming-release-notes/7304.md
…itions (code rabbit)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/desktop-client/src/components/filters/FiltersMenu.tsx`:
- Around line 521-524: The render condition in FiltersMenu.tsx prevents account
filters from showing inputs by including `field !== 'account'`; remove that
`field !== 'account'` guard so account filters are allowed to render when they
have values — keep the existing checks `type !== 'boolean'`, `(field !== 'payee'
|| !isPayeeIdOp(op))`, and `!isNoValueAccountOp(op)` so the
`onBudget`/`offBudget` no-value operators still hide the editor while normal
`is`, `contains`, `matches`, `oneOf`, etc. render correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4378683-e308-4d77-9993-955c56ac8a04
📒 Files selected for processing (1)
packages/desktop-client/src/components/filters/FiltersMenu.tsx
sk10727-a11y
changed the title
sk10727-a11y
changed the title
matt-fidd
force-pushed
the
master
branch
2 times, most recently
from
5c7c70d to
d262f7d
Compare
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
1 participant
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.
Description
Fixes an issue where switching filter operators between exact-match (is, one of) and text-based operators (contains, matches) would populate the input with UUIDs instead of human-readable names.
This PR adds logic to correctly transform values when switching between operator types for ID-based fields (accounts, categories, category groups, payees):
Unique to the accounts filter, there are account no-value operators (is on budget, is off budget):
Note:
This ensures the input always matches the expected format for the selected operator and prevents UUID leakage in the UI.
Related issue(s)
Fixes #7123.
Testing
Tested the following transitions across accounts, categories, category groups, and payees:
Account-specific cases:
How to test
Checklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
Changeset
src/components/filters/FiltersMenu.tsxView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
loot-core
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
api
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
cli
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged