Skip to content

perf: drop runLayoutPipeline from header/footer effect deps#267

Closed
jedrazb wants to merge 2 commits intomainfrom
perf/avoid-duplicate-layout-pipeline
Closed

perf: drop runLayoutPipeline from header/footer effect deps#267
jedrazb wants to merge 2 commits intomainfrom
perf/avoid-duplicate-layout-pipeline

Conversation

@jedrazb
Copy link
Copy Markdown
Contributor

@jedrazb jedrazb commented Apr 16, 2026

Summary

  • The header/footer re-layout useEffect listed runLayoutPipeline in its deps array. That callback's identity churns on every keystroke because the document prop changes whenever the parent pushes a new doc into history, which re-runs this effect and fires a redundant full layout pass on top of the one already scheduled by the transaction handler.
  • Fix: replace runLayoutPipeline in the dep array with the concrete pagination inputs that should trigger re-layout — HF content, margins, pageSize, columns, pageGap, zoom, sectionProperties. Each of these is reference-stable across keystrokes (margins/pageSize/columns come from useMemo([sectionProperties]); sectionProperties itself is preserved by fromProseDoc's baseDocument path).

Why both commits

The first commit simply dropped runLayoutPipeline from the dep array. That fixed the keystroke duplicate but silently broke another path: under the old code, changing margins / pageSize / sectionProperties also churned runLayoutPipeline's identity (those values are in its useCallback deps) and piggy-backed a full re-layout through this effect. Dropping the dep without replacement meant ruler-drag and Page Setup apply no longer reflow pages until the next keystroke. The second commit addresses the review finding by listing the concrete inputs instead of relying on callback identity.

Performance impact

Local benchmark, 310-page document, 10 keystrokes at document start (tests/performance-large-docs.spec.ts), n=3 runs each = 30 measurements per branch:

Branch start-of-doc avg start-of-doc peak
main 81.7 ms 158 ms
first commit of this PR (regression) 64.3 ms 109 ms
this PR HEAD 70.3 ms 121 ms

~14% faster than main while correctly re-laying out on margin/pageSize/sectionProperties changes.

Why now

Found while auditing #263 — a much larger incremental-layout PR. The duplicate-pipeline issue was masking that PR's perf claims (every "fast incremental" pass was being followed by a full pass anyway). Even without the rest of #263, this dep-fix gives most of the keystroke-latency improvement #263 was reaching for, without the architectural cost.

Audit details and recommendation for #263: see the comment on that PR.

Test plan

  • bun run typecheck clean
  • tests/performance-large-docs.spec.ts start-of-doc avg drops from ~82ms to ~70ms locally (n=30)
  • Static review: margins / pageSize / sectionProperties changes still trigger re-layout (they're now explicit deps)
  • CI green
  • Manual: ruler-drag reflows pages; Page Setup apply reflows pages

🤖 Generated with Claude Code

The header/footer re-layout effect listed runLayoutPipeline in its deps
array. That callback's identity churns on every keystroke because the
`document` prop changes whenever the parent pushes a new doc into
history, so the effect re-ran and fired a redundant full pipeline pass
on top of the one already scheduled by the transaction handler.

Local benchmark (310-page document, 10 keystrokes at document start):
  before: ~129ms avg per keystroke
  after:  ~66ms avg per keystroke

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 16, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docx-editor Ready Ready Preview, Comment Apr 16, 2026 8:34am

Request Review

Review of #267 (PR that drops runLayoutPipeline from this effect's deps)
caught a real regression: under the OLD code, changing margins /
pageSize / columns / zoom / sectionProperties implicitly churned
runLayoutPipeline's identity (those values are in its useCallback deps)
and piggy-backed a full re-layout through this effect. The first commit
of #267 fixed the keystroke-churn but dropped that side-channel without
replacement, so ruler-drag and Page Setup apply no longer reflow pages
until the next keystroke.

Fix: list the concrete pagination inputs in the dep array instead of
relying on runLayoutPipeline's identity. Each of these is
reference-stable across keystrokes (margins/pageSize/columns come from
useMemo([sectionProperties]); sectionProperties itself is preserved by
fromProseDoc's baseDocument path), so the perf gain is preserved.

Local benchmark (310-page document, 10 keystrokes at document start,
n=3 runs = 30 measurements):
  main:                    ~82ms avg
  first commit of #267:    ~64ms avg (fast but buggy — regression above)
  this commit:             ~70ms avg (fast AND correct)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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