Conversation
) Delivers the P0 phase of issue #3345. P1 is gated behind a maintainer go/no-go decision and P2 through P4 are gated behind P1, so P0 plus the selection design note is the whole unblocked scope. P0 is also explicitly a no-code phase; no manifest, lockfile, dependency, or source file changed. The spike compiled packages/cli against an out-of-tree install of upstream ink 7.1.1 and ran behavioural probes against both packages. Findings that change the picture the issue painted: - Upstream 7.1.1 still accumulates <Static> output in fullStaticOutput at the same rate as the fork within one <Static> identity. Both retained 61,990 characters for the same workload. Upstream's #950 reset fires on <Static> remount, which this app performs on resize, history trim, markdown toggle, and clear, so upstream is better on that axis than the fork, which never resets. The size of the advantage is workload dependent and was not measured on a production session. - Upstream's measureText and wrapText caches are unbounded; the fork bounds its full-string styled-character cache. The fix for that (ad9e3ea) is on upstream master and in no published release, and 7.1.1 is still the newest release. - Neither package exposes the live-region terminal anchor. Both geometry APIs are live-layout relative and omit preceding <Static> rows, so the anchor must be forced structurally rather than queried. That invariant is not yet proven in a PTY, which leaves one P0 obligation open. - Under an app-owned viewport, upstream measureElement already returns scroll-translated child coordinates, so the fork's scroll-offset subtraction has no replacement to build. Recommendation for the selection gate is NO-GO on the interim selection port and on an immediate switch to 7.1.1, taking the issue's "patch now, then evaluate" sequence instead. The report states the conditions that would reverse it. The decision belongs to maintainers. Refs #3345
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (5)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
Contributor
WalkthroughBefore this PR, the upstream ink migration did not have a documented P0 inventory, spike artifacts, or a recorded selection rationale in the project plans. After this PR, those planning documents are added under project-plans/issue3345, giving the migration a written inventory, spike scripts/evidence/report, and a selection design note for review and follow-on work. Release NotesDocumentation
Changes
Magnitude🎯 2 (M) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
…the fix (#3345) The first P0 pass recommended NO-GO on migration and "patch the fork, then reassess". Two of its inputs were wrong or missing, and correcting them changes the recommendation to a two-track plan: fix the leak now, and commit to upstream as the destination. What changed: - I never inspected fork 7.1.0. Having done so: the fork's latest is 7.1.0 from 2026-06-24 with no npm publish or repo push since, and it does NOT contain upstream vadimdemedes/ink#950. Upgrading the fork does not fix the leak. Its answer is <StaticRender>, a fork-only API that does zero the accumulator but writes 6.7x its workload even when chunked per item, and whose own docs say incremental invalidation for growing lists is unsolved. Compiling against fork 7.1.0 costs only 3 errors in one test file, so it is cheap to adopt and still the wrong destination. - The leak does not affect the default configuration. ui.useAlternateBuffer defaults to true and schema defaults really are materialised into settings.merged, so the default session takes AlternateBufferLayout, which mounts no <Static> anywhere in its tree. The exposed population is users who explicitly opted out, excluding screen-reader and CI. For them growth is superlinear: bounded history re-emitted in full on each of six refreshStatic() remounts into an accumulator that never resets. The fix is upstream's own change, backported: 11 added lines across two files. Measured on the pinned fork it resets the accumulator to 0 on remount, exactly matching upstream 7.1.1. The same anchors applied unchanged to fork 7.1.0. It survives a real bundle:cli build, and the published bin prefers the bundle, so it reaches users. Terminal behavior after the reset is not yet tested and is recorded as a prerequisite. The selection gate moves from NO-GO to conditional. A spike walking upstream's public DOMElement tree resolved a cell to a UTF-16 offset and painted inverse video through the public Transform, compiling clean with no deep imports, which retracts the earlier claim that a tree port needs private internals. The remaining cost is parity and roughly 137 direct Text import sites. Adds evidence E23 through E32 and the patch itself to the reproduction appendix. Migration sequence is A, C, B, D, and A, C, and B can each land while still building on the patched fork. Refs #3345
…ath cache (#3345) The report framed its memory case entirely on fullStaticOutput. That accumulator is unreachable in the default configuration, because alternate buffer is the default and mounts no <Static>. So this issue does not describe the growth users see in ordinary use, and the report now says so up front instead of implying otherwise. Adds E34 recording the leak history that does cover ordinary use: #2852 (closed, allocator churn from an O(N-squared) streaming pipeline), #3114 (closed, added post-GC plateau verdicts for JSC heap, Bun external memory and dirty WebKit Malloc), #3329/#3343 (shell retention), and #2905, which is still OPEN and holds the unattributed native signature of 22.8 GB physical footprint with ~13.8 GB dirty WebKit Malloc and ~8.6 GB IOAccelerator on an image-bearing resumed session. Also records two constraints from that work: RSS is not a valid leak criterion under Bun and JSC, and no native surface is allocated by LLxprt source. Adds E33 for the one Ink-side memory behavior that does apply in every render mode. Rendering 20,000 distinct strings and sampling heapUsed after forced GC, reproducible across two runs: pinned fork 6.4.8 ~131.9 MB fork 7.1.0 ~1.9 MB upstream 7.1.1 ~120.1 MB The pin caches arrays of StyledChar objects bounded at 10,000/1,000,000; fork 7.1.0 uses a compact StyledLine bounded at 2,000/100,000; upstream 7.1.1's caches are unbounded. Two consequences cut against earlier conclusions: on this axis the pin is close to the worst of the three and upstream 7.1.1 as published would not improve it, and fork 7.1.0 is roughly 69x better while costing 3 diagnostics in one test file. That makes a 6.4.8 to 7.1.0 bump a candidate default-mode improvement independent of both the static accumulator and the migration, without changing the destination argument. Measurement limits are stated: JS heap only, synthetic, one string shape, nothing native. Refs #3345
…ender path (#3345) E33 measured the styled-character cache in isolation and concluded fork 7.1.0 was ~69x better, which led to recommending a 6.4.8 -> 7.1.0 bump as a default-mode memory improvement. Measuring the whole Ink render path reverses that. The recommendation is withdrawn; taking it would have made default-mode memory worse. Adds E35. The existing issue-2852 harness has text/media/reasoning modes, all of which drive the streaming buffer and none of which render through Ink, so the post-GC plateau verdicts from #3114 cannot see a render-path leak. This adds that dimension: a Bun probe rendering the alternate-buffer shape (terminal-sized, overflow hidden, no <Static>), 38 distinct Text lines per frame, Bun.gc(true) checkpoints sampling bun:jsc heapStats and process.memoryUsage, with vmmap -summary sampled against the pid. Identical 18,000-frame workload per build. Post-GC JSC heap, MB: turn fork 6.4.8 fork 7.1.0 upstream 7.1.1 1 116.36 84.36 67.31 3 114.64 236.84 108.55 6 115.19 464.47 173.18 The pinned fork plateaus within 1% across turns 2-6. Fork 7.1.0 grows ~76 MB per 3,000 frames and upstream 7.1.1 ~21 MB, neither settling. Upstream also grows external linearly 36.75 -> 90.57 MB, consistent with its unbounded measure and wrap caches. So the current pin is the best of the three on this axis, and both candidate upgrade targets introduce a render-path leak it does not have. That is a constraint the migration has to answer, not an argument against it. All three drive WebKit Malloc to ~5 GB virtual with dirty of 198.0 / 527.9 / 897.8 MB and footprints of 230.3 / 562.2 / 930.8 MB, from Ink rendering alone with no streaming, media or shell activity. That reproduces the allocator high-water signature #2852 attributed elsewhere. IOAccelerator stays flat, so this does not reproduce that component of #2905. E33 and the report subsection built on it are marked superseded rather than deleted. The probe is published in the reproduction appendix. Workload is adversarial (every line distinct), so build-to-build comparison is sound but absolute figures are not a production forecast. Refs #3345
E35's native figures were endpoint-only: vmmap was sampled on a wall-clock timer, uncorrelated with the post-GC checkpoints. That left the important question unanswered and invited the misreading that the pinned fork is at fault, since the endpoint row showed 5.0 GB virtual and 198 MB dirty. Re-measured with vmmap sampled from inside the target at each checkpoint, immediately after Bun.gc(true), so every turn has matched JS and native rows. Pinned fork 6.4.8 over 18 turns of 3,000 frames, 54,000 frames total: footprint peaks at 699.3 MB on turn 7, then settles to about 683 MB and holds for eleven turns. Dirty WebKit Malloc follows the same shape, virtual is capped at 5120, and JSC heap is flat at ~106 MB throughout. There is a large one-time high-water on turn 1, 81 MB to 691 MB, and no growth after it. So the pinned fork does not leak in the render path. The earlier endpoint figures were that one-time high-water, not accumulation, and the evidence now says so explicitly. Fork 7.1.0 over the same probe grows linearly on every metric: footprint 255.0 MB at turn 1 to 2150.4 MB at turn 10, about 210 MB per turn, against 683 MB flat on the pin. Virtual is not capped either, 4608 rising to 7373. That sharpens what the missing Ink guard (#3365) is for. It is a prevention gate rather than a tracker for a current defect: nothing leaks in the render path today, but the obvious upgrade path reaches multi-gigabyte footprint within an ordinary session and no existing check would fail on it. The per-turn probe is added to the reproduction appendix. Workload caveat is unchanged: every line of every frame is distinct, so the absolute high-water is not a production forecast. The plateau-versus-linear distinction is the durable result. Refs #3345 #3365
Collaborator
Author
|
Closing this. It was a documents-only pull request, which is not a reasonable thing to ask anyone to review and merge. The content is not lost. The full P0 record is now on #3345 as five comments, readable without a checkout:
Parts 1, 2 and 4 carry a correction at the top. The render-path figures in them were measured under Ink's default frame throttle, so the probes rendered about 1.3% of the frames they requested. The build-to-build comparison stands and the conclusion survived re-measurement, but the absolute numbers should not be quoted as full-rate rendering. The parts of this work that were actually shippable have shipped or are shipping as code:
The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Delivers P0 only of #3345: the inventory-and-spike report for replacing the
pinned
@jrichman/ink@6.4.8fork with upstreamink, plus the selection portdesign note the issue requires before any P1 selection work. Documentation only.
No manifest, lockfile, dependency, or source file changed, which is what P0
explicitly requires ("No mainline dependency change").
Recommendation: fix the leak now on the pinned fork with an eleven-line
backport of upstream's own change, and commit to upstream Ink as the migration
destination. The selection gate is conditional on a bounded prototype.
Findings that drive it, all measured:
ui.useAlternateBufferdefaultsto
trueand schema defaults really are materialised intosettings.merged(verified by executing the real
mergeSettings, not by reading the schema),so the default session takes
AlternateBufferLayout, which mounts no<Static>. The exposed population is users who explicitly opted out,excluding screen-reader and CI. For them growth is superlinear: bounded
history re-emitted in full on each of six
refreshStatic()remounts into anaccumulator that never resets.
latestis 7.1.0 from2026-06-24 with no publish or push since, and it does not contain
vadimdemedes/ink#950. Its<StaticRender>does zero the accumulator butwrites 6.7x its workload even chunked per item, and its own docs say
incremental invalidation for growing lists is unsolved. Compiling against fork
7.1.0 costs only 3 errors in one test file, so it is cheap to adopt and still
the wrong destination.
reset the accumulator to 0 on remount, matching upstream 7.1.1 exactly. The
same anchors applied unchanged to fork 7.1.0, and the patch survives a real
bundle:clibuild, which the published bin prefers. Terminal behavior afterthe reset is untested and recorded as a prerequisite.
upstream's public
DOMElementtree resolved a cell to a UTF-16 offset andpainted inverse video through the public
Transform, compiling clean with nodeep imports. That retracts this PR's earlier claim that a tree port needs
private internals. The remaining cost is parity and roughly 137 direct
Textimport sites.
Migration sequence is A, C, B, D, and A, C, and B can each land while still
building on the patched fork because none depends on selection.
The decision belongs to maintainers; this PR publishes the data and argues a
position, it does not enact one.
Dive Deeper
Files
project-plans/issue3345/plan.mdproject-plans/issue3345/p0-evidence.mdproject-plans/issue3345/p0-spike-scripts.md.mjsfiles since the repository does not accept new JS sourcesproject-plans/issue3345/p0-report.mdproject-plans/issue3345/selection-design-note.mdEvery claim in the report and design note carries an
E<n>citation into theevidence pack or a file-and-line citation into source that was read. All 33
evidence items (E0 through E32) are referenced, every reference resolves, and no
evidence item is orphaned.
Method
packages/cliwas compiled against an out-of-tree install ofink@7.1.1bymerging a temporary tsconfig that redirects only
compilerOptions.paths.ink.Compiling against the fork yields 0 errors; compiling against upstream yields
26 errors across 11 files, every one attributable to the swap. The catalog is
a floor, not a ceiling: TypeScript stops after the first incompatible attribute
per JSX element, so
VirtualizedListreports onlyoverflowYeven thoughscrollTopandscrollbarThumbColorare equally absent. The type-level absencelists (E3, E4, E5) are the authoritative inventory.
Behavioural probes ran both packages side by side for static retention, static
remount, text-cache growth, geometry, alternate-screen write sequences, the test
harness's interactive-mode detection, and the community viewport primitives.
Corrections this makes to the issue's own inventory
The issue's P1 list was assembled by source inspection and asked P0 to validate
it. All five items are confirmed, two with corrections. P0 also found surfaces the
list omitted:
Text.terminalCursorFocus/terminalCursorPosition,useApp().rerender,Instance.recalculateLayout/getSelection, thetextWrapunion change,
userSelectremoval, Node>=22, and the fork-onlyResizeObserver,getPathToRoot,StyledChar,clearStringWidthCache,setStringWidthFunction, geometry, scroll, and text exports.Two further corrections:
alternateBuffertoalternateScreendoes not preservebehaviour. The fork homes and clears every alternate-buffer frame; upstream
appends static output inline and erases only prior live lines.
overflow: hiddenplus a negative top margin),upstream
measureElementalready returns scroll-translated child coordinates.At
scrollOffset=5, row 5 measures at the viewport's ownyand row 0measures at
y=-4. The fork's scroll-offset subtraction has no replacement tobuild, and a naive port would double-subtract.
Community viewport packages
All three named packages were installed, inspected, and evaluated individually
against the current
VirtualizedList/ScrollProvider/ScrollableListcontracts, with residual gaps recorded per package and a per-package verdict.
ink-scroll-view'sControlledScrollViewwas run against upstream 7.1.1 anddoes supply working clip and translate. All three are rejected as the viewport
abstraction, with the specific missing contracts listed as the issue requires.
The recommendation is to reuse the measured clip-and-negative-margin technique
rather than take a dependency.
Licensing
Four fork-only modules (
selection.js,scroll.js,layout.js,vertical-gap.js) carryCopyright 2025 Google LLCandSPDX-License-Identifier: Apache-2.0headers inside a package whose declaredlicense is MIT. Upstream 7.1.1 ships no such headers. Recorded for the
license and attribution review the issue already schedules. P0 makes no legal
determination.
Review process
OCR could not review this change: it excludes
.mdasunsupported_ext, andocr review --previewreported all five files as excluded. An independentcontent audit was run instead. It returned 3 HIGH, 8 MEDIUM, and 5 LOW findings.
All were triaged and applied, including two that materially changed the analysis:
the
<Static>remount cadence (six productionrefreshStatic()call sites, notone) and the community viewport evaluation (all three packages evaluated, not
just one primitive). The audit's counterargument to the recommendation is
represented in the report rather than discarded.
Reviewer Test Plan
Nothing executable changed, so review is a read plus optional reproduction.
Read
project-plans/issue3345/p0-report.mdfirst, thenselection-design-note.md.p0-evidence.mdis the backing data.Spot-check citations. Every
E<n>should resolve to a heading inp0-evidence.mdand actually support the claim. Source citations should pointat the named content.
Reproduce the compile spike using the procedure in
p0-spike-scripts.md:Expect 0 baseline errors and 26 spike errors across 11 files.
Reproduce a memory probe.
static-retention.mjsandstatic-remount.mjsare the two that carry the memory argument. Expect 61,990 on both packages,
and a reset to 0 on upstream but not the fork after a key change.
Challenge the recommendation. It is a judgment on top of the data and the
issue explicitly invites challenge. The report lists four conditions that
would reverse it.
Testing Matrix
Documentation only; no runtime behaviour to exercise per platform. Verification
was run on macOS:
npm run format(clean),npm run lint(0),npm run typecheck(0),npm run test(0),npm run build(0),scripts/check-doc-placement.ts(pass),prettier --checkon all five files(pass), and
bun scripts/start.ts --version(0.11.0, exit 0). Thestepfun-37smoke prompt loaded the profile and reached the provider, thenreturned
API Error: 400 you have no active step plan subscription, which isaccount state on the StepFun side and not a code path this change touches.
Linked issues / bugs
This PR makes progress on #3345. It deliberately does not close it: #3345
spans P0 through P4, and this delivers P0 plus the gated selection design note.
P1 waits on the maintainer go/no-go decision this report is written to inform.
One P0 obligation remains open and is recorded as such in
plan.md(criterionA5, partially met): PTY verification of the structurally forced row-zero
viewport anchor across entry, redraw, resize, clear, failure rollback, signals,
and teardown.