Scroll to and focus the message put into edit mode by ArrowUp - #100432
Scroll to and focus the message put into edit mode by ArrowUp#100432MelvinBot wants to merge 1 commit into
Conversation
…ts and gets focus Co-authored-by: Fitsum Abebe <FitseTLT@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
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. |
|
@MelvinBot request my review and make it ready for review |
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.
|
|
🤖 Android test steps verification results: The documented steps can't be exercised on Android native. The ArrowUp-to-edit hotkey is gated off on any narrow-layout touch device by Setup caveat: the test account had no workspaces, so no chat with consecutive report previews existed. The tester built 14 messages in the Concierge chat to get scrollable history instead. That reproduces the "target message is outside the render window" precondition but not the exact report-preview layout from the issue.
The fix itself still needs verification on desktop web, where the bug actually reproduces. Android shows no regression. |
|
@FitseTLT @ChavdaSachin One of you needs to 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] |
|
Done — this PR is out of draft and your review is requested. Going out of draft also auto-added Two things still need a human before this can merge:
|
![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: 839039619a
ℹ️ 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".
|
|
||
| // The action being edited can be scrolled out of the list's render window, in which case its editor never mounts and never | ||
| // takes focus. Ask the list to scroll to it so it mounts, gets focused and is visible while it's edited. | ||
| requestScrollToEditingAction(lastReportAction.reportActionID); |
There was a problem hiding this comment.
When an expense report takes the shouldDisplayMoneyRequestActionsList branch in MoneyRequestReportView, this shared composer still creates the request, but only ReportActionsList consumes it; MoneyRequestReportActionsList never reads or clears the request. If the newest editable comment is outside that unified FlashList's render window, pressing ArrowUp therefore still leaves the inline editor unmounted and unfocused. Handle the request in both list implementations or place the handling in their shared list infrastructure.
Useful? React with 👍 / 👎.
|
|
||
| // A message put into edit mode from the composer (ArrowUp) can sit outside the list's render window, so its editor never mounts and never | ||
| // takes focus. Scroll to it here, where the rendered indexes are known, so the row mounts and the message stays visible while it's edited. | ||
| const {pendingScrollToEditingReportActionID} = useReportActionActiveEdit(); |
There was a problem hiding this comment.
While a message is being edited, setEditingMessage and setCurrentEditMessageSelection update fields in this same context on essentially every keystroke or selection change. Adding useReportActionActiveEdit() here makes the entire ReportActionsListContent rerender whenever any of those unrelated fields change, repeatedly rebuilding list props and callbacks for large histories while the user types. Put the pending scroll ID in a narrowly scoped context or event channel so the list subscribes only to scroll requests.
Useful? React with 👍 / 👎.
garrettmknight
left a comment
garrettmknight
left a comment
There was a problem hiding this comment.
Looks good from a product perspective, but this is a pretty niche bug. Let's fix it while we're here.
3 participants
Explanation of Change
Pressing
ArrowUpin an empty composer puts the newest editable message into edit mode, but on desktop web the list never scrolled to that message and the inline editor never got focus.The message list is a virtualizing
InvertedFlashListwith a finitedrawDistance. When tall content sits above the newest message (for example several consecutive report previews) and the user has scrolled up, the target row is outside the render window, soReportActionItemMessageEditnever mounts — and focus (autoFocus) plus the browser's native scroll-into-view only happen on mount. The one edit-start scroll that exists is gated to mobile Chrome (ReportActionItemMessageEdit.tsx), so desktop web was excluded, and theArrowUphandler only saved a draft with no follow-up scroll.The fix asks the list to scroll to the action that
ArrowUpjust put into edit mode:ReportActionEditMessageContext.tsx— addspendingScrollToEditingReportActionIDplusrequestScrollToEditingAction/clearPendingScrollToEditingAction. The composer and the list both already live under this provider, so it is the natural place to pass the request between them.ComposerWithSuggestions.tsx— aftersaveReportActionDraft, theArrowUpbranch callsrequestScrollToEditingAction(lastReportAction.reportActionID).ReportActionsList.tsx— consumes the request, resolves the action's index againstrenderedVisibleReportActions(the same array the list renders, and the same lookup already used foractionBadgeTargetIndex), and callsreportScrollManager.scrollToIndex(...). The row then mounts,autoFocuslands, and the message is visible while it is edited.The scroll is scoped to the
ArrowUpshortcut, so starting an edit by clicking Edit comment on an already-visible message is unchanged.AI Tests
Run locally on this branch:
npm run typecheck— passednpm run lint-changed— passednpm run spell-changed— passed (3 files, 0 issues)npm run react-compiler-compliance-check -- check <changed files>—ReportActionsList.tsxandReportActionEditMessageContext.tsxpass.ComposerWithSuggestions.tsxreports 7 pre-existing "Cannot access refs during render" errors on both compilers; verified identical on the unmodified file (same 7 errors, line numbers shifted only by this diff), so no regression and no new compiler divergence.npm test -- --silent --testPathPattern "(ReportActionsListTest|ReportActionComposeTest|ReportActionItemMessageEditTest|ReportActionMessageEditLayoutTest|ComposerTest)"— 6 suites, 61 tests passed200. Manual verification of the flow is still needed.Fixed Issues
$ #99760
PROPOSAL: #99760 (comment)
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
Offline tests
// TODO: The human co-author must fill out the offline tests they ran before marking this PR as "ready for review".
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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