Removed the Onyx.connect usage for introSelected from NextStepUtils.ts and ReportNameUtils.ts.#93883
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cdccadb15
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| allPolicyTags: policyTags, | ||
| conciergeReportID: conciergeReportID ?? undefined, | ||
| reportAttributes: currentValue?.reports, | ||
| isTrackIntentUser: isTrackOnboardingChoice(introSelected?.choice), |
There was a problem hiding this comment.
Recompute report attributes when intro choice changes
When NVP_INTRO_SELECTED changes after DERIVED.REPORT_ATTRIBUTES already has a currentValue, the derived compute runs with only that key in sourceValues. Because needsFullRecompute does not include this new dependency and the incremental updates list also ignores it, the function returns the previous value before reaching this isTrackOnboardingChoice usage. That leaves report/thread names that depend on shouldShowMarkAsDone (for example “submitted” vs “marked as done”) stale until some unrelated report update happens; please trigger a full recompute or otherwise enqueue affected reports when this key changes.
Useful? React with 👍 / 👎.
|
@ahmedGaber93 @Valforte One of you needs to 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] |
|
@ahmedGaber93 , sorry for the ping. @Krishna2323 will review this PR, as it is a follow-up to our previous PR. |
| * @param sessionAccountID - accountID of the current user | ||
| * @param sessionEmail - email of the current user | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/max-params |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This newly added // eslint-disable-next-line @typescript-eslint/max-params on completeSplitBill has no accompanying comment explaining why the rule is suppressed. eslint-disable comments hide the violation from the seatbelt baseline entirely, so they should only be used for permanent suppressions and must always carry a justification so future maintainers understand the intent.
This suppression was only added because threading the new isTrackIntentUser argument pushed the parameter count over the limit — that reads like a temporary/convenience suppression rather than a permanent one. Prefer widening the baseline with SEATBELT_INCREASE (which keeps the violation visible so the over-long parameter list can still be refactored later, e.g. into a single params object), or, if the disable is intentional and permanent, add a justification comment:
// eslint-disable-next-line @typescript-eslint/max-params -- <reason this parameter list is intentionally kept positional>
function completeSplitBill(See LINTING.md for guidance on choosing between eslint-disable and SEATBELT_INCREASE.
Reviewed at: 73368cb | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@Krishna2323 , while testing the changes, I found a bug in We are using Before our changes, it was using: The issue now is that when an expense has something that needs to be fixed, it shows "Mark as done" instead of "Fix". I think "Fix" label should continue to be used for these cases, even for track-intent users. |


Explanation of Change
This is a follow-up PR to remove the
Onyx.connectusage forintroSelectedfromNextStepUtils.tsandReportNameUtils.ts, which was introduced in this PR: #92254.Fixed Issues
$ #86223
PROPOSAL: #86223 (comment)
Tests
Prerequisites:
TRACK_WORKSPACEorPERSONAL_SPEND)Test 1:
Test 2:
Test 3:
Test 4:
Test 5:
Prerequisite:
Steps:
This expense was put on hold. Please review the comments for next steps.) and the LHN reflects the latest status correctly ('marked as done')Test 6:
Test 7:
Prerequisite:
Steps:
Test 8:
Prerequisite:
Steps:
Test 9:
Prerequisite:
Steps:
Test 10:
Prerequisites:
Steps:
Test 11:
Prerequisite:
Steps:
Test 12:
Prerequisites:
Steps:
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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.mov
Android: mWeb Chrome
mweb-chrome.mov
iOS: Native
ios.mov
iOS: mWeb Safari
mweb-safari.mov
MacOS: Chrome / Safari
web.mov