[No QA] Revert "[No QA] Fix import/no-cycle - part 1 - follow up" - #100456
[No QA] Revert "[No QA] Fix import/no-cycle - part 1 - follow up"#100456MelvinBot wants to merge 1 commit into
Conversation
…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>
|
🤖 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
Caveat on the console check: |
|
|
|
The failing check is not related to this PR — I did not push a fix. Analysis: The only real failure is 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:
A single assertion whose duration swings from 9s to 56s across concurrent runs is contention, not code. Nothing merged to This PR also can't reach that test: it only touches Why this test specifically, and how to de-flake it
Two tests take that path, and the log shows the cost is entirely one-time:
Under The targeted fix is to stop charging the cold Babel load to one arbitrary test — warm it once in a 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. |





Explanation of Change
Reverts #100009 (merge commit
8ed3660f6c377227b194f5d1b0e32c0859935678) at the request ofroryabraham.That PR turned
src/libs/Middleware/register.tsfrom a side-effect module into an exportedregisterMiddlewares()function guarded by ahasRegisteredflag, and updated every caller to import and call it. This revert restores the previous behavior:register.tsruns itsaddMiddlewarecalls at module scope, and callers go back to the bare side-effect importimport '@libs/Middleware/register';.Files restored to their pre-#100009 form:
src/setup/index.ts— side-effect import instead of aregisterMiddlewares()call inside the default setup function.storybook/preview.tsx— side-effect import instead of a call beforeOnyx.inittests/ui/SearchPageTest.tsx,tests/unit/TransactionGroupListItemTest.tsx— side-effect importstests/utils/TestHelper.ts— no explicit registration; it already imports@src/setup, which registers the middlewares on importtests/unit/MiddlewareRegistrationTest.ts—require('@libs/Middleware/register')instead of calling the functiontests/unit/MiddlewareEntryPointTest.ts—require('@src/setup')at module scope instead of callingappSetup(), so the setup-dependency mocks that PR added are no longer neededConflict resolution:
HandleMovedScanFailedExpensesmiddleware landed onmainafter #100009 merged, sogit revertconflicted in two files. Both conflicts were resolved to keep it:src/libs/Middleware/register.ts—HandleMovedScanFailedExpensesis still registered, at module scope, immediately beforeSaveResponseInOnyx(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 13The 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".
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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'sGITHUB_BASE_REFis set to an empty string, so the script resolves the base ref toorigin/and errors out before checking anything. No components or hooks are modified by this revert (the changed.tsxfiles 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
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari