Repository navigation
[No QA] Fix import/no-cycle - part 12 - #101419
roryabraham merged 8 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
be1e316 to
876c8a6
Compare
Move the new-action subscriber registry into Report/reportActionSubscribers and the task deletion paths into Task/TaskDeletion, so action modules can notify subscribers or cancel a task without importing the Report hub. buildOldDotURL and openExternalLink become leaves too, which lets actions/Session drop its dependency on actions/Link. Each hub keeps re-exporting the names callers still reach it for, so those imports are unchanged. Drop the buildOldDotURL re-export from actions/Link and the SubscriberCallback type export, both dead once Session moved to the leaf. import(no-cycle) findings drop 264 -> 261 on this base; the wider -55 in the plan stays gated on parts 8-11 landing.
876c8a6 to
78d61c1
Compare
…eport-action-subscribers-v1
|
@codex review |
…eport-action-subscribers-v1 # Conflicts: # src/libs/actions/Session/index.ts
|
@MelvinBot review |
|
🤖 Reviewed the diff and drove the three test flows on web. No behavior change found — the moved bodies are line-for-line identical apart from the import renames, and all three flows pass live. Four things to fix, none blocking correctness. 1. The move silently dropped 8 explanatory commentsThis is the main one. A "bodies unchanged" refactor is exactly the kind of diff a reviewer skims, so comment loss slips through. Every one of these existed on
The new file-header docblocks are good, but they explain why the file exists, not what the code does. Please carry the originals over — the 2.
|
| Step | Status |
|---|---|
| ✅ | |
| ✅ | |
Test 3 — OldDot link keeps the email paramCaptured URL:https://www.expensify.com.dev/inbox?authToken=<token>&email=melvinbottestuser%2B...%40gmail.com — correct email, single & before email=, no doubled separators. The host then failed DNS, which is a CI sandbox limit, not an app defect.![]() ![]() |
✅ |
No JS console errorsagent-device does not expose a console reader on web. Proxy check: no in-app error banners in any capture, and the network log showed only expected API calls (OpenReport, CancelTask, OpenTroubleshootSettingsPage, OpenOldDotLink). Worth a human confirming the console directly. |
I did not test the offline variant of Test 3, so the "no authToken when offline" assertion is unverified.
…libs, drop dead Link mock
|
@thelullabyy 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] |
|
No product review needed. |
…eport-action-subscribers-v1 # Conflicts: # src/libs/actions/Link.ts
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-09-21.at.22.44.49.movScreen.Recording.2026-09-21.at.22.44.15.mov |
|
🚧 roryabraham 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/roryabraham in version: 9.4.91-0 🚀
|
|
No help site changes are required, so I did not create a draft PR. This PR is an internal refactor that moves helpers out of How I checked
The closest thing to a behavior touch is @LukasMod, no linked help site PR was needed for this one. If you expect a customer-visible change I missed, reply with the flow and I will re-check and open the draft PR. |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.91-3 🚀
Bundle Size Analysis (Sentry): |







Explanation of Change
Task.ts,IOU/Hold.ts, andSession/index.tseach import the Report actions hub (src/libs/actions/Report/index.ts) oractions/Linkto reach one helper that has nothing else to do with those files. This PR moves each helper into a leaf module and points the caller at it, so no caller needs the hub for it anymore.Changes
Report/reportActionSubscribers.tsnotifyNewActionandsubscribeToNewActionEvent. The registry is a module-scope array that imports only theReportActiontype.useReportActionsNewActionLiveTailandMoneyRequestReportActionsListfor subscribing.Task.ts,IOU/Hold.ts,Chronos.ts, and seven IOU payment flows (PayMoneyRequest,PerDiem,RejectMoneyRequest,SendInvoice,SendMoney,Split,TrackExpense) for notifying.Report/index.tskeeps its own import foraddActions,createNewReport, andshowReportActionNotification, and no longer re-exports either name.notifyNewActionon the hub. 22 of those mocks existed only for that stub and are removed. The four suites that assert on it (RequestMoneyTest,PayMoneyRequestTest,SplitTest,RejectMoneyRequestTest) now mock the leaf.actions/TaskDeletion.tsdeleteTask,clearTaskErrors, andgetNavigationUrlOnTaskDelete.Task.tsimported the hub: they neednavigateToConciergeChatAndDeleteReport,getMostRecentReportID, andoptimisticReportLastData.TaskViewandDynamicReportDetailsPageimport them from the new path. Bodies and comments are unchanged.src/libs/buildOldDotURL.tsandsrc/libs/openExternalLink.tsactions/Link.ts.buildOldDotURLreads the current email fromCurrentUserStore, which replaces theONYXKEYS.SESSIONmirrorLink.tskept for it.UrlUtils:CONFIG.tsimportsUrlUtilsandEnvironmentimportsCONFIG, so that placement would add a cycle.Session/index.tsimports both leaves and dropsLink.tsentirely.Link.tsstill re-exportsopenExternalLink, since 27 modules import it from there, and drops thebuildOldDotURLre-export that knip flagged onceSessionstopped using it.Cycle count tested with
npx oxlintCounting
import/no-cyclefindings only:Parts 8 through 11 are still open and remove parallel paths, so the combined drop is larger than what any single branch shows on its own.
Nothing here touches the UI or adds a dependency.
Fixed Issues
$ #99650
PROPOSAL:
Tests
Test 1: Live message in the open report
Covers
notifyNewActionin its new module. Smoke check only: the list renders from the report actions data, so this catches load and runtime errors, not a missed notification.Test 2: Delete a task that is the only comment
Covers
deleteTask,getNavigationUrlOnTaskDelete, and theTask/TaskDeletionimport path inDynamicReportDetailsPage.Test 3: OldDot link keeps the email param
Covers
buildOldDotURLandopenExternalLink. The email now comes fromCurrentUserStoreinstead of the mirrorLink.tsused to keep, so that param is the thing to watch.email=<your email>, with a single separator before it and no doubled?or&.authTokenin the URL is expected, though a missing one is also fine: the request for it falls back to no token.email=<your email>and now has noauthToken.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
Test1:
web.1.mov
Test2:
web-2.mov
Test 3:
web-3.mov