Skip to content

Clean up release/0.5.0 (screenshot size contract, README, displayUrl mirrors) - #14

Merged
T-Crypt merged 5 commits into
oh-my-pocketfrom
cleanup-pr12
Oct 13, 2026
Merged

T-Crypt merged 5 commits into
oh-my-pocketfrom
cleanup-pr12

Conversation

@T-Crypt

@T-Crypt T-Crypt commented Oct 13, 2026

Copy link
Copy Markdown
Owner

Follow-up cleanup for the merged release/0.5.0 (PR #12), from the 2026-10-13 review of PRs #11 to #13.

Changes:

  • README.md: drop the stale "not yet on Windows" sentence from the browser limitations bullet. Windows support shipped in 0.5.0, and the same README already states it.
  • src/server/browser/page.ts: the viewport screenshot now reports the page's CSS size at capture (clientWidth/clientHeight) instead of the preset viewport size. The bitmap is captured at the CSS layout width, which is 15px narrower than the preset on the default desktop viewport because of the scrollbar. The old return told the model the shot was 1280 CSS pixels wide when it was 1265, so a point read near the right edge could not be mapped back to a valid click position.
  • test/browser.test.ts: new test that pins the screenshot size contract (bitmap size equals reported size times scale) and records that keyboard zoom is unreachable in headless Chromium.
  • src/server/browser/urls.ts, web/browser.js: the two displayUrl copies are now named as mirrors of each other in their doc comments, so an edit to one reminds of the other.

Proof: node --test test/browser.test.ts 10/10, npm run check and npm run lint clean, npm test 461/461.

Review: two rounds. The first review caught the scrollbar break in the size contract (the initial no-op verdict was wrong) and showed that two full-suite test failures are load sensitive flakes that pass in isolation and at the base commit, not a regression. The fix round and re-review closed both findings. Final verdict: clean.

Run folder: ~/handoffs/oh-my-pocket/2026-10-13-pr11-13-cleanup (review.md, plan.md, pr12-fix.md, rereview-pr12.md).

@T-Crypt

T-Crypt commented Oct 13, 2026

Copy link
Copy Markdown
Owner Author

Review findings (two rounds, final verdict: clean).

First review:

  1. The screenshot size contract was broken at the unzoomed baseline: the bitmap is captured at the CSS layout width (1265px, excluding the scrollbar) but the code reported the preset viewport width (1280px), so the documented contract (bitmap width = reported width x scale) failed. The initial no-op verdict for this finding was wrong, and the note blaming the gap on JPEG compression was not physical.
  2. The two full-suite test failures (a compaction timing test and a subagents-ui dock test) are not pre-existing: both pass at the base commit and in isolation on this branch. They are load sensitive flakes under full-suite load, not a regression.

Fix round: the viewport branch of screenshot() now reports Math.round(view.clientWidth/clientHeight) as the size (the scale term is unchanged), the doc comment says the reported size is the page's CSS size at capture, and a new test pins the contract (red before the fix at 1265 vs 1280, green after).

Re-review: clean. The fix matches the plan's Branch B exactly, the fullPage branch and viewport application are untouched, and the two updated assertions (1280 to 1265) expect the true scrollbar-affected width with the scale assertions still pinning the contract.

Non-blocking observation for a follow-up: on the DPR-2 presets (mobile, tablet) the bare fit scale term omits the 1/DPR clip factor, so the bitmap-vs-reported contract is off by the DPR factor there. It is pre-existing on the base commit and outside this change.

Run folder: ~/handoffs/oh-my-pocket/2026-10-13-pr11-13-cleanup

@T-Crypt
T-Crypt changed the base branch from main to oh-my-pocket October 13, 2026 14:41
@T-Crypt

T-Crypt commented Oct 13, 2026

Copy link
Copy Markdown
Owner Author

Retargeted from main (a stale mirror of upstream 0.13.0) to oh-my-pocket, the fork's default branch. Merged origin/oh-my-pocket in; no force push. check and lint pass. npm test: 628 pass, 1 fail in test/visible-models-ui.test.ts ("the owner's switches decide which models the model menu offers"), which is flaky on this code (passed then failed on rerun) and outside this diff.

@T-Crypt
T-Crypt marked this pull request as ready for review October 13, 2026 14:52
@T-Crypt
T-Crypt merged commit 671e247 into oh-my-pocket Oct 13, 2026
@T-Crypt
T-Crypt deleted the cleanup-pr12 branch October 13, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant