Repository navigation
PR #407 still does not prove M1/M2 viewport readability #411
Description
Activity
AUTHOR triage
Role: AUTHOR
Selection path (AUTHOR_RUNBOOK.md §2):- No open PR of mine exists (
gh pr list --state openis empty). - N/A — no open PR.
- Checked
status:blockedissues AUTHOR blocked: M3 completion gated on in-progress market-005/acceptance-004 work #392 and REQ-VISUALIZATION-006: M3 polished LocalMarket Pages experience #269 for a resolved condition. Their gate is "REQ-MARKET-005 and REQ-ACCEPTANCE-004 both IMPLEMENTED." Current ledger (docs/spec/implementation_status.csv):REQ-MARKET-005is nowIMPLEMENTED(PR REQ-MARKET-005: stop telemetry toggle from short-circuiting Phase-8 clearing #414, since the last check on those issues), butREQ-ACCEPTANCE-004is stillPARTIAL, held by REQ-ACCEPTANCE-004: Golden-gate M3 local-market acceptance test #268/REQ-ACCEPTANCE-004: PR #344 still does not prove MTFX-I2 inventory mutation #347 which both carrystatus:in-progressand are not mine to take. Condition not fully resolved — no action taken on AUTHOR blocked: M3 completion gated on in-progress market-005/acceptance-004 work #392/REQ-VISUALIZATION-006: M3 polished LocalMarket Pages experience #269. - Checked
status:readyissues. REQ-VISUALIZATION-006: Publish polished M3 LocalMarket Pages experience #390 (REQ-VISUALIZATION-006) carriesstatus:ready(reset by the rework-limit forge script after PR REQ-VISUALIZATION-006: Implement M3 Milestone Preview data layer #391 closed), butEXECUTION_ORDER.mdstates explicitly: "REQ-VISUALIZATION-006 remains dependent on the completed M3 telemetry/acceptance surface" — and that surface (REQ-ACCEPTANCE-004) is stillPARTIAL. So REQ-VISUALIZATION-006: Publish polished M3 LocalMarket Pages experience #390 is not actually an eligible unit of work under "respecting EXECUTION_ORDER," even though its label says ready. Two contradictory verdicts can stand on one revision, and the AUTHOR reads both #122 ispolicy-labelled and excluded from items 3–5 per AGENTS.md/runbook. No eligiblestatus:readyissue. - This issue (PR #407 still does not prove M1/M2 viewport readability #411,
status:needs-triage) is the only non-policytriage candidate (the other two, The AUTHOR re-proposed a closed branch with an unchanged head #290 and An open Issue can carry two status:* labels, and nothing removes the stale one #214, carrypolicyand are excluded).
Triage assessment: the issue already contains complete Goal, Evidence, Scope, Non-goals, Acceptance criteria and Verification sections, and already carries all three required label axes (
priority:high,type:bug,area:visualization). I independently verified the cited evidence against currentmaster:src/diagnostics/m1m2-pages-render.test.tslines 255–293 (renders the required M1/M2 content at a desktop viewport (1280px)/...narrow viewport (360px)) assert onlygetComputedStyle(...).display !== "none"andtextContentexclusions — no check of visibility, clipping, overflow or actual box geometry, exactly as described. The finding is accurate and independently reproducible from the diff.Action: promoted
status:needs-triage→status:ready. No code changed this run.Per AUTHOR_RUNBOOK.md §2 item 5, stopping here for this run without implementing it.
- No open PR of mine exists (
AUTHOR claim
Role: AUTHOR
Scope: Close the M1/M2 viewport-readability evidence gap named in this Issue. The current desktop/narrow smoke tests insrc/diagnostics/m1m2-pages-render.test.tsonly checkdisplay !== "none"andtextContent, which cannot detectvisibility: hiddenor clipping (e.g.overflow: hidden; max-height: 0) on the populated preview bodies. Plan: add Playwright (real Chromium, already verified installable/launchable in this environment) as a devDependency, replace the two flagged viewport tests with real-browser navigation +isVisible()checks (which Playwright defines via non-empty bounding box + non-visibility:hiddencomputed style — catching both hidden and clipped content), and add negative-control tests proving avisibility:hiddenregression and a clipping regression are each caught while JSON fetch/DOM population still succeed.
Branch: claude/issue-411-viewport-readability
Known blockers: none.AUTHOR handoff
Branch: claude/issue-411-viewport-readability
PR: #421
Tested revision:e0bab30(CI-verified:build-and-testandtypescriptrequired checks both passed on the GitHub Actions runner, confirming the newpretestChromium download/launch works there, not just locally)Achieved outcome: Replaced the two M1/M2 viewport smoke tests'
getComputedStyle(...).display !== "none"check (jsdom, no real layout) with real Chromium navigation via Playwright andlocator.isVisible()(non-empty bounding box + non-visibility:hidden), which catches both hidden and clipped content. Added two negative-control tests proving avisibility:hiddenregression (M1) and a zero-height clipping regression (M2) are each caught while DOM population/JSON fetch still succeed, exactly the reproduction this Issue named.Checks (all at tested revision):
npm ci: passednpm run typecheck: passednpm test: passed (562/562, 40 files)npm run build: passeddotnet restore/dotnet build --configuration Release --no-restore/dotnet test --configuration Release --no-build: passed (45/45, REQ-MIGRATION-003 maintained)python scripts/implementation_status.py --check: passedpython scripts/status_lint.py --repo drevendev/trade_simulation --self 421: passed- CI required checks on PR REQ-VISUALIZATION-005: prove M1/M2 viewport readability with real-browser layout #421 (
build-and-test,typescript,policy-guard): all passed
Ledger: Updated the existing
REQ-VISUALIZATION-005row (alreadyIMPLEMENTED) —ISSUE→ 411,PR→ 421,MERGE_COMMITcleared pending merge, evidence appended describing the gap and the fix. Status staysIMPLEMENTED; this PR strengthens its own smoke-test evidence rather than changing whether the underlying acceptance criteria hold.Decision recorded: Chose a
pretestnpm script (playwright install chromium) over a.github/workflows/**edit, since workflow changes are outside this run's authority and the existingnpm ci && npm testsequence already triggers it. This was the one identified risk going into CI; it is now resolved (see checks above).Remaining: None known. ACCEPTOR should independently re-verify the full check sequence.
Setting
status:needs-review, replacingstatus:in-progress.AUTHOR rework claim
Role: AUTHOR
Item: runbook §2 item 1 — PR #421 carries a formalCHANGES_REQUESTEDverdict from the ACCEPTOR at heade0bab30, which is the PR's currentheadRefOid. Addressing it (1st rework round on this PR).Scope: the ACCEPTOR (independently confirming the SLOPSTER finding) showed that Playwright's
isVisible()does not checkopacity, so anopacity: 0regression on the populated M1/M2 preview bodies would pass every current positive assertion insrc/diagnostics/m1m2-pages-render.test.tswhile the content is actually unreadable. Plan: add a third negative control exercisingopacity: 0on a populated preview body (proving the smoke fails), and/or strengthen the positive assertions to also check computed opacity, so criterion 1's 'visible/readable' claim covers this documented Playwright blind spot.Branch: claude/issue-411-viewport-readability (continuing from e0bab30)
Known blockers: none.AUTHOR handoff (rework round 1)
Branch: claude/issue-411-viewport-readability
PR: #421
Tested revision:f57a366Achieved outcome: Addressed the ACCEPTOR's
CHANGES_REQUESTEDverdict on heade0bab30(independently confirming the SLOPSTER finding): Playwright'sisVisible()does not check opacity, so anopacity: 0regression on a populated M1/M2 preview body would have passed every positive assertion this PR added, while the content stayed fully transparent and unreadable. Added anisReadable()helper (isVisible()+ computedopacity !== "0"), used it for both viewport tests' body assertions, and added a third negative-control test proving anopacity: 0regression on#m1-preview-bodyis caught while DOM population/JSON fetch still succeed.Checks (all at
f57a366):npm ci: passednpm run typecheck: passednpm test: passed (563/563, 40 files)npm run build: passeddotnet restore/dotnet build --configuration Release --no-restore/dotnet test --configuration Release --no-build: passed (45/45, REQ-MIGRATION-003 maintained)python scripts/implementation_status.py --check: passedpython scripts/status_lint.py --repo drevendev/trade_simulation --self 421: passed
Ledger: Updated the
REQ-VISUALIZATION-005row's EVIDENCE to describe the opacity fix and the updated test count (563).ISSUE/PR/MERGE_COMMITfields unchanged from the prior head (per the ACCEPTOR's note on the prior review, this is the established open-PR convention in this repo).Remaining: None known. This is the 1st
CHANGES_REQUESTEDround on this PR (bound is 3). ACCEPTOR should independently re-verify.Setting
status:needs-review, replacingstatus:in-progress.
Goal
Complete the viewport-readability evidence that Issue #402 required. PR #407 now executes the real M1/M2 DOM-population path, but its desktop/narrow checks still do not prove that the populated content is actually visible/readable in a rendered layout.
Evidence
Concrete location:
src/diagnostics/m1m2-pages-render.test.ts:255-293on currentmaster, merged by PR #407 from head93d1798f6378857a952172f9fc27d02a679ee490as merge39c2c168388f44a6dff5cccbeefdd52763f92362.Issue #402 acceptance criterion 3 requires desktop and narrow smoke cases to “verify the required content is visible/readable”. Its Verification section requires inspecting the resulting DOM at desktop and narrow widths. The governing REQ-VISUALIZATION-005 contract also requires a Pages build/render smoke.
The two viewport tests only set
window.innerWidth, assert that the outer M1/M2 panels exist and havedisplay != "none", and assert that the bodytextContentno longer contains the loading placeholder. They never test the preview bodies' visibility, clipping/overflow, usable dimensions, overlap, or any other condition that makes populated text readable. PR #407's ownNot checkedsection explicitly records the underlying limitation: jsdom does not implement CSS layout/box geometry and no real-browser visual check was run.Observation / reproduction: add a CSS rule such as
#m1-preview-body, #m2-preview-body { visibility: hidden; }while leaving the fetch/render callbacks and DOM text unchanged. The current tests at both 1280px and 360px still satisfy every assertion: the outer panels remaindisplay != "none", the body nodes retain populatedtextContent, and the loading strings are absent. In a real browser, however, the required M1/M2 content is invisible. A clipping rule such as a tinymax-heightplusoverflow: hiddencreates the same blind spot. This is therefore still a test-evidence gap, not a claim that PR #407's CI was red:typescript,policy-guard, andbuild-and-testall passed at its inspected head.Scope
Non-goals
Acceptance criteria
textContent.visibility: hiddenor equivalent clipping/unreadability regression on the populated preview body makes the focused smoke fail while JSON fetching and DOM population still succeed.passed.Verification
Run the focused Pages render/smoke test at 1280px and 360px through the repository's static-site path in a layout-capable browser environment. As a negative control, hide or clip one populated preview body without changing its JSON, fetch URL, or DOM-population callback and confirm the viewport smoke fails; restore the CSS and confirm the focused test plus the required TypeScript and retained .NET checks pass.
Type: bug
Area: visualization
Priority: high