Add onSelected handler to Chronos timer dropdown option#87823
Conversation
The timer option in ChronosTimerHeaderButton was missing an onSelected callback, so selecting "Start timer" from the dropdown popover was a no-op. This adds the handler to call sendCommentToChronos when the timer option is selected from the dropdown menu. Co-authored-by: Tim Golen <tgolen@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
This comment was marked as resolved.
This comment was marked as resolved.
Tests both the main button press and dropdown menu options: - Renders the Start Timer button - Main button press triggers addComment with start command - Dropdown shows both Start Timer and Schedule OOO options - Start Timer dropdown selection triggers addComment - Schedule OOO dropdown selection navigates correctly Co-authored-by: Tim Golen <tgolen@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('renders the Start Timer button', async () => { |
There was a problem hiding this comment.
I think this test is not necessary since the other tests wouldn't work if the start button doesn't render.
There was a problem hiding this comment.
Removed this test.
| renderComponent(); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| // Open the dropdown menu |
There was a problem hiding this comment.
Use the give/when/then format for test comments
There was a problem hiding this comment.
Updated all test comments to use given/when/then format.
| ); | ||
| }); | ||
|
|
||
| it('shows both Start Timer and Schedule OOO options in the dropdown menu', async () => { |
There was a problem hiding this comment.
This test also isn't necessary since the other tests are testing the functionality of the buttons and they wouldn't pass if the items weren't rendered.
There was a problem hiding this comment.
Removed this test as well.
This comment was marked as resolved.
This comment was marked as resolved.
…comments - Removed 'renders the Start Timer button' test (redundant with other tests) - Removed 'shows both options in dropdown' test (redundant with functional tests) - Reformatted test comments to given/when/then format Co-authored-by: Tim Golen <tgolen@users.noreply.github.com>
| })); | ||
|
|
||
| jest.mock('@hooks/useAncestors', () => ({ | ||
| __esModule: true, |
There was a problem hiding this comment.
@MelvinBot I don't think this is necessary, is it?
There was a problem hiding this comment.
You're right — it's not strictly necessary. useAncestors calls useOnyx on report collections, which would return undefined in the test's empty Onyx store, and getAncestors would return [] either way. The mock just avoids those three Onyx subscriptions, but the test passes without it. Removed it.
There was a problem hiding this comment.
@MelvinBot this mock is still here... which I think the hook is OK, but I was specifically referring to if __esModule: true, is necessary. If it's not, please remove it from all mocks.
There was a problem hiding this comment.
Tested this — __esModule: true is required. Without it, the Babel interop treats the mock as a CommonJS module and wraps the entire object as default, so useCurrentUserPersonalDetails becomes { default: () => ({...}) } instead of just () => ({...}). The test suite fails with:
TypeError: (0 , _useCurrentUserPersonalDetails.default) is not a function
This happens because LocaleContextProvider (and other consumers) import these hooks as default imports, and Jest's module interop needs __esModule: true to resolve default correctly. Keeping them as-is.
| default: () => ({accountID: 12345, timezone: {automatic: true, selected: 'America/Los_Angeles'}}), | ||
| })); | ||
|
|
||
| type PopoverMenuItem = { |
There was a problem hiding this comment.
@MelvinBot Can you use existing types for this (and the type below)?
There was a problem hiding this comment.
Done — replaced both custom types with PopoverMenuItem and PopoverMenuProps imported from @components/PopoverMenu.
|
All code checks are passing (ESLint, Prettier, typecheck, all 8 test jobs, etc.). The only failing check is |
Co-authored-by: Tim Golen <tgolen@users.noreply.github.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
…nts/PopoverMenu Co-authored-by: Tim Golen <tgolen@users.noreply.github.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@dukenv0307 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] |
melvin-bot
Bot
requested review from
joekaufmanexpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,194 @@ | |||
| /* eslint-disable @typescript-eslint/naming-convention */ | |||
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
ESLint rule disables without justification can mask underlying issues and reduce code quality. The file-level /* eslint-disable @typescript-eslint/naming-convention */ lacks a comment explaining why the naming convention rule needs to be disabled for this file.
Add a justification comment, for example:
/* eslint-disable @typescript-eslint/naming-convention -- Mock module paths use non-standard naming conventions required by jest.mock */Reviewed at: 4e810c6 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| function MockPopoverMenu({isVisible, menuItems, onClose, onItemSelected}: PopoverMenuProps) { | ||
| if (!isVisible) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
ESLint rule disables without justification can mask underlying issues. The eslint-disable-next-line for @typescript-eslint/no-unsafe-assignment lacks an explanation.
Add a justification comment, for example:
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- jest.requireActual returns an untyped module object
const {View, Text, TouchableOpacity} = jest.requireActual('react-native');Reviewed at: 4e810c6 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| /** | ||
| * Finds the dropdown arrow button (the split button with expanded accessibility state). | ||
| */ |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
ESLint rule disables without justification can mask underlying issues. The eslint-disable-next-line for @typescript-eslint/no-unsafe-member-access lacks an explanation.
Add a justification comment, for example:
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -- Accessing untyped props on RNTL rendered elements to find the dropdown button
const dropdownButton = buttons.find((btn) => btn.props.accessibilityState?.expanded !== undefined);Reviewed at: 4e810c6 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
…/App into claude-fixChronosTimerDropdown
|
@AndrewGable 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] |
|
Timer dropdown button doesn't show Start Timer or Stop Timer on iOS Native. Should we fix it here? Screen.Recording.2026-04-14.at.09.55.50.mov |
|
Thanks @dukenv0307, I've raised #87828 to fix that one. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-04-14.at.23.25.09.movAndroid: mWeb ChromeScreen.Recording.2026-04-14.at.09.49.35.moviOS: HybridAppScreen.Recording.2026-04-14.at.23.11.13.moviOS: mWeb SafariScreen.Recording.2026-04-14.at.09.43.03.movMacOS: Chrome / SafariScreen.Recording.2026-04-14.at.09.41.00.mov |
|
🚧 @AndrewGable 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.3.60-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a bug fix that restores an existing |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.60-22 🚀
|
5 participants
Explanation of Change
PR #87543 added a second dropdown option ("Schedule OOO") to
ChronosTimerHeaderButton, converting it from a single button into a splitButtonWithDropdownMenu. The timer option was missing anonSelectedcallback, so selecting "Start timer" from the dropdown popover calledonOptionSelected?.(item)which was undefined — a no-op. This adds theonSelectedhandler to the timer option so thatsendCommentToChronosis called when the user selects it from the dropdown menu.Fixed Issues
$ #87815
PROPOSAL: #87815 (comment)
AI Tests
npm run typecheck-tsgo): passednpm test -- --testPathPattern=Chronos): 13/13 passedreact-compiler-compliance-check: could not run due to pre-existing stale git lock file in CI environment (unrelated to this change)Human Tests
QA
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))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./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos