ReportActionCompose 3/6: ComposerProvider with subcomponents#87639
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…orchestrator-leaves
… as thin wrappers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@DylanDylann 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5bbacc3ca
ℹ️ 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".
| disabled={isBlockedFromConcierge || isEmojiPickerVisible()} | ||
| setIsCommentEmpty={() => {}} | ||
| onEnterKeyPress={handleSendMessage} | ||
| shouldShowComposeInput={shouldShowComposeInput} |
There was a problem hiding this comment.
Restore one-way composer autofocus latch
This wrapper no longer passes any didHideComposerInput state into ComposerWithSuggestions, even though that component’s autofocus condition explicitly depends on !didHideComposerInput. The previous one-way latch prevented refocusing after the compose input had been hidden once; without it, flows where SHOULD_SHOW_COMPOSE_INPUT toggles off and back on can unexpectedly autofocus and reopen the keyboard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The one-way latch (didHideComposerInput) was dead code in the original orchestrator. Our PR doesn't pass it (undefined), which produces identical behavior:
- Default case:
shouldShowComposeInputstartstrue→ original latch would befalse→!false = true. Our!undefined = true. Same. - Small screens (native/mWeb):
ReportFooterunmounts the entire composer whenshouldShowComposeInputgoesfalse. On re-show, the component remounts fresh — the latch never had a chance to fire. - Large screens (web): The refocus effect checks
willBlurTextInputOnTapOutside || shouldAutoFocus. On wide layoutswillBlurTextInputOnTapOutside()returnstrue, short-circuiting the check —shouldAutoFocus(and therefore the latch) is never evaluated.
The blueprint PR (#86899) removes the prop and latch entirely from ComposerWithSuggestions. Our split-plan does the same cleanup in PR 4.
There was a problem hiding this comment.
@MelvinBot Could you recheck if this is safe to remove didHideComposerInput
There was a problem hiding this comment.
@MelvinBot, You need to check the code to verify that we are not missing anything. Should not rely on above comments
There was a problem hiding this comment.
Verified directly from code on main — yes, safe to remove. The prop is already dead code today.
didHideComposerInput exists in only one file: ComposerWithSuggestions.tsx (type at line 152, destructured at line 249, used at line 296). No caller passes it. ComposerInput.tsx:104-131 is the sole place ComposerWithSuggestions is rendered, and it does not include didHideComposerInput in its JSX props. ReportActionCompose.tsx has zero references to it — the state hook and one-way latch effect that used to produce this value have already been removed.
Since the prop is optional (didHideComposerInput?: boolean), it defaults to undefined. The only usage is:
// ComposerWithSuggestions.tsx:296
const shouldAutoFocus = ... && !didHideComposerInput;!undefined === true, so the term is already a no-op. Removing it changes zero runtime behavior.
Edge cases checked:
- Large screens (web desktop): Composer stays mounted when
shouldShowComposeInputflips false (ReportFooter.tsx:90), butdidHideComposerInputis alreadyundefinedthere too — no behavioral difference. - Tablets: Same as large screens —
isSmallScreenWidthis false, composer stays mounted, prop alreadyundefined. - Small screens: Composer unmounts entirely when
shouldShowComposeInputis false, so the latch was never needed.
There was a problem hiding this comment.
@MelvinBot You need to check deeper to understand why don't pass didHideComposerInput into ComposerWithSuggestions in ComposerInput is fine
There was a problem hiding this comment.
Dug deeper. Here's the full trace of every code path where didHideComposerInput could matter.
The only logic reference is at ComposerWithSuggestions.tsx:296:
const shouldAutoFocus = (shouldFocusInputOnScreenFocus || !!draftComment)
&& shouldShowComposeInput && areAllModalsHidden() && isFocused
&& !didHideComposerInput;!undefined === !false === true — so omitting the prop only differs from passing true. The question is: can the old latch ever fire (didHideComposerInput=true) while the composer stays mounted?
The latch fires when shouldShowComposeInput goes false. The only writer that sets it to false is ReportActionItemMessageEdit.tsx:557 (user focuses the edit composer for an existing message).
shouldAutoFocus has 4 consumers — here's what happens in each when didHideComposerInput is undefined instead of true:
| # | Usage | Location | Impact of undefined vs true |
|---|---|---|---|
| 1 | autoFocus prop |
line 967 | None. React ignores autoFocus after mount. At mount time, shouldShowComposeInput defaults to true, so the old latch initialized to false — same as undefined. |
| 2 | updateMultilineInputRange |
line 840 | None. Empty [] deps — only runs once at mount, same initial value as above. |
| 3 | Modal-close refocus | line 827 | None. Condition is willBlurTextInputOnTapOutside || shouldAutoFocus. The latch only differs on large screens (where the composer stays mounted per ReportFooter.tsx:90), and on large screens willBlurTextInputOnTapOutside returns true — short-circuiting the OR. |
| 4 | Delayed RHP focus | lines 662-686 | Theoretically different but near-impossible: only fires when shouldDelayAutoFocus=true (stacked SearchReport RHP) AND the user edited a message AND finished editing. In this case the old code blocked refocus, the new code allows it — arguably better behavior. |
Platform-by-platform breakdown
Small screens (native + mWeb): ReportFooter.tsx:90 renders the composer only when shouldShowComposeInput \|\| !isSmallScreenWidth. On small screens, !isSmallScreenWidth is false, so when shouldShowComposeInput=false, the entire composer tree unmounts. On re-show it remounts fresh — the latch never persists across mount boundaries. didHideComposerInput is irrelevant.
Large screens, web wide layout: Composer stays mounted (!isSmallScreenWidth=true keeps the condition true). willBlurTextInputOnTapOutside() returns true (it's !getIsNarrowLayout() on web). This short-circuits all the relevant OR conditions, making shouldAutoFocus redundant for the modal-close refocus path (usage #3).
Large screens, web narrow layout: willBlurTextInputOnTapOutside() returns false, but isSmallScreenWidth would be true → composer unmounts → same as small screen case.
Bottom line: Safe to remove. In every reachable scenario, undefined produces identical behavior to what the old latch produced. The only theoretical gap (stacked SearchReport RHP + mid-edit toggle) is both unreachable in practice and results in better behavior if it ever fired.
There was a problem hiding this comment.
@adhorodyski At this point
Large screens (web): The refocus effect checks willBlurTextInputOnTapOutside || shouldAutoFocus. On wide layouts willBlurTextInputOnTapOutside() returns true, short-circuiting the check — shouldAutoFocus (and therefore the latch) is never evaluated.
shouldAutoFocus is used in other places. Could you also prove that it's safe in all places? Your above point only prove the safe in one place
There was a problem hiding this comment.
@DylanDylann verified directly in code — walking through each shouldAutoFocus consumer in ComposerWithSuggestions.tsx:
Mount-only (unaffected by latch removal):
autoFocusprop on<Composer>(line 967) — React readsautoFocusonly at mount. Latch init isuseState(!shouldShowComposeInput); Onyx default istrue→ latch startsfalse→!false === !undefined === true. Same.updateMultilineInputRangeeffect (line 840) —[]deps, mount-only. Same as above.
Post-mount (where the latch could matter):
3. Modal-close / screen-refocus effect (line 827) — guard is (willBlurTextInputOnTapOutside || shouldAutoFocus) && .... Per platform:
- Web wide:
willBlurTextInputOnTapOutside=!getIsNarrowLayout()→true→ short-circuits OR,shouldAutoFocusnever evaluated. - Web narrow / native phone:
isSmallScreenWidth=true.ReportFooter.tsx:90unmounts the whole composer whenshouldShowComposeInput=false(!shouldHideComposer && (shouldShowComposeInput || !isSmallScreenWidth)). Remount resets state — latch never persists. - Native tablet (iPad):
willBlurTextInputOnTapOutside=falseand!isSmallScreenWidth=truekeeps the composer mounted — the one platform where the latch could theoretically differ. But the effect has a second guard at line 831:if (editFocused) { inputFocusChange(false); return; }— if the user is actively in edit mode, refocus is blocked anyway.editFocusedis the real safety net; the latch was a redundant second layer.
- Delayed RHP focus effect (lines 662-686) — requires
shouldDelayAutoFocus=true(stacked SearchReport RHP). Incompatible with the edit-mode flow that triggers the latch. Unreachable combo.
Only writer of shouldShowComposeInput=false is ReportActionItemMessageEdit.tsx:557 (via setShouldShowComposeInputKeyboardAware), so the latch was only ever tied to message-edit flows.
Safe to remove in every reachable path.
…d architecture Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8e718f3 to
1bb139c
Compare
|
No product review needed |
|
Let's move this section into a new component |
There was a problem hiding this comment.
It's clean to have a new component for this
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bb139c8f3
ℹ️ 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".
This is planned for PR 5 where we introduce useAttachmentUploadValidation hook. Extracting now would require prop-drilling validateAttachments which the hook eliminates. I'd leave it as-is for now. |
…context for isBlockedFromConcierge Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@DylanDylann responded to all comments, pushed updates with the latest commit. back to you |
|
Checking again |
| onPasteFile={onPasteFile} | ||
| onClear={submitForm} | ||
| disabled={isBlockedFromConcierge || isEmojiPickerVisible()} | ||
| setIsCommentEmpty={() => {}} |
There was a problem hiding this comment.
setIsCommentEmpty prop is redundant now, please remove this prop
| disabled={isBlockedFromConcierge || isEmojiPickerVisible()} | ||
| setIsCommentEmpty={() => {}} | ||
| onEnterKeyPress={handleSendMessage} | ||
| shouldShowComposeInput={shouldShowComposeInput} |
There was a problem hiding this comment.
@MelvinBot Could you recheck if this is safe to remove didHideComposerInput
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-04-16.at.16.16.34.movAndroid: mWeb ChromeScreen.Recording.2026-04-16.at.16.13.54.moviOS: HybridAppScreen.Recording.2026-04-16.at.16.23.45.moviOS: mWeb SafariScreen.Recording.2026-04-16.at.16.15.13.movMacOS: Chrome / SafariScreen.Recording.2026-04-16.at.16.09.51.mov |
|
Yes, removing The prop is effectively dead code in all scenarios:
The blueprint PR (#86899) also removes this prop entirely from |
|
@adhorodyski Three questions. The rest looks fine |
…tor-leaves' into decompose/composer-pr3-orchestrator-leaves
|
Applied requested changes and responded here #87639 (comment) to the latch removal aspect, all ready. |
|
|
||
| /** Function to set whether the comment is empty */ | ||
| setIsCommentEmpty: (isCommentEmpty: boolean) => void; | ||
| setIsCommentEmpty?: (isCommentEmpty: boolean) => void; |
There was a problem hiding this comment.
I mean we should remove this prop totally. Do you plan to do it in PR 4?
There was a problem hiding this comment.
Yess let's leave this for PR4 as I have other props that will be dropped there as per #87097 (comment)
|
🚧 @cristipaval 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/cristipaval in version: 9.3.61-0 🚀
Bundle Size Analysis (Sentry): |
|
I reviewed the changes in this PR. This is a purely internal refactoring that decomposes the No user-facing behavior, UI labels, features, or settings are changed — the composer renders and functions identically before and after this PR. No help site documentation updates are required. |

Explanation of Change
PR 3/6 in the ReportActionCompose decomposition series (issue #87097).
Wraps the 834-line
ReportActionComposemonolith inComposerProvider(from PR 1) and extracts 10 child components, each subscribing only to the contexts it needs. The orchestrator shrinks from 834 → 355 LOC with only 2 context subscriptions remaining (for shared attachment/DropZone state).Extracted components (all new files):
ComposerBoxComposerLocalTimeComposerFooterComposerSendButtonComposerExceededLengthAgentZeroAwareTypingIndicatorComposerActionMenuComposerInputWrapperComposerEmojiPickerOther changes:
ComposerProvider:onValueChangenow also callssetValue(v)to keep text context in syncSendButton: removedmemo()wrapper (React Compiler handles memoization)ReportActionCompose: removedmemo(),onSubmitAction(dead code),SuggestionsReftype (re-exported from ComposerContext), alluseCallback/useMemowrappersonSubmitActionimport, uses keyPress insteadWhat stays inline in the orchestrator (355 LOC):
submitForm(message submission logic) — moves touseComposerSubmithook in PR 4useAttachmentUploadValidation+ DropZone render — moves to split hooks +ComposerDropZonein PR 5validateAttachments,attachmentFileRef) which prevents clean separation without the hook extractions from PRs 4+5Context subscription design:
exceededMaxLength, Meta for attachment refs)ComposerFooteris context-free (pure layout), its children self-subscribe independently💡 I recommend reviewing this with Hide whitespace.
Fixed Issues
$ #87097
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests above.
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