Use landscape-aware scrollable layout on Edit Agent and AI Rule pages#93446
Conversation
…e pages In landscape on native, un-dock the footer button into the scrollable view and give the multiline prompt input a minimum height, matching AddAgentPage. Portrait behavior is unchanged. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
I analyzed the checks — there's no code failure to fix. The only red check is Everything substantive is green:
The Next step: complete the |
|
@MelvinBot pull main and fix conflict, simplify Explanation of Change |
…AIRuleFooter Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com> # Conflicts: # src/pages/workspace/rules/AgentRules/AddAgentRulePage.tsx # src/pages/workspace/rules/AgentRules/EditAgentRulePage.tsx
|
🤖 Merged What conflicted:
Resolution: Kept I also simplified the Explanation of Change in the PR description and updated the file paths to their renamed locations.
|
|
@MelvinBot suggest QA steps |
|
🤖 Suggested QA steps (for the human co-author to verify and paste into the QA Steps section). This PR only changes native landscape behavior on three form pages, so QA should cover landscape + portrait on iOS/Android native. The pages are:
Pre-req: an account with the AI Agents / Agent Rules feature enabled. Landscape (the fix) — repeat on iOS Native and Android NativeFor each of the three pages below, rotate the device to landscape before/while the page is open:
On each page in landscape, verify:
Portrait (regression check) — iOS/Android native + webFor the same three pages in portrait:
All platforms
|
DetailsExplanation of ChangeFixed Issues$ #93182 TestsFor each of the three pages below, rotate the device to landscape before/while the page is open:
On each page in landscape, verify:
Offline testsSame as Tests QA StepsSame as Tests
PR Author Checklist
@MelvinBot please set the PR body to the content in the details section above. Just copy |
melvin-bot
Bot
requested review from
heyjennahay and
mkhutornyi
and removed request for
a team
|
@mkhutornyi 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const {translate} = useLocalize(); | ||
| const styles = useThemeStyles(); | ||
| const {windowWidth, windowHeight} = useWindowDimensions(); | ||
| const shouldUseScrollableLayout = useIsInLandscapeMode() || (isMobile() && windowWidth > windowHeight); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The expression useIsInLandscapeMode() || (isMobile() && windowWidth > windowHeight) along with the useWindowDimensions() call is duplicated identically across EditPromptPage.tsx, AddAgentRulePage.tsx, EditAgentRulePage.tsx, and the pre-existing AddAgentPage.tsx. This violates the DRY principle and increases the risk of the logic drifting between files.
Extract this into a dedicated custom hook, e.g.:
// src/hooks/useShouldUseScrollableLayout.ts
import useIsInLandscapeMode from '@hooks/useIsInLandscapeMode';
import useWindowDimensions from '@hooks/useWindowDimensions';
import {isMobile} from '@libs/Browser';
export default function useShouldUseScrollableLayout(): boolean {
const {windowWidth, windowHeight} = useWindowDimensions();
return useIsInLandscapeMode() || (isMobile() && windowWidth > windowHeight);
}Then consume it in each page with a single const shouldUseScrollableLayout = useShouldUseScrollableLayout();.
Reviewed at: da1cf10 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const {translate} = useLocalize(); | ||
| const styles = useThemeStyles(); | ||
| const {windowWidth, windowHeight} = useWindowDimensions(); | ||
| const shouldUseScrollableLayout = useIsInLandscapeMode() || (isMobile() && windowWidth > windowHeight); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
Same duplicated landscape/scrollable detection logic as in EditPromptPage.tsx and EditAgentRulePage.tsx. Extract to a shared custom hook (e.g., useShouldUseScrollableLayout) to eliminate the duplication.
Reviewed at: da1cf10 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const {translate} = useLocalize(); | ||
| const styles = useThemeStyles(); | ||
| const {windowWidth, windowHeight} = useWindowDimensions(); | ||
| const shouldUseScrollableLayout = useIsInLandscapeMode() || (isMobile() && windowWidth > windowHeight); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
Same duplicated landscape/scrollable detection logic as in EditPromptPage.tsx and AddAgentRulePage.tsx. Extract to a shared custom hook (e.g., useShouldUseScrollableLayout) to eliminate the duplication.
Reviewed at: da1cf10 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: da1cf10da5
ℹ️ 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 {translate} = useLocalize(); | ||
| const styles = useThemeStyles(); | ||
| const {windowWidth, windowHeight} = useWindowDimensions(); | ||
| const shouldUseScrollableLayout = useIsInLandscapeMode() || (isMobile() && windowWidth > windowHeight); |
There was a problem hiding this comment.
On mobile web in portrait, focusing this multiline input can shrink the live viewport height below the width, and useWindowDimensions() is based on that resized viewport. When that happens this condition flips shouldUseScrollableLayout even though the device was not rotated, which changes FormProvider from the non-scroll branch to the ScrollView branch and remounts/moves the focused input and footer while the user is typing. Use a real orientation signal or cached/screen dimensions for the mobile-web fallback; the same expression was added to the two Agent Rule pages as well.
Useful? React with 👍 / 👎.
| const {translate} = useLocalize(); | ||
| const styles = useThemeStyles(); | ||
| const {windowWidth, windowHeight} = useWindowDimensions(); | ||
| const shouldUseScrollableLayout = useIsInLandscapeMode() || (isMobile() && windowWidth > windowHeight); |
There was a problem hiding this comment.
@MelvinBot is there any pattern of using isMobile() && windowWidth > windowHeight throughout the app other than AddAgentPage to check if landscape mode on mobile web?
|
Short answer: no. Why the extra For context, ~30 other files use If you'd prefer this not be a copy-pasted expression across four files, it could be extracted into a small shared hook (e.g. |
|
@MelvinBot remove There's bug in msafari-bug.mov |
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@mkhutornyi @NicolasBonet 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 — removed the const shouldUseScrollableLayout = useIsInLandscapeMode();
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movandroid-landscaope.movAndroid: mWeb ChromeiOS: HybridAppios.movios-landscape.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
mkhutornyi
left a comment
mkhutornyi
left a comment
There was a problem hiding this comment.
I left mobile web untouched because of regression in portrait mode, which makes even worse than production.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @NicolasBonet 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/NicolasBonet in version: 9.4.8-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. I reviewed the changes in this PR. This is a pure landscape-orientation layout fix for three pages (Edit Agent prompt, Add Agent Rule, Edit Agent Rule):
There are no user-facing changes that the help site documents:
I checked the existing Agent docs ( Because no help site content is affected, I did not create a draft docs PR. @mkhutornyi, please confirm you agree no help site changes are needed for this PR. If you believe a help article should be updated, let me know which behavior to document and I'll create the draft PR. |
|
🚀 Deployed to staging by https://github.com/NicolasBonet in version: 9.4.8-0 🚀
Bundle Size Analysis (Sentry): |
|
Deploy Blocker ##93631 was identified to be related to this PR. |
|
Deploy Blocker ##93639 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.8-3 🚀
|
6 participants
Explanation of Change
In landscape orientation on iOS/Android, the docked footer (Save/Delete) button ate into the limited vertical space and the page couldn't scroll to reveal the full form, making the multiline prompt input hard to use.
AddAgentPagealready fixed this with a landscape-aware scrollable layout. This PR applies the same pattern to its three sibling pages:src/pages/settings/Agents/Fields/EditPromptPage.tsxsrc/pages/workspace/rules/AgentRules/AddAgentRulePage.tsxsrc/pages/workspace/rules/AgentRules/EditAgentRulePage.tsxIn landscape, the footer moves into the scroll content and the multiline input gets a minimum height. Portrait is unchanged.
Fixed Issues
$ #93182
Tests
For each of the three pages below, rotate the device to landscape before/while the page is open:
On each page in landscape, verify:
The footer button (Save on Add/prompt pages, Save/Delete on the Edit Rule page) is not stuck to the bottom of the screen — it sits at the end of the scrollable content.
The whole page scrolls, so the full form (heading, subtitle, input, disclaimer, and footer button) is reachable.
The multiline prompt input keeps a sensible minimum height (it isn't collapsed to a thin line).
You can type into the input, scroll down, and tap the footer button without the keyboard/footer obscuring the field.
Verify that no errors appear in the JS console
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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.