Improve viewport overlay update order and regression coverage

[nightt5879]

Summary

#454 already fixed freehand selection alignment by adding a post-layout refresh. This follow-up finalizes the SVG viewBox and scroll offsets before rendering viewport-only changes, so selected overlays use the final viewport without a corrective second pass.

The change cycle is extracted into an internal hook for testing. Batches containing element changes retain the post-layout refresh because component updates can recalculate layout. Native scrolling and mixed operation batches are handled separately, and the lifecycle findings and test boundaries are documented.

Regression tests use the shared testing fixture, real viewport transforms, the production change hook, and real RoughSVG output. Browser tests cover freehand, rectangles, images, arrows, mind maps, multiple selection, and text layout changes.

Related Issue

Closes #461.

The lifecycle improvement applies to viewport-only changes. Removing updateActiveSection from all element-update paths remains outside this change.

Validation

• Applicable checks pass: lint, format check, tests, and build
• Relevant E2E and manual tests are complete or not applicable

Evidence, screenshots, or compatibility notes:

• npm run lint and npm run build: passed.
• npm run test -- --run: 36 tests passed, including 8 focused viewport tests.
• npx nx e2e web-e2e --workers=3 --reporter=line: 24 tests passed across Chromium, Firefox, and WebKit (21 new regression runs and 3 existing smoke runs).
• Reversing the viewport/render order caused 5 focused tests to fail on outline coordinates; the production order was restored afterward.
• Changed source files pass formatting and focused TypeScript checks. Full formatting still reports 126 unchanged files; the existing Playwright configuration has a reporter tuple type error at apps/web-e2e/playwright.config.ts:32.
• Final selection screenshots are generated as Playwright artifacts. No .drawnix serialization changes.
• The author completed manual testing of the local frontend.

AI Assistance

Did you use AI tools to generate or substantially modify code, tests, or documentation in this pull request?

• No
• Yes

If yes:

• Tool/model: Codex/GPT-6 Astra

• AI-assisted work: Lifecycle investigation, implementation, regression tests, documentation, and local automated/browser validation.

• My review and validation: I reviewed and understand the submitted changes, discussed the lifecycle behavior and scope, and manually tested the local frontend. Codex ran the automated checks listed above.

• I reviewed and understand the AI-assisted changes and can explain and maintain them

Checklist

• The title and description are in English, and the change is focused
• I reviewed and understand every submitted change
• Tests, documentation, and translations were updated when needed

[cloudflare-workers-and-pages]

Deploying drawnix with    Cloudflare Pages

Latest commit:	4b4f306
Status:	✅  Deploy successful!
Preview URL:	https://13467e53.drawnix.pages.dev
Branch Preview URL:	https://nightt5879-issue-461-viewpor.drawnix.pages.dev

View logs

[pubuzhixing8]

Review: unit test structure

Scope note: this comment covers the unit tests only. Feedback on apps/web-e2e/src/viewport.spec.ts will follow in a separate comment.

1. The behaviour this PR changes is never asserted directly

The stated goal is "selected components draw against the final inputs once, without a corrective pass". Of those three properties, drawn after the viewBox was finalised is never asserted — it is inferred from whether the coordinates come out right. That inference is why the spec has to bring in the whole viewport geometry model (the 33-line attachViewportLayout, real updateZoom / setViewport / updateViewportByScrolling).

Recording the viewBox at draw time turns the ordering into a direct assertion:

const roughSVG = PlaitBoard.getRoughSVG(board);
const original = roughSVG.rectangle.bind(roughSVG);
const draws: { viewBox: string | null; args: unknown[] }[] = [];
vi.spyOn(roughSVG, 'rectangle').mockImplementation((...args) => {
  draws.push({ viewBox: host.getAttribute('viewBox'), args });
  return original(...args);
});

expect(draws).toHaveLength(1) and expect(draws[0].viewBox).toBe(host.getAttribute('viewBox')) then pin "drawn once" and "drawn after the viewBox update" together, and most of the surrounding scaffolding stops being load-bearing.

2. The real browser-scroll path is not covered

All four scroll-related cases call updateViewportByScrolling(board, scrollLeft, scrollTop) directly, bypassing the actual listener in packages/react-board/src/hooks/use-board-event.ts:20. So the browser-scroll handling itself has no coverage:

• the isFromViewportChange branch in the listener (the "this scroll was self-inflicted, do not feed it back" suppression) never runs;
• updateViewportContainerScroll's clamp branch and its isValidLeftOrTop branch never run (jsdom stores scrollLeft verbatim and reports scrollWidth/offsetWidth as 0);
• the isFromScrolling lifetime is only checked for batches that carry ops. The path where a clamped scroll write re-enters updateViewportByScrolling, leaving an empty next batch that still has to clear the flag, has no coverage at all — and that is the path the new unconditional setIsFromScrolling(board, false) fixes.

3. Suggested split

packages/react-board/src/hooks/use-board-change.spec.ts (new). This is where the viewport branch logic belongs, and it does not need real drawing — the subject is orchestration, so a fake component that records the call sequence is enough (no RoughSVG, no geometry math). Building the board straight from @plait/core's setupTestingBoard also removes the cross-package deep import ../../../../react-board/src/hooks/use-board-change, and useBoardEvent becomes a normal same-package relative import.

const calls: string[] = [];
const viewBoxAtRender: (string | null)[] = [];

class TestComponent extends CommonElementFlavour<TestElement, PlaitBoard> {
  initialize() {
    super.initialize();
    this.getRef().updateActiveSection = () => calls.push('updateActiveSection');
  }
  onContextChanged() {
    calls.push('render');
    viewBoxAtRender.push(PlaitBoard.getHost(this.board).getAttribute('viewBox'));
  }
}

Four cases, matching the paths that can actually occur:

a. set_selection only — render, do not touch the viewport

await act(async () => { Transforms.setSelection(board, selection); });
expect(calls).toEqual(['render']);
expect(host.getAttribute('viewBox')).toBe(before);

b. set_viewport only — viewBox finalised before the render, no corrective pass

const before = host.getAttribute('viewBox');
await act(async () => { BoardTransforms.updateZoom(board, 2); });
expect(calls).toEqual(['render']);
expect(host.getAttribute('viewBox')).not.toBe(before);
expect(viewBoxAtRender).toEqual([host.getAttribute('viewBox')]);

c. Browser scrolling — driven through the real listener

renderHook(() => {
  useBoardChange(board, listRender, noop);
  useBoardEvent(board, { current: viewportContainer } as React.RefObject<HTMLDivElement>);
});

await act(async () => {
  viewportContainer.scrollLeft = 40;
  viewportContainer.scrollTop = 30;
  viewportContainer.dispatchEvent(new Event('scroll'));
});

expect(host.getAttribute('viewBox')).toBe(before);  // scrolling does not rebuild it
expect(isFromScrolling(board)).toBe(false);         // the flag is consumed by this flush
expect(calls).toEqual(['render']);

d. Ordinary content update — render first, then viewport, then the active refresh

await act(async () => { Transforms.setNode(board, { points: nextPoints }, [0]); });
expect(calls).toEqual(['render', 'updateActiveSection']);

Asserting the sequence rather than coordinates also fixes a blind spot in the current refreshes after rendering when an element change is batched with zoom: because the element changed, onContextChanged takes its value.element !== previous.element branch and redraws on its own, so that test still passes even if updateActiveSection is reduced to a no-op. The sequence assertion does not.

packages/drawnix/src/plugins/freehand/freehand.component.spec.ts — down to 2 cases. Keep only the it.each over zoom in / zoom out, asserting the three properties that are genuinely about the freehand component:

expect(draws).toHaveLength(1);                                // drawn once
expect(draws[0].viewBox).toBe(host.getAttribute('viewBox'));  // drawn on the new viewBox
expect(rectangle).toHaveBeenLastCalledWith(
  x - p, y - p, w + 2 * p, h + 2 * p,
  expect.objectContaining({ strokeWidth: ACTIVE_STROKE_WIDTH })
);                                                            // correct geometry

The remaining six cases are all exercising useBoardChange branches rather than freehand, and three of them (finalizes a zoom batched after native scrolling, applies an origin change batched after native scrolling, does not skip element layout in a batch that includes native scrolling) describe batches that cannot occur through the UI: a microtask checkpoint runs after every event-listener callback, so the scroll handler's apply always flushes before any other handler runs, and both writers of isFromScrolling produce a batch of exactly one op. They are constructed by calling updateViewportByScrolling and a transform synchronously inside one act(). I would drop them and add coverage if such a batch ever becomes real.

4. Assertions that can go

Inside expectOutline:

• expect(outlines[0]).toBe(rectangle.mock.results[...]?.value) — overlaps with toHaveLength(1)
• expect(outlines[0].querySelector('path')).not.toBeNull() — asserts that RoughSVG draws a path
• expect(activeHost.querySelectorAll(RESIZE_HANDLE_CLASS_NAME)).toHaveLength(4) — asserts ActiveGenerator / hasResizeHandle, unrelated to ordering

Inside the cases:

• expect(state.board.viewport.zoom).toBe(zoom) — asserts BoardTransforms.updateZoom
• expect(state.board.operations).toEqual([]) — asserts board.operations = [] in core's apply
• expect(host.getAttribute('viewBox')).not.toBe(oldViewBox) and expect([scrollLeft, scrollTop]).not.toEqual(oldScroll) — subsumed by the draws[0].viewBox assertion
• the two expect(...).not.toHaveBeenCalled() inside the act callback — assert core's microtask batching
• the afterChange spy can go entirely; passing () => {} is enough, since the draw assertions already prove the cycle ran

5. Two prerequisites

1. ResizeObserver needs a stub. useBoardEvent unconditionally does new ResizeObserver(...), and jsdom has no such global, so case (c) throws without one. packages/react-board/vite.config.ts currently has no setupFiles.
2. Keep the geometry helper local to each spec. The two specs live in different packages; sharing it would reintroduce a cross-package import. The react-board copy needs the host getBoundingClientRect that follows scrollLeft/scrollTop; the freehand copy can be smaller once it no longer tests scrolling.

---

Verification behind the above: npx vitest run in packages/drawnix passes (9 files / 33 tests); reverting use-board-change.ts to the old wrapper.tsx logic fails 6 of the 8 freehand cases, so the suite does bind the behaviour; oxfmt --check passes on the four changed files; tsc -p packages/drawnix/tsconfig.spec.json --noEmit reports nothing from the new files. One small correction for viewport-regressions.md: the mutation check reports "five of these tests fail on incorrect coordinates", but a full revert fails six, and the two pure-zoom cases fail on expected "Mock" to be called once, but got 2 times — draw count, not coordinates.

[pubuzhixing8]

Follow-up: e2e tests, and a suggestion to drop viewport-regressions.md

E2E — no change requests

I went through apps/web-e2e/src/viewport.spec.ts and have nothing to ask for. It is genuinely thorough: five element types plus mind text layout and multiple selection, across three browsers, driving the real toolbar, wheel and drag interactions rather than the board API.

One candid reviewer note rather than a change request — the volume and density here are past what I can meaningfully review by reading. If these start flaking in CI, whoever picks them up will be reverse-engineering them from scratch. Worth being aware of as a maintenance cost; not a reason to change anything now.

Please consider dropping packages/drawnix/testing/viewport-regressions.md

Three reasons:

1. Most of it is PR-description material. The lifecycle finding, the mutation check, and the test-boundary notes are an account of this one investigation. The PR description and the commit messages already carry that, and that is where a reader looks for it.
2. The location reads as design documentation. packages/drawnix/testing/ is a source directory whose index.ts is the barrel for the shared testing fixture added in test(drawnix): add shared testing board fixture #468. Prose about a past investigation sitting next to shipped fixtures will be taken as a current description of how the code works.
3. It goes stale immediately. If the test restructuring from my previous comment lands, most of the "Test boundary" section describes tests that no longer exist. It also already carries one inaccuracy: the mutation check is reported as "five of these tests fail on incorrect coordinates", but a full revert of use-board-change.ts fails six, and the two pure-zoom cases fail on expected "Mock" to be called once, but got 2 times — draw count, not coordinates. (Which makes the correction at the end of my previous comment moot if the file goes.)

Nothing valuable is lost by removing it. The part that needs to survive in the repo is the comment already sitting in use-board-change.ts explaining why viewport-only batches finalise the viewBox before rendering, and why batches containing element changes keep the post-layout refresh.