Repository navigation
Skip Wide RHP state in EditableCell outside the RHP - #104834
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot please update an issue in pr description to this one #104858 |
|
I updated the Fixed Issues section of the PR description. It now links Performance: Avoid Wide RHP subscriptions in expense table cells outside the RHP. |
|
I tested and updated the PR, @MelvinBot could you mark this pr ready to review |
|
This PR is now marked as ready for review. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: eff7459c40
ℹ️ 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".
| const isDisplayedInWideRHP = useIsDisplayedInWideRHP(isInNarrowPaneModal); | ||
| const [isEditIconFocused, setIsEditIconFocused] = useState(false); | ||
| const isEditable = isLargeScreenWidth && !shouldUseNarrowLayout; | ||
| const isEditable = isLargeScreenWidth && (!shouldUseNarrowLayout || isDisplayedInWideRHP); |
There was a problem hiding this comment.
On an mWeb phone in landscape whose viewport exceeds the large-screen breakpoint, useResponsiveLayout() reports both isLargeScreenWidth and shouldUseNarrowLayout as true. SearchMoneyRequestReportPage still calls useRHPWidth() at src/pages/Search/SearchMoneyRequestReportPage.tsx:196, so its route can also make isDisplayedInWideRHP true; this OR then enables inline editing even though the screen is using the narrow layout. The previous wrapper explicitly required !isSmallScreenWidth before treating a registered route as wide, so this decision also needs that guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch on the missing small-screen check, though it doesn't enable editing there: the report rows fall back to the narrow row, which passes no canEdit, so the only effect was the cell padding. Moved the check into useIsDisplayedInWideRHP anyway so behavior matches the old wrapper exactly, and added a test.
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp104834_android_native.movAndroid: mWeb Chrome104834_android_web.moviOS: HybridApp104834_ios_native.moviOS: mWeb Safari104834_ios_native.movMacOS: Chrome / Safari104834_web_chrome.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 rlinoz 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/rlinoz in version: 9.5.21-0 🚀
|
|
No help site update is needed: this PR only stops expense table cells from re-rendering when the right-hand panel changes, and inline editing works exactly as before. |
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.5.23-0 🚀
|
|
No help site update is needed: this PR only stops expense table cells from re-rendering when the right-hand panel changes, and inline editing works exactly as before. |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.5.23-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Every editable cell in the expense table (amount, tag, category and similar cells in Spend and in the expense report table) subscribed to the Wide RHP state. That state changes each time a side panel opens, closes or changes width. So every cell in every row re-rendered whenever the right-hand panel moved, even when the table was in the main pane and the panel had nothing to do with it.
Now a cell only reads the Wide RHP state when it is actually shown inside the right-hand panel. Cells in the main pane skip that read and no longer re-render when the panel opens or closes. Whether a cell can be edited stays the same as before:
Fixed Issues
$ #104858
PROPOSAL:
Tests
Offline tests
Same as tests, but with the network connection turned off. Verify the app behaves the same and no errors appear.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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
🤖 Generated with Claude Code