PR7: Remove global Onyx reference from getOriginalReportID - #96669
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cc87765ad
ℹ️ 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".
… ensuring consistency across the codebase
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.
|
…oserWithSuggestions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebc9743265
ℹ️ 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".
|
job 2, typescript failing |
|
@situchan The PR is ready |
ReviewLooks good — this is a clean, correct continuation of the "remove global Onyx from What changed: Verified:
Minor observations (non-blocking)
I did not run typecheck/CI locally; the automated checks on the PR remain the source of truth for that. |
Potential regression foundDraft auto-save while editing a message can silently stop working in active reports. In const {saveDraft: debouncedSaveReportActionDraft} = useDebouncedSaveDraft(
useCallback(
(comment) => saveReportActionDraft(reportID, editingReportAction, reportActions, comment),
[reportID, editingReportAction, reportActions], // reportActions is new
),
);
The debounce window is 1000ms. So in a busy report, if a report action arrives within 1s of the user's last keystroke, the queued draft-save is dropped. Before this PR the closure depended only on Repro: open a high-traffic chat, start editing a message, type, pause, and while another message/reaction lands in that report (<1s), navigate away or reload before typing again → the in-progress edit is not persisted as a draft. This defeats the stated purpose of the debounced save ("allows one to navigate somewhere else and come back to the comment and still have it in edit mode"). Suggested fix: keep the debounced callback identity stable — read the latest Other things I checked (no functional regression)
I analyzed this from the code; I did not runtime-verify the draft-loss repro. Confidence: medium-high on the mechanism. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd4f9140b1
ℹ️ 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".
| saveReportActionDraft(reportID, editingReportAction, reportActions, comment); | ||
| }, | ||
| [reportID, editingReportAction], | ||
| [reportID, editingReportAction, reportActions], |
There was a problem hiding this comment.
Keep edit-draft debounce independent of report actions
Including the whole reportActions object in this debounced callback makes any report-action Onyx update in the active chat (for example, another message arriving or being edited) recreate the callback; useDebounce cancels the pending trailing call whenever its func changes. In narrow-layout edit mode, if that happens within the draft-save debounce window after the user types and they then navigate away or reload, the latest edit draft is never written to Onyx, so keep the debounced saver stable and pass/resolve the needed owner actions without depending on the mutable collection.
Useful? React with 👍 / 👎.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid2.movAndroid: mWeb ChromeiOS: HybridAppios2.moviOS: mWeb SafariMacOS: Chrome / Safariweb2.mov |
Regression Test CasesEmoji reactions (
Editing a message + draft persistence (
Deleting a comment (
Priority focus: Steps 7 and 8 target the one real regression risk (debounced edit-draft save being cancelled in active reports). Steps 2, 6, 11 cover thread-parent resolution, which is where the removed global fallback mattered most. If the debounce fix is applied, re-verify 7–8 specifically. |
|
@cretadn22 please address above comments. |
…component to simplify parameters and improve readability. Remove unnecessary comments in deleteReportComment function.
|
@codex review |
|
@MelvinBot review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Re-review — the flagged regression is fixed ✅The debounce regression I raised earlier has been correctly addressed in How the fix resolves it: Verification detail & remaining minor notes (non-blocking)
Runtime check: I tried to smoke-test the emoji-reaction add/remove and edit-draft-persist happy paths on web, but couldn't — those flows are gated behind a message-row hover, and the web driver has no hover/long-press/right-click primitive, so the UI never surfaced. This is a test-tooling limitation, not a code concern; the assessment above is from code analysis + green CI. I agree with |
Regression sweep — no new regressions found ✅I traced every path this PR touches. The one real regression previously flagged (debounced edit-draft save being cancelled in busy reports) has been fixed in Why the core change is safe: Call-site-by-call-site verification
Cases I explicitly checked for divergence from the old global-fallback behavior — all match:
Remaining non-blocking items (unchanged from prior review)
Runtime check: I couldn't drive these flows on web — reactions/edit/delete are all reached through a hover-revealed toolbar, and the ArrowUp/Escape edit path needs raw key events; the web test driver supports neither. So the assessment above is from code analysis + the green test suite (which includes the new thread-parent and one-transaction unit tests for exactly these paths). Bottom line: the debounce regression is fixed, and I found no further regressions in the |
|
@cretadn22 are all comments addressed? Please pull main |
|
@situchan All requested updates are in place and ready for review |
|
🚧 mollfpr 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. |
Explanation of Change
Fixed Issues
$ #66419
PROPOSAL: #66419
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Screen.Recording.2026-07-28.at.00.39.10.mov