Declarative render shell, 8 bug fixes, accessibility, and maintainability debt - #2
Merged
Conversation
Promotes the translate-instead-of-re-record technique from the path level (draw/lineCache.ts) to the picture level, and extends it to the time axis, which currently has no cache at all and re-lays out every label every frame. Three independent scroll-layer slots (line, candles, axis), each a cached SkPicture composited at a per-frame horizontal offset. Constrained by drawPicture ignoring globalAlpha, so scroll layers are used only at composite alpha 1 with a live-draw fallback otherwise — the same gate draw/index.ts already applies to the grid picture. Records the accepted trade-off on time-axis label fades and the scope note that lineCache already captures most of the line-side win. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Generic cross-frame cache for a scroll layer: an SkPicture recorded once in build-time screen coordinates and re-composited each frame at a horizontal offset, instead of re-recorded. Promotes the technique lineCache.ts already applies at the path level up to the picture level. Nothing consumes this yet — no behavior change. Key is a push-built positional array owned by the slot rather than named kFoo fields: three consumers (line, candles, axis) have barely-overlapping key dimensions, so named fields would force every consumer's dimensions into this module. Split numeric/reference arrays because palette colors are strings and formatter identity is a function reference, and merging them would make the hot per-frame comparison polymorphic. Zero per-frame allocation — both arrays live on the slot and are written by index. dx is always recomputed against the build-time reference, never accumulated (same invariant as lineCache.ts:166-175). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Agent worktrees under .claude/worktrees/ contain full copies of src/, so jest was collecting every suite three extra times — 40 suites / 772 tests instead of 11 / 226. Git already excludes the path via .git/info/exclude; jest had no equivalent rule, which made every verification run during parallel work silently meaningless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Interval selection, label-key generation, overlap resolution and label-set identity for the cached time-axis scroll layer, extracted as pure worklets with a type-only Ctx2D import so jest can exercise them. Not wired in yet — timeAxis.ts, draw/index.ts and engine/step.ts are untouched, so there is no behavior change. Overlap resolution is stride decimation on absolute time, not greedy left-to-right. Greedy is deterministic for fixed input but NOT stable under scroll: the chain is anchored on whichever label is currently leftmost, so the frame a label leaves the left edge, every survivor swaps for its neighbour — a visible flicker once per interval. The old per-label alpha tiebreak was masking exactly this with a crossfade, and alpha is gone in the new design. Keeping a label iff round(t/interval) % stride === 0 makes its fate depend only on its own timestamp, so scrolling can never flip a survivor to a dropout. Label-set identity carries interval and stride alongside count/first/last: count+first+last alone collides for real (same span and count, different stride) and that difference changes the picture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds example build:android:release and docs/android-profiling.md, the Android counterpart to run-and-profile's iOS CPU A/B discipline. Every command in the doc is marked verified-on-emulator or explicitly unverified. Main finding: dumpsys gfxinfo framestats is BLIND to this app and must not be used as the A/B metric. Measured on a release build over 103 clean frames, ui_record (the HWUI display-list record pass) p50 = 0.10ms — empty. SurfaceFlinger --list shows no separate SurfaceView layer, so react-native-skia presents an externally-updated texture: HWUI composites one quad while all engine work happens on a thread framestats never samples. The 138ms 'total' is the engine's own frame pacing, not cost. Recorded as a negative result with a re-test recipe rather than deleted. Primary metric is therefore per-thread CPU, with Perfetto for the per-frame timeline. A rendering change should move the main thread; a delta in mqt_v_js means the example app's workload changed instead. Also documents three traps hit during verification: hw.gpu.enabled=no is still persisted in the liveline_test AVD (the -gpu host launch flag overrides it, and getprop ro.hardware.egl cannot detect the difference — only the SurfaceFlinger GLES string can); example/android is expo-prebuild output and untracked, so it is absent in a git worktree; and this Android 34 build emits 23 framestats columns, so parsers must key on column name, not index. Emulator only. Physical-hardware baseline remains open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two changes to drawTimeAxis, both invisible on screen. Per-label alpha, crossfades, edge fades and the alpha-based overlap tiebreak are all untouched. 1. Label text is memoized by key. formatTime(key / 100) is a pure function of key, so a label's text cannot change while its key and the formatter both stay the same. The create-labels loop nonetheless called the formatter for EVERY target on EVERY frame and then overwrote each existing label's text with a byte-identical string. At the ~6 labels a 30s window shows, that is ~360 formatter calls per second, and defaultFormatTime allocates a Date, three padStart results and a template string per call — several thousand short-lived allocations per second on the UI thread, producing text that was already correct. The formatter now runs only for a key seen for the first time. A formatter swap is caught by reference identity (same trade gridLayer.ts makes for kFormatValue) and re-texts live labels in place WITHOUT touching their alphas, so re-formatting cannot restart a label's fade-in. 2. Still-targeted labels are no longer deleted at alpha 0. Found by the new tests, not by reading. `targets` deliberately spans one interval beyond each edge, so the buffer keys sit outside the visible x-range where edgeAlpha returns 0. Those keys churned: created at alpha 0, decayed, deleted, then re-created and re-formatted on the very next frame, forever. That is wasted work on its own and it also defeated (1), since each re-creation is a genuinely unseen key. Adding `&& !isTarget` to the delete parks them at alpha 0 instead — one Map entry (targets is capped at 30), invisible because the draw pass filters at alpha < 0.02. Adds src/draw/__tests__/timeAxis.test.ts — this file had no coverage before. 8 tests pinning the memoization, the formatter-swap re-text, that a swap does not restart fade-in, and that stationary frames after the first cost zero formatter calls. 12 suites / 234 tests. Not yet measured on device; this is the change the A/B will be run against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All four hit while measuring the timeAxis memoization on 2026-07-30. - Relaunching resets in-app toggles. The FPS counter was ON for arm 1 (set in an earlier session) and OFF for every later arm, because relaunch resets React state and it defaults off. An always-on frame callback in one arm and not the others cost ~15 points and looked exactly like host drift. The arm screenshots are the check: `56 fps` when on, blank `fps` when off. - Settle is ~50s, not 20. A swapped src/ makes Metro re-bundle; windows opened too early read 1-5%, which is the app still loading, not the app being fast. - Log uptime at the START and END of each arm. Observed 50.7 right after a build/install/bundle, draining over minutes; a mid-arm climb is invisible when sampled once, and a moving load voids the comparison outright. - Know the effect size before trusting whole-process CPU. It cannot resolve ~1-2k JS allocations/sec on this box. That last one is now measured rather than assumed. A head-to-head bench of drawTimeAxis (V8, 20k frames/arm, interleaved, 5 rounds) puts the memoization at 35.1% faster on the JS half — 5.61us -> 3.64us per frame, distributions non-overlapping. At 60fps that saves ~0.12ms/sec of JS work, roughly 0.01% of a core on V8, which is orders of magnitude below what a CPU-time A/B can see. The device A/B measured null (and slightly negative) for exactly that reason. Note the bench uses a no-op ctx, so it covers only the JS half of drawTimeAxis. The ~60-80 Skia JSI calls per frame are NOT measured by it and remain unquantified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Targets react-native-graph's rendering shape — a declarative Skia tree with
continuous motion as a <Group transform> driven by a shared value — while
keeping the blocked-JS-thread guarantee and current visual output.
Feasibility verified in the Skia sources rather than assumed: sksg/
Container.native.ts compiles the tree once on JS and drives all shared-value
prop updates through a Reanimated mapper on the UI thread, and AnimatedProp<T>
= T | {value: T} confirms Group.transform accepts a shared value. Structural
changes are the only thing that touches JS, and this tree has none after mount.
Records the ceiling: variable-count content (axis labels, candles, particles,
orderbook) cannot enter the declarative tree without forfeiting the guarantee,
so it stays inside <Picture> nodes. This is graph-shaped, not a port.
The fill is deliberately not split — it is a semi-transparent gradient polygon,
so abutting seams at ~0.75 AA coverage, overlapping double-darkens, and a
non-AA clip split can gap at fractional dx (and dx cannot be rounded: ~0.17px
of motion per frame would judder). Only the opaque stroke splits cleanly.
States plainly that this is an architecture goal, not a performance one: it
removes about one draw call per frame and will not be measurable, against a
measured baseline where the entire time axis is 4.2% of a core.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replaces <Canvas><Picture/></Canvas> with react-native-graph's shape:
<Canvas>
<Group transform={scrollTransform}>
<Picture picture={scrollPicture} />
</Group>
<Picture picture={screenPicture} />
</Canvas>
Ships as a STRUCTURAL NO-OP. screenPicture is a pure rename of the old
`picture` and takes the full frame unchanged; scrollPicture is the 1x1 empty
placeholder and nothing records into it yet; scrollTransform is driven from the
frame callback but dx is always 0. Verified pixel-identical on the iOS
simulator — line, gradient fill, grid, badge, pulse ring, reference line and
axis labels all unchanged.
The tree is exactly 4 nodes with no conditionals and no .map(), and must stay
that way. sksg/Container.native.ts compiles the tree once on the JS thread and
then applies shared-value prop updates from a Reanimated mapper on the UI
thread; only a STRUCTURAL change re-enters JS via redraw(). A conditional node
here would silently cost the blocked-JS-thread guarantee, which is the whole
reason the engine exists. A comment above the JSX says so.
LivelineEngine.picture -> screenPicture is not a public API break: index.tsx
exports only Liveline, LivelineTransition and the ./types types, so the engine
interface is internal.
No test covers these two files — both pull in the native Skia binding.
Correctness here rests on typecheck plus the on-device visual check.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Additive: assembleLineTail, slot.scratch, slot.fillScratch and updateLinePaths are behaviourally untouched, so the combined path still renders identically. - lineScrollDx(slot, layout) — the one place toX(tRef) - xRefAtBuild is written, now shared by the combined path and the split tail so they cannot disagree. Same formula as scrollLayer.ts's scrollLayerDx. - linePrefixPath(slot) — returns slot.prefix as-is, no copy. Confirmed safe: assembleLineTail does scratch.addPath(prefix) then offset() on the SCRATCH, so the prefix is never translated. Pinned by a test running 200 scrolling frames and asserting the prefix's verbs deep-equal their build-time snapshot. - assembleLineTailStroke(...) — slot-owned tailScratch, moveTo(cutX + dx) plus the same two drawSplineTail cubics, in current screen coords. The tail is re-emitted rather than sliced out of the combined path: composing it as addPath(tail, extend) would insert a zero-length connector verb and break verb-for-verb parity with today's output. Two extra cubicTo per frame is cheaper than that divergence. The fill is deliberately NOT split — it is a semi-transparent gradient polygon, so abutting halves seam at ~0.75 AA coverage and overlapping halves double-darken. See the design doc. +5 tests (234 -> 239): prefix unmutated over 200 frames, tail scratch allocated once, tail starts exactly at cutX+dx, combined path equals prefix+tail verb-for-verb, fill deep-equals today's construction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stream A caught that src/draw/scrollLayer.ts already exists on this branch (3db71f1) with the slot type, the dx formula and the alpha gate integration needs — and that the design doc mentioned none of it, nor listed it in either workstream's files. Integration should reuse it, not re-derive dx and alpha handling. Also disambiguates scrollLayerDx (a pure function returning a number) from scrollTransform (a shared value holding [{translateX}]), which earlier wording conflated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ent traps Four independent cleanups from the best-practices audit. 1. peerDependencies react-native-gesture-handler >=3.0.0 -> >=2.30.0. The v3 floor existed only because useLivelineEngine imported the type `LegacyComposedGesture`, which is a v3 name. That import was also just wrong: this hook always returns a single Gesture.Pan() and never composes, so `ComposedGesture | GestureType` was an over-broad union. Narrowed to `GestureType`, which exists in both majors. The floor mattered because Expo SDK 55 pins ~2.30.0, so every SDK 55 consumer hit a peer conflict and an expo-doctor failure on install. Verified end-to-end, not by inspection: installed react-native-gesture- handler@2.30.0 at the repo root and ran the real typecheck. src/ compiles clean and all 239 tests pass. Confirmed the test has teeth by re-adding the v3-only import under that install and watching it fail with TS2724 "has no exported member named 'LegacyComposedGesture'". Two earlier attempts at this verification (tsconfig paths override, then installing only into the example workspace) silently kept resolving to 3.1.0 and proved nothing. The example app stays on 3.1.0 deliberately, so its expo-doctor warning remains — that is the example's pin, no longer a library constraint. 2. files[] listed android, ios, cpp, *.podspec and react-native.config.js. None of them exist; this is a JS-only library. npm pack still emits 255 files / 1.3 MB, unchanged. 3. Liveline.tsx documents why `androidWarmup` and `opaque` are NOT set on <Canvas>. androidWarmup is a GPU->CPU pixel readback inside onDraw (SkiaPictureView.java) and is catastrophic on a continuously-invalidating view. `opaque` is what selects SurfaceView over TextureView on Android and would likely composite cheaper, but an opaque surface has no destination alpha and both drawEdgeFade and drawEmpty composite with destination-out, which would erase to black. 4. README documents hoisting config props. Liveline is memo'd with a shallow compare, and inline object/function props do more than waste a render: the engine mirrors config into a shared value every commit and the frame loop's idle detection keys off that object's identity, so a chart fed fresh props each render can never go idle. Placed next to the FlatList recipe, which is exactly where one row's tick re-renders every row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the declarative shell. The single-series line's prefix stroke is now recorded once into scrollPicture and translated by <Group transform> instead of being re-recorded each frame; screenPicture draws the tail stroke, the whole fill polygon, and everything else. Multi-series and candle mode are untouched by construction. Fixes a defect a concurrent review found in 2e1c4d7: `scrollDx` initialised to 0 and `dx` was the literal 0, so the guard `if (scrollDx.value !== dx)` was `0 !== 0` and the scrollTransform write was UNREACHABLE. The shared-value -> <Group transform> binding had therefore never executed, which means that commit's "verified pixel-identical" screenshot proved the tree renders but proved nothing about the binding it was supposed to de-risk. That binding is now verified positively, not assumed: forcing dx = scrollDx+40 made the prefix stroke visibly detach and slide 40px clear of the fill, which simultaneously proves scrollPicture is populated, the transform prop fires on the UI thread, and there is NO double-draw (no stroke was left behind on the fill edge, so screenPicture really is drawing the tail only). Probe reverted. The split is gated on a lineCacheHits hit computed BEFORE drawFrame, so the picture can never be recorded from a prefix the frame's own screen pass has already moved past — one re-record per miss, on the following frame. Alpha gate returns 1 or 0 only, never a partial: drawPicture ignores globalAlpha, so reveal morph, scrub dimming and degen shake all fall back to the unchanged combined path. Z-order note: the prefix stroke now composites below the fill rather than above. This is a no-op through the public API — theme.ts derives fillTop/fillBottom from the same rgb as the line, and `color?: string` is the only colour knob, so the fill hue can never differ from the stroke hue. Removes src/draw/timeAxisLayer.ts and its 41 tests. It had no consumer anywhere, shipped to users via files[]/src, and the review found it carried both a missing worklet-ordering NOTE and a label-set identity that cannot detect a scale change — with a test asserting that defect was correct. Recoverable from 302df29 / 82d8f6b on perf/scroll-layer-architecture. Tests 239 -> 224 (+26 lineScrollLayer, -41 timeAxisLayer). Verified on the iOS simulator. NOT verified: the scrub fallback path — synthetic input cannot engage the pan gesture (documented in run-and-profile), so that needs a human. This is an architecture change, not a speedup. It removes roughly one draw call per frame against a measured baseline where the entire time axis is 4.2% of a core. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Picture re-recording stays paced at ~60fps (MIN_FRAME_INTERVAL_MS), but translating an already-recorded picture is nearly free, so the scroll layer's <Group transform> now advances on every vsync — 120fps on ProMotion — while recording cost is unchanged. This is the actual payoff of putting scroll in a transform. Before this, the dx write sat BELOW the pacing gate, so the transform was capped at 60 along with the recording and the split had bought a draw call rather than smoother motion. A concurrent review flagged exactly that (its finding 10) and it was right. The iOS side needed nothing: example/ios/.../Info.plist already carries CADisableMinimumFrameDurationOnPhone=true (Expo default), without which iOS hard-caps any app at 60 regardless. dx cannot simply be recomputed on a skipped vsync — it needs `layout`, which is computed inside engineStep — so it is linearly extrapolated from the last two RECORDED frames. Extrapolating observed motion rather than recomputing from windowSecs/chartW is deliberate: it keeps no copy of the engine's time-advance rules so it cannot drift out of sync with them, and pause, window transitions and time-debt catch-up all fall out correctly for free. Every recorded frame overwrites with the exact value, so error cannot accumulate past one vsync. Guarded three ways: only when a scroll layer is actually compositing, only when a usable rate has been observed, and only across a gap short enough for that rate to still hold (MAX_SCROLL_EXTRAPOLATION_MS = 20). A quiescence resume, a return from background or a JS stall leaves the transform untouched rather than flinging it somewhere it never was. The math lives in src/engine/scrollExtrapolate.ts rather than inline, so jest can reach it — useLivelineEngine.ts pulls in the native Skia binding and can't be unit-tested, and the simulator renders at 60Hz so this branch barely executes there. A sign error would have shipped invisibly and surfaced only on real hardware. +13 tests (224 -> 237) covering direction of travel, every guard boundary, and a 2s 120-vs-60Hz simulation asserting divergence stays within one frame of motion. KNOWN CONSEQUENCE, needs real hardware to judge: the prefix now moves at 120Hz while the tail is re-recorded at 60Hz, so on skipped vsyncs they shear by one frame of scroll — chartW / (windowSecs * refreshHz), about 0.25px on a 10s window and 0.08px on 30s. Sub-pixel, but a shimmer at the join rather than a clean offset. The simulator renders at 60 and cannot show this either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four independent quality reviews (reuse, simplification, efficiency,
altitude). Three findings were raised by 2-3 reviewers independently; the
efficiency pass corrected the simplification pass on one, which is why item 7
is narrower than first proposed.
Deleted as dead:
- scrollLayer.ts's entire clip API — setScrollLayerClip, clipX/Y/W/H, and the
header's ctx.clipRect/translate/drawPicture example. None of it executed:
the sole consumer bakes the clip into the recording and composites through
<Group transform>. The module's headline example was not how the module was
used.
- scrollLayerDx. It and lineCache.ts's lineScrollDx had identical bodies and
BOTH doc comments claimed to be "the ONE place" the subtraction is written.
ScrollLayerSlot also loses tRef/xRefAtBuild — copied from the line slot, so
a mirror that could go stale rather than a source.
Simplified:
- The scroll layer's key drops from 16 pushed dimensions to 4. Thirteen were
copied verbatim from LineCacheSlot's own key to ask one question: has the
prefix been rebuilt? A monotonic buildRev counter answers it, and is
strictly MORE correct — the old key would have silently missed a fourteenth
dimension had one been added to lineCacheHits.
- canCompositeLineScroll replaces lineScrollLayerAlpha, which returned only
0 or 1 and encoded a transform condition (shake) as an opacity.
- The extrapolation rationale was written four times, three near-verbatim.
Full text now lives only in scrollExtrapolate.ts.
Fixed:
- The liveness guard compared against a SECOND empty picture, so before the
first recorded frame it read "layer is live" when nothing was compositing.
Now a boolean on EngineState, set where the recorded frame already knows.
- scrollTransform.value = [{ translateX: dx }] allocated an array and an
object per assignment — up to 240/sec at 120Hz on the UI thread, in a
library that pools paints and rects to avoid exactly this. Now a 3-slot
preallocated ring, rotate-mutate-assign, zero steady-state allocation.
(A shared value must be reassigned to notify its mapper, so mutation alone
is not an option.)
- assembleLineTail no longer builds the combined path when it will be
discarded — but ONLY when splitting AND there is no fill. With fill on,
scratch still feeds the fill polygon and is genuinely needed.
Hardened:
- MIN_FRAME_INTERVAL_MS and MAX_SCROLL_EXTRAPOLATION_MS are coupled by two
inequalities that nothing expressed. Raising the former to 22 — a plausible
"record at 45fps" tuning — silently reverts the scroll layer to juddering,
with no test failing and the symptom visible only on 120Hz hardware. Both
inequalities are now documented and asserted in engine/__tests__/
constants.test.ts, turning a silent hardware-only regression into a red test.
- QUIESCENT_FRAME_THRESHOLD's comment claimed "~1.5s at 60fps"; it counts
vsyncs, not recorded frames, so it is ~0.75s on ProMotion. Pre-existing, but
this branch is the first code that depends on the distinction.
Deliberately NOT done, each for a stated reason:
- Publishing an analytic scrollDxPerMs slope from step.ts to replace
observeScrollRate. Very likely the better design — exact rather than
observed, and it would work on the first vsync after a resume where the
current code freezes — but it is a design change, not a cleanup.
- Extracting makeChartLayout (touches the candle and multi-series pipelines).
- Reusable ctx in canvas2d.ts (~2200 allocs/sec, the largest single win found,
and entirely pre-existing).
- drawTimeAxis's per-label save()/restore() (~1000 JSI calls/sec, pre-existing).
Tests 237 -> 218: removed tests for deleted code, added constants.test.ts.
Verified on the iOS simulator — rendering unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Unreleased section predated 33c23cb and 8e674cb. Adds the full-refresh scroll transform and the coupled frame-timing constants, including the unverified 120Hz-prefix-vs-60Hz-tail shear — the simulator renders at 60Hz and cannot show it, so it is recorded as an open question rather than a claim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`yarn lint` was reporting 14 errors from files under .claude/worktrees/ — full repo copies that are not in the working tree. Same trap already fixed for jest via modulePathIgnorePatterns; git excludes the path via .git/info/exclude. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… disable PLAN_MAINT #7. The three control bars (window pills, mode toggle, series chips) each repeated the same bar chrome and SlidingIndicator wiring. - <PillBar> owns the shared chrome and the indicator gate. - useBarStyle(ws, isDark) memoizes the seven style scalars that were being recomputed inline on every render — and a live chart re-renders every tick. - useChipStyle is deliberately separate: chips also branch on seriesToggleCompact, which the other two bars know nothing about. The file-level `eslint-disable react-native/no-inline-styles` is now gone entirely rather than narrowed. Deliberately NOT unified: the series bar has no indicator (chips toggle independently, there is no single active one), and per-button onLayout wiring stays at the call sites because the two bars key their layout maps differently (number secs vs string mode) — pulling that in would have needed a render-prop API costing more coupling than the duplication did. The <Canvas> subtree is untouched — verified, zero hunks in that region. Its node structure must stay fixed or the blocked-JS-thread guarantee is lost. Verified by a style-parity check across all 12 combinations of windowStyle x theme x seriesToggleCompact, each crossed with active/inactive, hidden/visible and multi/non-multi, flattened the way RN flattens a style array: all identical. Not visually verified — repointing Metro would have disturbed four concurrent agents, and screenshotting the primary checkout would have shown unmodified code and proved nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PLAN_MAINT #5. lineCache and candleCache declared overlapping invalidation-key fields under inconsistent names — kMin/kMax vs kMinVal/kMaxVal for the same layout.minVal/maxVal. Seven common fields now live in a shared LayoutKey in pathCache.ts, which already held the precedent (CachePath, ensured). Allocation-free property preserved and documented: the key object is allocated once per slot in createXCacheSlot, writeLayoutKey mutates in place on the miss branch only, and layoutKeyMatches is seven direct === compares with no array, loop or temporary. A docblock warns against "generalizing" it into a field-name array — this runs 60-120x/sec on the UI thread. pathCache.ts stays Skia-free via a structural KeyedLayout subset rather than importing ChartLayout, mirroring the CachePath precedent, so jest can still exercise cache logic with fake path recorders. kDataSource stays per-slot: the doc counted it among the common fields, but it is not derivable from layout so writeLayoutKey(key, layout) cannot write it. buildRev semantics untouched, so lineScrollLayer's contract is unchanged. Zero test edits needed — neither cache test ever referenced a k* field by name; they drive the public functions and assert on recorded path ops. Follow-up: draw/gridLayer.ts has a third slot with the same shape and would fold in cleanly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PLAN_MAINT #3 and #2. #3 carries a real bug fix. **The bug:** EngineState declares eight per-series Maps. `seriesAlpha` has the same lifecycle as the four that were pruned — written per series, read in two places — but was never pruned or cleared anywhere. A long-lived chart whose series ids churn leaked one entry per dead id, forever. It leaked *because* pruning was four hand-written near-identical loops, so adding a fifth map meant remembering to write a fifth loop. Registration now lives next to the field declarations: `perSeriesMaps(s)` returns the maps that must be pruned together and `pruneByIds(currentIds, maps)` does it. Adding a ninth map is one line in one place. Audited all eight rather than assuming: - smoothValuesScratch — .clear()-ed every multi frame, correctly excluded (a test asserts the exclusion). - lastMultiStashRevs/lastMultiStashData — their separate prune LOOKS like the same lossy size>length proxy, but the insert loop runs before the check, so a same-count id swap transiently pushes size above length and the dead id is caught. They must stay excluded: they are keyed by the PREVIOUS series set, and pruning against live ids would destroy the reverse-morph data. - pausedMultiData is a ninth map the doc didn't count — a nullable whole-map snapshot with no cross-series-set lifetime. Excluded, documented. Call site confirmed non-per-frame: it sits inside the existing shrink guard, which already allocated a Set there. **#2:** the same 16-line ChartLayout literal appeared three times, re-verified byte-identical on this branch before extracting. toX/toY stay closures — one allocation per frame per chart, exactly as before — with a comment against "optimizing" them into shared mutable state, which would alias across the three pipelines. No existing test modified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PLAN_MAINT #6 — the prerequisite for the pipeline extraction (#1). Tests 218 -> 319 across the branch. Written to catch mutations, not for coverage percentage. Each significant test pins a specific one-line regression, e.g.: - hover at x=0: `hoverPixelX === null` weakened to `!hoverPixelX` would freeze the crosshair mid-drag at the left edge. - Three "deliberate non-condition" tests in quiescence: adding !cfg.showPulse, !cfg.showMomentum or !cfg.orderbookData. Pulse defaults true, so that one would silently disable quiescence for nearly every chart. - Log-space window transition: 10s->40s halfway is 20 (geometric mean), asserted `not` 25 — pins logLerp against a linear rewrite. - 60Hz vs 120Hz convergence, paired with a test proving an uncorrected lerp WOULD diverge, so removing the dt term is caught rather than masked. - 300-frame convergence asserting toBe(40) bit-exactly, paired with a proof that an unsnapped exponential never gets there — a residual epsilon invalidates the range and grid caches forever. - candleAtX: an exhaustive 1000-candle sweep over every bucket centre, which catches binary-search off-by-ones that spot checks miss. Also covers computeAdaptiveSpeed and computeCandleRange, which PLAN_MAINT omitted. One finding, source deliberately NOT changed: updateCandleRange is not exactly frame-rate independent. Its lerp is dt-corrected but the adaptive speed is recomputed from the current gap every frame, so 120Hz re-evaluates the schedule twice as often and lands slightly ahead (<1% of travel). engine/helpers.ts's updateRange avoids this by computing its speed once and passing it in. The test asserts the actual bounded drift rather than an aspirational exact match, and still catches removal of dt. Worth deciding later whether the candle pipeline should hoist its speed computation to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Liveline is memo'd, but a live tick produces a new `data` array so the memo fails and the WHOLE component re-rendered — including all three control bars and their style hooks — even though none of the controls' inputs changed. At the example app's 300ms tick that is ~3 wasted subtree reconciliations/sec; at its 50ms setting, 20/sec. Adds a memo boundary at <LivelineControls>, which also calls useBarStyle and useChipStyle inside itself so those leave the tick path too, with three memo'd bars behind it. A tick now re-renders Liveline only; React bails at the boundary, skipping three bars, two style hooks, three SlidingIndicators and the two icon <Canvas>es. The two btnLayouts maps were pushed DOWN into the bars that produce them rather than hoisted into a hook — that state was only ever read by its own bar, so a measurement now re-renders one bar instead of the whole chart. The non-obvious part: consumers pass inline arrows (the example app's onModeChange and onSeriesToggle are both inline, and `series` is rebuilt each parent render). The parent re-renders every tick, so those props are new identities every tick and the memo would have failed 100% of the time — decorative rather than effective. useStableValue projects arrays through a content signature and useStableHandler is a latest-ref wrapper, so the guarantee holds regardless of consumer discipline rather than depending on it. No data, value, candles, series, lineData, liveCandle or engine shared value crosses the boundary; the chip descriptors deliberately carry id/label/color only. Public API unchanged: index.tsx byte-identical, LivelineProps field count unchanged. The <Canvas> subtree is byte-identical too — verified mechanically, not by eye, because a conditional node there would silently cost the blocked-JS-thread guarantee. NOT verified at runtime: react-test-renderer and @testing-library/react-native are both absent, so Liveline cannot be mounted in jest, and the running simulator serves the primary checkout rather than the worktree. The evidence is a prop-by-prop static argument for tick-stability, not a measured render count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
**orderbook.ts — the big one.** `mixColor` called `rgbColor`, which does `new Float32Array([...])` per call, once per active label inside the render loop. With MAX_LABELS = 50 alive that is up to 3,000 allocations/sec on the UI thread, in a library that pools paints, paths and rects precisely to avoid this. candlestick.ts already avoids the identical trap by precomputing bull/bear colors outside its loop; the orderbook could not, because each label's blend varies per-label per-frame. Now a preallocated `colorPool` of MAX_LABELS Float32Array(4) on OrderbookState, with `mixColorInto(out, from, to, t)` writing in place. Pooling was chosen over cache-keying deliberately: `colorStrength` is a continuous per-label-per-frame value (intensity x fadeIn x fadeOut), so quantizing it to a key would either band a fading label visibly or grow a cache entry per frame. Aliasing argument, checked at the call site rather than assumed: canvas2d's applyStyle calls `paint.setColor(style)` synchronously inside fillText/strokeText, copying the four floats into Skia's native paint state before returning. The loop is strictly sequential (mixColorInto -> fillStyle -> fillText), so the previous value is consumed before the next slot is written, and each iteration uses a distinct slot indexed by loop position. **Hoisted per-frame closures** — small individually, but the codebase's whole discipline is that these do not exist in the frame path: - draw/index.ts `collapseC` was allocated on EVERY candle frame but only invoked during the brief reveal-collapse window; its definition is now gated inside that branch. - candlestick.ts `bodyRect`/`spatialDim` and dot.ts `drawChevrons` hoisted to module-level worklets taking their captured values as parameters. All three hoists verified declared ABOVE their callers — the babel worklet transform loses const hoisting, so a helper referenced before its own assignment captures as undefined and crashes ONLY in release Hermes builds, which neither jest nor the debug simulator catches. candlestick.test.ts asserts exact native call counts and passes unmodified, so the call sequence is unchanged. **crosshair.ts** — drawCrosshair skipped its outline `if (tooltipOutline)` while drawMultiCrosshair drew it `if (tooltipOutline !== false)`: opposite defaults for an omitted argument. README and types.ts both document the default as `true`, so drawCrosshair now matches. No behavior change today (the single call site passes an explicit boolean) — a latent trap removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
54ed706 widened the peerDependency to >=2.30.0 but the lockfile still recorded >=3.0.0, so `yarn install --immutable` failed with YN0028 — i.e. CI would have broken on that commit. Caught by an agent that hit it while trying to install. One line; regenerated rather than hand-edited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oll rate
All three pre-existing or introduced-yesterday, none caught by any gate.
**1. timeDebt over-accrued ~3x per second of quiescence (SEVERE).**
`dt = now_ms - s.lastFrameTimestamp`, and `lastFrameTimestamp` is written in
exactly one place — the recording path. The quiescent early-return credited
`timeDebt += dt/1000` without touching it, so consecutive skipped frames
credited 16.7ms, 33.3, 50, 50, 50... (clamped at MAX_DELTA_MS). At 60Hz that
is ~3.0s of debt per 1s of real time, and it got WORSE at 120Hz.
Pause a chart, leave it 10s, unpause: ~28s of debt instead of 10, so
`now = Date.now()/1000 - timeDebt` puts the chart ~18s further into the past
than when it paused — the window snaps right, then rips forward under
PAUSE_CATCHUP_SPEED_FAST.
Fixed with a second clock, `lastAccrualTimestamp`, advanced on every frame that
credits debt. `lastFrameTimestamp` still is not written in the quiescent
branch, so both of its documented properties survive: the frame that breaks
quiescence still gets one large clamped dt, and the pacing gate still measures
"when did state last actually advance". The two clocks are equal on every
recorded frame and diverge only across skips — exactly the interval that was
being double-counted.
**2. A suspended frame loop lost the whole background interval (HIGH).**
setActive(false) stops accrual; wall-clock does not stop. Resume dt is clamped
to 50ms, so a 60s absence credited 0.05s. A PAUSED chart therefore jumped
forward ~60s on foreground, dumping a minute of data off the left edge.
All setActive calls now route through one path that stamps Date.now() on the
first suspension and credits the elapsed time on resume, scaled by
pauseProgress — so unpaused charts (pauseProgress 0) get exactly zero credit
and keep their correct advance-to-wall-clock behaviour, with no JS-side pause
knowledge required. `active={false}` routes through the same path: a chart
scrolled off a FlatList for a minute had the identical bug.
**3. scrollDx discontinuities poisoned observeScrollRate (introduced 33c23cb).**
dx is an offset OF A PARTICULAR recorded picture. Two consecutive recorded
frames 16ms apart can report -35 then 0 with the line perfectly continuous,
because the prefix was rebuilt in between and the new one already bakes in
that 35px. Differencing them yields +2.1 px/ms describing nothing physical;
the next paced-out vsync would shove the <Group transform> ~35px right and the
following frame would snap it back — a flick per line-cache rebuild, on
high-refresh hardware only. The gap bound could never catch it: a rebuild
happens on an ordinary 16ms frame, inside every timing guard.
observeScrollRate now takes `layerChanged` (scrollActive toggled, or
lineCache.buildRev moved) and returns 0, which makes extrapolateScrollDx
return null — "leave the transform exactly where it is". The guard does not
latch.
+21 tests (319 -> 340). timeAccrual.ts is Skia-free so jest can reach it: 1s/1s
accrual at 60 and 120Hz over 10s and 60s runs, a replay of the OLD arithmetic
asserting the ~28s-per-10s regression, and end-to-end property tests that a
paused chart's displayed instant survives 10s quiescent + 60s backgrounded
while an unpaused one advances by exactly the absence.
NOT tested: the hook wiring itself (native Skia import). Bug 3's symptom needs
120Hz hardware — the simulator is 60Hz and never paces out.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…i-exit leak Six confirmed bugs plus allocation cleanup, from a deep review of the least- reviewed file in the repo. Adds src/engine/__tests__/step.test.ts — the first behavioural test of engineStep itself, mocking the draw layer so the engine's DECISIONS are testable without a Skia binding. Confirmed non-vacuous: reverting step.ts + state.ts makes 9 of its 14 tests fail. 1. BLANK CANVAS. `visible.length < 2` returned before every draw call, so when data existed but nothing was in the window — a stalled feed, or returning from background — the recorded picture was literally empty. No grid, no empty state, nothing. The multi pipeline already handled the same case correctly; single-series now matches it verbatim. 2. STALE LIVE VALUE. `out.valueText` was written only by the single-series pipeline, and the hook treats null as "leave unchanged", so `showValue` + a switch to candle mode froze the readout on its last line-mode number forever. Candle now publishes the same number drawBadge prints two lines above (so they cannot disagree); multi publishes an explicit '' since there is no single "the" value. 3. `candles.slice()` EVERY CANDLE FRAME on the raw buffer — 60k objects/sec on a 500-candle chart at 120Hz — for a consumer that only fires when the user changes candle width. Now gated on candlesRev. Subtlety preserved: `.width` must still track the previous frame's width every frame, so only the array copy is gated. 4. PER-SERIES MAPS NEVER PRUNED ON LEAVING multi mode. All pruning was gated inside the multi pipeline, which stops running once isMultiSeries goes false — so displayValues, seriesAlpha, the scratch maps and lineCaches (which retain SkPaths) all survived. Sibling of the seriesAlpha leak fixed in aef4ddf. The agent widened the predicate to cover multi->candle too. 5. A series added while already hidden faded OUT instead of never appearing, and briefly widened the Y range. Seeds at target alpha, not 1. 6. REVERSE-MORPH STASH ALIASED THE LIVE CANDLE. `visible.slice()` froze the array but its last element IS s.displayCandle, lerped in place every frame, so the "frozen" stash kept moving. Now copied into a pooled slot; pooling is safe by construction (written only on hasData frames, read only on !hasData). 8. updateGridLayer got raw dt in the line and multi branches while candle passed pausedDt and documents why. Fixed both. 9. Per-frame allocations: empty-array sentinels for the points/candles slots, pooled scratch for oldVisible and the close-price object. hoverEntries could NOT be pooled — it is retained in lastHoverEntries and read on scrub fade-out frames — so it starts as a read-only sentinel and allocates only inside the active-hover branch. 7 was assessed a FALSE POSITIVE and deliberately not "fixed": cfg is written to a shared value and therefore deep-cloned into the UI runtime, so cfg.lineData is a per-commit clone, not the caller's array. Slicing it would add an unconditional O(n) copy per frame — precisely the bug #3 describes. state.ts reconciled by 3-way merge: this work and c24ac65 added disjoint field sets from the same base. Tests 340 -> 354. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
**Reusable ctx.** createCanvas2D built a fresh ~29-allocation object (23 interface methods, 3 helper closures, the save stack, the dash array) on EVERY recording — 60/sec per Liveline instance, plus each nested grid/scroll-layer re-record. It was unavoidable only because every method closed over `canvas`, which is new per recording. The ctx now lives on SkiaCache and is retargeted: closures read a mutable `cv` instead of a captured const. Zero allocations on every call past the first per cache. Signature and return type unchanged, so no call site moved. Per-recording state is enumerated in a numbered comment block and reset in one place (`retarget`): canvas, pooled path contents, the adopted-path pointer, lineDash, the save stack, fonts, and all 13 style props. The build path routes through retarget too, so it is the single source of truth and the first recording is identical to every later one. CRITICAL invariant, documented on the field: one ctx PER CACHE, never module scope. Two recordings are live simultaneously — engine/gridLayer.ts and engine/lineScrollLayer.ts each open a nested recorder during the main frame's recording, and each already passes its own SkiaCache precisely so pooled paints cannot alias. One ctx per cache inherits that isolation exactly; a single shared ctx would let the nested recording scribble into the main one, intermittently. **Per-label save/restore in drawTimeAxis.** Each save() allocated a 14-field StyleSnapshot AND issued a canvas.save() JSI call recording a save/restore pair into the picture — 6-10 of each per frame, plus 12-20 extra recorded ops. It only protected four properties the next iteration reassigns unconditionally. Hoisted the loop-invariant ones; globalAlpha is set per label and restored once after the loop. Merged with care rather than copied: the agent's base was `main`, so a file copy would have silently reverted this branch's label-text memoization, off-screen churn fix and closure hoists. 3-way merged against main as the base (clean, zero conflicts) and verified both sides survived. typecheck then caught what jest could not — the agent's test built a TimeAxisState without the memoization's formatTimeRef field; tests passed regardless because jest does not typecheck. Adds timeAxisCallBudget.test.ts (5 tests) kept separate from timeAxis.test.ts: disjoint concerns (call budget vs formatter memoization) and different fake-ctx harnesses. One test asserts saveCount === 0 while >= 3 labels draw, which pins this change directly. Verified on the simulator: Line, Candles, Multi, Dashboard, Orderbook. Dashboard is the strongest case — four concurrent instances means four caches and four contexts, which is exactly where a shared-ctx aliasing bug would show. Tests 354 -> 359. yarn install --immutable passes; bob build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Read the spec back and verified it claim by claim against 0350072. All eleven claims match — the four-node tree with no conditionals, prefix-stroke-only scroll layer, the fill deliberately unsplit and still built whole, the binary alpha gate via scrollLayerUsable, dx never accumulated (no `dx +=` anywhere in src/), no double-draw, no text in the scroll layer, and all three addendum claims including the 4-dimension key. The one divergence was omission rather than contradiction: 33c23cb's high-refresh scroll transform — recording paced at ~60fps while the <Group transform> advances every vsync, with extrapolation and its three guards — shipped with no design record at all, despite being the most consequential runtime behaviour on the branch. Written up now, including the unverified 120Hz-prefix-vs-60Hz-tail shear, which needs ProMotion hardware the simulator cannot provide. Status corrected from "implementation dispatched" to implemented. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A red screen naming src/engine/state.ts and a for...of helper — both edited that day — turned out to be a stale Metro on host 8082 whose module map was corrupted, while the healthy one served iOS on 8083. Same repo, one platform broken, and the error pointed straight at innocent code. Also records that adb reverse does NOT apply here: the app connects to 10.0.2.2, the emulator's NAT route to the host, which bypasses reverse entirely. Mapping the port did nothing and looked like it should have. Adds the one-command diagnostic — curl each port for the exact bundle URL — which removes the emulator, the app's cached error screen and your assumptions in a single step. It took three wrong hypotheses to reach it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fuller
changed the title
docs/superpowers/specs/ held two working documents from the session that produced this branch. Neither belongs in the repo: - the scroll-layer architecture design describes the time-axis scroll layer, which was measured, priced and PARKED on another branch — it is not in this PR at all - the declarative-shell design is a pre-implementation record whose content is now in the code comments and the CHANGELOG docs/android-profiling.md stays: it is a real procedure with commands someone will run again, including the two preconditions that have each silently invalidated an Android measurement on this project before. Removes the CHANGELOG's pointer to the deleted spec rather than leaving it dangling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fuller
changed the title
An independent validation pass found the `showValue` fix from c49bb03 was INCOMPLETE, and that the blank-canvas fix in the same commit widened the same hole. Three early returns draw the empty/loading state and bail without publishing `out.valueText`. Since `null` means "leave unchanged" to the caller, the readout keeps showing whatever the PREVIOUS mode last wrote: - the shared no-data return (all modes) - multi-series with no visible series - single-series `visible.length < 2` — added by c49bb03 itself So a chart mounted with no data, or one that loses every point out of the window, or a switch to a multi-series config whose data has not arrived yet, still showed a stale price from another mode. That is exactly the bug c49bb03 claimed to close, surviving on the paths where no chart is drawn at all. Fixed with one `publishBlankValue(out, cfg)` helper called at all three sites rather than three copies — the reviewer's point was that this is one defect class in three places. It writes '' (a real value that overwrites) rather than null, and only when `cfg.showValue` is set, so charts without the readout publish nothing and cost nothing. Also closes a coverage gap the same review found: `slot.buildRev++` in `updateLinePaths` had NO test. It is the sole invalidation signal for the scroll-layer picture and the input to `observeScrollRate`'s `layerChanged` guard, yet deleting it failed nothing — `lineScrollLayer.test.ts` bumps the field by hand, so nothing asserted a real rebuild moves it. Both fixes mutation-verified rather than assumed: - removing the three `publishBlankValue` calls fails 2 of the new tests - removing `slot.buildRev++` fails 3 of the new tests (previously 0) Tests 359 -> 365. Note for the record: the new lineCache tests passed jest while FAILING typecheck (wrong `makeLayout` arity). Jest does not typecheck — green tests are not a green gate. Same trap as the timeAxis test earlier on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The README's peer table still said `react-native-gesture-handler: >=3.0.0`, and the prose below it said the library "is not tested against gesture-handler 2.x anymore" — while package.json declares `>=2.30.0` and the CHANGELOG describes widening it precisely so Expo SDK 55 (which pins ~2.30.0) does not hit a peer conflict. So the published README actively contradicted the fix it shipped alongside: an SDK 55 consumer reading it would conclude their gesture-handler version was unsupported and either skip the library or needlessly bump gesture-handler, defeating the whole point of 54ed706. Found by an independent API/docs review. Table corrected and the prose rewritten to state that both majors are supported, why the 2.30 floor exists, and that compatibility was verified by installing 2.30.0 and running the typecheck rather than by inspection. All four documented peer ranges now match package.json exactly. Also adds example/eas.json with a `preview` profile (Android APK, iOS device build) for producing an installable artifact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The chart is a Skia surface, so VoiceOver and TalkBack previously encountered an unlabelled blank region — `git grep accessibilit src/` returned zero hits. For a library whose purpose is displaying a live number, that was the largest gap between this and something a team would adopt. Two public props: `accessibilityLabel` (default 'Live chart') and `testID`. The built-in controls DERIVE their test ids from testID, so Detox/Maestro can drive the window pills, mode toggle and series chips without further props. The design constraint, and why the obvious approach was wrong: the live value lives on the UI thread as a shared value updated 60-120x/sec, while accessibility props are JS-thread React props. Bridging per frame would destroy the architecture the whole library exists for. The implementation does not bridge at all. `engine.valueText` is the LERPED value — the engine eases toward each incoming number over several frames — so announcing it would speak an interpolated figure that never existed. The authoritative number is already a JS-thread prop. So there is no runOnJS, no extra mapper, and ZERO change to useLivelineEngine.ts or step.ts. Cost with no screen reader running: one promise and one screenReaderChanged subscription at mount, then nothing — no interval, no state, no per-frame work. With a reader active: a 1Hz sample of a ref, committing only when the FORMATTED string changed (a live feed jitters below display precision constantly; comparing raw numbers would announce once a second forever). Ticks flow into a ref, never into state, so the memo boundary added in 6aa05c3 still holds. Also fixed, and not asked for: the icon-only mode toggle was two entirely unnamed buttons; compact series chips are bare coloured dots with no text; the faded-but-mounted series bar was offering a reader controls nobody can see; and the showValue overlay is a TextInput updating at frame rate that a reader would have announced continuously — now hidden from assistive tech. +15 tests (365 -> 380) on the Skia-free announce logic, including the sub-precision-jitter silence case. Canvas subtree verified byte-identical. NOT verified with a real screen reader — no VoiceOver session was run. The zero-cost-when-off and no-re-render-on-tick claims rest on reading the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lently
Three defects found by an API/docs review.
1. `parseColorRgb` accepted `#` + 3-8 hex digits but only expanded the 3-digit
form, so a CSS Color 4 shorthand like `#38fc` produced `parseInt('', 16)` =
NaN for the blue channel, propagating silently into gradients and paints.
Now expands 3- and 4-digit, returns only for 6/8, and falls through to the
grey fallback for any other length rather than mangling a partial parse.
`color` is typed as a plain string with no documented format restriction,
so a consumer piping a design token or colour-picker value through it got
garbage rather than an error.
2. Unparseable input (named colours, garbage) fell back to grey with no signal
at all. Now warns under __DEV__ naming the offending value.
The warning is de-duplicated against the last-warned value, which the
original fix did not do: `parseColorRgb` is called from the FRAME PATH —
draw/dot.ts parses `palette.line` and `palette.badgeOuterBg` while
scrub-dimming — so an unconditional warn would fire up to 60 times a second
for as long as a bad colour was set. That is noisier than the silent
fallback it replaced. One string compare per call, no allocation, and a bad
colour still reports exactly once.
3. `LivelineTransition` rendered nothing, silently, when `active` matched no
child key (a typo, or a conditionally-absent child) — blank chart, no
signal, self-healing only when `active` later changed to a real key. Now
warns under __DEV__ naming both the bad value and the keys that DO exist.
Also corrects a comment in theme.ts claiming "Dot — always semantic": the dot
paints from `palette.line` unconditionally, and the eight momentum/glow/badge
palette fields it referred to are computed but read by nothing. The fields are
kept (removing them means touching types.ts and fixtures owned elsewhere) with
an accurate comment instead.
Adds src/__tests__/theme.test.ts (15 tests) — the module had none. Covers
3/4/6/8-digit hex, case, rgb()/rgba(), invalid lengths, named colours, a
NaN-freedom sweep, and that resolveTheme derives fillTop/fillBottom from the
same rgb as `line` (the invariant the scroll-layer z-order relies on).
Tests 380 -> 395.
NOT acted on: the agent also reported resolveTheme has "zero callers" and that
Liveline.tsx never imports theme.ts. That is false — Liveline.tsx:41 imports it
and calls it at 745, 762 and 774. Verified before discarding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The safety net for PLAN_MAINT #1 — extracting three inline pipelines from a ~1,900-line function that share one EngineState. Existing coverage was 17 regression tests pinning bugs fixed today; this pins the surface. Asserts EngineState field values, which draw functions ran, how many times, and in what ORDER (via jest invocationCallOrder merged across nine mocks). Deliberately never asserts argument tuples — a concurrent refactor is reshaping those signatures, and tuple assertions would break for reasons unrelated to correctness. Covers: golden 5-frame sequences per pipeline (~16-22 EngineState fields each, explicit toEqual rather than snapshot files); the full six-direction mode transition matrix plus a line->multi->line round trip; and the ordering invariants that are load-bearing but currently only documented in comments — the scroll-layer gate running after updateHoverState and before drawFrame, grid-before-draw in all three pipelines, and pausedDt at all three updateGridLayer sites. 12 mutations applied, run, reverted — 12 caught. One (flipping the line pipeline's window buffer) escaped the first draft because the buffer lands on no EngineState field; a group reading layout.rightEdge off the call closed it. Independently re-verified here: mutating one pausedDt to dt fails exactly the grid-dt test. THREE FINDINGS FOR THE REFACTOR, all pinned as-is on purpose so the extraction changes them deliberately rather than by accident: - `rangeInited` is ONE flag shared by all three pipelines, and it is a real stale-state leak. After line->candle, updateCandleRange sees it already true and LERPS the Y range away from the line's range instead of snapping to the candle's. Symmetric for candle->multi. If the extraction gives each pipeline its own range state, two tests here will fail — that failure is the signal. - Cleanup is asymmetric: the five per-series maps are pruned on leaving multi mode, but nothing prunes candle state or line state on exit. - drawMultiFrame gets raw dt while drawFrame/drawCandleFrame get pausedDt. An extraction that unifies the three call sites silently changes multi-series pause behaviour. Also documents what resisted characterization: the scroll-layer gate is unreachable under a mocked draw layer (lineCacheHits needs a prefix only the real drawFrame builds), so the mock had to write the cache slot — without that every scroll-layer test would have been vacuously green. A trap for anyone extending this file. Tests 395 -> 437. No source file touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Not cosmetic — this width already blocked real work. A pooled-decimation optimisation was REVERTED rather than add a 17th positional parameter to drawLine. At 15 parameters, adjacent same-typed arguments are silently swappable and the compiler cannot help; drawLine alone passed six consecutive numbers. drawLine 15 -> 4 lineCacheHits 8 -> 3 drawCandlesticks 15 -> 3 updateLinePaths 13 -> 7 engineStep 12 -> 4 updateHoverState 12 -> 7 updateRange 12 -> 7 updateWindowTrans 11 -> 8 updateCandleCache 10 -> 8 paintLineCurve 12 -> 10 Two mechanisms, chosen per function. Where the parameters were a genuine bag of unrelated per-frame values: a POOLED args struct on EngineState, created once in createEngineState and overwritten in place — the precedent multiSeriesEntryScratch already sets. Where the list was re-deriving fields of an object the function already received: just pass that object. The naive version of this change would have been a regression. These run 60-120x/sec on the UI thread, so an options object allocated per call is exactly the per-frame allocation this codebase pools paints, paths and rects to avoid. Net effect is the opposite: THREE per-frame object literals removed (two in step.ts, plus one per visible series per frame in drawMultiFrame), zero added. Verified by scanning every added non-test line for object/array literals — the only ones are in the create* functions, which run once per chart. Behaviour-preserving, and that is verified rather than asserted: the 42 characterization tests added in bd9b693 pass unchanged. They pin EngineState values, draw-call counts and call ORDER across all three pipelines, so a behavioural drift would have shown. Re-confirmed they still have teeth after the signature change by mutating one pausedDt to dt and watching exactly the grid-dt test fail. Non-mechanical bits worth knowing: - lineCacheHits now derives its key from `visible`, so it indexes it. The short-circuit order was restructured so the null-prefix and length/rev checks run BEFORE any indexing — an empty array can never reach the index. - FrameInputs moved to engine/types.ts: defining it in step.ts made state.ts import from step.ts, which imports state.ts at runtime. Type-only so it was erased, but not a cycle worth leaving for a bundler to trip over. - s.lineCacheRef and s.lineDrawArgs are now shared across all three pipelines. Safe because exactly one pipeline draws per frame and every call site writes ALL fields before calling — checked at each of the three drawLine sites and both drawCandlesticks sites, and documented on the fields. - One test got structurally WEAKER and was strengthened rather than accepted: lineCache.test.ts asserted kLen by passing an impossible length once the key is derived. Replaced with an extra INTERIOR point (same first/last), which isolates kLen exactly, plus three previously-untested derived fields. Deliberately left: updateCandleWindowTransition (19) and updateCandleRange (14) in candleHelpers.ts — the worst offenders, but out of the scope given. The WindowTransInputs/RangeInputs pattern ports directly and they are the obvious next increment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fuller
changed the title
…en series Review findings from the pre-merge pass on PR #2: - Retire-then-dispose every replaced SkPicture (screen ~60/sec, grid and scroll layers per rebuild) one frame after its successor composites, instead of leaving native pictures to the UI runtime's GC. - Accumulate suspended-time credit via runOnUI so the += and the frame callback's drain are serialized on one runtime (double-credit race on fast background/foreground flaps). - Prune hiddenSeries against live series ids: a removed-then-re-added series no longer returns hidden, and dead ids no longer let the last-visible-series guard be defeated into a blank chart. - Move the parseColorRgb dev-warn dedupe off a worklet-captured module binding onto the runtime's own global. - Document (README/CHANGELOG) that formatValue/formatTime must be pure: label text is cached per tick and re-evaluated on formatter identity change, not per frame. - Type-pair drawMultiFrame's lineCaches/lineCacheRef so passing one without the other can't silently disable caching; pool the pairing on EngineState (no per-frame literal). - Mechanical: scroll ring-buffer size as a named constant; shared useBtnLayouts hook; signature builders get a reference pre-check and keep control-byte delimiters; unit tests for mixColorInto (+4). Gates: tsc, eslint, jest 25 suites / 441 tests (characterization suite unchanged), yarn install --immutable, bob build. Verified live on the iOS simulator with the dispose path active every frame. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Started as a rendering refactor to match react-native-graph's shape. Reviews along the way surfaced real defects, so it became a bug-fix, accessibility and debt-clearing pass. 36 commits, tests 160 → 437.
No performance claim is made. The CPU case for the original refactor was measured and did not hold up — the whole time axis is 4.2% of a core, and a 35% improvement to its JS was undetectable on device. What this delivers is eight user-visible bug fixes, accessibility support, a large reduction in per-frame allocation, and six of the seven
PLAN_MAINTitems.Bugs fixed
All pre-existing. None caught by typecheck, lint or tests.
timeDebtaccrued ~3×/sec during quiescenceshowValuefreezes permanently after a mode switchseriesAlphanever pruned#38fcproduced aNaNcolour channelLivelineTransitionrenders blank, silentlyactivenaming a missing child key warned nothingPlus a
scrollDxdiscontinuity that would have flicked the scroll layer ~35px sideways once per line-cache rebuild — 120Hz-only, invisible to every check available here.Architecture
<Canvas><Picture/></Canvas>→ a fixed four-node tree with the scroll layer under a<Group transform>. Verified in the Skia sources that this keeps the blocked-JS-thread guarantee: the tree compiles once on JS, then shared-value prop updates flow through a UI-thread mapper. The transform advances every vsync (120fps on ProMotion) while recording stays paced at ~60.Accessibility
Previously zero — VoiceOver and TalkBack met an unlabelled blank region. Adds
accessibilityLabelandtestID(controls derive their ids from it), labels the icon-only mode toggle and compact series chips, and hides the frame-rateTextInputoverlay from assistive tech.Costs nothing when no screen reader runs: no interval, no state, no per-frame work. With one active, ≤1 React render/sec, comparing the formatted string so a sub-precision-jitter feed stays silent.
Per-frame allocation removed
createCanvas2D~1,860/sec · orderbook colours ~3,000/sec · axissave()/restore()~500 objects + ~1,000 JSI calls/sec · a 500-candle.slice()at 120Hz · controls out of the tick path · net −3/frame from the options-struct work.Maintainability —
PLAN_MAINT#2–#7 (only #1 left)makeLayoutextracted from three identical literals · per-series pruning registered beside the field declarations · sharedLayoutKey·PillBarextracted, file-wide inline-styles disable removed · options structs replacing 15-parameter signatures (drawLine15→4,engineStep12→4) · +42 characterization tests forengineStep.The characterization suite is the notable one: it pins
EngineStatevalues, draw-call counts and call order across all three pipelines, and the options refactor passed it unchanged — behaviour preservation verified, not asserted. 12 mutations applied, 12 caught.It also mapped the remaining hazards for #1:
rangeInitedis one flag shared by all three pipelines and leaks Y-range state across mode switches; cleanup is asymmetric;drawMultiFrametakes rawdtwhere the others takepausedDt. All pinned as-is so the extraction changes them deliberately.Consumer-facing
>=2.30.0— the v3 floor came from one unnecessary type import and brokeexpo-doctorfor every Expo SDK 55 consumer. Verified by installing 2.30.0 and confirming the check could fail first.files[]cleaned ofandroid/ios/cpp/*.podspec— none exist.package.json),colorformats documented, prop-hoisting guidance, and whyandroidWarmup/opaqueare not set.Verification
typecheck · lint · 24 suites / 437 tests ·
yarn install --immutable·bob build.Device: iOS simulator, Android emulator (GPU-accelerated — Line, Candles, Multi, Orderbook all correct), and a physical iPhone 16 Pro.
Not verified — read before merging
🤖 Generated with Claude Code
https://claude.ai/code/session_011N9XLUqavTstRW9dZo83YR