Repository navigation
Keep merchant rules import button active offline for the all-skipped path - #96976
Conversation
Co-authored-by: Joe Ph <gijoe0295@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. |
|
The only failing check is PR Author Checklist / checklist, and it's not something I can push a code fix for. It fails because Every other check is passing or still running — none of them have genuinely failed. To get this green: edit the PR description to complete the author checklist and write out the Tests / Offline tests / QA steps (or add |
|
@MelvinBot update test in ImportedMerchantRulesPageTest.ts to satisfy shouldDisableButtonWhenOffline change from ImportedMerchantRulesPage.tsx |
Co-authored-by: Joe Ph <gijoe0295@users.noreply.github.com>
|
🤖 Added tests covering the Since the offline behavior lives in the render path (not in the two pure helpers the file already tested), I renamed
The existing Verification
The render test mirrors the sibling |
…lineButton Co-authored-by: Joe Ph <gijoe0295@users.noreply.github.com>
|
🤖 Fixed by merging the latest Root cause: The A typecheck regression had landed on Fix: Merged the now-fixed The only other red check is |
|
🤖 Confirming the fix landed — As noted earlier, the failure was a typecheck regression on |
|
🤖 The failing checks are now resolved. After I merged the fixed The only remaining red check is |
DetailsExplanation of ChangeThe "Import" button in the merchant-rules import flow was disabled whenever the device was offline. This blocked the client-side-only path where every parsed row is a duplicate of an existing rule and/or references a category that doesn't exist — in that case The offline guard lived hard-coded in the shared
Fixed Issues$ #96842 Tests
Offline testsQA Steps
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-01.at.10.44.23.movAndroid: mWeb ChromeScreen.Recording.2026-07-30.at.00.09.31.moviOS: HybridAppScreen.Recording.2026-07-30.at.01.08.53.moviOS: mWeb SafariScreen.Recording.2026-07-29.at.23.59.05.movMacOS: Chrome / SafariScreen.Recording.2026-07-29.at.23.51.48.mov |
|
✅ Done — I've set the PR body to the provided content. You can see it on PR #96976. |
|
Fixed — the Root cause: The check wasn't failing on an unchecked box — it was crashing. The Fix (PR body only, no code changes):
The re-run triggered by the body edit (run 30683777921) is green, and no other checks are failing. |
|
@MelvinBot add test file to PR description |
|
✅ Done — I added the merchant-rules-test.csv test file to the PR description. It now appears in a dedicated Test file section and is linked from the "Upload the ... file" step in both the Tests and QA Steps sections. |
|
|
Re-applied the fix — the What happened: I fixed this earlier (run passed at 04:21), but the checklist heading got reverted to Root cause (unchanged): The Fix (PR body only):
|
|
✅ Both changes are already in place on the PR body — no edit was needed:
|
|
@MelvinBot Mark the Pull Request ready for review |
|
@gijoe0295 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 — I've marked PR #96976 as ready for review. |
|
There's nothing to fix or push here — every code check on this PR is passing (tests, typecheck, ESLint, Oxfmt, build, Storybook, etc.). The only failing check is Check independent approval (step "Verify peer review"). That's not a code failure — it's a process gate that stays red until an independent reviewer approves the PR. Pushing a commit won't turn it green; it just needs a human review/approval. Next step: get a reviewer to approve the PR and this check will clear on its own. |
|
@MelvinBot please update the implementation to address the Codex P2 comment and Justin's follow-up above. Problem: Requested fix (scopes the bypass to only the case that's actually local, and stops relying solely on the button being disabled):
This keeps the fix scoped to the reported bug (re-importing a CSV where all rows already exist/reference invalid categories) while making sure the offline case is actually handled at the point the network request is made, not just at the button. |
Co-authored-by: Joe Ph <gijoe0295@users.noreply.github.com>
|
The only failing check is Check independent approval, and it isn't something a code push can fix. Every real CI check on this PR is green — Bun tests, ESLint, Oxfmt, typecheck, React Compiler Compliance, Storybook, unit tests (jobs 1–8), builds, perf tests, spellcheck, and the rest all pass. Check independent approval is a process gate: it requires an approval from a reviewer who is independent of the PR (i.e. someone other than the author/assignee). Right now the only approval on the PR is from No fix pushed, since there's no code-level failure to address. |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
Co-authored-by: Joe Ph <gijoe0295@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd51a81252
ℹ️ 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".
|
@gijoe0295 see my additional review comments if you agree, then you can have them fixed too |
…ts offline guard Co-authored-by: Joe Ph <gijoe0295@users.noreply.github.com>
|
@MelvinBot Fix Oxfmt check failed |
Co-authored-by: Joe Ph <gijoe0295@users.noreply.github.com>
|
Fixed the Oxfmt check in a9f42b8. The failure came from my previous commit: removing |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Justin is currently OOO so going to proceed with merging this having reviewed it.
|
🚧 inimaga has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/inimaga in version: 9.4.54-0 🚀
|
|
🤖 I reviewed this PR against the help site files in What this PR changes: It keeps the Import button active while offline for the merchant-rules import flow, but only for the client-side-only path where every parsed row is a duplicate and/or references a nonexistent category (so no API call is needed). It does this by adding an optional Why no docs update: The relevant article,
The article doesn't document offline vs. online button states (nor should it — that's an implementation-level UX detail, not part of the user-facing workflow), so there's nothing to add or correct. Since no changes are needed, I did not create a draft help site PR. @gijoe0295, please confirm this assessment. If you believe the offline behavior (or anything else in this PR) should be reflected in the help site, let me know and I'll create the draft PR. |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.4.54-4 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
The "Import" button in the merchant-rules import flow was disabled whenever the device was offline. This blocked the client-side-only path where every parsed row is a duplicate of an existing rule and/or references a category that doesn't exist — in that case
ImportedMerchantRulesPage.importRulesshort-circuits and builds the "No merchant rules have been added…" confirmation modal locally, with no API call. Since no network is required for that path, the button should stay active offline.The offline guard lived hard-coded in the shared
ImportSpreadsheetColumnscomponent (isDisabled={isOffline}), which is correct for its other consumers (categories, tags, members, per diem, company cards, wallet transactions) that need a live server round-trip. Rather than change behavior for everyone, this PR:shouldDisableButtonWhenOfflineprop (defaulttrue, preserving existing behavior for every other importer) toImportSpreadsheetColumns, applied asisDisabled={shouldDisableButtonWhenOffline && isOffline}.shouldDisableButtonWhenOffline={false}from the merchant-rules page so its button stays active offline.Test file
merchant-rules-test.csv
Fixed Issues
$ #96842
PROPOSAL: #96842 (comment)
Tests
go offline
Offline tests
QA Steps
go offline
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectionAvatar, I verified the components usingAvatarhave been tested & I retested again)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)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./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: HybridApp
Screen.Recording.2026-08-01.at.10.44.23.mov
Android: mWeb Chrome
Screen.Recording.2026-07-30.at.00.09.31.mov
iOS: HybridApp
Screen.Recording.2026-07-30.at.01.08.53.mov
iOS: mWeb Safari
Screen.Recording.2026-07-29.at.23.59.05.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-29.at.23.51.48.mov