Repository navigation
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
… screen the user can actually see while the phone keyboard is open. That includes the case from the issue where the visible area is shifted down (`offsetTop` above 0). It hasn't been checked on a real phone yet. Headless browsers can't open a real keyboard, so the browser tests use a stand-in for the browser's visible-area measurements (`window.visualViewport`). The operator retest you planned is still the real check, along with the GPT-6 Astra review. Cause. Chrome on Android leaves the page at 384×692 when the keyboard opens and just moves the visible area down to the text box. The dialog's backdrop is pinned to the full page, so its title and close button stayed at y 43–83, above the visible area. Fix. - New hook, `src/hooks/useVisualViewportFrame.ts`: while the keyboard leaves less visible space than the dialog's backdrop covers, it moves and shortens the backdrop to fit what's visible. It listens to the visible area's resize and scroll events and to window resize (which covers rotation). When the keyboard closes, or the dialog closes, it removes all its listeners and inline styles, so the normal layout comes back. That includes the Linux desktop titlebar spacing. On desktop this case never comes up, so the layout there is unchanged. - If the focused field ends up hidden after the dialog moves or shrinks, it is scrolled back into view. This only happens when the dialog's position actually changes, so it doesn't undo the user's own scrolling. - I left pinch-zoom alone so a zoomed-in reader can still pan around. - `CreationDialog.tsx`: only while the keyboard is limiting the space, the header gets tighter padding and the description is cut to one line. Without this, only about 33px of the prompt was visible. The description is still read out in full by screen readers. Nothing is hidden or disabled, and the draft, selections and navigation are untouched. Other dialogs using this shell. The shared shell is used only by New Task and the goal dialog. Other pop-ups (the task dialog, follow-up and confirmation dialogs) have their own code and are unchanged. Tests. - Browser tests (`e2e/creation-keyboard-viewport.pw.ts`): run against the production build in Chromium, using the measured values (382.133px visible, shifted 186.133px down). For both dialogs they check that: - the title, close button and backdrop are inside the visible area, and enough of the prompt shows to edit it. - every action can be scrolled into view. - the dialog follows when only the visible area's position changes. - closing the keyboard restores the original layout, keeps the 40px close button and the draft, and sends nothing. A third test rotates the phone with the keyboard open, checks that closing the dialog removes all listeners, and reopens it with the keyboard still up to check it isn't left in an old position. All three fail without the fix and pass with it. - Unit tests (`useVisualViewportFrame.test.tsx`): cover following and restoring, opening with the keyboard already up, pinch-zoom, rotation, and listener cleanup. - Existing suites: all 2938 unit tests pass. Lint and the type check are clean. 80 existing browser tests pass, covering the New Task layout, desktop titlebar/window-control, goal and task-dialog suites. WebKit (Safari's engine) wasn't run, because the project's browser tests only run Chromium. Previews. I added two screenshots of the visible area with the keyboard open, one for New Task and one for New Goal.
|
The CreationDialog correction addresses the original finding. While this initial implementation was running, I reproduced the same keyboard viewport problem in three additional production dialogs on the same physical Samsung SM-S908B/Android16/Chrome155, authenticated exact source1b1e904. I added the evidence to issue3243, but the initial task understandably retained its original snapshot. Please extend this same PR with one scoped Claude follow-up before review:
These are visibility defects confirmed by directly viewing OS screenshots. DOM elementFromPoint alone can report a hit for a control above the visible viewport. Tests should cover real rendered controls at positive viewport offsets, opening while the keyboard is already visible, restoration, orientation and listener cleanup. Reuse the shared hook where appropriate; avoid unrelated refactors. Do not hide/disable controls, discard drafts or change permissions/configuration/credentials. No phone comment/task/To-do was submitted during reproduction. Edit and test only; ProPR owns Git/PR finalization. Once these cases are included, update the PR title/summary around its final dialog scope, distinguishing simulated viewport tests from the operator's pending actual-phone retest. Independent GPT-6 Astra review and physical-phone validation remain required before merge. |
|
✅ Applied the requested follow-up changes in commit b691eb4 Summary of ChangesThe three dialogs from your hardware report now follow the visible area the same way New Task does, using the existing shared hook ( What changed
Tests (simulated viewport, production build, Chromium)
One thing to look at on the phone: with only 318px visible, the Logs file list takes up to 35% of the dialog, which leaves about 60px of log text. It works and scrolls, but it's tight. I left it alone to keep the change scoped. I captured three previews of the visible band above the simulated keyboard, one per dialog. Suggested PR title: Keep mobile dialog headers and close controls visible above the on-screen keyboard (New Task, Start a goal, Follow Up, Logs, Create sheet To-do) Suggested summary: The shared visual-viewport hook now keeps five dialogs inside the visible area while an on-screen keyboard is open: New Task, Start a goal, Follow Up on Task, the Logs viewer (and through the same shell, Prompt and diff), and the mobile Create sheet with New To-do expanded. This was checked only with simulated Visual previewFollow Up on Task above the keyboard
Simulated visual viewport (384×692 layout, 382.133px visible, offsetTop 146.4): heading, close, the resolved Issue destination, the comment draft and both actions inside the visible band. Production build, headless Chromium; not a physical-phone capture. Original unavailable: managed storage is unavailable or the upload has expired. Logs search above the keyboard
Simulated visual viewport (318.133px visible, offsetTop 185.333): Log Files title, close, the selected file, the literal search 'acceptance' (1 / 6), match navigation, clear, and the scrolling log with the current match. Not a physical-phone capture. Original unavailable: managed storage is unavailable or the upload has expired. Create sheet New To-do above the keyboard
Simulated visual viewport (382.133px visible, offsetTop 310.667): the Create sheet's title and close, the selected repository, the draft and Add To-Do inside the visible band. Not a physical-phone capture. Original unavailable: managed storage is unavailable or the upload has expired. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…visible area the same way New Task does, using the existing shared hook (`useVisualViewportFrame`). That's Follow Up on Task, Logs search, and New To-do inside the mobile Create sheet. While the keyboard is open, the heading and close button now sit inside the visible band at the offsets you measured. Every browser test uses a simulated `window.visualViewport`, because headless Chromium can't open a real keyboard. Your retest on the physical phone is still the real check.
The three dialogs from your hardware report now follow the visible area the same way New Task does, using the existing shared hook (`useVisualViewportFrame`). That's Follow Up on Task, Logs search, and New To-do inside the mobile Create sheet. While the keyboard is open, the heading and close button now sit inside the visible band at the offsets you measured. Every browser test uses a simulated `window.visualViewport`, because headless Chromium can't open a real keyboard. Your retest on the physical phone is still the real check.
**What changed**
- **Follow Up on Task (`FollowupModal.tsx`):** this dialog stays mounted while it's closed, so the hook gained an optional `active` flag. It only adds its listeners while the dialog is open and removes them when it closes. The comment draft, the destination text ("Post a comment to Issue …"), the submit and receipt handling, and the comment field's focus are unchanged.
- **Logs (`TaskDialog.tsx`):** the hook is added to the shared shell behind the Logs, Prompt and diff viewers. It only acts while the visible area is smaller than the layer, so Prompt and diff behave exactly as before when no keyboard is open. The pane's height cap became `min(85vh, 100%)` so the dialog still fits a rotated phone with the keyboard up.
- **Create sheet (`MobileBottomSheet.tsx`):** the hook is on the sheet's outer layer, and the sheet's height cap now includes `100%` so it shrinks to the visible area. That shared sheet is also used by the More and Usage sheets. They don't change unless a keyboard is open over them.
- Nothing is hidden or disabled. No drafts are dropped, and permissions, configuration and credentials are untouched.
**Tests (simulated viewport, production build, Chromium)**
- A new file, `e2e/dialog-keyboard-viewport.pw.ts`, has 4 tests using your exact measurements (offsetTop 146.4, 185.333 and 310.667; heights 382.133 and 318.133). For each control it checks three things together, since the hit test alone can report a control the keyboard has pushed out of view:
- the control is inside the visible band.
- a scrolling container isn't cutting it off.
- a tap at its centre hits it.
- Each dialog covers typing a short draft, rotating with the keyboard open, closing the keyboard (layout, draft and focus come back), opening while the keyboard is already up, and removing all listeners on close.
- Follow-up also posts with the keyboard open: the receipt takes focus inside the visible band and the duplicate-post guard holds. Logs runs the literal search (1 / 6), steps next and previous through matches with the log scrolling to each, clears, switches files, and returns focus to the button that opened it. The To-do test keeps the selected repository, submits the draft, closes the sheet, and still follows the New Task link.
- All 4 new tests fail without the component changes, with the heading above the band just like on the phone.
- One unit test was added for the `active` flag. To share helpers, I moved the simulated keyboard into `e2e/keyboard-viewport.fixture.ts` and the To-do test data into `e2e/quick-add-todo.fixture.ts`.
- One small hook change: the scroll that keeps the focused field in view is now skipped when the browser lacks it (as in jsdom), matching how `FollowupModal` already does it.
- **Results:** 2939/2939 unit tests pass, and lint and the type check are clean. 102 browser tests pass across 15 suites, including the existing Prompt scrolling, mobile and desktop diff, follow-up, To-do and Create sheet suites. WebKit wasn't run.
**One thing to look at on the phone:** with only 318px visible, the Logs file list takes up to 35% of the dialog, which leaves about 60px of log text. It works and scrolls, but it's tight. I left it alone to keep the change scoped.
I captured three previews of the visible band above the simulated keyboard, one per dialog.
**Suggested PR title:** Keep mobile dialog headers and close controls visible above the on-screen keyboard (New Task, Start a goal, Follow Up, Logs, Create sheet To-do)
**Suggested summary:** The shared visual-viewport hook now keeps five dialogs inside the visible area while an on-screen keyboard is open: New Task, Start a goal, Follow Up on Task, the Logs viewer (and through the same shell, Prompt and diff), and the mobile Create sheet with New To-do expanded. This was checked only with simulated `visualViewport` geometry taken from the Samsung SM-S908B / Chrome 155 measurements in #3243, in headless Chromium. The operator's retest on the physical phone and the independent GPT-6 Astra review are still required before merge.
PR: #3244
Comment by: @integry (ID: 6253369637)
Model: Claude Opus 5.5
integry
changed the title
|
/review gpt-6-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR applies a shared viewport frame to all five requested surfaces, with substantial regression coverage. It needs a localized correction before merge because measuring the original layout can discard the user’s scroll position. ✅ Shared lifecycle handling — The hook handles reopening, removes its listeners on cleanup, and bypasses framing during pinch zoom. ✅ Behavioral coverage — The added browser tests exercise drafts, submission receipts, search navigation, repository selection, and dismissal. This review is based on static analysis; no commands were run. The supplied issue description is truncated, so assessment of its requirements is limited to the visible text. Current checks show no failures and six pending. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Preserve scroll positions during measurement
SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Exercise scroll-only viewport eventsThe fixture always dispatches both S2: 🟢 Strengthen creation visibility assertionsUse the existing ScoreThe implementation is focused and well covered, but the shared measurement path introduces a scroll-preservation regression requiring correction. Current checks have no reported failures, with six still pending. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
Please address Astra blocker F1 and the small test improvements S1/S2 in one Claude refinement, together with the directly confirmed Logs readability problem below. Do not merge yet. Astra review at b691eb4: #3244 (comment). Preserve affected descendant scroll positions while measuring/restoring the viewport frame, and add the requested regression for a selected match near the end remaining visible across a further viewport event. Exercise scroll-only and resize-only notifications independently. Use the existing clipped/hit-tested visibility helper consistently for the creation controls. The final-head physical Samsung retest now passes header/close visibility for all five dialogs. Follow Up and the To-do draft survive keyboard dismissal; literal Logs search, Next match and focus restoration work. Actual phone proof also found a remaining usability failure in Logs with a real selected execution filename and keyboard:
Keep the change scoped to viewport framing and usable dialog/log layout. Do not hide or disable necessary actions, discard drafts, change permissions, configuration or credentials. Edit/test only; ProPR owns Git/PR finalization. Another independent Astra review and exact-head real-phone retest will follow. The existing b691eb4 review and hardware evidence remain recorded with their scope. |
|
✅ Applied the requested follow-up changes in commit 3bf07a4 Summary of ChangesI've fixed F1, S1, S2 and the Logs readability problem; all unit and browser tests pass. The browser tests use a simulated F1 – scroll positions kept while measuring (
S1 – scroll-only and resize-only events: S2 – creation controls: New Task and Start a goal now use Logs readability (
With your measurements (384×692, visible area 318.133px at offsetTop 185.333) and the real filename Validation
I haven't merged anything. Previews show the phone Logs view, the unchanged desktop Logs view, Follow Up and New Task. Visual previewFollow Up above the keyboard
Visible band only, 382.133px at offsetTop 146.4 (simulated); heading, close and actions shown. Original unavailable: managed storage is unavailable or the upload has expired. Logs search above the keyboard (phone)
384x692 layout, visual viewport 318.133px at offsetTop 185.333 (simulated). Production output filename issue-28-2026-10-17T21-15-00-788Z-output.txt and a 400-line log: the file list gives up height, the selected name takes one line, and the log keeps several complete lines with match 1/6 visible. Original unavailable: managed storage is unavailable or the upload has expired. New Task above the keyboard
Visible band only, 382.133px at offsetTop 186.133 (simulated). Original unavailable: managed storage is unavailable or the upload has expired. Logs at desktop width (unchanged)
1280x820 desktop Logs dialog with search; the compact keyboard layout does not apply here. Original unavailable: managed storage is unavailable or the upload has expired. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
|
Additional actual-phone acceptance failure to handle sequentially after the current refinement. Keep one writer on this PR; do not interrupt/restart the active comment6253635834 task. I tested New Task at head b691eb4 with the actual Samsung SM-S908B/Android16 keyboard in landscape. Rotating from portrait dismisses the keyboard, so I explicitly scrolled/focused Prompt again and reopened it before measuring. The IME reports mInputShown=true and mInFullscreenMode=false, and the actual OS screenshot shows the Chrome page above the keyboard (not an OS fullscreen editor). Measured production geometry:
Please make the shared creation shell usable in this genuinely short keyboard frame, including New Task and New Goal. Keep a reachable close action and allow the editor/options/actions to be reached through ordinary touch scrolling when they cannot all fit together. Preserve drafts, repository/model/options and return to portrait. Avoid fixing this by hiding/disablement of actions, discarding the draft, forcibly closing the keyboard/dialog, or moving controls outside the scrollable area. Maintain desktop/native titlebar behavior. A bounding-box-only test or a landscape keyboard viewport260px high does not cover this89px case. Add a production-rendered regression using the actual measured landscape dimensions/offset, with control hit areas and access through scrolling, keyboard dismissal and restoration. Physical hardware retest and independent Astra review remain required on the resulting final head. Edit/test only; ProPR owns Git/PR finalization. |
|
Additional independent browser results to handle sequentially with the already queued landscape case. Preserve one writer and do not interrupt the active refinement. At exact head b691eb4, the same13 production-build suites completed once per engine with retries=0: Chromium148/148 passed; WebKit145 passed and3 failed. No skips. Preserve these failures rather than treating WebKit as green:
The independent environment uses the pinned Playwright image and verified identical runtime dependency inputs, with no host ports or credentials mounted. These are exact failure locations from the completed WebKit report. The operator will independently rerun the corrected final head in both engines and on the phone. Keep fixes scoped to the current modal/scrolling behavior and meaningful test portability; ProPR owns Git/PR finalization. |
|
Validation execution note for the already queued landscape/WebKit batch; no additional product feature or repository-wide testing-policy change is requested. The preceding full browser command reached its25-minute timeout. Its test runner/workers were terminal, but the waiting shell continued polling for Playwright processes. The operator verified those states and stopped only that fingerprinted monitor shell; the same Claude task resumed with its worktree intact. That run is incomplete, not passing. For the queued refinement, run the meaningful affected unit/browser suites on the combined corrected tree. The operator will run the independent combined Chromium/WebKit checks after the queued functional cases settle; repeated full-suite runs between intermediate commits add delay while the known corrections remain pending. Use foreground bounded commands or completion checks that distinguish terminal/zombie processes and avoid matching the monitor itself. Preserve failures, incomplete runs and exact source scope in the report. Do not interrupt or restart the currently active writer. |
… unit and browser tests pass. The browser tests use a simulated `window.visualViewport`, so your real-phone retest is still the real check. I've fixed F1, S1, S2 and the Logs readability problem; all unit and browser tests pass. The browser tests use a simulated `window.visualViewport`, so your real-phone retest is still the real check. **F1 – scroll positions kept while measuring** (`propr-ui/src/hooks/useVisualViewportFrame.ts`) - The hook used to remove the dialog's frame and re-measure on every viewport event. For that moment the log grew taller, so its scroll position was cut back, which is exactly what Astra described. - Now it measures an empty, invisible copy of the dialog's backdrop and never touches the live dialog to do it. It compares against the current frame first and only writes when the frame actually changed. A repeat event with the same geometry changes nothing; a new unit test checks this. - Fixing this exposed a second problem the old reset had been covering up. When the hook brought the focused field back into view, it also scrolled the dialog box itself, which pushed the header up. It now scrolls only the field's own scroll area. When a whole dialog scrolls (Follow Up in landscape), it stops before the dialog's heading would scroll away. - **New regression:** go to the last match (6/6, near the end of a 400-line log), then send a scroll-only event, a resize-only event and another scroll-only event. The log's scroll position stays identical and the match stays visible. This test fails on b691eb4: the position drops from 12648 to 12512. **S1 – scroll-only and resize-only events:** `setKeyboard` and the unit-test viewport can now send either event on its own. The caret-pan steps send only `scroll`. New Task, Start a goal and the hook's unit test also cover a `resize` on its own. **S2 – creation controls:** New Task and Start a goal now use `expectShown` for headings, close buttons and every action. That means each control must be fully drawn, not clipped, and receive a tap at its centre. **Logs readability** (`LogFilesModal.tsx`, plus `group` on the `TaskDialog` backdrop). These changes apply only while the keyboard is shrinking the dialog: - **File list:** it gives up height first, down to a 56px minimum, and still scrolls to every file. The "Select a log file" label is kept for screen readers only. - **Selected filename:** it shows on one line with an ellipsis. Its full text is unchanged, so the heading and the log's accessible name still use the complete filename. - **Spacing:** the padding is tighter, and the log always keeps room for at least three full lines. - **Very short heights:** if the area is still too short (landscape), the whole content area scrolls instead. - **Unchanged:** literal search, 44px controls, focus/Escape, and the layout without a keyboard or on desktop. With your measurements (384×692, visible area 318.133px at offsetTop 185.333) and the real filename `issue-28-2026-10-17T21-15-00-788Z-output.txt`, the log now shows about five full lines, with match 1/6 fully visible. The test checks at least three full lines after typing, after Next/Previous, after each further viewport event and after switching files. It also confirms the filename wraps to two lines when the keyboard is closed. **Validation** - 2,940 unit tests pass, and ESLint (`--max-warnings 0`) and `tsc` are clean. - All 694 Chromium browser tests pass. The full run hit my 25-minute limit at test 516 with no failures, so I ran the remaining 178 separately. - After the last small change (keeping the hook's scroll search inside the dialog), I rebuilt and reran the 28 affected browser tests (keyboard dialogs, To-do, Logs); all pass. I haven't merged anything. Previews show the phone Logs view, the unchanged desktop Logs view, Follow Up and New Task. PR: #3244 Comment by: @integry (ID: 6253635834) Model: Claude Opus 5.5
|
✅ Applied the requested follow-up changes in commit d82c312 Processed 3 comments:
Summary of ChangesI've fixed the 89px landscape case for New Task and New Goal and the three WebKit failures. On the combined tree, the affected browser suites pass in both Chromium and WebKit. Nothing has been checked on the phone yet: the browser tests use a simulated Landscape keyboard (New Task, New Goal)
New regression test (
WebKit failures
Validation (bounded foreground runs, retries=0, no skips)
Previews of both shells in the 89px band, with the header and then the reached actions, are attached. Visual previewStart a goal actions reached by touch scrollingCancel and Start goal reached by touch scrolling, while the close control stays in place. Original unavailable: managed storage is unavailable or the upload has expired. Inline preview unavailable: GitHub size limits or upload failure. No preview files were committed. The preview could not be uploaded to GitHub. New goal landscape keyboard scrolled
Original unavailable: managed storage is unavailable or the upload has expired. Start a goal in the 89px landscape keyboard band
The shared creation shell for New Goal at the same measured landscape geometry: heading and close control are inside the band, with the prompt below. Original unavailable: managed storage is unavailable or the upload has expired. New Task actions reached by touch scrolling
The same band after scrolling the pane with trusted touch drags in Chromium: Cancel, Plan first and Run task are fully visible under the header, which doesn't move. Original unavailable: managed storage is unavailable or the upload has expired. New task landscape keyboard scrolled
Original unavailable: managed storage is unavailable or the upload has expired. New Task in the 89px landscape keyboard band
Production build at the measured 748×298 layout with the visible band 89.333px tall, 208.533px down (simulated visualViewport). The sticky header with Close task creation sits inside the band, with the focused prompt draft below it. Original unavailable: managed storage is unavailable or the upload has expired. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…l and the three WebKit failures. On the combined tree, the affected browser suites pass in both Chromium and WebKit. Nothing has been checked on the phone yet: the browser tests use a simulated `window.visualViewport`, so your hardware retest and the Astra review are still needed on the final head. I've fixed the 89px landscape case for New Task and New Goal and the three WebKit failures. On the combined tree, the affected browser suites pass in both Chromium and WebKit. Nothing has been checked on the phone yet: the browser tests use a simulated `window.visualViewport`, so your hardware retest and the Astra review are still needed on the final head. **Landscape keyboard (New Task, New Goal)** - When the visible band is shorter than 20rem (320px) while the keyboard is open, the creation shell now scrolls as one pane. The header sticks to the top, so the title and the 40px close button stay inside the band. Prompt, repository, Advanced Options and the action buttons are all reached by ordinary scrolling. - To fit a 44px control in that band, the header loses its vertical padding, the description becomes screen-reader-only, and the pane extends over the layer's padding. - Nothing is hidden, disabled or closed, and taller bands keep the previous layout. Desktop and the native titlebar are unchanged, because this mode only applies while the pane is clipped to the keyboard band. - The hook that keeps the focused field visible (`useVisualViewportFrame.ts`) now scrolls whichever container is actually scrolling, and keeps the field below the sticky header instead of under it. **New regression test** (`creation-keyboard-viewport.pw.ts`), one per shell: - It runs against the production build at your measured 748×298 layout with the 89.333px band at offset 208.533. - In portrait it fills a draft, repository, agent and model. It then rotates (keyboard dismissed), refocuses Prompt, reopens the keyboard and checks every control's position and tap target. - Controls are reached by scrolling: trusted touch drags in Chromium, and the browser's own scroll-into-view in WebKit, because Playwright has no touch or wheel input for mobile WebKit. - It then dismisses and reopens the keyboard, returns to portrait, and checks the draft and options are kept, the layout is restored, and no non-GET request was sent. - Against the previous shell code it fails, because only 19px of the prompt is visible. In that run the old header and close button stayed inside the band, unlike on your phone at b691eb4, so the test catches the clipped editor and actions but not that header position. **WebKit failures** 1. **Create sheet New Task link:** this was a real sticky-header occlusion. After editing the To-do, WebKit left the sheet scrolled 45px (Chromium 206px). The link was then inside the scroll area but under the sticky header, so `scrollIntoViewIfNeeded` treated it as visible and didn't move it. The sheet now has scroll padding for its header (`scroll-pt-16` in `MobileBottomSheet.tsx`). The visibility and tap assertion is unchanged. 2. **More sheet `settle()`:** it now waits on the animations still running and tolerates a cancelled one. All geometry, label and focus checks are kept. 3. **Review/diff test:** only the touch swipe needs Chromium (it uses the DevTools touch API). Other engines bring the lines back with the same real wheel input, and the rest of the journey runs in every engine. **Validation (bounded foreground runs, retries=0, no skips)** - Chromium: - 38/38 across creation-keyboard-viewport, dialog-keyboard-viewport, mobile-menu-branding, task-review-diff-mobile, quick-add-todo, new-task and new-task-layout. - 61/61 across automations, goals-regressions and creation-confirm-native-chrome. - WebKit: the same 38 tests, 38/38. The second group was not run in WebKit. - Unit tests: 1507 passed (the hook test and `src/components`). Typecheck and lint on the changed files are clean. - After the last edit, which only moved a preview capture, I re-ran just the two 89px tests in Chromium. - I didn't run the full suite. The landscape tests each take about 25–35s because of the many touch drags. Previews of both shells in the 89px band, with the header and then the reached actions, are attached. PR: #3244 Comment by: @integry (ID: 6253758239) Comment by: @integry (ID: 6253832086) Comment by: @integry (ID: 6254098395) Model: Claude Opus 5.5
|
/review gpt-6-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe shared viewport hook and compact dialog layouts address the reported keyboard geometry directly, but the focused-field scrolling needs a localized correction before merge. ✅ Non-destructive measurement — The hook measures an empty sibling instead of temporarily expanding the live dialog, avoiding the previous descendant scroll-clamping problem. ✅ Lifecycle cleanup — Viewport listeners are removed on dismissal, and the always-mounted Follow Up component explicitly controls activation. ✅ Meaningful visibility assertions — Browser regressions check clipping and centre hit targets, including the measured 89px landscape band. This is a static review; no commands were run. The supplied issue objective contains a truncation notice, so unseen acceptance criteria were not assessed. Current checks report no failures and six pending checks. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F2: 🔴 Keep the editing caret visible
SuggestionsThese are optional follow-ups and are not sent to S3: 🟢 Exercise an actual Logs resizeThe Logs scroll-preservation regression dispatches its resize-only event with the same ScoreThe implementation has strong lifecycle handling and targeted regression coverage, but viewport changes can still hide the active editing line in the landscape case this PR addresses. A focused correction is needed; current checks have no reported failures but remain incomplete. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix F2 S3 |
|
✅ Applied the requested follow-up changes in commit 6cf9c37
Summary of ChangesI've fixed F2 and added the S3 Logs resize case. A tall Prompt in New Task or New Goal now keeps the line being typed on in view when the keyboard area moves or changes height. Both suites pass in Chromium and WebKit, but nothing has been checked on the phone; the browser tests still use a simulated F2 – keep the editing line visible (
S3 – a real Logs resize ( Test results
I also added screenshots of New Task and New Goal in the landscape band with the edited line visible. I didn't find any other problems outside F2 and S3. Visual previewStart a goal: editing line stays visible in the 89px landscape band
Same multiline draft and viewport events in the New Goal dialog; the line holding the caret stays above the keyboard. Original unavailable: managed storage is unavailable or the upload has expired. New Task: editing line stays visible in the 89px landscape band
Visible band above a simulated 89px landscape keyboard (748×298 layout) after a pan and a height change: the edited line 9 of a 12-line prompt is shown under the sticky header, not the prompt's first line. Original unavailable: managed storage is unavailable or the upload has expired. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…mpt in New Task or New Goal now keeps the line being typed on in view when the keyboard area moves or changes height. Both suites pass in Chromium and WebKit, but nothing has been checked on the phone; the browser tests still use a simulated `window.visualViewport`. I've fixed F2 and added the S3 Logs resize case. A tall Prompt in New Task or New Goal now keeps the line being typed on in view when the keyboard area moves or changes height. Both suites pass in Chromium and WebKit, but nothing has been checked on the phone; the browser tests still use a simulated `window.visualViewport`. **F2 – keep the editing line visible** (`propr-ui/src/hooks/useVisualViewportFrame.ts`) - **What changed:** when the focused textarea is taller than the space under the sticky header, `revealWithin` now works out where the caret's line is. It measures this in a hidden copy of the textarea, so lines that wrap on their own are counted the way the field shows them. Then: - if that line is already showing, the scroll position is left alone, so a reader's own scrolling is kept. - if lining up the top of the field would also show that line, it does that (the old behaviour). - otherwise it scrolls just far enough to bring the line into view. - **Heading:** the step that pulls a non-sticky heading back into view can no longer push the editing line under the keyboard. - **Pixel rounding:** Chromium rounds the pane's scroll position to whole pixels, which left the line 0.67px short. The scroll amount is now rounded outward so none of the line is cut off. - **Other dialogs:** the fix is in the shared hook, so New Task, New Goal, Follow Up, Logs search and the New To-do sheet all get it. Shorter fields and non-textarea fields behave as before. - **New tests** (`propr-ui/e2e/creation-keyboard-viewport.pw.ts`), one each for New Task and New Goal: a 12-line draft with the caret on line 9 in the 89px landscape band. The test opens the keyboard, scrolls the pane slightly, then sends a pan-only event and a separate height-change event. It checks that the caret's line is fully below the header and above the keyboard each time, and that the pan doesn't move the reader's scroll. It also checks that typing goes into line 9 and the heading and close button stay visible. With the old hook these tests fail: the caret's line ends up 179px below the visible area. **S3 – a real Logs resize** (`propr-ui/e2e/dialog-keyboard-viewport.pw.ts`) With the last match selected, the test now shrinks the visible area by 40px and then restores it, using height-change events only. It checks that the selected match is still the last one, the match is visible after each change, and the title and close button stay visible. **Test results** - The Creation and Dialog browser suites (11 tests) pass in both Chromium and WebKit, with no retries. - The hook unit tests pass (6), and type checking and ESLint (`--max-warnings 0`) are clean on the changed files. - WebKit isn't installed in this container, so I set it up temporarily under `/tmp`; no repository files changed for that. I also added screenshots of New Task and New Goal in the landscape band with the edited line visible. I didn't find any other problems outside F2 and S3. PR: #3244 Comment by: @integry (ID: 6254595052) Model: Claude Opus 5.5
|
/review gpt-6-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR is merge-ready within the supplied scope, conditional on the pending Validate Changes check completing successfully. No merge-blocking defect was verified in the current diff. ✅ Preserves live scroll positions — The viewport hook measures an empty sibling instead of temporarily resizing the live dialog, and skips mutations when the frame is unchanged. ✅ Handles constrained layouts — Creation dialogs gain a sticky header and whole-pane scrolling for short keyboard bands; Logs reserves readable content space. ✅ Exercises meaningful interactions — Added regressions cover independent scroll/resize notifications, draft preservation, receipts, match navigation, reopening, and listener cleanup. This was a static review; no commands or tests were run. The supplied issue specification contains a truncation notice, so requirements beyond its visible text could not be assessed. Current checks report no failures, with one pending. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S4: 🟢 Cover soft-wrapped caret positionsThe new S5: 🟢 Verify the final physical-device behaviorThe simulated viewport tests provide strong layout coverage, but they replace viewport events and cannot reproduce the operating system’s keyboard and browser caret-panning behavior completely. A final physical-phone check of the corrected landscape interaction would complement them. Historical device failures are not evidence that the current implementation still fails. ScoreThe implementation addresses the visible objective with focused production changes and substantial regression coverage. Static review found no actionable defect; final readiness remains conditional on the pending validation check. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
Additional confirmed production-layout case at reviewed head6cf9c37. Please handle this as one localized refinement on the existing Claude route; preserve the fixed89px Samsung behavior, multiline caret work and all prior regressions. Independent local Android36 Pixel5 emulator, clean Chrome133, actual Gboard keyboard (not a mocked visualViewport and not OS fullscreen editing), production UI with the existing API fixtures:
Make the shared creation shell usable when the keyboard band is smaller than the combined header and one44px action. Keep established touch-target sizes and ordinary scroll access to editor/options/actions and a reachable close action; returning toward the top to reach Close is acceptable when everything physically cannot fit simultaneously. Do not force the keyboard closed, hide/disable actions, or discard drafts/options. Keep the sticky-header behavior for the already-tested89px case where it fits, and preserve desktop/native behavior. Exercise both New Task and New Goal with a production-rendered65.091px regression and actual scrolling/hit checks; also retain multiline caret visibility. I have not independently qualified New Task at65px yet: in that earlier probe its keyboard dismissed, so its keyboard-closed geometry is explicitly not a pass or failure for this case. The152-case Chromium run at6cf passed independently; WebKit is still running. Astra8/no blockers and green CI at6cf did not cover this newly observed65px layout, so I am holding the merge. Please run focused affected tests on the combined correction, report their exact scope and preserve failure results; the operator will do final independent qualification. |
|
✅ Applied the requested follow-up changes in commit 4b79523 Summary of ChangesI've fixed the 65px case in the shared creation shell. When the visible band is too short to fit the header and one 44px action, the header now scrolls with the dialog instead of staying stuck on top of the buttons. Cancel / Start goal / Run task can be scrolled fully into view and tapped. Close is reached by scrolling back to the top, which you said was acceptable. The 89px case keeps its sticky header. Nothing has been checked on a phone or the emulator: the browser tests still use a simulated What changed
Test results (Chromium unless noted, no retries)
Previews show the 65px band for both dialogs: the caret line in the Prompt, the actions after scrolling, and Close after scrolling back up. Visual previewNew Goal, 65px band: actions reached
At the pane's maximum scroll, Cancel and Start goal show whole, take a tap at their centre and are not covered by a sticky header. Original unavailable: managed storage is unavailable or the upload has expired. New Goal, 65px band: scrolled back to Close
Scrolling back to the top brings the heading and close button into view. Original unavailable: managed storage is unavailable or the upload has expired. New Goal, 65px keyboard band: caret line
The visible band with the Start a goal Prompt focused and its caret line in view. Original unavailable: managed storage is unavailable or the upload has expired. New Task, 65px band: actions reached
After touch-scrolling the pane, Cancel, Plan first and Run task show at full 44px height, no longer under the header. Original unavailable: managed storage is unavailable or the upload has expired. New Task, 65px band: scrolled back to Close
Scrolling back to the top brings the heading and close button into view. Original unavailable: managed storage is unavailable or the upload has expired. New Task, 65px keyboard band: caret line
The visible 851x65 band above a simulated Gboard keyboard. The focused Prompt shows the line holding the caret. Original unavailable: managed storage is unavailable or the upload has expired. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
… the visible band is too short to fit the header and one 44px action, the header now scrolls with the dialog instead of staying stuck on top of the buttons. Cancel / Start goal / Run task can be scrolled fully into view and tapped. Close is reached by scrolling back to the top, which you said was acceptable. The 89px case keeps its sticky header. Nothing has been checked on a phone or the emulator: the browser tests still use a simulated `window.visualViewport`, so your independent qualification is still the real check. I've fixed the 65px case in the shared creation shell. When the visible band is too short to fit the header and one 44px action, the header now scrolls with the dialog instead of staying stuck on top of the buttons. Cancel / Start goal / Run task can be scrolled fully into view and tapped. Close is reached by scrolling back to the top, which you said was acceptable. The 89px case keeps its sticky header. Nothing has been checked on a phone or the emulator: the browser tests still use a simulated `window.visualViewport`, so your independent qualification is still the real check. **What changed** - **`propr-ui/src/components/CreationDialog.tsx`**: the sticky header and its 44px scroll padding now only apply when the dialog's space is at least 4.8rem (76.8px) and at most 20rem. The cutoff is the 41px header plus a 44px action. In your 65px case the space is about 57px, so the header scrolls away. In the Samsung 89px case it is about 81px, so the header stays sticky. Nothing changes when the keyboard isn't framing the dialog, so desktop and native layouts are unaffected. Touch targets stay the same (Close 40px, actions 44px), the keyboard is not forced closed, and actions, drafts and options are left alone. - **`propr-ui/src/hooks/useVisualViewportFrame.ts`**: one small fix. When the dialog scrolls back to show the heading, it now rounds that amount down, so the caret's line can't end up under the keyboard by part of a pixel. The first run of the new test caught this: the line was cut by 0.92px. - **`propr-ui/e2e/creation-keyboard-viewport.pw.ts`**: - A new 65.09px test runs for both New Task and New Goal at your 851×289 layout. It sets up a repository, Advanced Options agent/model and a multi-line Prompt with the caret on line 4. - It checks that the caret line is visible, then touch-scrolls in Chromium (WebKit uses the browser's scroll-into-view) to Model and to each action. - Each action is checked at 44px and is the element hit at its centre, including at the furthest scroll position. - It then scrolls back up through Agent and Repository to Close, checks that typing lands on the caret line and stays visible after a resize, and confirms that draft and options survive dismissal with no non-GET requests. - The 89px test now also asserts that the header is still sticky. - Two test helpers were adjusted: the visibility helper no longer treats the header as covering Close itself, and the touch drag starts inside the band once the header has scrolled away. **Test results (Chromium unless noted, no retries)** - Before fixing the dialog, the new 65px test failed for both dialogs with the old dialog code (Model could not be brought into view). It passes with the fix. - One failure along the way: my first version also changed how the heading is restored for fields that aren't being typed in. That broke `dialog-keyboard-viewport.pw.ts:70` (Follow Up on Task in landscape: the heading started at y=127, above the band starting at 203.5). I reverted that part, and the suite passes on the final code. - Final code: - `creation-keyboard-viewport` and `dialog-keyboard-viewport`: 13/13 passed in Chromium and 13/13 in WebKit (run locally with the dependencies extracted by hand). - `creation-confirm-native-chrome`, `new-task`, `new-task-layout`, `new-task-recovery`, `new-task-intake-failure`, `goals-regressions`, `quick-add-todo`: 73/73 passed in Chromium. I did not run these in WebKit. - Unit tests `useVisualViewportFrame.test.tsx` and `modalShells.test.tsx`: 8/8 passed. ESLint with zero warnings and `tsc --noEmit` are clean on the changed files. - I did not run the full browser suite. Previews show the 65px band for both dialogs: the caret line in the Prompt, the actions after scrolling, and Close after scrolling back up. PR: #3244 Comment by: @integry (ID: 6255123034) Model: Claude Opus 5.5
|
/review gpt-6-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR provides shared visual-viewport framing and substantial regression coverage, but needs localized changes before merge: the new height constraints leave controls inaccessible in short keyboard bands in the Create sheet and Logs viewer. ✅ Non-destructive measurement — The hook measures an empty sibling instead of temporarily expanding the live dialog, preserving descendant scroll positions. ✅ Lifecycle handling — Viewport listeners are removed on dismissal, stylesheet positioning is restored, and persistently mounted dialogs are reframed on reopening. ✅ Meaningful visibility assertions — Browser regressions check clipping and centre hit targets, including creation controls in the measured 65px and 89px bands. Current checks report 18 passed, zero failed or pending, and 24 neutral/skipped. This review is static; no commands were run. The supplied original issue excerpt is truncated, so no requirements beyond the visible objective are inferred. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F3: 🔴 Short Create sheets obscure controls
F4: 🔴 Short Logs frames collapse content
SuggestionsThese are optional follow-ups and are not sent to S6: 🟢 Cover soft-wrapped caret positionsThe caret mirror handles wrapping, but the new caret regressions deliberately use short, unwrapped lines. A long paragraph with a backward selection would exercise wrapping and selection-direction calculations. This is optional coverage; no defect in those calculations was established. ScoreThe shared hook and regression coverage are strong, and current checks are green. Two statically demonstrated short-viewport failures remain in changed dialog layouts and require localized corrections. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix F3 F4 Please address both short-frame blockers from the exact-head Astra review in one Claude follow-up, retaining the fixed New Task/New Goal 65px and89px behavior, caret and existing portrait regressions. I independently inspected the sheet sticky header and the Logs constrained flex hierarchy; these need whole-dialog scroll access when header plus an action/readable content cannot fit. Exercise the actual mobile Create → expanded New To-do at a below-md width (748px) with the89.333px band, and Logs at851×289/65.091px; also cover their ordinary portrait recovery, drafts/repository, literal search/match navigation, full readable lines/current match, dismissal and focus return. All relevant controls must each be reachable by normal scrolling while the keyboard stays up, without shrinking touch targets, forcing the keyboard closed or discarding state. Include bounded focused Chromium and WebKit regressions and retain failures accurately. The operator is continuing independent emulator checks; no physical-phone final claim yet. |
|
🔄 Starting work on follow-up changes requested by @integry I'll analyze the 1 request and implement the necessary changes. Processing comment ID: 6256068930✓ |
|
⌛ Processing Delayed: Claude's usage limit was reached while processing requests from @integry. The job has been automatically rescheduled and will restart 11:02 PM UTC on 10/18/2026. Job ID: pr-comments-batch-integry-propr-3244-claude-opus55-3243-claude-opus-5-5-keep-crea-ratelimit-retry-e310c1b0c0f88c58 will run again after delay. |
|
/fix F3 F4 Recover the same incomplete correction, on the existing Claude Opus5.5 route. The prior request6256068930 ran into Claude usage limits; its task is now terminal/cancelled. ProPR automatic retry job pr-comments-batch-integry-propr-3244-claude-opus55-3243-claude-opus-5-5-keep-crea-ratelimit-retry-e310c1b0c0f88c58 is terminal/completed with status skipped, reason already_processed. No implementation container remains and PR head is still4b79523ac4b497c7fd0e88a2b45d82116b9f0d98. This is one supported recovery request, not a second concurrent writer. Preserve any retained worktree edits through ProPR's normal recovery; do not commit/push manually. The operator independently reproduced both Astra blockers on an Android36 emulator with actual Gboard (not mocked visualViewport), production-rendered exact4b UI and existing API fixtures:
Fix both dialogs for normal scroll access to controls/content when the header and a44px control cannot coexist. Preserve the current creation65px/89px fixes and caret work, portrait layouts, task destinations and receipts, literal Logs search, match navigation/current match, focus restoration, To-do draft/repository and dismissal. Do not force the keyboard closed or shrink targets. The existing89.333px below-md To-do regression and65.091px Logs regression requested by Astra must pass, plus this approximately69px To-do boundary where one44px control can fit. Keep at least normal readable text/current-match access when scrolling through the short Logs frame; retain portrait readable-line coverage. Independent validation at4b already passed154Chromium+154WebKit cases across13 affected suites, zero retries/skips/flakes, plus13focused unit tests. Those passing tests do not cover the independently proven F3/F4 layouts, so the merge remains held. Please run bounded focused Chromium/WebKit and unit regressions on the correction and report exact scope/failures. Operator will review the final head with Astra, inspect source and actual screenshots, and independently retest before merging. No new access grants, secrets, unrelated feature work, or production deployment. |
|
/fix F3 F4 The user reports Claude is available again and asks us to continue. Resume the same two outstanding mobile blockers on the existing Claude route, preserving any retained partial work through ProPR normal recovery. Head is still4b79523ac4b497c7fd0e88a2b45d82116b9f0d98 and no implementation container is active for this PR. Previous requests stopped at provider usage limits without a new commit. Do not treat their started checkmarks as completed implementation. This new request is necessary because the older scheduled PR retry would be skipped by the confirmed started-receipt defect tracked separately in issue3247; leave that issue's src/jobs files to its own writer. All scope and independent measurements are in request6256333343 and Astra review6256060447: F3 mobile Create → New To-do at below-md width749 with real69.12px keyboard band leaves only8px below the sticky header; Add To-Do/View Todos remain clipped after trusted swipes. F4 Logs at851×289 with real65.091px band collapses its body; Search/Next match remain0px visible after36touch gestures. Exact-source production UI, actual emulator Gboard, screenshots directly inspected, no real tasks/to-dos/API writes, original OS settings restored. Preserve creation65px/89px action/Close access, multiline caret behavior, portrait readability, drafts/options/repository, literal Logs search/current match/navigation and focus restoration. Let each control/content be reached by normal scrolling with the keyboard up; retain44px targets and do not discard state or force the keyboard closed. Independent4b validation already passed154Chromium+154WebKit cases and13focused units; it does not qualify the remaining failures. Run bounded focused Chromium/WebKit and unit regressions on the correction, report exact scope/failures, and do not claim physical-phone validation. The operator will request fresh Astra review and independently test the final head before an authorized merge. No credentials, access grants, unrelated changes, manual commits/pushes, or production deployment. Additional physical Samsung evidence gathered while this request is still pending (same source4b79523): with real Samsung keyboard at748x298 and89.333px visualViewport height, NewTask and NewGoal action centres can be reached after7/8 trusted touch swipes, drafts/settings survive and Close stays reachable. HOWEVER direct OS screenshot review shows their sticky40px header overlays the top10.67px of the44px Cancel/submit buttons: action y29.333..73.333, header0..40. Include this same sticky-header occlusion correction in both creation shells as well as expanded Create/To-do. A centre hit test plus ancestor clipping is insufficient; assert the complete target is clear of sibling sticky header, preserving target size. No writes were submitted. Multiline line9 was also exercised with real landscape IME:24px line box y68.8..92.8 versus visual bottom89.333; screenshot glyphs remain readable. Do not treat linebox-only clipping as conclusive caret failure, but preserve and independently verify actual caret/glyph visibility after correction. Operator evidence /tmp/propr-pr3244-4b79523-phone-resumed on validation host is not present in your container; numerical observations above are self-contained. Original rotation restored; owned drafts cleared. |
|
✅ Applied the requested follow-up changes in commit 17434f1 Processed 2 comments:
Summary of ChangesI fixed F3 and F4 in the shared shells. In the browser regressions, every control and the log can now each be scrolled fully into view in the short bands from the review, with the keyboard still up. Nothing was tested on a phone or the emulator: the browser tests still use a simulated There were no leftover edits from the earlier runs that hit usage limits (the worktree was clean at F3: New To-do in the mobile Create sheet (
F4: Logs (
Shared hook fixes (
New tests
Results (no retries, nothing skipped)
Trade-offs to know about
Found separately (already on main, not changed here): with no keyboard at 851×289, the Logs viewer only has 25px for the log. The test now only checks that the log returns to that same size after the keyboard closes. Previews show both bands for New To-do and the 65px Logs case. Visual previewLogs Close, 65px band
Scrolling the dialog back to its top brings the Log Files heading and Close into the band. Original unavailable: managed storage is unavailable or the upload has expired. Logs content, 65px band
After Next match, the log shows three full lines with the current match highlighted. Original unavailable: managed storage is unavailable or the upload has expired. Logs search, 851×289 with 65px band
Search field, match count and Previous/Next match scrolled into the 65.09px band; the file list stays beside them. Original unavailable: managed storage is unavailable or the upload has expired. New To-do actions, 69px band (749px wide)
View Todos and Add To-Do wholly visible in the Pixel 5-measured 69.12px band. Original unavailable: managed storage is unavailable or the upload has expired. New To-do Close, 69px band
Create heading and Close reached by scrolling back up in the 69.12px band. Original unavailable: managed storage is unavailable or the upload has expired. Todo sheet short 69.12
Original unavailable: managed storage is unavailable or the upload has expired. New To-do actions, 89px band
View Todos and Add To-Do (44px) scrolled wholly into the 89.333px band with the keyboard up. Original unavailable: managed storage is unavailable or the upload has expired. New To-do Close, 89px band
Scrolling back to the top of the sheet brings the Create heading and Close into the band. Original unavailable: managed storage is unavailable or the upload has expired. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…essions, every control and the log can now each be scrolled fully into view in the short bands from the review, with the keyboard still up. Nothing was tested on a phone or the emulator: the browser tests still use a simulated `window.visualViewport`, so your emulator/Gboard run is still the real check. I fixed F3 and F4 in the shared shells. In the browser regressions, every control and the log can now each be scrolled fully into view in the short bands from the review, with the keyboard still up. Nothing was tested on a phone or the emulator: the browser tests still use a simulated `window.visualViewport`, so your emulator/Gboard run is still the real check. There were no leftover edits from the earlier runs that hit usage limits (the worktree was clean at `4b79523`), so this is a fresh implementation. **F3: New To-do in the mobile Create sheet** (`MobileBottomSheet.tsx`) - While the keyboard clips the sheet, its area becomes the reference for the sheet's height rules. When that height is under 6.75rem (the 61px header plus one 44px control), the header scrolls away with the form instead of sticking. - In that short band the 20px bottom padding is also dropped, since the keyboard already covers the home-gesture bar. Without this, Add To-Do stayed 7px clipped at the bottom of the 69px band even at full scroll. - Portrait (382px band) and the 180px landscape band keep the sticky header. Touch targets stay 44px. **F4: Logs** (`TaskDialog.tsx`, `LogFilesModal.tsx`) - While the keyboard clips the dialog, its outer padding goes to 0. Under 10rem the whole dialog scrolls as one: the header scrolls away and each part takes its natural height, so nothing collapses to zero. - In that band the log keeps its own scroll area, at least three lines tall. At 851px wide the file list sits beside it, so a swipe there scrolls the dialog rather than the log. - The Prompt viewer uses the same shell, so it gets the same short-band layout. **Shared hook fixes** (`useVisualViewportFrame.ts`), both found by the new tests: - When the hook scrolls the dialog back to show its heading, it no longer pushes a focused search field below the keyboard. Before, the search box ended up at y=83–127 in a 65px band. - It now measures the visible area inside a scroll area's border. A 1px border had left the search box clipped by 1px. **New tests** - `e2e/short-band-keyboard-viewport.pw.ts` covers: - New To-do at 748×298 with the 89.333px band, and at 749 wide with the 69.12px band. Your report didn't give that layout's height, so I assumed 289. - Logs at 851×289 with the 65.091px band. - Chromium scrolls with real touch drags; WebKit uses the browser's own scroll-into-view because Playwright can't send real touch input there. - Each check confirms the control is fully visible, not clipped, and is what a tap at its centre hits. The tests also check 44px targets, the draft and repository, literal search, Previous/Next with the current match and three full log lines, keyboard dismissal, Escape and focus returning to the opener, and that nothing is posted. - I moved the Logs task stubs into `e2e/task-logs.fixture.ts` and added a touch-scroll helper to `keyboard-viewport.fixture.ts`. **Results** (no retries, nothing skipped) - **Fail before the fix:** on the `4b79523` source all 3 new cases fail. Ignoring the layout-property checks, the failures are Uncategorized and Add To-Do not fully visible or tappable, and the search box at y=78–122 in the 65px band. - **Chromium:** 52/52 across the new spec plus `dialog-keyboard-viewport`, `creation-keyboard-viewport`, `quick-add-todo`, `mobile-menu-branding`, `task-header-retry-logs`, `task-logs-prompt-dialogs` and `task-review-diff-mobile`. The new spec also passed 9/9 when repeated three times. The 6 other creation suites that use the hook passed 65/65. - **WebKit:** the same 8 files passed 52/52. I set WebKit up locally from extracted packages. - **Units:** 6 files, 54/54. ESLint with zero warnings and `tsc --noEmit` are clean. - One flaky failure during development came from the touch helper measuring before a short scroll glide had stopped. The helper now waits for it, and it hasn't recurred. **Trade-offs to know about** - **Search row and match can't both fit in 65px.** Moving to a match still centres it, as it does at every other size. In the 65px band that scrolls the search row out of view, so the reader swipes back up for the next tap. I didn't change that match-scrolling code, which this PR doesn't otherwise touch. - **File buttons are taller than the band.** A file button with a long name is 74px, more than the 65px band. Its name scrolls fully into view and can be tapped, but the whole button can't be shown at once. **Found separately (already on main, not changed here):** with no keyboard at 851×289, the Logs viewer only has 25px for the log. The test now only checks that the log returns to that same size after the keyboard closes. Previews show both bands for New To-do and the 65px Logs case. PR: #3244 Comment by: @integry (ID: 6256333343) Comment by: @integry (ID: 6256495310) Model: Claude Opus 5.5
|
/review gpt-6-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR centralizes visual-viewport framing and adds compact, scrollable layouts for keyboard-constrained dialogs. The current diff addresses the previously reported short-band failures; I found no verified merge blocker. Merge readiness is conditional on the pending checks completing successfully. ✅ Preserves live scroll geometry — ✅ Handles very short bands — The Create sheet releases its sticky header, while Logs allows whole-dialog scrolling and retains a separately scrollable text area. ✅ Tests observable usability — Added regressions check clipping, centre hit targets, retained drafts, match navigation, viewport-event variants, and listener cleanup. This was a static review of the supplied diff and context; no commands or tests were run. The linked issue excerpt is explicitly truncated, so conclusions cover its visible requirements. Authoritative current checks report zero failures and six pending; historical failures are not treated as current. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S7: 🟢 Assert To-do editing visibilityIn S8: 🟢 Exercise soft-wrapped caret positions
ScoreThe implementation addresses the visible objective with localized changes and substantial behavioral coverage. No actionable defect was verified; browser execution remains unverified by this review, and current checks are still pending. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
1 participant
Opening the Android keyboard can pan the visual viewport while the page keeps its original height, leaving dialog headers and close buttons above the visible area. Keep New Task, New Goal, Follow Up, Logs search, and the expanded mobile To-do sheet inside that area, preserving drafts, search, dismissal, focus return, and Linux titlebar spacing. Logs must retain readable text space while searching with the keyboard open.
The shared viewport hook follows resize/scroll events and restores stylesheet positioning and listeners on dismissal. Production-rendered regressions cover positive offsets, reopening, orientation, receipts, literal search, and To-do interactions. Pinch zoom retains its existing behavior.
Closes #3243.
Current validation at
b691eb4:Independent Chromium: all 148 discovered cases passed exactly once, without retries or skips. Independent viewport/Follow Up unit checks: 10 passed. WebKit completed the same148 cases once:145 passed,3 failed.
Actual Samsung/Android keyboard: all five portrait header/close checks passed; drafts and literal search survived. Direct screen inspection and geometry found only 11px of usable log text space at a 16px line height, so Logs readability remains a blocker.
Astra review scored 6 and raised F1 about possible scroll clamping during measurement. A separate operator probe on the real phone, with the actual keyboard geometry and one explicit synthetic scroll notification, preserved the bottom scroll offset exactly (16709.066 → 16709.066). That narrow negative result does not prove natural pans or other engines safe. Preserve this distinction and validate the targeted regression rather than treating the static trace as an executed reproduction.
One Claude refinement is addressing the readable-space failure, F1 assessment/correction and test suggestions S1/S2. Its resulting exact head needs another independent review and browser/physical-phone qualification before merge.
Actual keyboard-open landscape adds a second hardware blocker: after explicitly reopening the IME (normal mode), layout748×298 has only89.333px visible at offsetTop208.533. New Task close is above that band and its actions are clipped below it. The measured case is queued sequentially behind the active writer; shared creation controls must remain reachable through ordinary scrolling. The device rotation settings and owned draft have been restored.
Queued WebKit corrections: the unedited failure trace frame confirms the New Task row is covered by the sticky Create header after scrolling it into view in the keyboard viewport fixture. The other failures are a canceled finite-animation promise in a settling helper and a Chromium-only CDP call after the cross-engine review/diff assertions. Preserve the behavior assertions and classify engine-specific gesture coverage explicitly.