[No QA] Fix import/no-cycle - part 3 - #99910
Merged
roryabraham merged 6 commits intoSep 7, 2026
Merged
Conversation
melvin-bot
Bot
requested review from
flaviadefaria and
youssef-lr
and removed request for
a team
August 31, 2026 14:22
|
@youssef-lr 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] |
LukasMod
force-pushed
the
fix/no-cycle-part3-log-network
branch
2 times, most recently
from
August 31, 2026 15:10
e580079 to
b211421
Compare
LukasMod
marked this pull request as draft
August 31, 2026 16:12
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
roryabraham
requested review from
roryabraham and
thelullabyy
and removed request for
flaviadefaria and
youssef-lr
August 31, 2026 22:42
LukasMod
force-pushed
the
fix/no-cycle-part3-log-network
branch
2 times, most recently
from
September 1, 2026 09:58
4a2cde3 to
2142573
Compare
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
LukasMod
marked this pull request as ready for review
September 1, 2026 11:16
roryabraham
previously approved these changes
Sep 1, 2026
roryabraham
left a comment
roryabraham
left a comment
Contributor
There was a problem hiding this comment.
Otherwise, this seems like a safe mechanical change. Nonetheless, I'll await C+ review
Contributor
|
@LukasMod jest tests are failing |
Contributor
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-09-08.at.02.09.30.mov |
Contributor
Author
|
Yup, it was failing on main too. Rebasing all branches |
…ithout importing libs/Network libs/Log imported `post` from libs/Network, which imports MainQueue, which imports libs/Request, SequentialQueue and NetworkStore, all three of which import libs/Log. That one edge held the 15-file network cluster inside oxlint's import/no-cycle graph. Every log packet is queued with shouldProcessImmediately: false, so `post` pushed it and returned without calling MainQueue.process. So the queue and the function that puts a request on it move to Network/MainQueueStore, which imports nothing that reaches libs/Log. MainQueue re-exports push, clear and getAll, so its consumers do not change, and libs/Log imports `enqueue` rather than `post`. No function body changes. `post` is `enqueue` plus the process kick it already made conditionally. import/no-cycle goes from 534 findings across 148 files to 500 across 133, and the libs/Network and libs/Request subsystem leaves the cycle graph. No other oxlint rule count changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
libs/Network runs the main queue bootstrap as a module load side effect: flush the persisted request queue, then start the drain interval once the leader tab is elected. Before the MainQueueStore split, libs/Log imported `post` from libs/Network, so hundreds of modules pulled the bootstrap in. Now the only edge is the `post` import in libs/Reauthentication, which the middleware registry reaches via API/makeRequest, so the bootstrap still runs before any request can be made on every platform. That chain is load bearing and invisible. Nothing failed if a link was dropped or made lazy, the queue just stopped draining. This test loads libs/API with libs/Network mocked and asserts the module was reached, so only the reachability is checked and the real bootstrap never runs. Verified green-red-green: stubbing out the `post` import in libs/Reauthentication fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
libs/Network flushed the persisted request queue and started the drain interval as a side effect of being imported. Before the MainQueueStore split, libs/Log imported `post` from libs/Network, so hundreds of modules pulled that in. After the split the only edge was the `post` import in libs/Reauthentication, reached via API/makeRequest and the middleware registry. Still reachable on every platform, but nothing showed the import was load bearing, and dropping or lazy loading it would have left the queue undrained with no other signal. Wrap the block in `startMainQueue` and call it once from the app setup, next to the other startup wiring that deliberately sits outside the React lifecycle. Position within setup does not matter: on native ActiveClientManager.isReady() resolves immediately so the callback runs on a microtask after setup returns, and on web it waits for ActiveClientManager.init() from Expensify.tsx. This replaces the reachability test added in the previous commit, which guarded an import chain that no longer carries the wiring. Knip covers the remaining failure mode: delete the call and the export goes unused. No behaviour change. import/no-cycle stays at 504 findings across 134 files, with an identical finding set before and after. Full Jest run matches the baseline: 12 failures across DateUtilsTest, SubscriptionUtilsTest and UnreadIndicatorsTest, all three confirmed failing without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LukasMod
force-pushed
the
fix/no-cycle-part3-log-network
branch
from
September 3, 2026 06:41
05f7468 to
26a6e09
Compare
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Contributor
|
@LukasMod conflicts here |
38 tasks
…g-network-v2 # Conflicts: # src/setup/index.ts
Contributor
Author
|
resolved |
roryabraham
approved these changes
Sep 7, 2026
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Contributor
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
Contributor
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
69 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.74-0 🚀
|
Contributor
|
🤖 No help site changes are required for this PR. This is an internal module refactor to break an
I also checked Because no article needs an edit, I did not create a draft docs PR — there is nothing to write that would be accurate and non-duplicative. @LukasMod, there is no linked help site PR to review, since no |
Contributor
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.74-2 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Why
libs/Logimportedpostfromlibs/Network.libs/NetworkimportsMainQueue, andMainQueueimportslibs/Request,SequentialQueueandNetworkStore, all three of which importlibs/Log. Every cycle in that 15-file cluster runs through that one edge.Log never needed the whole of
post. Every log packet is queued withshouldProcessImmediately: false(serverLoggingCallbackinLog.ts), sopostpushed the request and returned without callingMainQueue.process. The comment on that check already says why: a Log command must not re-enterMainQueue.processfrom insideMainQueue.process. So Log only ever used the queueing half.src/libs/Network/MainQueueStore.ts(new)push,clear,getAllmove out ofMainQueueunchanged, plusreplaceAll(a setter for the assignmentprocessused to make directly) andenqueue(the first half ofpost, body verbatim).src/libs/Network/MainQueue.tscanMakeRequest,replayandprocess; re-exportsclearandgetAllso its consumers do not change.pushis no longer exported, nothing outside the module used it.src/libs/Network/index.tspostisenqueue(...)followed by theprocessMainQueue()call it already made conditionally. The main queue bootstrap (leader election, then the drain interval) moves from a module load side effect into astartMainQueue()function.src/setup/index.tsstartMainQueue().src/libs/Log.tsenqueueinstead ofpost.No function bodies change. Two nuances:
processMainQueue()now runs just after thenew Promiseexecutor rather than at the end of it. Same call, same tick.libs/Network. Before the split,libs/Logpulledlibs/Networkinto hundreds of modules, so the bootstrap was guaranteed to be armed. Now the only remaining edge is thepostimport inMiddleware/Reauthentication, reached via the middleware registry, so it is started explicitly from app setup instead of relying on that import surviving.Result
With oxlint
import/no-cycleas the only enabled rule: 534 to 500 findings (-34), 148 to 133 files (-15). With #99670 and #99768 also applied: 458 to 424 findings, 113 to 98 files.Fixed Issues
$ #99650
PROPOSAL:
Tests
Smoke test
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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.native.mov
Android: mWeb Chrome
android.web.mov
iOS: Native
ios.native.mov
iOS: mWeb Safari
ios.web.mov
MacOS: Chrome / Safari
web.mov