[Payment due @rojiphil] Disable Chronos timer header button while OpenReport is in flight#89932
Conversation
Use the per-report RAM_ONLY_REPORT_LOADING_STATE Onyx key (specifically isLoadingInitialReportActions) to disable the start/stop timer button so the user can't fire start/stop comments before the report has finished loading. Co-authored-by: Cursor <cursoragent@cursor.com>
The OpenReport optimisticData leaves isLoadingInitialReportActions=true when the request is queued offline, which would otherwise lock the button indefinitely. Gate the disabled state behind !isOffline so comments queued offline are sent on reconnect, and add a UI test covering that path. Co-authored-by: Cursor <cursoragent@cursor.com>
melvin-bot
Bot
requested review from
heyjennahay and
rojiphil
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e56f9d5b0
ℹ️ 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".
| <View style={[styles.flexRow, styles.alignItemsCenter, styles.justifyContentEnd]}> | ||
| <ButtonWithDropdownMenu<ChronosAction> | ||
| success={!isTimerRunning} | ||
| isDisabled={shouldDisableButton} |
There was a problem hiding this comment.
When isLoadingInitialReportActions flips while the dropdown is already open (for example, a background OpenReport from ReportActionsList runs after the user has opened the Chronos menu), this only disables the split buttons. ButtonWithDropdownMenu still keeps the existing PopoverMenu rendered and builds its menuItems directly from options, so selecting the already-visible Start/Stop Timer item still calls sendCommentToChronos() during the in-flight OpenReport, bypassing the new guard. The timer option itself needs to be disabled/guarded, or the menu closed, when shouldDisableButton is true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
(Tim's AI agent) Agreed — ButtonWithDropdownMenu keeps PopoverMenu mounted with menuItems built from options, so a dropdown that was already open before isLoadingInitialReportActions flipped to true would still let the user select Start/Stop Timer and call sendCommentToChronos. Fixed in 238d1fa: each option in options now sets disabled: shouldDisableButton, and its onSelected short-circuits when shouldDisableButton is true (defense in depth, since the disabled flag alone isn't enough if the popover was already rendered with the prior closures). Also added a regression test (should not send a start timer command from an already-open dropdown when OpenReport flips to in-progress) that opens the dropdown, then flips the loading state, then asserts pressing the visible Start Timer item does not call addComment.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const buttons = screen.getAllByRole('button'); | ||
| expect(buttons.length).toBeGreaterThan(0); | ||
| buttons.forEach((btn) => { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line for @typescript-eslint/no-unsafe-member-access lacks a justifying comment explaining why the rule is suppressed.
Add a comment explaining the reason, for example:
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -- React test instance `.props` is untyped
expect(btn.props.accessibilityState?.disabled).toBe(true);Reviewed at: 1e56f9d | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
(Tim's AI agent) Agreed — CONSISTENCY-5 requires a justification on every eslint-disable directive. Fixed in 54cb34b by appending -- React test instance \.props` is typed as `unknown`; we read the rendered accessibilityState directly here` to the disable line.
| const buttons = screen.getAllByRole('button'); | ||
| expect(buttons.length).toBeGreaterThan(0); | ||
| buttons.forEach((btn) => { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line for @typescript-eslint/no-unsafe-member-access lacks a justifying comment explaining why the rule is suppressed.
Add a comment explaining the reason, for example:
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -- React test instance `.props` is untyped
expect(btn.props.accessibilityState?.disabled).not.toBe(true);Reviewed at: 1e56f9d | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
(Tim's AI agent) Agreed — CONSISTENCY-5 requires a justification on every eslint-disable directive. Fixed in 54cb34b by appending -- React test instance \.props` is typed as `unknown`; we read the rendered accessibilityState directly here` to the disable line.
ButtonWithDropdownMenu keeps the PopoverMenu rendered with menu items built from `options`, so selecting an already-visible Start/Stop Timer item from a dropdown opened before isLoadingInitialReportActions flipped true would still call sendCommentToChronos. Mark each option as disabled and short-circuit its onSelected when shouldDisableButton is true, and add a regression test for the popover-already-open path. Co-authored-by: Cursor <cursoragent@cursor.com>
Address the CONSISTENCY-5 review notes by appending a `-- ` justification explaining why the no-unsafe-member-access rule is suppressed when reading accessibilityState off the React test instance. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
ESLint enforces `unicorn/no-array-for-each`, so loop over rendered buttons with `for…of` instead of `.forEach`. Also add a regression test that covers the Schedule OOO `onSelected` short-circuit when the dropdown is opened first and `OpenReport` flips to in-flight afterward, which raises patch branch coverage on this file. Co-authored-by: Cursor <cursoragent@cursor.com>
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp89932-android-hybrid-001.mp4Android: mWeb Chrome89932-mweb-chrome-001.mp4iOS: HybridApp89932-ios-hybrid-001.mp4iOS: mWeb Safari89932-mweb-safari-001.mp4MacOS: Chrome / Safari89932-mweb-chrome-001.mp4 |
melvin-bot
Bot
changed the title
|
🎯 @rojiphil, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
Co-authored-by: Carlos Martins <cmartins@expensify.com>
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.3.69-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This PR adds a loading-state guard to the Chronos timer header button — a purely internal UI behavior change. There are no existing help site articles under |
|
Tested well in staging |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.69-18 🚀
|
|
🤖 Payment issue created: #90265 |
5 participants
Explanation of Change
OpenReportis in process (but not when the app is offline). This should prevent stale report actions from giving the start/stop timer an incorrect state.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/633688
Tests
Offline tests
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
2026-05-07_10-08-52.mp4
Made with Cursor