Fix stale 'from' filter persisting for workspace auditor#82475
Fix stale 'from' filter persisting for workspace auditor#82475
Conversation
… search filters Use Onyx.set (full replace) instead of Onyx.merge when opening Advanced Filters to ensure stale fields like 'from' don't persist. Also clear the 'from' filter when the search type changes, similar to how 'status' and 'columns' are already reset. Fixes #82462 Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Reviewing.... |
|
Hey @MelvinBot I have few questions about the expected behaviour and the implementation. The PR seems to be working fine. Questions about Expected Behavior
Questions about Incomplete/Wrong Implementation
cc: @Julesssss |
|
Great questions @Krishna2323. I've investigated the codebase in depth to answer each one. These are genuinely important observations about the completeness of this fix. Questions about Expected Behavior1. Should Yes, this is a valid gap. Each view injects a distinct set of implicit filters (
Critically, all of these filter keys ( So yes, an approver switching from the Approve view would carry stale 2. Should user-explicit The current code does not distinguish between implicit (view-injected) and explicit (user-set) Given this, clearing That said, this is a product decision -- if preserving user-explicit filters across type changes is considered important, an approach like tracking filter provenance (implicit vs explicit) would be needed, which adds complexity. 3. What should the auditor's default landing view be? Currently, the default is determined by For an auditor who is NOT an approver/payer/exporter, the visibility logic at
So auditors land on Submit, which is scoped to Questions about Incomplete/Wrong Implementation4. Why is This is a valid concern. Looking at const applyChanges = useCallback(() => {
const updatedFilters = {
type: selectedItem,
...(hasTypeChanged && {
groupBy: null,
status: CONST.SEARCH.STATUS.EXPENSE.ALL,
has: filteredHasValues,
}),
};
updateAdvancedFilters(updatedFilters); // uses Onyx.merge (no `true` second arg)
}, ...);This path:
This is the same class of bug through a different code path (Advanced Filters > Type page vs the filter bar dropdown). The same clearing logic should be applied here. 5. Should Yes -- as detailed in the answer to question #1 above, only clearing Specifically, the type-change block at 6. Is the Looking at const formValues = useFilterFormValues(queryJSON);
useEffect(() => {
updateAdvancedFilters(formValues, true); // Onyx.set -- full replace
}, [formValues]);This The A possible scenario where it could matter: if the SummaryThe reviewer's observations are well-founded. The key gaps are:
These seem like issues that should be addressed before merging. cc @Julesssss |
|
@MelvinBot so what do you think about the feedback? Can we solve this without the wider refactor? Recall your original proposal here: #82462 (comment) |
|
@Julesssss Yes, I think the feedback can be addressed without a wider refactor. Krishna's points are valid but they're all extensions of the same pattern already in this PR — not a different architecture. Here's my take: What can be solved with small additions to this PRPoints #1 and #5 (clear all view-specific filters, not just if (updatedFilterFormValues.type !== searchAdvancedFiltersForm.type) {
updatedFilterFormValues.columns = [];
updatedFilterFormValues.status = CONST.SEARCH.STATUS.EXPENSE.ALL;
updatedFilterFormValues.from = undefined;
updatedFilterFormValues.to = undefined; // Approve view
updatedFilterFormValues.action = undefined; // All views set action
updatedFilterFormValues.payer = undefined; // Pay view
updatedFilterFormValues.exporter = undefined; // Export view
updatedFilterFormValues.reimbursable = undefined; // Pay view
updatedFilterFormValues.exportedOn = undefined; // Export view
updatedFilterFormValues.has = filterValidHasValues(...);
}This is the same approach — no new abstractions or refactoring needed. Point #4 ( const updatedFilters = {
type: selectedItem,
...(hasTypeChanged && {
groupBy: null,
status: CONST.SEARCH.STATUS.EXPENSE.ALL,
has: filteredHasValues,
from: undefined,
to: undefined,
action: undefined,
payer: undefined,
exporter: undefined,
reimbursable: undefined,
exportedOn: undefined,
}),
};Note: Since What can be deferred (out of scope for this issue)Point #2 (implicit vs explicit filter distinction): The original proposal acknowledged this trade-off. Clearing all view-specific filters on type change is the right behavior regardless — when you switch from "Expense Report" to "Expense", the Point #3 (auditor default landing view): This is a separate product question about whether auditors should default to Expenses instead of Submit. It's not related to the stale filter persistence bug. Could be filed as a separate issue. Point #6 ( SummaryThe changes needed to address the feedback are:
All three are small, additive changes within the same pattern. No wider refactor needed. |
|
@MelvinBot could you please implement the changes you mentioned in your last comment? |
When switching search types, stale view-specific filters (to, action, payer, exporter, reimbursable, exportedOn) were persisting alongside the already-cleared 'from' filter. This caused the same class of bug for Approve, Pay, and Export views that was fixed for Submit. Also adds the same clearing logic to SearchFiltersTypePage.tsx, which handles type changes through the Advanced Filters > Type page path. Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
@Krishna2323 I've pushed a commit addressing the gaps you identified. Here's what changed: Changes made1. The type-change block now clears 2. The 3. I left the existing Not addressed in this commitQuestion #3 (auditor default landing view) — This is a separate product decision about whether auditors should land on Expenses (unscoped) instead of Submit (scoped to self). The current behavior is driven by cc @Julesssss |
| from: undefined, | ||
| to: undefined, | ||
| action: undefined, | ||
| payer: undefined, | ||
| exporter: undefined, | ||
| reimbursable: undefined, | ||
| exportedOn: undefined, |
There was a problem hiding this comment.
@MelvinBot this should be null not undefined, else Onyx won't clear them properly
There was a problem hiding this comment.
Good catch — Onyx.merge ignores undefined values, so these fields weren't actually being cleared. Fixed in 2d28ff4: changed all seven fields (from, to, action, payer, exporter, reimbursable, exportedOn) from undefined to null.
Note: the SearchFiltersBar.tsx path still uses undefined and that's correct there — it builds a fresh object passed to buildQueryStringFromFilterFormValues (which converts to a query string), not through Onyx.merge.
Onyx.merge ignores undefined values, so the view-specific filters (from, to, action, payer, exporter, reimbursable, exportedOn) were not actually being cleared when changing type through the Advanced Filters Type page. Using null ensures Onyx properly removes these keys. Co-authored-by: Jules <Julesssss@users.noreply.github.com>
|
The failing check is
This is unrelated to the code changes in this PR. It's a process check that passes once a reviewer (e.g. @Julesssss) posts the standard reviewer checklist comment. No code fix is needed — a reviewer just needs to copy-paste the reviewer checklist template into a PR comment. |
|
The failing check is unrelated to the code changes in this PR. Analysis: The only failing check is Evidence: The PR changes only touch |
Julesssss
left a comment
There was a problem hiding this comment.
@MelvinBot okay final thing. Could you define a set of those specific keys that will be set to null and reuse in both places them to clean this up.
|
Okay that seemed to be applied correctly. Shared some additional review feedback. |
|
The failing check is unrelated to the code in this PR. Check: Analysis: The Resolution: A reviewer (e.g. @JmillsExpensify who is requested) needs to post their Reviewer Checklist comment on this PR. Once posted, the check will pass on the next run. |
| updatedFilterFormValues.status = CONST.SEARCH.STATUS.EXPENSE.ALL; | ||
| updatedFilterFormValues.from = undefined; | ||
| updatedFilterFormValues.to = undefined; | ||
| updatedFilterFormValues.action = undefined; | ||
| updatedFilterFormValues.payer = undefined; | ||
| updatedFilterFormValues.exporter = undefined; | ||
| updatedFilterFormValues.reimbursable = undefined; | ||
| updatedFilterFormValues.exportedOn = undefined; |
There was a problem hiding this comment.
@MelvinBot could you define a set of those specific keys that will be set to null and reuse in both places them to clean this up (here and SearchFiltersTypePage)
There was a problem hiding this comment.
Done in 7e06860. Defined VIEW_SPECIFIC_FILTER_KEYS in SearchAdvancedFiltersForm.ts (alongside the existing DATE_FILTER_KEYS and AMOUNT_FILTER_KEYS) and used it in both:
SearchFiltersBar.tsx— iterates withfor...ofand sets each toundefined(this path builds a local object forbuildQueryStringFromFilterFormValues, not Onyx directly)SearchFiltersTypePage.tsx— spreads viaObject.fromEntrieswithnullvalues (this path usesOnyx.mergewhich requiresnullto clear)
This comment was marked as off-topic.
This comment was marked as off-topic.
…ge handlers Co-authored-by: Jules <Julesssss@users.noreply.github.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@Krishna2323 I think Melvins latest changes make sense, let me know what you think. It followed your initial requests too. |
|
Reviewing... |
|
The latest changes look good to me as well! Proceeding with testing and completing the checklist. |
|
@MelvinBot I’ve been testing this fix, but I’m not able to reproduce the original bug on the
I also tried applying date filters through the filter bar UI while staying on the Submit tab, and that correctly keeps From what I can see, typing into the search box replaces the entire query string, so stale Question:
@Julesssss were you able to reproduce the bug on your end? Monosnap.screencast.2026-02-18.22-13-14.mp4 |
|
Good catch @Krishna2323. I investigated the code paths in detail and I believe I can explain why you can't reproduce this on The
|
|
Hmm good question. QA are also not able to reproduce this bug. That's my fault, I picked an issue for us to test and din't notice that it hadn't been reproduced by QA originally 😖
|
|
Okay lets call this an edge case and drop the potential fix. It's not worth the fix currently. @Krishna2323 we can pay out the full bounty, thanks for your help here |
Explanation of Change
When a workspace auditor navigates to Reports > Expenses, they land on the "Submit" view which hardcodes a
from:[self]filter scoping results to only their own submissions. When they then apply additional filters (date range, category, tag) or open Advanced Filters, the stalefromfield persists in the Onyx form and carries forward into the rebuilt query string, so they only ever see their own expenses instead of all submitters.This PR fixes the stale
frompersistence through two changes:Use
Onyx.set(full replace) instead ofOnyx.mergewhen opening Advanced Filters — InopenAdvancedFilters, we now passtruetoupdateAdvancedFiltersso the Onyx form is fully replaced with the current URL-derived filter values. This prevents stale fields likefromfrom surviving when they're no longer part of the active query.Clear
fromwhen the search type changes — When the user switches the search type (e.g., from Submit to Expenses), we now resetfromtoundefined, consistent with howstatusandcolumnsare already reset on type change.Fixed Issues
$ #82462
PROPOSAL: #82462 (comment)
Tests
Offline tests
These changes only affect how the Onyx filter form is synced when applying filters. No network requests are involved in the filter form update logic, so offline behavior is unchanged.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari