Skip to content

[No QA] Revert "[No QA] Fix import/no-cycle - part 1 - follow up" - #100456

Draft
MelvinBot wants to merge 1 commit into
mainfrom
claude-revertExplicitMiddlewareRegistration
Draft

[No QA] Revert "[No QA] Fix import/no-cycle - part 1 - follow up"#100456
MelvinBot wants to merge 1 commit into
mainfrom
claude-revertExplicitMiddlewareRegistration

Conversation

@MelvinBot

Copy link
Copy Markdown
Contributor

Explanation of Change

Reverts #100009 (merge commit 8ed3660f6c377227b194f5d1b0e32c0859935678) at the request of roryabraham.

That PR turned src/libs/Middleware/register.ts from a side-effect module into an exported registerMiddlewares() function guarded by a hasRegistered flag, and updated every caller to import and call it. This revert restores the previous behavior: register.ts runs its addMiddleware calls at module scope, and callers go back to the bare side-effect import import '@libs/Middleware/register';.

Files restored to their pre-#100009 form:

  • src/setup/index.ts — side-effect import instead of a registerMiddlewares() call inside the default setup function
  • .storybook/preview.tsx — side-effect import instead of a call before Onyx.init
  • tests/ui/SearchPageTest.tsx, tests/unit/TransactionGroupListItemTest.tsx — side-effect imports
  • tests/utils/TestHelper.ts — no explicit registration; it already imports @src/setup, which registers the middlewares on import
  • tests/unit/MiddlewareRegistrationTest.tsrequire('@libs/Middleware/register') instead of calling the function
  • tests/unit/MiddlewareEntryPointTest.tsrequire('@src/setup') at module scope instead of calling appSetup(), so the setup-dependency mocks that PR added are no longer needed

Conflict resolution: HandleMovedScanFailedExpenses middleware landed on main after #100009 merged, so git revert conflicted in two files. Both conflicts were resolved to keep it:

  • src/libs/Middleware/register.tsHandleMovedScanFailedExpenses is still registered, at module scope, immediately before SaveResponseInOnyx (the ordering constraint is unchanged)
  • tests/unit/MiddlewareEntryPointTest.ts — the expected middleware count stays at 14, not the pre-[No QA] Fix import/no-cycle - part 1 - follow up #100009 value of 13

The middleware list and its ordering are otherwise unchanged.

Fixed Issues

$ #99650
PROPOSAL:

Tests

// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".

  • Verify that no errors appear in the JS console

Offline tests

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."

  • Verify that no errors appear in the JS console

AI Tests

Run locally on the revert branch, all passing:

  • npm run lint-changed
  • npm run typecheck
  • npm run spell-changed ✅ (7 files, 0 issues)
  • npm test -- tests/unit/MiddlewareEntryPointTest.ts tests/unit/MiddlewareRegistrationTest.ts ✅ (4 tests)
  • npm test -- tests/ui/SearchPageTest.tsx tests/unit/TransactionGroupListItemTest.tsx ✅ (27 tests)
  • npm test -- tests/unit/APITest.ts tests/unit/APIWriteWhenReadyTest.ts tests/unit/MiddlewareTest.ts tests/unit/LoadTestMiddlewareTest.ts tests/unit/LoggingMiddlewareTest.ts ✅ (96 tests)

Could not run locally:

  • npm run react-compiler-compliance-check check-changed — the runner's GITHUB_BASE_REF is set to an empty string, so the script resolves the base ref to origin/ and errors out before checking anything. No components or hooks are modified by this revert (the changed .tsx files are two Jest tests and .storybook/preview.tsx, and the only change in each is an import line), so CI should cover it.

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 steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

…le-part1b-register-middlewares-explicitly"

This reverts commit 8ed3660, reversing
changes made to 000e925.

Conflicts resolved to keep HandleMovedScanFailedExpenses, which landed on
main after PR 100009 merged:
- src/libs/Middleware/register.ts: restored module-scope addMiddleware calls,
  keeping HandleMovedScanFailedExpenses registered before SaveResponseInOnyx.
- tests/unit/MiddlewareEntryPointTest.ts: restored the module-scope require of
  @src/setup, keeping the expected middleware count at 14.

Co-authored-by: Rory Abraham <roryabraham@users.noreply.github.com>
@MelvinBot
MelvinBot requested a review from a team September 6, 2026 17:34
@melvin-bot melvin-bot Bot added Melvin-Test-Android Melvin-Test-Web Triggers Melvin to run the testing steps of the PR on web labels Sep 6, 2026
@MelvinBot

MelvinBot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Web test steps verification results: ✅ pass

App loaded to Home with no HTTP/network errors, and opening the Concierge chat rendered full report content, confirming SaveResponseInOnyx middleware still processes API responses after the revert to module-scope registration.

Concierge chat report open with rendered message content

Step Status
App loads to Home with no console errorsHome screen rendered fully (tabs, composer, action buttons); all network requests returned status=200; no 'error' or 'Something went wrong' text found on screen. Direct JS console capture unavailable on web platform for agent-device.
Home screen after completing onboarding Join step
Middleware still processes API responses - opened report renders contentClicked Inbox tab, OpenReport API returned status=200, and the Concierge chat report rendered its full message text, sender name, and timestamp - proof the response was saved into Onyx by the middleware.
Concierge chat report open with rendered message content
Re-check console/network after navigation - no new errorsPost-navigation network dump showed all requests (Log, OpenReport, PusherPing, AuthenticatePusher, bundle fetches) returning status=200; no error text found on screen.

Caveat on the console check: agent-device has no JS-console capture on the web platform (logs returns UNSUPPORTED_OPERATION), so "no console errors" rests on all network requests returning 200 and no error text appearing on screen — not a literal console transcript.


view run · view recording

@MelvinBot

MelvinBot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Android test steps verification results: ✅ pass

App loaded to Home with no errors after onboarding, the Concierge report opened and rendered its full message content (confirming SaveResponseInOnyx middleware is registered and persisting API responses), and the app remained stable and error-free after subsequent navigation.

Inbox/LHN after back navigation, no errors, app responsive

Step Status
App loads to Home with no errors (no error banner, no crash, no "Something went wrong")After completing onboarding (Join, notification prompt, work-email skip, intent selection, name entry), Home rendered normally with the greeting 'Good evening, Melvin.', bottom nav (Home/Inbox/Spend/Workspaces/Account), and Concierge prompt box. A full snapshot grep for error/crash/'something went wrong' text returned no matches.
Home screen loaded with no error banners
Middleware still processes API responses — opening a chat/report from the LHN renders its content (only works if SaveResponseInOnyx middleware is registered)Opened the Concierge chat from the Inbox (LHN). The report screen fully rendered: header 'Concierge' with subtitle 'Your personal AI agent', and the message list showed the actual message content ('Welcome to Expensify' / 'Hi! I'm Concierge...' and 'Ask me anything!' / 'Concierge can answer questions, update expenses, and more.'). This content only appears if the API-fetched report data was persisted into Onyx by the SaveResponseInOnyx middleware and re-rendered from the store.
Concierge report open with rendered message content
After navigating, no new errors appeared and the app is still responsiveTapped Back from the report, returning cleanly to the Inbox/LHN list still showing the Concierge chat preview text. Tapped the Home tab afterward and the app continued to respond to taps with no error banners, crashes, or stuck states. A full-snapshot grep for error/crash text again returned no matches.
Inbox/LHN after back navigation, no errors, app responsive

No errors were found at any point in the run — no error banners, no "Something went wrong" screens, no crashes.


view run · view recording

@MelvinBot

Copy link
Copy Markdown
Contributor Author

⚠️ Claude's API is currently overloaded (HTTP 529). This is a temporary issue on Anthropic's side. Try again in a few minutes, or check status.anthropic.com.

Run details

@MelvinBot

Copy link
Copy Markdown
Contributor Author

The failing check is not related to this PR — I did not push a fix.

Analysis: The only real failure is Bun testsoxcTransformer > leaves test files on babel-jest so jest.mock is hoisted, which timed out after 5000ms. It's a CI resource-contention flake, not a logic failure. (Check independent approval is the only other red check and just needs a human review.)

Evidence: The exact same test timed out on six other PRs in the same ~10 minute window, all reporting the failure at ~18:26 UTC:

Run Branch Duration
34050626156 fix-04/90366 9,053ms
34048907052 removeVendorGateIntacct 16,475ms
34050430083 claude-fetchPickerCategoriesTagsOnDemand 27,189ms
34051499873 krishna2323/issue-96354 39,395ms
34049026270 this PR 47,515ms
34049949975 fix/remove-beta-flag-93854-part-6 56,556ms

A single assertion whose duration swings from 9s to 56s across concurrent runs is contention, not code. Nothing merged to main between 15:00 and 18:00 UTC, so the regression isn't from a code change either — every Bun tests run before ~17:25 UTC passed.

This PR also can't reach that test: it only touches src/setup, src/libs/Middleware/register.ts, and four test files. The failing test exercises config/babel/oxcJestTransformer.js, which imports none of them.

Why this test specifically, and how to de-flake it

oxcJestTransformer.js:11 builds the babel-jest transformer, but the expensive work — loading babel.config.js and the full RN + React Compiler preset chain — is only paid on the first babelTransformer.process() call.

Two tests take that path, and the log shows the cost is entirely one-time:

Under bun test --parallel (8 threads) on a loaded runner, that cold load blows past bun's default 5000ms per-test timeout. Both tests are synchronous, so bun can't interrupt them — it flags a timeout at 5s while the call keeps running, which is why the reported duration far exceeds the limit.

The targeted fix is to stop charging the cold Babel load to one arbitrary test — warm it once in a beforeAll with a generous timeout, or give both babel-path tests an explicit third-argument timeout (it(name, fn, 60_000)).

I'm deliberately not making that change here: it's a repo-wide CI fix that shouldn't ride along in a revert PR, and it would land unreviewed on six other affected branches' behalf. roryabraham — re-running the Bun tests job should get this PR green. If you want the de-flake, reply and I'll open a separate PR for it.

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

Labels

Melvin-Test-Android Melvin-Test-Web Triggers Melvin to run the testing steps of the PR on web

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants