You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up of #1094 / PR #1098 (merged as 1e766a0). Three review items deliberately
left out of that PR: none is a defect, and the fix there is correct without them.
Grouped into one issue because they touch the same two files and one validation run
covers all three.
1. waitForViewportSettled returns a third read, not the value it verified
returnreadViewport(page);// re-reads the DOM after the poll already approved `current`
The poll validates current (two identical reads plus the settledWhen
postcondition) and then discards that string to read the viewport again. The value is
the same in practice — nothing moves the viewport in between — but the returned value
is not the verified value, which is the same class of hazard #1094 removed.
Same shape at the fit_view step, where fitted and fittedTransform come from two
separate reads even though the idempotence assertion treats the latter as canonical:
Proposal: extract the regex out of readViewport into a pure parseViewport(transform: string): Viewport, have the poll keep the transform it
accepted, and return parseViewport(settledTransform). readViewport becomes parseViewport(await readTransform(page)). Behaviour-neutral; it closes the remaining
staleness window and removes two CDP reads per wait.
This is the item that justifies its own validation run: the helper has 14 call sites
in the file.
2. The two clamp steps in test 1 lack expect(clicks).toBeGreaterThan(0)
Test 3 has it; the zoom_out and zoom_in clamp steps in test 1 do not. Without it,
a button already disabled on entry gives clicks === 0, so movedFrom is never
satisfied and the step fails as an opaque 15 s the canvas viewport never reached the expected settled state instead of the
diagnostic value diff (Expected: 0.25 / Received: 0.88) — the outcome PR #1098
explicitly set out to avoid for the clamp steps.
Low real risk: the preceding step asserts both buttons enabled, so zero clicks
requires a product change that would already redden that assertion first. One line
each, and it makes the three clamp steps consistent.
3. Doc/comment nit: the 0.92 and 0.880331 figures do not reconcile
docs/ui-ux/canvas-zoom-navigation.md (the Fit View scale row) and the comment at
the fit_view step both read:
the unclamped fit is min(1000/1090, 672/315) ≈ 0.92 — measured 0.880331 live
1000/1090 = 0.917 against a measured 0.880; the ~4 % gap is fitView()'s padding.
As written, a future reader spends time hunting a discrepancy that is not one. Adding "before fitView()'s padding" resolves it in both places. The argument itself
stands — both values are more than 2x away from the 2 bound.
Explicitly out of scope
Shortening intervals: [150, 150, 150, 200] to cut the ~150 ms the sentinel now
costs every wait. The sibling helper measured the fitView() commit landing at
20–47 ms, so a shorter first interval would be safe — but there is no pre-fix
runtime baseline to size the win against, so it stays a guess.
Removing the unreachable current !== UNREAD_TRANSFORM guard. Dead but harmless,
and it documents the sentinel's intent.
Item 1 needs the full spec-validation pass (all four tests, --trace=on, force-fail
per test, zero backend errors). Items 2 and 3 ride along on that run. No tag change
expected: all four tests stay @stable. No QA-CHECKLIST.md change: the eight
bullets are already [x] and coverage is unaffected.
Follow-up of #1094 / PR #1098 (merged as
1e766a0). Three review items deliberatelyleft out of that PR: none is a defect, and the fix there is correct without them.
Grouped into one issue because they touch the same two files and one validation run
covers all three.
1.
waitForViewportSettledreturns a third read, not the value it verifiedtests/tests-automations/regression/ui-ux/canvas-zoom-navigation.spec.tsThe poll validates
current(two identical reads plus thesettledWhenpostcondition) and then discards that string to read the viewport again. The value is
the same in practice — nothing moves the viewport in between — but the returned value
is not the verified value, which is the same class of hazard #1094 removed.
Same shape at the
fit_viewstep, wherefittedandfittedTransformcome from twoseparate reads even though the idempotence assertion treats the latter as canonical:
Proposal: extract the regex out of
readViewportinto a pureparseViewport(transform: string): Viewport, have the poll keep the transform itaccepted, and return
parseViewport(settledTransform).readViewportbecomesparseViewport(await readTransform(page)). Behaviour-neutral; it closes the remainingstaleness window and removes two CDP reads per wait.
This is the item that justifies its own validation run: the helper has 14 call sites
in the file.
2. The two clamp steps in test 1 lack
expect(clicks).toBeGreaterThan(0)Test 3 has it; the
zoom_outandzoom_inclamp steps in test 1 do not. Without it,a button already disabled on entry gives
clicks === 0, somovedFromis neversatisfied and the step fails as an opaque 15 s
the canvas viewport never reached the expected settled stateinstead of thediagnostic value diff (
Expected: 0.25 / Received: 0.88) — the outcome PR #1098explicitly set out to avoid for the clamp steps.
Low real risk: the preceding step asserts both buttons enabled, so zero clicks
requires a product change that would already redden that assertion first. One line
each, and it makes the three clamp steps consistent.
3. Doc/comment nit: the
0.92and0.880331figures do not reconciledocs/ui-ux/canvas-zoom-navigation.md(the Fit View scale row) and the comment atthe
fit_viewstep both read:1000/1090 = 0.917against a measured0.880; the ~4 % gap isfitView()'s padding.As written, a future reader spends time hunting a discrepancy that is not one. Adding
"before
fitView()'s padding" resolves it in both places. The argument itselfstands — both values are more than 2x away from the
2bound.Explicitly out of scope
intervals: [150, 150, 150, 200]to cut the ~150 ms the sentinel nowcosts every wait. The sibling helper measured the
fitView()commit landing at20–47 ms, so a shorter first interval would be safe — but there is no pre-fix
runtime baseline to size the win against, so it stays a guess.
current !== UNREAD_TRANSFORMguard. Dead but harmless,and it documents the sentinel's intent.
tests/helpers/ui/adjust-screen-view.ts's ownwaitForViewportSettled. That one seedsprevious = nulland separates itsreads by 50 ms, so it does not carry the canvas-zoom-navigation: "Fit View centers every node" asserts a scale drop that Fit View is not required to produce #1094 bug; it reaches ~108 specs and needs
its own PR if
settledWhenis ever worth porting there.Validation
Item 1 needs the full spec-validation pass (all four tests,
--trace=on, force-failper test, zero backend errors). Items 2 and 3 ride along on that run. No tag change
expected: all four tests stay
@stable. NoQA-CHECKLIST.mdchange: the eightbullets are already
[x]and coverage is unaffected.