Fix APP-B2Q (1/3): don't throw in clearComposer when composer ref is not set#94617
Conversation
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
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.
|
|
I’ve debugged the sentry event locally, and here’s the result. breadcrumbs map
Root causePressing Send runs
throw new Error('The composerRef.clearWorklet function is not set yet. This should never happen, and indicates a developer error.');The Proposed solutionThe fix should both stop the crash and avoid silently dropping the attachment in the residual window where the composer ref hasn't re-attached yet.
code example: const clearComposer = () => {
const tryClear = (attemptsLeft: number) => {
const clearWorklet = composerRef.current?.clearWorklet;
if (clearWorklet) {
scheduleOnUI(clearWorklet);
return;
}
// The composer lives on an RNSScreen that gets frozen by react-native-screens while another screen
// (e.g. ReportAddAttachment) is on top. When we clear right after navigating back, the ref may not have
// re-attached yet. Retry on the next frame instead of throwing/dropping the submit, but bound the retries
// so we never loop forever if the composer is truly unmounted.
if (attemptsLeft <= 0) {
Log.hmmm('[ComposerProvider] Skipping clearComposer because composerRef.clearWorklet never re-attached');
return;
}
requestAnimationFrame(() => tryClear(attemptsLeft - 1));
};
tryClear(CONST.COMPOSER.CLEAR_WORKLET_MAX_RETRIES);
}; |
|
@MelvinBot Can you honst compare this proposal #94617 (comment) with your proposal and implement the better one. |
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
Honest comparison — and I implemented yoursShort version: your proposal is better, so I replaced mine with it. Pushed in a72a078. The deciding factor: my fix had a silent data-loss bugI traced the attachment-send path end to end, and your core claim is correct. On the attachment path,
There is no independent send path — Call-chain evidence (file:line)
That's the problem with my original fix: it Why retry beats both alternatives
The retry waits for the frozen What I changed
Lint, prettier, and typecheck pass. The PR body's explanation still describes the old approach — I left that for the human co-author to refresh when filling in the test steps.
|
|
@MelvinBot please update: Explanation of ChangePlease update it to describe the last fix Tests
Offline testsN/A. QA StepsSame as Test step PR Author ChecklistPlease mark all checklist items as checked |
|
@MelvinBot Please mark this PR as ready for review |
melvin-bot
Bot
requested review from
ahmedGaber93
and removed request for
a team
|
@ahmedGaber93 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] |
|
🤖 Marked this PR as ready for review per Heads up: the |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-06-26.at.4.39.04.PM.movAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-06-26.at.4.06.36.PM.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-26.at.3.30.41.PM.mov |
![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: a72a078c25
ℹ️ 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".
| if (attemptsLeft <= 0) { | ||
| Log.hmmm('[ComposerProvider] Skipping clearComposer because composerRef.clearWorklet never re-attached'); | ||
| return; |
There was a problem hiding this comment.
When this fallback is reached after useAttachmentPicker.addAttachment has already stored a file in attachmentFileRef.current, the pending file is left in the provider even though the send was skipped. If the composer reattaches after the 60-frame window or the user later sends another message in the same mounted provider, validateAndSubmitDraft prioritizes attachmentFileRef.current and will send that stale attachment with the next draft. Please clear or otherwise fail the pending attachment state when abandoning the retry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Please address this comment #94617 (comment)
There was a problem hiding this comment.
@MelvinBot Please implement this change
if (attemptsLeft <= 0) {
// We never got the composer back, so the attachment can't be sent. Drop the pending file so a
// future send in this same provider doesn't piggyback the stale attachment onto an unrelated message.
attachmentFileRef.current = null;
Log.hmmm('[ComposerProvider] Skipping clearComposer because composerRef.clearWorklet never re-attached');
return;
}
|
🤖 Done — the PR body now reflects your request:
|
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
@MelvinBot Please update the OP's linked proposal to be #94617 (comment)
|
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks! I would love someone from SWM to review this as well before proceeding
staszekscp
left a comment
staszekscp
left a comment
There was a problem hiding this comment.
The code looks fine, but there is one issue I'd like to raise about worklets. 😄
After talking to one of the maintainers it seems that if we'd like to bump the worklets package to the newest version, the app will stop working. The reason is that some values that we pass as a worklet dependency are not serialized by the package (eg. Date). For now they fail silently, but from the newest version they're going to throw exceptions.
One of the examples is a short line if (listBehavior === CONST.REGULAR) in src/components/KeyboardDismissibleFlatList/KeyboardDismissibleFlatListContext.tsx. In this case the whole CONST object gets serialized and it will cause exceptions with the nearest bump. I wanted to raise this now so we are already aware of a potential issue
Just to clear up - this code is fine, I'm talking about some other places that we need to think about
|
🚧 mountiny 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/mountiny in version: 9.4.25-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.4.25-2 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
This is 1 of 3 independent fixes for the JS throws bucketed under Sentry APP-B2Q (
#93845). It addresses error #1:The composerRef.clearWorklet function is not set yet.clearComposer()readcomposerRef.current?.clearWorkletand threw whencomposerRef.currentwasnull. The provider resetscomposerReftonullon unmount, so a request to clear while the composer is unmounted/remounting — navigating away right after sending, or areact-native-screensfreeze/unfreeze where the ref hasn't re-attached yet — hit thenullref and threw aJSErrorthat propagated across the JSI bridge.On the attachment-send path,
clearComposeris not cosmetic — it is the trigger for the actual send:clearWorkletruns the native input clear, whoseonClearhandler callsvalidateAndSubmitDraft→addAttachmentWithComment. There is no independent send path, so simply returning early when the ref is missing would stop the crash but silently drop the user's attachment.The fix therefore retries instead of throwing or dropping the submit.
clearComposernow pollscomposerRef.current?.clearWorkleton each animation frame, waiting for the frozenRNSScreento unfreeze and re-attach the ref, then fires the real clear/send. The retries are bounded byCONST.COMPOSER.CLEAR_WORKLET_MAX_RETRIES(60 frames ≈ 1s) so we never loop forever if the composer is genuinely gone, in which case it falls back toLog.hmmm. On the normal text-send path the ref is already present, so the clear fires on the first attempt and behavior is unchanged.Fixed Issues
$ #93845
PROPOSAL: #94617 (comment)
Tests
+(compose menu).Offline tests
N/A.
QA Steps
Same as the Test steps above.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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