Repository navigation
[3276 by Claude Opus 5.5] Short landscape task viewport leaves changed files covered and untappable - #3277
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…indow, the changed file can now be scrolled into view with a finger or the mouse wheel, tapped, its diff opened and closed. I tested this in headless Chromium against stubbed task data. It has not been run against real API data or on a real phone. Cause. From 640px wide up, the task page uses the desktop layout. It pins the task header and gives only the area below it its own scroll. In a short window that area was too thin: about 100px in my reproduction, 58px in your measurement. The pinned Timeline and Files headings and the log bar covered nearly all of it, so the file row could never be tapped. Fix. - New `useShortWorkspace.ts`: it measures how much height the header leaves for the timeline/files area. It counts the collapsed log bar and the pinned headings, and allows about 184px for rows. It also respects the visible area when an on-screen keyboard shrinks it, but ignores pinch zoom. The measurement doesn't depend on which layout is active, so it can't flip back and forth. - In `TaskDetails/index.tsx`, when there isn't enough room: - the whole task scrolls as one page, the way phones already do. - the header scrolls away with it, but all its metadata and actions are still there when you scroll back up. - the Timeline and Files headings still pin, now to the top of the page. - the execution-log bar stays pinned at the bottom, and opening it keeps its normal height. - extra scroll padding keeps a focused or revealed row clear of the pinned headings and the log bar. - Unchanged: tall desktop windows, portrait phones, how the timeline heading sits above its content, and returning focus to the file row when the diff closes. The diff dialog's existing scroll lock still holds the page in place. Tests. The new `e2e/task-details-short-viewport.pw.ts` covers 844×390 touch landscape, 880×480 desktop, 320×640 and 390×844 portrait, and 1280×820 desktop. Each uses a completed PR task with a long two-line title and subtitle, 8 runs and one modified 121-line Markdown file. It: - scrolls only with real touch swipes or wheel input. - checks that a tap at the row's centre actually reaches the row and that the visible area is usable. - opens and closes the diff, then checks focus and scroll position are restored. - opens and closes the execution log. - scrolls back up and opens the header's "More task actions" menu. On the old code the two short cases fail the way the issue describes: at 844×390 the row can't be scrolled to a tappable spot, and at 880×480 only 51px of the area is visible. One existing test changed behaviour. In `task-timeline-heading-layering.pw.ts`, the case that docks the task beside the list at 1280×820 with 150% text now uses the new page-scroll layout. There, the enlarged header is 423px tall and leaves only about 97px for rows, the same problem as the issue. The test now scrolls the whole page and still checks that nothing paints over the Timeline heading. Results. All 270 tests in the `task-*` browser suites pass, as do 424 TaskDetails unit tests, eslint with zero warnings and tsc. The new spec passed 3 times in a row. I didn't touch any files from #3273, #3274 or #3275, and added no dependencies. The 844×390 and 880×480 previews show the revealed file row and the opened diff. Separately, Gmail, Google Calendar and Google Drive need authorizing in your claude.ai connector settings before they can be used. This task didn't need them.
|
/review gpt-6-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR addresses the cramped landscape workspace by making the whole task scroll when the measured space beneath its header is insufficient. The implementation is focused and appears merge-ready within the supplied scope, conditional on pending checks completing successfully. ✅ Adaptive layout — The hook measures the actual header and available viewport height, accommodating wrapping content and enlarged text instead of relying solely on screen width. ✅ Observer cleanup — Resize observers and viewport listeners are removed when the measured elements change or the component unmounts. ✅ Interaction-focused coverage — The new tests exercise touch and wheel scrolling, file-row hit testing, diff opening and closing, focus restoration, and execution-log controls across five viewport configurations. This was a static review; no commands or tests were run. The issue description contains a truncation notice, so requirements beyond its supplied excerpt could not be assessed. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Exercise live viewport transitionsThe new tests initialize each viewport independently. A follow-up test could resize an already-scrolled task between tall desktop, short landscape, and portrait layouts, checking file reachability and restored scroll ownership. This would strengthen coverage of the observer-driven transitions in S2: 🟢 Repeat authenticated mobile acceptanceThe added browser tests use stubbed task data, while the original report came from authenticated testing with real data. Repeating that original scenario on a mobile browser would provide useful confirmation of header dimensions and touch behavior. This is additional acceptance evidence, not a demonstrated code defect. ScoreThe change directly addresses the reported layout problem and includes substantial interaction coverage. No merge-blocking defect was established from the supplied code; the authoritative check summary reports no failures and six pending checks. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
Independent exact-head validation at ba5e743 found a reproducibility problem in the new short-viewport browser spec. Please refine it through the existing Claude route before merge. The production UI was built from that exact head with unchanged dependencies. All 424 TaskDetails unit tests passed. In a fresh browser run with PROPR_CAPTURE_PREVIEWS=1 and retries=0, the new 844x390 landscape case failed line154 with a 6px scroll difference after closing the diff, and the 1280x820 desktop case failed the same assertion with 4px. The other three new viewport cases and all nine timeline-heading cases passed. The file opener and diff did work in both failing cases. An operator-only diagnostic copy changed only the scroll-settlement wait from three consecutive stable animation frames to24, retaining the <=1px assertion, normal trusted touch/wheel input, and unchanged product source. Both previously failing cases then passed: landscape scrollTop was673 before capture,673 after capture and673 after close; desktop73/73/73. This points to the pre-interaction momentum/settlement measurement rather than an established layout defect. An independent rotation probe also initially missed its first tap while using a short100ms post-swipe wait; that probe is being measured separately. Please make the committed test reliably observe the end of touch/wheel momentum and measure the baseline at the actual pre-open position. Retain the strict scroll/focus/hit-test assertions and real input; do not loosen the1px limit, force clicks, or assign scrollTop. If investigation establishes a product restoration defect, fix that instead and explain the evidence. Repeat the five viewport cases with and without preview capture and preserve the timeline-heading coverage. No unrelated changes or permission/configuration work. Final authenticated signed-native integration remains an operator release gate. |
|
✅ Applied the requested follow-up changes in commit 7129482 Summary of ChangesYour exact-head failures didn't reproduce in this container, but while ruling out a product cause I found and fixed a real restoration defect in the short layout. That defect plausibly explains the 6px landscape failure. The 1280×820 failure is still unconfirmed: my best explanation is that the wheel scroll hadn't finished when the baseline was measured, and the new wait now covers that. Product defect. In the short layout, the bottom scroll padding was
The top edge and every position at 1280×820 moved 0px. I set the padding in Test changes (
Results:
Limits:
There are no visual previews because the change isn't visible in a still image. Separately, Gmail, Google Calendar and Google Drive need authorizing in your claude.ai connector settings before they can be used; this task didn't need them. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…, but while ruling out a product cause I found and fixed a real restoration defect in the short layout. That defect plausibly explains the 6px landscape failure. The 1280×820 failure is still unconfirmed: my best explanation is that the wheel scroll hadn't finished when the baseline was measured, and the new wait now covers that. Your exact-head failures didn't reproduce in this container, but while ruling out a product cause I found and fixed a real restoration defect in the short layout. That defect plausibly explains the 6px landscape failure. The 1280×820 failure is still unconfirmed: my best explanation is that the wheel scroll hadn't finished when the baseline was measured, and the new wait now covers that. **Product defect.** In the short layout, the bottom scroll padding was `scroll-pb-12` (48px), but the pinned log bar is 37px (36px plus a 1px top border). When a file row sat fully visible within about 12px above the bar, putting focus back on it as the diff closed counted it as covered and scrolled the page. A diagnostic run placed the row at fixed distances from each edge, then opened and closed the diff: | Row's gap above log bar | 0px | 2px | 5px | 10px | 20px+ | |---|---|---|---|---|---| | Scroll movement, 844×390 and 880×480 | 11px | 9px | 6px | 1px | 0 | The top edge and every position at 1280×820 moved 0px. I set the padding in `useShortWorkspace.ts` to exactly the bar's height (`scroll-pb-[calc(2.25rem+1px)]`). After that change, every position moved 0px in all three sizes. The 1280×820 layout has no such padding, so its 4px isn't explained by this defect. **Test changes** (`e2e/task-details-short-viewport.pw.ts`): - **Waiting for the scroll to end:** the old check was three animation frames without movement. It now waits for the scroll's `scrollend` event and then 300ms with nothing moving. If `scrollend` never fires, it falls back to 5s. - **Baseline position:** it is now measured where the row rests just before it is tapped: after the swipe's momentum, after the preview capture, with a fresh tap point. - **New check in the 880×480 case:** the mouse wheel moves the row to 1–6px above the log bar, then the diff is opened and closed. With the old padding this fails with an 8px difference; with the fix it passes. - **Timing:** I removed a duplicate wait inside the scroll-into-view step and gave these five cases a 60s timeout. Each case does many swipes and each now waits for its momentum to end, so landscape takes about 14s on an idle host and 30–32s under full CPU load, which is over the default 30s. - The 1px limit, the focus and tap-target checks, and real touch/wheel input are unchanged. There are no forced clicks, and the test never sets the scroll position itself. **Results:** - **Repeat runs:** the five viewport cases plus the nine timeline-heading cases passed three times with preview capture and three times without, 84/84. - **Under full CPU load:** the five viewport cases passed once with capture and once without. - **Unit tests, lint and type check:** all 424 TaskDetails unit tests pass, and eslint (zero warnings) and tsc are clean. - **One failure along the way:** before the 60s timeout, an overloaded run timed out in the landscape case and in the unchanged timeline-heading enlarged-text case. **Limits:** - **Old wait never failed here.** It never returned before the scroll ended in this container: not with the CPU slowed, not under load, not with smooth-scrolling Chromium. The 1280×820 explanation therefore comes from your 24-frame result, not from a failure I reproduced. - **Edge diagnostic was not real input.** It set the scroll position directly, and it is not committed. - **Remaining gates:** a real phone, real API data, and signed-native integration are still your release gates. There are no visual previews because the change isn't visible in a still image. Separately, Gmail, Google Calendar and Google Drive need authorizing in your claude.ai connector settings before they can be used; this task didn't need them. PR: #3277 Comment by: @integry (ID: 6279336708) 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 addresses cramped task workspaces by allowing the header to scroll away while retaining access to the execution log. The changes are focused and appear merge-ready within the supplied review scope, conditional on pending checks and the stated operator release gate. ✅ Adaptive layout — The hook measures available space and header height, observes resizing, and leaves the existing phone layout intact. ✅ Strict interaction checks — The revised browser test measures the baseline after settlement and preview capture, preserving real input, focus restoration, hit testing, and the ≤1px scroll assertion. ✅ Layering coverage preserved — The existing enlarged-text test follows the new scroll owner while retaining its heading coverage checks. This was a static review; no commands or tests were run. The original issue excerpt is explicitly truncated, so requirements beyond the visible excerpt could not be assessed. Current checks report no failures and six pending checks. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S3: 🟢 Exercise live viewport transitionsConsider extending ScoreThe implementation is focused, and the follow-up retains strict assertions while improving settlement and baseline measurement. No merge blocker was established from the supplied code; runtime confirmation remains subject to pending checks and the operator integration gate. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
CI failed: Validate Changes Please investigate and fix this CI failure.
Failure evidence
|
Checks FailedValidation failed during setup, tests, CLI packaging, linting, or build checks. View Logs |
|
✅ Applied the requested follow-up changes in commit 2eae2f8 Processed 2 comments:
Summary of ChangesI fixed the Validate Changes failure. The cause was a race in this PR, not the logs dialog: the listener-leak check in What went wrong. The new Fix (tests only, no product code changed):
Verification:
This change isn't visible in the UI, so there are no previews. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…in this PR, not the logs dialog: the listener-leak check in `e2e/dialog-keyboard-viewport.pw.ts` measured its baseline before the task page had finished loading. I fixed the Validate Changes failure. The cause was a race in this PR, not the logs dialog: the listener-leak check in `e2e/dialog-keyboard-viewport.pw.ts` measured its baseline before the task page had finished loading. **What went wrong.** The new `useShortWorkspace` hook adds a resize listener on the browser's visual viewport once the task header mounts. The test records the starting (`idle`) listener count right after `page.goto`, before the task data has loaded. So that count is 0, and after the dialog closes the count is 1, the hook's listener for the page itself. The dialogs weren't leaking anything. Locally, this failed every time in the Follow Up test and in both Logs tests (fonts loaded and blocked), not only the one CI reported. **Fix (tests only, no product code changed):** - `e2e/task-logs.fixture.ts`: a new `taskShown(page)` helper waits until the task page has rendered. It reuses the "more actions" button lookup that `openTaskAction` already uses. - `e2e/dialog-keyboard-viewport.pw.ts`: the Follow Up and Logs tests now call `taskShown(page)` before measuring `idle`. The baseline therefore includes the task page's own listeners. The check still fails if a dialog leaves a listener behind. **Verification:** - `dialog-keyboard-viewport`, `task-logs-*` and `task-details-short-viewport` pass, each run twice with retries off (46 passed, 0 failed). Before the fix, `dialog-keyboard-viewport` failed 9 of 15 runs. - ESLint (`--max-warnings 0`) and `tsc --noEmit` pass on the changed files. This change isn't visible in the UI, so there are no previews. PR: #3277 Comment by: @propr-dev[bot] (ID: 6280778125) Comment by: @github-actions[bot] (ID: 6280778384) 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 addresses short viewports by letting the task header scroll away and giving the workspace its content height. The changes are focused and appear merge-ready within the reviewed scope, conditional on pending checks completing successfully. ✅ Adaptive layout — The hook measures available space and header height, preserves the existing phone layout, and cleans up its observers and listeners. ✅ Strict interaction coverage — The browser tests retain real touch/wheel input, hit testing, focus restoration, and the ≤1px scroll-restoration assertion. The baseline is now measured after settlement and preview capture. This was a static review; no commands or tests were run. The supplied original issue is truncated, so unseen acceptance criteria could not be assessed. Current checks report no failures and six pending checks; authenticated signed-native integration remains the stated operator release gate. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S4: 🟢 Cover live layout transitionsConsider testing portrait-to-landscape rotation and short-to-tall resizing while a file row is visible. The new cases validate initial viewport layouts, while ScoreThe implementation is focused, and the revised tests address the reported settlement problem without weakening assertions. Static review found no demonstrated blocker; runtime results and the remaining checks still need confirmation. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
1 participant
AI Implementation Summary
Closes #3276
Branch:
3276/claude-opus-5-5-short-landscape-task-view-20261019-2225-x6vCommits: ✅ Changes committed (ba5e743)
AI Processing Completed
Execution Details:
Summary:
I fixed #3276. At 844×390 touch landscape and in an 880×480 desktop window, the changed file can now be scrolled into view with a finger or the mouse wheel, tapped, its diff opened and closed. I tested this in headless Chromium against stubbed task data. It has not been run against real API data or on a real phone.
Cause. From 640px wide up, the task page uses the desktop layout. It pins the task header and gives only the area below it its own scroll. In a short window that area was too thin: about 100px in my reproduction, 58px in your measurement. The pinned Timeline and Files headings and the log bar covered nearly all of it, so the file row could never be tapped.
Fix.
useShortWorkspace.ts: it measures how much height the header leaves for the timeline/files area. It counts the collapsed log bar and the pinned headings, and allows about 184px for rows. It also respects the visible area when an on-screen keyboard shrinks it, but ignores pinch zoom. The measurement doesn't depend on which layout is active, so it can't flip back and forth.TaskDetails/index.tsx, when there isn't enough room:Tests. The new
e2e/task-details-short-viewport.pw.tscovers 844×390 touch landscape, 880×480 desktop, 320×640 and 390×844 portrait, and 1280×820 desktop. Each uses a completed PR task with a long two-line title and subtitle, 8 runs and one modified 121-line Markdown file. It:On the old code the two short cases fail the way the issue describes: at 844×390 the row can't be scrolled to a tappable spot, and at 880×480 only 51px of the area is visible.
One existing test changed behaviour. In
task-timeline-heading-layering.pw.ts, the case that docks the task beside the list at 1280×820 with 150% text now uses the new page-scroll layout. There, the enlarged header is 423px tall and leaves only about 97px for rows, the same problem as the issue. The test now scrolls the whole page and still checks that nothing paints over the Timeline heading.Results. All 270 tests in the
task-*browser suites pass, as do 424 TaskDetails unit tests, eslint with zero warnings and tsc. The new spec passed 3 times in a row. I didn't touch any files from #3273, #3274 or #3275, and added no dependencies.The 844×390 and 880×480 previews show the revealed file row and the opened diff. Separately, Gmail, Google Calendar and Google Drive need authorizing in your claude.ai connector settings before they can be used. This task didn't need them.
Detailed Logs:
af414cd5-6b2b-49ee-ba31-ba0e9591f51dLog files stored at:
/usr/src/app/logs/executions/issue-3276-2026-10-19T23-00-40-212Z-conversation.json/usr/src/app/logs/executions/issue-3276-2026-10-19T23-00-40-212Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3276.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!
Visual preview
844x390 touch landscape: diff opened by tap
The file's diff, opened by an ordinary tap after touch scrolling.
Original unavailable: managed storage is unavailable or the upload has expired.
880x480 short desktop window: diff
The file's diff, opened by a click on the revealed row.
Original unavailable: managed storage is unavailable or the upload has expired.
844x390 touch landscape: changed file reachable
The task header has scrolled away with the page. The changed Markdown file row is in open workspace between the pinned Timeline heading and the execution-log bar.
Original unavailable: managed storage is unavailable or the upload has expired.
880x480 short desktop window: changed file reachable
A short desktop window scrolls the whole task, so the file row can be reached with the mouse wheel.
Original unavailable: managed storage is unavailable or the upload has expired.