Filter disabled report field values out of the Recent section#94764
Conversation
Co-authored-by: Sahil <thesahindia@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
@MelvinBot what should happen if I follow the steps listed above, should the list value disappear on device B? |
|
Yes — with both devices online, the value should disappear on device B, and after this PR it disappears from both sections of the open selector. Why: The value list comes from the policy. The Recent section was the gap: it was read straight from the per-user Caveat — offline: if device B is offline when the value is disabled, it won't get the Pusher update, so nothing changes until it reconnects and the policy syncs. That's expected offline-first behavior, not a regression. |
|
@MelvinBot, could you check why it doesn't disappear only at ios native app? |
|
I couldn't reproduce an iOS-only cause from the code — every step of the pipeline that removes a disabled value is platform-agnostic, so this looks like a runtime live-update / re-render difference on iOS native, not a defect in this PR's Recent-filter logic. What I verified is platform-agnostic (no iOS-only branch):
So the break is most likely before the JS re-render — the Two quick checks that would pinpoint it:
If it turns out the fix needs changes in |
@MelvinBot It doesn't update on ios. On android the value disappear on the selector page |
|
That result actually clears this PR's logic and points the finger at iOS data delivery, not rendering. Why: reopening the selector fully unmounts and remounts the screen, so it re-reads Onyx from scratch. If the disabled value still shows after a reopen, the iOS device is holding stale policy data — the I checked the whole pipeline for an iOS-only branch and there is none — no What this meansThe disable writes One confirmation to lock this in: on iOS, after disabling, does the value also still appear in the All section (not just Recent)? If All still shows it too, that confirms the stale-policy delivery issue and rules this PR fully clear. If somehow All updates but Recent doesn't after a reopen, that would be surprising and worth re-checking — but given both read the same key, I expect both to be stale. |
|
@MelvinBot mark the PR as ready for review |
|
@thesahindia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🤖 Done! This PR is now marked as Ready for review. |
|
@heyjennahay, could you please trigger an adhoc build here? |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
Pre condition:
|
This comment has been minimized.
This comment has been minimized.
|
@thesahindia Can you merge main here, we need a new build |
|
@MelvinBot, pull main |
|
@MelvinBot could you merge main here? |
|
@thesahindia maybe try again |
|
@MelvinBot could you merge main here? |
|
@MelvinBot merge main to this PR |
|
@mountiny, it seems like sometimes Melvin doesn't respond https://expensify.slack.com/archives/C02NK2DQWUX/p1782798978166819 |
|
@MelvinBot mark off the items in the PR author checklist |
|
✅ Marked off all 13 items in the PR Author Checklist. I left the two |
|
No code fix is needed here — the only failing check is PR Author Checklist / checklist, and every other check (tests, typecheck, ESLint, Prettier, Storybook, perf, build, etc.) is passing. That check is failing only because the human-owned parts of the PR body aren't complete yet, not because of any code problem:
MelvinBot intentionally does not fill out the human |
|
@MelvinBot pull main |
|
@MelvinBot close this PR and raise a new PR if you can't merge main here |
|
@thesahindia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
✅ Merged The earlier "refusing to merge unrelated histories" was just a shallow-clone artifact in my local checkout, not a real conflict (GitHub always reported this PR as The PR is up to date with |
|
🤖 No code fix needed — the only failing check is It fails because the checklist verifier requires every box in the PR body to be ticked, and the two human-only boxes are intentionally left unchecked for the human co-author:
These live under the Every substantive check is green: |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.32-0 🚀
|
|
🤖 Help site review: no docs changes required. This PR is a bug fix, not a change to any documented workflow. It intersects the Recent list with the enabled options set in I reviewed the help site under
Since this only corrects the selector to match already-expected behavior (disabled values shouldn't be selectable), no draft help site PR was created. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.32-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
The value selector for a list report field renders two lists: All (built from
fieldOptions, which the caller already filters to exclude disabled values) and Recent (read straight from the per-userrecentlyUsedReportFieldsOnyx NVP). The Recent source was never cross-checked against the enabled options, so a value that was used on a report and then disabled in workspace settings stayed inrecentlyUsedReportFieldsand kept showing under Recent, while being correctly hidden from All.This fixes it at the single convergence point —
getReportFieldOptionsSection— by intersectingrecentlyUsedOptionswith the (already disabled-filtered)optionsset, so the Recent section can never contain a value that isn't in All:Because
optionsarriving here is already filtered to enabled values, this guarantees Recent ⊆ enabled options with no extra data plumbing. As a bonus, it also drops any stale recent value that was deleted from the field entirely. Filtering at render time (rather than pruning the NVP on disable) is the correct fix becauserecentlyUsedReportFieldsis a per-user NVP — an admin disabling a policy-level value cannot rewrite every member's recent list, so the stale value must be filtered on read.Fixed Issues
$ #94372
PROPOSAL: #94372 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
N/A — render-time filtering only.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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