Skip to content

Fix: render 'Vendor unavailable' instead of raw vendor ID in merchant rules after disconnecting accounting - #98619

Open
MelvinBot wants to merge 7 commits into
mainfrom
claude-fixRulesVendorUnavailableFallback
Open

Fix: render 'Vendor unavailable' instead of raw vendor ID in merchant rules after disconnecting accounting#98619
MelvinBot wants to merge 7 commits into
mainfrom
claude-fixRulesVendorUnavailableFallback

Conversation

@MelvinBot

@MelvinBot MelvinBot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Fixes the merchant-rule vendor summary after a workspace disconnects its accounting integration.

Merchant rules intentionally persist only the external vendorID; the display name is resolved from the policy's connection data at render time. When the connection is removed, that data is no longer available. The previous tier-3 fallback rendered the raw external ID, which is not meaningful to users.

This PR keeps the existing three-tier resolution behavior:

  1. Render the vendor name when it is present in the active vendor-matching list.
  2. Render Vendor unavailable (or Supplier unavailable for Xero) when the active list is loaded but does not contain the ID.
  3. When the list is not yet available or no active vendor-matching source remains, preserve findVendorByID() so a known historical name continues to render during hydration or after an export-mode change. If no connection can resolve the ID, render the existing unavailable copy instead of exposing the raw vendorID.

The change is applied to both merchant-rule descriptions:

  • getMerchantCodingRulesTableData for the revamped rules table.
  • getRuleDescription for the legacy rules view.

The unit tests now cover the unresolved/disconnected state in both paths while retaining coverage for a resolved vendor, an active-list miss, a stale inactive connection, an export-mode change, and Xero's supplier wording. No backend change is required.

Fixed Issues

$ #98602
PROPOSAL: #98602 (comment)

Tests

  1. In a staging test workspace, enable Accounting and Rules, connect Sage Intacct, and enable the vendorMatching beta with Onyx.merge('betas', ['vendorMatching']); in the web console.
  2. Go to Workspace settings > Rules, create a merchant rule with a merchant and a Sage Intacct vendor, and confirm the rule summary shows the vendor's name.
  3. Disconnect Sage Intacct from Workspace settings > Accounting.
  4. Return to Rules and find the created rule. Verify its summary says Update vendor to "Vendor unavailable", never the raw external vendor ID.
  5. Reconnect the integration (or use a workspace with a loaded matching-vendor list) and verify a valid vendor rule still renders the vendor's name.
  6. Verify the browser console contains no new errors during the flow.

Offline tests

  1. With a vendor rule and its matching-vendor data already loaded, disconnect the network and revisit Rules. Verify the cached, resolvable vendor name continues to render.
  2. In a state where the vendor cannot be resolved from policy connection data, verify the rule renders Vendor unavailable rather than the raw external ID. Restoring connectivity must not change a valid vendor's name to the unavailable copy.

QA Steps

Same as Tests. Test the Sage Intacct disconnect flow on staging and verify that the rule displays Vendor unavailable rather than an external vendor ID. Also verify that a valid, resolvable vendor continues to display its name.

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline tests section
    • I added steps for Staging testing in the QA Steps section
    • I added steps to cover the failure scenario: the disconnected/unresolvable vendor
    • I turned off my network connection and tested it while offline
    • I tested this PR with a High Traffic account against the staging or production API
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms and verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors
  • I followed proper code patterns
    • I verified that comments added to the code explain the relevant fallback behavior
    • I verified that the modified copy is grammatically correct and reuses existing translations
  • No new reusable code pattern was introduced
  • I reviewed the applicable code-review guidance
  • I verified both merchant-rule render paths affected by this shared behavior
  • No CSS, assets, message-editing code, generic UI component, Storybook story, or deeplink behavior was changed
  • I added unit tests for the bug fix
  • I retested after merging main into this PR

Screenshots/Videos

To be added with C+ validation.

…on knows the vendor

Co-authored-by: Alex Beaman <Beamanator@users.noreply.github.com>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

I looked into the failing checks — there's no code fix to push. The two failures are both human-gated process checks, not real code/test failures:

Failing check Why it's red Resolution
PR Author Checklist / checklist The author-checklist boxes aren't ticked, and it can't verify engineering membership for the bot author. The human co-author needs to complete the Tests / QA sections and tick the checklist boxes (Melvin intentionally leaves these for a person to fill out).
Check independent approval No independent reviewer approval yet. Needs a reviewer to approve.

Every actual code and test check is green — Bun tests, test jobs 1–8, typecheck, ESLint, Oxfmt, Storybook tests, check, React Compiler Compliance, and the builds all pass. So the failing checks will clear once you fill out the checklist/tests and the PR gets an independent approval; nothing in the diff needs changing.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Nothing has changed since my previous comment — the same two checks are red, and both are human-gated process checks, not code/test failures:

  • PR Author Checklist / checklist — the author-checklist boxes need to be ticked and the Tests/QA sections filled in (Melvin intentionally leaves these for the human co-author).
  • Check independent approval — needs an independent reviewer approval.

Every actual code and test check is green (Bun tests, all 8 test jobs, typecheck, ESLint, Oxfmt, Storybook tests, React Compiler Compliance, builds). There is no code fix to push — these will clear once the checklist/tests are completed and the PR gets an approval.

@Beamanator Beamanator changed the title Fix: render 'Vendor unavailable' instead of raw vendor ID in merchant rules after disconnecting accounting [HOLD] Fix: render 'Vendor unavailable' instead of raw vendor ID in merchant rules after disconnecting accounting Aug 14, 2026
@MelvinBot

Copy link
Copy Markdown
Contributor Author

There are no code-related check failures to fix here — all code checks (Bun tests, ESLint, Oxfmt, typecheck, test (job 1–8), React Compiler, Storybook, perf, builds) are passing.

The only two failing checks require human action, not a code push:

  • PR Author Checklist / checklist — fails because 0/38 author-checklist items are checked. The PR body still has the // TODO: The human co-author must fill out… placeholders for the Tests and QA Steps sections. I can't check these boxes on your behalf — they need a human to fill in the test steps and verify each item.
  • Check independent approval — needs an independent reviewer's approval.

This PR is also still marked [HOLD]. To get the checks green: fill out the Tests/QA sections and check off the author checklist, then get an independent review. Once that's done, ping me if any actual CI check starts failing and I'll dig in.

@Beamanator Beamanator changed the title [HOLD] Fix: render 'Vendor unavailable' instead of raw vendor ID in merchant rules after disconnecting accounting Fix: render 'Vendor unavailable' instead of raw vendor ID in merchant rules after disconnecting accounting Aug 14, 2026
@MelvinBot

Copy link
Copy Markdown
Contributor Author

Status is unchanged since my previous comment — there's still nothing for me to fix here with a code push.

All code checks are passing. The only two failures both require human action:

  • PR Author Checklist / checklist — the 38-item author checklist is unchecked and the Tests/QA Steps sections still have the // TODO: The human co-author must fill out… placeholders. I can't fill these in on your behalf.
  • Check independent approval — needs an independent reviewer's approval.

The PR is also still marked [HOLD]. Once the checklist/Tests/QA are completed by a human and an independent review lands, these will clear. If a real CI check (tests, lint, typecheck, build) starts failing, re-tag me and I'll fix it.

@Beamanator
Beamanator marked this pull request as ready for review August 18, 2026 20:52
@Beamanator
Beamanator requested a review from a team as a code owner August 18, 2026 20:52
@melvin-bot
melvin-bot Bot requested a review from abzokhattab August 18, 2026 20:52
@melvin-bot

melvin-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

@abzokhattab 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]

@melvin-bot
melvin-bot Bot requested review from dylanexpensify and removed request for a team August 18, 2026 20:52
Comment thread src/libs/MerchantTypeRulesUtils.ts Outdated
Comment thread src/pages/workspace/rules/MerchantRulesSection.tsx Outdated
Comment thread src/libs/MerchantTypeRulesUtils.ts Outdated
Comment thread src/pages/workspace/rules/MerchantRulesSection.tsx Outdated
@MelvinBot

Copy link
Copy Markdown
Contributor Author

I looked into the failing checks — there's nothing here I can fix with a code push. The only two failing checks are human-driven gates, not code failures:

  • PR Author Checklist / checklist — fails with PR Author Checklist is not completely filled out. Please check every box. The checkboxes in the PR body still need to be filled in by the human co-author (the Tests and QA Steps sections also have // TODO placeholders).
  • Check independent approval — fails because the PR hasn't received the required independent reviewer approval yet.

Every actual code check is passing (test jobs 1–8, typecheck, ESLint check, Bun tests, Storybook tests, Oxfmt, React Compiler Compliance, all build jobs, codecov, etc.).

To get this green: the human co-author fills out the checklist boxes + Tests/QA sections and marks the PR ready for review, then it goes through normal review to get the independent approval. No push from me is needed or would help.

@abzokhattab

Copy link
Copy Markdown
Contributor

@MelvinBot @codex review the PR and check for regressions, breaking changes or bugs

@Beamanator
Beamanator requested a review from a team as a code owner August 18, 2026 21:06
@melvin-bot
melvin-bot Bot requested review from Beamanator and removed request for a team August 18, 2026 21:06
@melvin-bot

melvin-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

@Beamanator 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]

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Nothing's changed since my previous comment — I re-checked and the only two failing checks are still non-code, human-driven gates that a push can't fix:

  • PR Author Checklist / checklistPR Author Checklist is not completely filled out. Please check every box. The checkbox items (and the Tests / QA Steps sections) still need to be filled in by the human co-author.
  • Check independent approval — no independent reviewer approval yet.

All actual code checks pass. To turn these green: the human co-author fills out the checklist + Tests/QA, marks the PR ready for review, and it goes through normal review for the independent approval. There's no fix for me to push.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Review: no blocking regressions

The core change is correct and safe. In tier 3 of the vendor-name resolution both render paths now fall back to the same Vendor unavailable / Supplier unavailable copy tier 2 already uses instead of leaking rule.vendorID, so the disconnect case (connection data dropped → findVendorByID misses) renders a stable label rather than the raw external ID. The historical-name branch (findVendorByID(...)?.name when a connection still knows the vendor, e.g. after an export-mode switch) is preserved. Changes are localized to the two render surfaces; no other rule surface still uses the raw-ID fallback.

Two things worth flagging (neither blocks merge):

1. The "no false unavailable flash" claim is overstated. The PR description says keeping findVendorByID primary means "no false unavailable flash for valid vendors mid-load." That only holds when a connection's vendor data is already in Onyx. During cold hydration — active integration present but its vendor list not yet synced and no other connection data loaded — a valid vendor now briefly renders Vendor unavailable before resolving, where it previously flashed the raw ID. The updated test VendorMatchingMerchantRulesTest.ts encodes exactly this state — buildQBOPolicy(undefined) is QBO-active with an unsynced vendor list, and it now asserts Vendor unavailable. This is low severity (transient, self-heals on hydration, and arguably cleaner than a raw-GUID flash), but the description should be tempered to say the mid-load fallback changed from raw-ID to unavailable, not that the flash was eliminated.

2. A sibling surface still leaks the raw ID (out of scope, pre-existing). The edit/add form's selected-vendor row at AddVendorPage.tsx:41 still does getMatchingVendorByID(policy, vendorID)?.name ?? vendorID, so opening a rule to edit after disconnecting accounting would still show the raw external vendor ID in that row. Same class of bug as the one being fixed, not introduced by this PR — worth a follow-up so the fix is consistent across the rules UI.

Details & other checks
  • Fix locations: MerchantTypeRulesUtils.ts:176-178 and MerchantRulesSection.tsx:89-91.
  • Translation keys vendorUnavailable / supplierUnavailable exist for en and es and are already used by tier 2 — no missing-copy risk.
  • ModifiedExpenseMessage.ts:512 keeps its own ?? entry.name ?? entry.externalID chain and is intentionally untouched — a different surface with its own name fallback, so no regression there.
  • Minor: searchTokens for an unresolved rule now contains "Vendor unavailable" instead of the raw external ID, so searching rules by external vendor ID no longer matches. Negligible.
  • Tests updated to match the new intent and read consistently with the implementation.

The remaining gap is human verification: the Tests/QA sections are still TODO placeholders, and the described repro (connect Sage Intacct → create rule → disconnect → verify Vendor unavailable) needs to be run on a real workspace before this is marked ready.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f7e7016880

ℹ️ 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".

Comment thread src/libs/MerchantTypeRulesUtils.ts Outdated
Beamanator and others added 2 commits August 18, 2026 15:15
@MelvinBot

Copy link
Copy Markdown
Contributor Author

Fixed the failing Oxfmt check — the only real code failure. (The other two failing checks, PR Author Checklist / checklist and Check independent approval, are human-driven gates, not code issues — the co-author needs to complete the checklist and the PR needs its independent review approval.)

What was wrong: src/pages/workspace/rules/MerchantRulesSection.tsx had a hand-wrapped translate(...) call spread across three lines. It fits on a single line (185 chars, under the repo's printWidth: 190), so oxfmt collapses it — CI failed with Oxfmt diff detected!.

Fix (b78474e): collapsed MerchantRulesSection.tsx:80 onto one line to match oxfmt output. It's a whitespace-only change (no behavior/type impact), so the already-passing tests, typecheck, and lint are unaffected.

Co-authored-by: Alex Beaman <Beamanator@users.noreply.github.com>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

Pushed a fix (473bf5b) for the two real check failures — both were in this PR's own files:

Verified locally: npm run fmt leaves no diff and cspell reports 0 issues on all three files. The rename is a mechanical variable rename (no test-behavior change).

The remaining Check independent approval failure is not a code issue — it just needs the required independent reviewer approval (and the PR Author Checklist filled out + Tests/QA sections completed by the human co-author).

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Re-checked — the checklist is now passing. The only remaining failing check is Check independent approval, which is a human-driven gate, not a code failure: the PR still needs an independent reviewer approval (reviewDecision is currently REVIEW_REQUIRED).

Every actual code check is green (all test jobs, typecheck, ESLint, Bun tests, Storybook, Oxfmt, React Compiler Compliance, all build jobs, codecov, etc.).

There's nothing for me to push here — this just needs a qualifying reviewer to approve the PR to turn the check green.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Update: the checklist check is now passing. The only remaining failing check is Check independent approval, which just needs an independent reviewer to approve the PR — it's a human review gate, not a code failure, so there's nothing for me to push.

A fresh CI run is currently in progress (tests, typecheck, ESLint, perf tests all re-running); those were green before and there are no code-level failures to fix. This PR is ready to go through normal review to clear the independent-approval requirement.

…t rule

Co-authored-by: Alex Beaman <Beamanator@users.noreply.github.com>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

I fixed the one ESLint error that belongs to this PR and pushed it. The rest of the ESLint check failures are pre-existing and unrelated to this PR.

Fixed & pushed (79f7893): the no-negated-variables errors in this PR's own test file — the variable notHydrated (a banned negated name) is renamed to pendingHydration at both tests/unit/VendorMatchingMerchantRulesTest.ts:190 and :249. npx eslint on the file is clean after the change.

Not fixed (unrelated / pre-existing on main): the other errors in the ESLint log are in files this PR does not touch — src/libs/actions/CompanyCards.ts, src/libs/Navigation/helpers/linkTo/index.ts, src/pages/workspace/companyCards/WorkspaceCompanyCardFeedSelectorPage.tsx, CardInstructionsStep.tsx — plus frozen eslint-seatbelt drift. I confirmed all of those files are byte-identical to origin/main, and the branch's config/eslint/eslint.seatbelt.tsv carries stale baseline rows that main has since removed (deprecatedGetReportName entries dropped from 26 → 9). This is branch staleness, not a code problem in this PR.

Why I didn't fix those

The CI lints the whole repo (bun scripts/lint.tseslint .) with the seatbelt in frozen mode, so it surfaces baseline drift for files outside the PR diff. Touching a dozen unrelated files' baselines (or those source files) would be scope creep the guidelines prohibit. The correct remedy is to merge current main into the branch, which refreshes the seatbelt baseline — but I couldn't do that safely here: this is a shallow CI checkout with no reachable common ancestor to main, so an automated merge isn't safe.

Recommendation: merge main into claude-fixRulesVendorUnavailableFallback (git merge main) to refresh the seatbelt baseline and clear the unrelated ESLint failures. My push already cleared the only ESLint error introduced by this PR.

(The checklist and Check independent approval checks are human-driven gates — the co-author still needs to fill out the checklist + Tests/QA sections and get an independent review approval.)

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Re-checked — still nothing to fix with a code push. Since my last comment the PR Author Checklist / checklist check now passes, so the only remaining failing check is:

  • Check independent approval — a human review gate that fails simply because the PR hasn't received the required independent reviewer approval yet.

All code checks are green (test jobs, typecheck, ESLint, Bun tests, build, codecov, etc.); baseline-perf-tests is still running (pending, not failed).

To clear the last check: the PR needs to be marked ready for review and go through normal review to earn the independent approval. No push from me is needed or would help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants