feat: finalize Bijou v3.0.0 for release#40
Conversation
- Implement Buffered Surface engine with 2D cell grids, masking, and affine transforms - Implement High-Performance Differential Renderer (renderDiff) for flicker-free TTY updates - Implement Programmable Rendering Pipeline with multi-stage middleware support - Implement Unified Animation Heartbeat (PulseMsg) synchronized to hardware refresh rate - Implement Reactive & Semantic Token Graph for adaptive, reference-based theming - Implement Fractal TEA (Sub-App Composition) via mount() and mapCmds() - Implement Render Throttling and Batching in the TEA runtime - Establish technical specifications for 17 upcoming features in docs/specs/ - Maintain backward compatibility for existing string-based components via V3 migration bridge
- Implement BCSS Engine with zero-dependency CSS parser - Implement BCSS Resolver supporting Specificity (ID > Class > Type) and Media Queries - Implement TokenGraph bridge in CSS via `var()` syntax for semantic color mapping - Integrate BCSS into the Rendering Pipeline via `bcssMiddleware` - Implement `runInWorker` to run TEA apps in Node.js background threads, keeping main thread responsive - Standardize `BijouNodeOptions` across core components (id, class, ctx) - Update `badge` to resolve global BCSS overrides
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
- Upgrade canvas() to support high-resolution Braille (2x4) and Quad (2x2) plotting - Support rich cell output (char, color, bg) from fragment shaders - Implement normalized UV mapping (0.0 to 1.0) for resolution-independent shaders - Add uniforms registry for passing external state into the shader pipeline - Fix infinite loop bug in quad resolution renderer - Update canvas tests to verify high-res modes and UV normalization
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a surface-first rendering core, programmable render pipeline, unified heartbeat (pulse), motion reconciliation, BCSS/token-graph theming, worker-thread IPC and recorder, shader/canvas v2, sub-app composition, many component API shifts to Surface/ViewOutput, extensive tests, examples, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Thread
participant Worker as Worker Thread
participant IO as Terminal I/O
participant App as Worker-side App
Main->>Worker: runInWorker(entry, options)
Main->>IO: subscribe stdin/resize
IO->>Main: stdin / resize events
Main->>Worker: send {type:'io:data', data} / {type:'io:resize', columns, rows}
Worker->>App: proxy input into update loop
App->>App: update(msg) -> view() => Surface
Worker->>Main: post {type:'render:frame', output: Surface}
Main->>IO: renderSurfaceFrame(io, current, target, style)
sequenceDiagram
participant Model as App Model
participant Pipeline as RenderPipeline
participant Layout as Layout Stage
participant Motion as Motion Middleware
participant Paint as Paint Stage
participant Post as PostProcess
participant Diff as Diff Stage
participant IO as Terminal Output
Model->>Pipeline: execute(RenderState with dt)
Pipeline->>Layout: compute LayoutNode / layoutRoot
Pipeline->>Motion: motionMiddleware.reconcile(layoutRoot, dt)
Pipeline->>Paint: paintMiddleware -> produce targetSurface
Pipeline->>Post: run PostProcess middlewares (bcss/grayscale/etc.)
Pipeline->>Diff: renderDiff(currentSurface, targetSurface)
Diff->>IO: minimal ANSI writes to terminal
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
- Add missing @flyingrobots/bijou-tui dependency to bijou-node - Add missing tsconfig reference from bijou-node to bijou-tui - Add explicit types to callback parameters in worker.ts and skeleton index.ts to satisfy strict check - Fix implicit 'any' types in sub-app and skeleton logic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a5338dff3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bijou-tui/src/runtime.ts (1)
233-255:⚠️ Potential issue | 🟠 MajorRefresh
currentSurfaceon every resize message, not just startup.Only the startup resize rebuilds the backing buffer. After a later terminal resize, the next Diff stage still compares the old-sized
currentSurfaceagainst a new-sizedtargetSurface, which can produce a bad frame before Output swaps in the clone.🪟 Suggested fix
bus.on((msg) => { if (!running) return; @@ const [newModel, cmds] = app.update(msg, model); model = newModel; + if (typeof msg === 'object' && msg != null && 'type' in msg && msg.type === 'resize') { + const resize = msg as ResizeMsg; + currentSurface = createSurface( + sanitizeDimension(resize.columns), + sanitizeDimension(resize.rows), + ); + } render(); executeCommands(cmds); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/runtime.ts` around lines 233 - 255, The currentSurface is not updated on subsequent terminal resize messages, causing diffs against an old-sized buffer; inside the bus.on message handler (before calling app.update/render/executeCommands) detect resize messages (e.g. using the existing isResizeMsg or msg.type === 'resize') and refresh/recreate the backing buffer so currentSurface matches the new terminal dimensions (update the same variable currentSurface to the new-sized surface) before proceeding to update/render/executeCommands; make this change in the bus.on callback alongside the existing isPulseMsg/isKeyMsg handling so subsequent frames diff against the correct targetSurface.
🟠 Major comments (31)
packages/bijou/src/core/theme/accessors.ts-42-54 (1)
42-54:⚠️ Potential issue | 🟠 MajorDon't swallow all token-graph failures in these fallbacks.
These
catchblocks also hide resolver bugs, cycles, or broken fallback tokens and silently convert them intostatus.muted/semantic.primary. Please restrict the fallback to the specific “unknown token” path and rethrow everything else.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/theme/accessors.ts` around lines 42 - 54, The status and ui accessor catch blocks in the accessors (status: (key) => { ... } and ui: (key) => { ... }) currently swallow all errors from tokenGraph.get; change them so they only perform the fallback when the thrown error indicates an unknown/missing-token condition (check error.message or a specific error property from tokenGraph, e.g. "Unknown token" or a recognisable error code) and rethrow any other exceptions (resolver bugs, cycles, etc.). Locate the two tokenGraph.get calls and replace the broad catch with logic that tests the error identity—return the fallback token only for that specific case, otherwise throw the error.packages/bijou-tui/src/motion/motion.ts-30-33 (1)
30-33:⚠️ Potential issue | 🟠 MajorClone
rectwhen retagging an existing layout node to prevent rect aliasing.The spread operator creates a shallow copy, so the returned node initially shares the same
rectreference as the inputcontent. WhileMotionReconcilerlater replacesnode.rectwith an interpolated version, the initial aliasing means any code readingnode.rectbeforereconcile()executes—or if the originalcontentis reused elsewhere—will read and potentially mutate a shared rect object. Cloningrectbreaks this reference chain and ensures animation state doesn't leak into reusable layout subtrees.🔧 Proposed fix
return { ...content, - id: options.key + id: options.key, + rect: { ...content.rect } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/motion/motion.ts` around lines 30 - 33, The returned object currently shallow-copies content (return { ...content, id: options.key }) which leaves content.rect aliased; update the return logic in motion.ts to deep-clone or clone the rect property when present so the new node gets its own rect object (e.g., create a new rect object from content.rect) while keeping id set to options.key; this prevents shared rect references that can be mutated before MotionReconciler runs and avoids leaking animation state from the original content/node.packages/bijou/src/ports/surface.ts-192-200 (1)
192-200:⚠️ Potential issue | 🟠 MajorDeep-clone
modifierswhenever a cell is copied.These copies are all shallow, so copied cells still share the same
modifiersarray reference. That meansget()leaks mutable internal state andclone()is not actually a deep clone. A smallcloneCell()helper reused across the file would fix the aliasing.Also applies to: 237-238, 247-248, 316-318, 348-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/ports/surface.ts` around lines 192 - 200, The copies of Cell objects (e.g., in maskCell) perform shallow copies and leak the same modifiers array reference; create a small helper cloneCell(cell: Cell): Cell that returns a new Cell with a shallow-copied modifiers array (modifiers: cell.modifiers ? [...cell.modifiers] : undefined) and use it wherever cells are copied (maskCell, get(), clone(), and the other locations mentioned) so modifiers is deep-cloned and no shared mutable state remains.packages/bijou/src/core/layout/flex.ts-104-113 (1)
104-113:⚠️ Potential issue | 🟠 MajorDon't drop leftover cells during the final flex pass.
The final pass floors each share and then zeros
remaining, so uneven splits leave space unassigned. Withwidth = 100and three{ flex: 1 }children, this produces33 / 33 / 33and leaves one column uncovered. Please carry the remainder forward and assign the leftover cells deterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/layout/flex.ts` around lines 104 - 113, In the final flex pass inside the flex distribution logic (the block guarded by spaceDistributed false that iterates over children, checks isConstrained and children[i]!.flex and computes share using activeFlex and remaining), don't simply Math.floor each share and zero out remaining; instead compute each floored share, accumulate the totalAssigned, compute leftover = remaining - totalAssigned, and then deterministically add +1 to the first `leftover` eligible children (e.g., iterate in index order and increment sizes[i] for unconstrained children with flex>0) so the final `remaining` is fully assigned and uneven splits are resolved predictably. Ensure you update sizes and clear remaining only after this leftover distribution.packages/bijou/src/ports/surface.ts-263-267 (1)
263-267:⚠️ Potential issue | 🟠 MajorClip
fill()against the original rectangle, not the clipped start.When
fxorfyis negative,xEnd = xStart + fw/yEnd = yStart + fhoverpaints the region. For example,fill(cell, -2, 0, 4, 1)currently writes 4 columns instead of the 2 visible columns. Compute the end fromfx + fw/fy + fhbefore clamping.Proposed fix
- const xEnd = Math.min(w, xStart + fw); - const yEnd = Math.min(h, yStart + fh); + const xEnd = Math.min(w, fx + fw); + const yEnd = Math.min(h, fy + fh);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/ports/surface.ts` around lines 263 - 267, The fill() method clips its end coordinates using the already-clipped start (xStart/yStart) which overextends when fx or fy are negative; compute xEnd as Math.min(w, fx + fw) and yEnd as Math.min(h, fy + fh) (i.e. derive ends from the original fx/fy plus fw/fh before clamping) and keep xStart = Math.max(0, fx) and yStart = Math.max(0, fy) so the written area is correctly limited to the visible rectangle in Surface.fill.packages/bijou-tui/src/animate.test.ts-62-68 (1)
62-68:⚠️ Potential issue | 🟠 MajorTurn these pulse loops into deterministic failures instead of hangs.
If an animation never settles, these tests still end in
await promise, so CI blocks indefinitely rather than failing. Assert completion after the pulse budget, or race the await against a timeout so regressions surface as test failures.Suggested pattern
- await promise; + await Promise.race([ + promise, + new Promise<never>((_, reject) => + setTimeout(() => reject(new Error('animation did not settle')), 50), + ), + ]);Also applies to: 93-99, 121-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/animate.test.ts` around lines 62 - 68, The pulse loops in animate.test.ts that call caps.pulse(0.016) and then await promise can hang CI if animations never settle (variables: settled, promise, caps.pulse, queueMicrotask); after the for-loop that iterates up to 1000 pulses, assert that settled is true (or that promise resolved) and fail the test if not, or replace the final await promise with a raced timeout (e.g., Promise.race with a short reject/timeout) so the test fails deterministically when the pulse budget is exhausted; apply the same change to the other occurrences around lines referencing the same pattern (the loops at 93-99 and 121-126).packages/bijou-tui/src/animate.ts-145-149 (1)
145-149:⚠️ Potential issue | 🟠 MajorKeep pulse animations compatible with plain
cmd(emit)calls.
createSpringCmd()andcreateTweenCmd()now dereferencecaps.onPulseunconditionally. Any existing direct invocation of the returnedCmdwithout capabilities will now throw or never resolve, which is a behavioral break for the public animation API. Please either provide a timer fallback whencapsis missing or make the requirement explicit and update callers before merging.Also applies to: 193-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/animate.ts` around lines 145 - 149, The pulse handlers in createSpringCmd and createTweenCmd dereference caps.onPulse unconditionally, breaking callers that invoke the returned Cmd as cmd(emit) without capabilities; modify both functions to check for caps?.onPulse and if missing use a built-in timer fallback (e.g., requestAnimationFrame loop or setInterval) that supplies dt to the same callback and returns a handle with a dispose/stop method, otherwise continue using caps.onPulse; ensure the fallback clears the timer when the animation resolves or when handle.dispose/stop is called so the Promise resolves/cleans up identically to the caps path (refer to the local variable handle and the caps.onPulse usage).packages/bijou/src/core/render/differ.ts-41-52 (1)
41-52:⚠️ Potential issue | 🟠 MajorPreserve terminal-default styling for uncolored cells.
Cell.fgis documented asundefined = terminal default, but bothsurfaceToString()andrenderDiff()coerce that case to#ffffff. That will emit white styling for plain cells and change appearance on terminals whose default foreground is not white. Render truly unstyled cells as plain text, or build the token without forcing a foreground color.Also applies to: 160-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/render/differ.ts` around lines 41 - 52, surfaceToString currently forces an undefined Cell.fg to '#ffffff', which overrides terminal default colors; change it so the TokenValue only includes a hex property when cell.fg is defined (i.e., omit hex for terminal-default), and apply the same fix in renderDiff (the block around renderDiff and the similar code at lines ~160-166) so uncolored cells are rendered as plain text/no explicit foreground instead of forcing white.packages/bijou-tui/src/app-frame.ts-450-460 (1)
450-460:⚠️ Potential issue | 🟠 MajorThe model-only update path is unreachable under the current
FramePage.updatecontract.The
FramePage.updateinterface (line 80) declares a return type of[PageModel, Cmd<Msg>[]], so theelsebranch handling barePageModelreturns (line 456) cannot execute for typed callers. TheArray.isArraycheck is therefore unnecessary. To support non-tuple returns, explicitly broaden theFramePage.updatesignature to allowPageModel | [PageModel, Cmd<Msg>[]]; otherwise, remove the dead code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/app-frame.ts` around lines 450 - 460, The updateResult handling assumes FramePage.update may return either a tuple or a bare PageModel, but the current FramePage.update signature only declares [PageModel, Cmd<Msg>[]], so the else branch is unreachable; either widen the FramePage.update return type to PageModel | [PageModel, Cmd<Msg>[]] (so callers like targetPage.update can legally return a plain PageModel) or remove the Array.isArray/else branch and always treat updateResult as the tuple form; update the FramePage.update declaration (and any implementors) or simplify the logic around targetPage.update/updateResult and PageModel/Cmd<Msg> accordingly.packages/bijou/src/factory.ts-80-81 (1)
80-81:⚠️ Potential issue | 🟠 Major
createBijou()has no way to configure the BCSS resolver.
resolveBCSSis hardcoded to return{}in bothfactory.tsandadapters/test/index.ts.CreateBijouOptionshas no field to accept a resolver or stylesheet. Contexts created here cannot participate in BCSS resolution—any component callingctx.resolveBCSS()will always receive an empty object.The
bijou-tuipackage works around this by mutating the context after creation ((ctx as any).resolveBCSS = ...), but this bypasses the type system and leaves the default factory unsuitable for CSS-aware components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/factory.ts` around lines 80 - 81, createBijou currently hardcodes resolveBCSS to return an empty object so contexts cannot perform BCSS resolution; add a new optional field on CreateBijouOptions (e.g., bcssResolver or stylesheetResolver) to accept a resolver function or stylesheet, use that value when building the context instead of the hardcoded () => ({}), and fall back to the existing no-op only if the option is not provided; also update the test adapter's default to pass a sensible stub via the new option rather than mutating ctx.resolveBCSS after creation.packages/bijou/src/factory.ts-71-72 (1)
71-72:⚠️ Potential issue | 🟠 MajorRemove the unsafe
unknowncast and explicitly convertThemetoTokenDefinitions.
createTokenGraphexpects a recursive token map ([key: string]: TokenInput | TokenDefinitions), butthemeObjis a structuredThemeinterface with specific properties (status,semantic,gradient,border,ui,surface). The cast hides a real shape mismatch at a public API boundary. Either convertThemeto the expected structure explicitly, updatecreateTokenGraphto acceptThemedirectly, or add a type guard with clear documentation of the conversion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/factory.ts` around lines 71 - 72, The code unsafely casts themeObj to TokenDefinitions; remove the "as unknown as TokenDefinitions" and either (preferred) implement an explicit converter (e.g., convertThemeToTokenDefinitions(themeObj)) that maps Theme's properties (status, semantic, gradient, border, ui, surface) into the recursive TokenDefinitions shape and call createTokenGraph(convertThemeToTokenDefinitions(themeObj)), or change createTokenGraph's signature to accept Theme directly (or add an overloaded signature) and implement a type guard function isTokenDefinitions/validateThemeShape to ensure the runtime structure matches TokenDefinitions; ensure you update references to createTokenGraph and document the conversion/guard, and delete the unsafe cast on themeObj.docs/specs/token-graph.spec.json-22-27 (1)
22-27:⚠️ Potential issue | 🟠 MajorSpecify the resolver behavior for invalid graphs.
The spec says cycles are detected and missing references are an edge case, but it never defines the observable result: throw, emit diagnostics and keep the previous value, or surface an explicit invalid token. BCSS hot-reload and DevTools will need that contract to behave consistently.
Also applies to: 34-42, 44-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/token-graph.spec.json` around lines 22 - 27, Update the spec to explicitly define the resolver contract for invalid graphs (cycles and missing references): choose and document one observable behavior (e.g., throw an error, emit diagnostics and retain previous resolved value, or return an explicit "invalid token" sentinel) and describe how consumers should detect and handle it; add this behavior to the "Token Graph resolution engine" section and to the related items "Support for cycle detection in references" and "Light/Dark mode bucket resolution" (and the other mentioned blocks around lines 34-42 and 44-47), include the exact runtime signals (exception types or diagnostic payload shape and preservation semantics) and a short example of expected outcome so hot-reload/DevTools can implement consistent handling.docs/specs/animation-heartbeat.spec.json-13-18 (1)
13-18:⚠️ Potential issue | 🟠 MajorCarry elapsed time in the heartbeat contract.
A frame pulse alone is not enough for springs/tweens once frames are dropped or the runtime FPS changes. Without a monotonic timestamp or
deltaMsinPulseMsg, the "slow systems" case becomes frame-rate dependent instead of time-based. Add elapsed-time and step-clamping/catch-up semantics to the spec.Also applies to: 32-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/animation-heartbeat.spec.json` around lines 13 - 18, The PulseMsg/heartbeat contract in the spec needs to carry elapsed-time so physics updates are time-based rather than frame-based: update the EventBus/heartbeat schema (PulseMsg) to include a monotonic timestamp (e.g., monotonicMs) and/or deltaMs (elapsedMs) and document step-clamping/catch-up semantics (maxDeltaMs and catchUpSteps) so spring/tween systems can accumulate and process fixed-time sub-steps; ensure the acceptanceCriteria mentions elapsed-time in the pulse and that the pulse frequency remains runtime-configurable (FPS), and update related entries (lines referenced 32-37) to reflect these fields and semantics.docs/specs/component-system.spec.json-17-19 (1)
17-19:⚠️ Potential issue | 🟠 MajorThe binder shape here is read-only.
A binder like
(model) => valuecan read state, but it cannot write user edits back into the parent model. That conflicts with the real-time field-to-model updates in the next criterion and the golden-path test. Define a bidirectional binding contract before standardizing this API.Also applies to: 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/component-system.spec.json` around lines 17 - 19, The binder shape "(model) => value" is read-only and cannot propagate user edits back to the parent model; update the Field-to-Model binding contract so binders are bidirectional by defining a concrete API (for example a Binder interface or a function signature that includes a setter) that supports both reading and writing — e.g., change the binder contract referenced as "Field-to-Model binding" / "(model) => value" to either accept a second parameter for a setter (model, set) or return an object with get/set methods (get(model): value, set(model, newValue): void), and update any uses referenced by the golden-path test and real-time validation to call the setter to push edits into the parent model.examples/v3-worker/main.ts-1-1 (1)
1-1:⚠️ Potential issue | 🟠 MajorUse the public Bijou Node entrypoint in the example.
Importing
../../packages/bijou-node/src/worker/worker.jsreaches into monorepo internals and will not reflect the published API surface. Examples should consume the same@flyingrobots/bijou-nodeentrypoint that end users get.Suggested import change
-import { isBijouWorker, runInWorker, startWorkerApp } from '../../packages/bijou-node/src/worker/worker.js'; +import { isBijouWorker, runInWorker, startWorkerApp } from '@flyingrobots/bijou-node';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/v3-worker/main.ts` at line 1, The example imports internals from the monorepo; replace the deep import with the public package entrypoint so consumers use the published API: update the import that currently brings in isBijouWorker, runInWorker, and startWorkerApp from '../../packages/bijou-node/src/worker/worker.js' to import those named exports from the public package (e.g., '@flyingrobots/bijou-node') so the example mirrors the published API surface.examples/v3-worker/main.ts-50-54 (1)
50-54:⚠️ Potential issue | 🟠 MajorUse
fileURLToPath()instead of.pathnamefor the worker entry path.
new URL(import.meta.url).pathnameproduces broken paths on Windows (e.g.,/C:/...), andpath.resolve()cannot fix the malformed path. Import and usefileURLToPathfromnode:url:import { fileURLToPath } from 'node:url'; const entryPath = fileURLToPath(import.meta.url); runInWorker({ entry: entryPath, }).then(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/v3-worker/main.ts` around lines 50 - 54, The entry path uses new URL(import.meta.url).pathname which yields invalid Windows paths; replace that with fileURLToPath(import.meta.url) (import fileURLToPath from 'node:url') and pass the resulting entryPath into runInWorker so the worker entry resolves correctly on all platforms (update the import and the assignment of entryPath where entryPath and runInWorker are used).packages/bijou-tui/src/runtime.ts-125-139 (1)
125-139:⚠️ Potential issue | 🟠 MajorAvoid mutating the caller’s context object permanently.
resolveBCSSis attached directly toctxand never restored. Any reused context — including a shared default context — can carry the last stylesheet into later runs that did not opt into CSS.♻️ Suggested cleanup
export async function run<Model, M>( app: App<Model, M>, options?: RunOptions<M>, ): Promise<void> { const ctx = options?.ctx ?? getDefaultContext(); + const previousResolveBCSS = (ctx as any).resolveBCSS; const useAltScreen = options?.altScreen ?? true; const useHideCursor = options?.hideCursor ?? true; const useMouse = options?.mouse ?? false; @@ if (options?.css) { @@ (ctx as any).resolveBCSS = (identity: any) => { return resolveStyles(identity, sheet, { width: ctx.runtime.columns, height: ctx.runtime.rows, }, ctx.tokenGraph, themeMode); }; } @@ bus.stopPulse(); bus.dispose(); + if (options?.css) { + if (previousResolveBCSS === undefined) { + delete (ctx as any).resolveBCSS; + } else { + (ctx as any).resolveBCSS = previousResolveBCSS; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/runtime.ts` around lines 125 - 139, The code mutates the caller's ctx by permanently assigning (ctx as any).resolveBCSS, which can leak styles into later runs; change this to save and restore any existing resolveBCSS before you overwrite it (e.g., const prevResolve = (ctx as any).resolveBCSS), assign the new resolver (using parseBCSS/resolveStyles and themeMode as shown), then ensure you restore (ctx as any).resolveBCSS = prevResolve in a finally block after the pipeline/middleware run (or attach the resolver to a local scoped context object passed to bcssMiddleware instead of mutating ctx) so the original context is not permanently modified.packages/bijou-tui/src/pipeline/middleware/css.ts-40-49 (1)
40-49:⚠️ Potential issue | 🟠 MajorKeep
useBCSS()on the same resolution path as runtime BCSS.Runtime injects a token-graph/theme-aware resolver on the context, but this helper bypasses it and calls
resolveStyles()with only the sheet and terminal size. Components usinguseBCSS()can therefore resolve different styles than components usingctx.resolveBCSS().🔁 Suggested fix
export function useBCSS(state: any, identity: { type: string, id?: string, classes?: string[] }) { + const resolveBCSS = state.ctx?.resolveBCSS as + | ((identity: { type: string; id?: string; classes?: string[] }) => any) + | undefined; + if (resolveBCSS) return resolveBCSS(identity); + const sheet = state.data['bcss_sheet'] as BCSSSheet | undefined; if (!sheet) return {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/pipeline/middleware/css.ts` around lines 40 - 49, useBCSS currently calls resolveStyles directly and bypasses the runtime-injected resolver on ctx; change it to prefer the context resolver: if state.ctx.resolveBCSS exists, call state.ctx.resolveBCSS(identity, sheet, terminal) (passing the same identity, sheet and terminal object) and return its result, otherwise fall back to calling resolveStyles(sheet, terminal) as before; keep the existing sheet retrieval and terminal shape and only alter the return path in useBCSS to reference state.ctx.resolveBCSS first.packages/bijou-tui/src/motion/reconciler.ts-16-24 (1)
16-24:⚠️ Potential issue | 🟠 MajorPrune stale motion entries after each reconciliation pass.
Removed nodes never leave
tracked, so long-lived trees will accumulate stale state and a later reuse of the sameidcan animate from the wrong rect/velocity.🧹 Suggested cleanup pattern
export class MotionReconciler { private tracked = new Map<string, TrackedMotion>(); + private seen = new Set<string>(); reconcile(node: LayoutNode, dt: number): void { + this.seen.clear(); + this.walk(node, dt); + for (const key of this.tracked.keys()) { + if (!this.seen.has(key)) this.tracked.delete(key); + } + } + + private walk(node: LayoutNode, dt: number): void { if (node.id) { + this.seen.add(node.id); this.processNode(node, dt); } for (const child of node.children) { - this.reconcile(child, dt); + this.walk(child, dt); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/motion/reconciler.ts` around lines 16 - 24, Reconciliation never removes entries from this.tracked, causing stale motion state to accumulate; after walking the tree in reconcile (using processNode for each node) collect the current node ids into a Set (e.g., currentIds) and then iterate the keys of this.tracked to delete any key not in currentIds so removed nodes are pruned; implement this cleanup at the end of reconcile (or in a top-level reconcileRoot) using the existing node.id values and this.tracked.delete(id).packages/bijou-tui/src/runtime.ts-55-72 (1)
55-72:⚠️ Potential issue | 🟠 MajorPlease route the one-shot render through the same setup path.
This branch returns before the initial resize sync, BCSS bridge, and any pipeline-based render setup run, so
pipe/static/accessiblemode can produce a different first frame than interactive mode for the same app.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/runtime.ts` around lines 55 - 72, The non-interactive branch short-circuits and writes the first-frame output directly (ctx.mode !== 'interactive', app.view/initModel, createSurface, paintMiddleware, surfaceToString, ctx.io.write) so it skips the same initial resize sync, BCSS bridge, and pipeline-based render setup used by interactive runs; change this by routing the one-shot render through the common initialization/render pipeline used for interactive mode (i.e., call the same setup routine before painting) — if such a routine doesn’t exist, factor out the setup steps (resize sync, BCSS bridge, pipeline middleware setup) into a helper (e.g., runInitialRenderSetup or similar) and call it from both the interactive flow and this non-interactive branch, then perform the view() -> paintMiddleware() -> surfaceToString -> ctx.io.write sequence so the first frame uses the exact same pipeline.packages/bijou/src/core/theme/graph.ts-88-113 (1)
88-113:⚠️ Potential issue | 🟠 MajorCycle detection is reused across sibling lookups, so repeated refs can false-positive.
resolveColor()mutates onevisitedset and passes it into transform resolution. A token that mixes the same helper twice can trip the circular-reference guard on the second lookup even though the graph is acyclic. Clone the set when descending, or remove refs on unwind.Suggested fix
if ('ref' in def) { if (visited.has(def.ref)) { throw new Error(`Circular token reference detected: ${Array.from(visited).join(' -> ')} -> ${def.ref}`); } - visited.add(def.ref); + const nextVisited = new Set(visited); + nextVisited.add(def.ref); const target = definitions.get(def.ref); if (!target) { throw new Error(`Token reference not found: ${def.ref}`); } let color: string; if (typeof target === 'object' && 'fg' in target) { - color = resolveColor(target.fg, mode, visited); + color = resolveColor(target.fg, mode, nextVisited); } else { - color = resolveColor(target as ColorDefinition, mode, visited); + color = resolveColor(target as ColorDefinition, mode, nextVisited); } // Apply transforms if (def.transform) { for (const t of def.transform) { - color = applyTransform(color, t, mode, visited); + color = applyTransform(color, t, mode, new Set(nextVisited)); } } @@ case 'mix': { - const otherColor = resolveColor({ ref: transform.with }, mode, visited); + const otherColor = resolveColor({ ref: transform.with }, mode, new Set(visited)); result = mix(dummyToken, { hex: otherColor }, transform.ratio ?? 0.5); break; }Also applies to: 119-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/theme/graph.ts` around lines 88 - 113, The circular-reference guard in resolveColor reuses the same visited Set across sibling lookups and transform descents, causing false positives; update resolveColor (and the transform branch that calls applyTransform) to either clone the visited set when recursing into a referenced token/transform (e.g., pass new Set(visited) into recursive resolveColor/applyTransform calls) or remove def.ref from visited after resolving (unwind) so sibling lookups don't see stale entries; ensure you apply the same fix for the second occurrence noted (the transform-resolution path around applyTransform) and keep references to symbols like resolveColor, applyTransform, visited, and def.ref to locate the changes.packages/bijou-tui/src/css/resolver.ts-39-41 (1)
39-41:⚠️ Potential issue | 🟠 MajorZero-specificity selectors are dropped.
getMatchSpecificity()uses0for a matched universal selector, butresolveStyles()treats0as “no match” and filters the rule out.* { ... }will never apply. Use a separate sentinel for “not matched” so specificity0can still participate in the cascade.Suggested fix
- if (specificity > 0) { + if (specificity >= 0) { matchedRules.push({ rule, specificity }); } @@ - if (specificity > 0) { + if (specificity >= 0) { // Media query rules have slightly higher base priority in CSS matchedRules.push({ rule, specificity: specificity + 0.1 }); } @@ -function getMatchSpecificity(identity: ComponentIdentity, selectors: BCSSSelector[]): number { - let maxSpecificity = 0; +function getMatchSpecificity(identity: ComponentIdentity, selectors: BCSSSelector[]): number { + let maxSpecificity = -1;Also applies to: 49-52, 106-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/css/resolver.ts` around lines 39 - 41, The validator treats a specificity of 0 as “no match”, so universal selectors (specificity 0) are being dropped: change getMatchSpecificity/resolve logic to use a separate sentinel for “no match” (e.g., NO_MATCH = -1) instead of reusing 0; update calls in resolveStyles (the matchedRules push sites and any filtering/ordering logic that currently checks specificity > 0) to check for specificity !== NO_MATCH (or specificity > NO_MATCH) so specificity 0 is considered a valid match and still participates in cascade/merging; adjust any sorting/merge code that assumes positive-only specificity accordingly (references: getMatchSpecificity, resolveStyles, the matchedRules pushes and subsequent filtering/ordering logic).packages/bijou-tui/src/driver.ts-133-135 (1)
133-135:⚠️ Potential issue | 🟠 MajorString-based views ignore resize steps.
Both capture paths rasterize strings with
stringToSurface(viewOutput, 80, 24). After a scripted resize, the app can react to the new terminal size but the captured frame still uses the hardcoded geometry, so snapshots drift from runtime behavior.Also applies to: 152-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/driver.ts` around lines 133 - 135, The string-based capture path is hardcoding 80x24 when calling stringToSurface, causing snapshots to ignore runtime resizes; update both occurrences that call stringToSurface(viewOutput, 80, 24) to use the current terminal dimensions instead (e.g., the driver/terminal width and height variables or a getSize()/cols/rows accessor available in this module) so frame rasterization matches actual runtime size (also fix the duplicate call at the other capture site noted around lines 152-154); ensure you reference stringToSurface, viewOutput, and frame when making the change.packages/bijou-tui/src/pipeline/pipeline.ts-40-40 (1)
40-40:⚠️ Potential issue | 🟠 MajorAsync middleware is part of the API contract, but the pipeline fire-and-forgets it.
RenderMiddlewarecan return aPromise, yetexecute()never awaits it. Async stages can run out of order, and rejected promises bypass thistry/catch. Either makeexecute()async and await the chain, or narrow the middleware type/docs to synchronous functions only.One possible fix
-export type RenderMiddleware = (state: RenderState, next: () => void) => void | Promise<void>; +export type RenderMiddleware = (state: RenderState, next: () => Promise<void>) => void | Promise<void>; @@ - execute(state: RenderState): void; + execute(state: RenderState): Promise<void>; @@ - function execute(state: RenderState): void { + async function execute(state: RenderState): Promise<void> { @@ - const next = () => { + const next = async (): Promise<void> => { if (index < chain.length) { const mw = chain[index++]; try { - void mw!(state, next); + await mw!(state, next); } catch (err) { if (state.ctx.io.writeError) { state.ctx.io.writeError(`[Pipeline Error] ${err instanceof Error ? err.stack : err}\n`); } - next(); + await next(); } } }; - next(); + await next(); }Also applies to: 54-55, 75-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/pipeline/pipeline.ts` at line 40, The pipeline currently allows RenderMiddleware to return a Promise but execute() does not await middleware, causing out-of-order execution and uncaught rejections; change execute() to be async and await the composed middleware chain (and any inner next() calls) so asynchronous middleware are awaited and thrown errors can be caught, updating any middleware composition/runner helpers used by execute() (referencing RenderMiddleware and execute) to return/await Promises and wrap the awaited chain in the existing try/catch to propagate errors correctly; alternatively, if you prefer synchronous-only middleware, narrow RenderMiddleware's signature to remove Promise returns and enforce that everywhere execute and related helpers expect sync middleware.packages/bijou-tui/src/subapp/mount.ts-11-20 (1)
11-20:⚠️ Potential issue | 🟠 Major
mount()currently exposes mapping hooks that never run.
mount()ignoresonMsgandmapCmd, and always returns[]. That makes mounted sub-app side effects impossible to forward even though the public API advertises mapped commands coming back out of this helper.Also applies to: 50-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/subapp/mount.ts` around lines 11 - 20, mount currently ignores the MountOptions.onMsg and mapCmd hooks and always returns an empty command list, so sub-app messages and side-effects are not forwarded; update the mount function implementation to consume the sub-app's returned tuple (model, cmds) or update result and map each Cmd<SubMsg> into Cmd<ParentMsg> using mapCmd when provided (otherwise map using onMsg), and return the mapped commands alongside the mounted model; refer to the MountOptions interface, the mount function, and the mapCmd/onMsg symbols to locate and change the code paths that currently discard commands.packages/bijou/src/core/theme/graph.ts-148-165 (1)
148-165:⚠️ Potential issue | 🟠 Major
set()acceptsTokenValue, butget()cannot resolve it later.
TokenInputincludesTokenValue, butset()stores that object verbatim.get()only handles{ fg: ... }token defs orColorDefinition, sograph.set('x', { hex: '#fff' })will fail withInvalid color definition.Suggested fix
set(path, definition) { - definitions.set(path, definition); + if (isTokenValue(definition)) { + definitions.set(path, { + fg: definition.hex, + bg: definition.bg, + modifiers: definition.modifiers, + }); + } else { + definitions.set(path, definition); + } for (const handler of subscribers) { handler(path); } },Also applies to: 171-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/theme/graph.ts` around lines 148 - 165, set stores TokenValue objects verbatim but get only handles TokenDefinition ({fg, bg, modifiers}) or ColorDefinition, so calling graph.set('x', {hex: '#fff'}) leads resolveColor to reject it; update the logic to normalize TokenValue into a resolvable form (either convert TokenValue {hex} into a ColorDefinition, or detect TokenValue in get and return its hex directly) by modifying the implementations around get and set and the definitions Map handling so that get(path) recognizes TokenValue (hex present) and returns {hex: value.hex} or ensures set(path, value) transforms TokenValue into a ColorDefinition/TokenDefinition before storing; reference functions/idents: get, set, resolveColor, definitions, TokenValue, TokenDefinition, ColorDefinition (also apply the same normalization change to the similar code block around lines 171-176).packages/bijou-tui/src/css/resolver.ts-62-70 (1)
62-70:⚠️ Potential issue | 🟠 MajorInternal
!importantbookkeeping leaks into the returned styles.
resolveStyles()returnsfinalStyles, but lines 68-70 write__important_*sentinel keys into that same object. Any downstream style merge/apply that iterates properties will see fake declarations.Suggested fix
- const finalStyles: ResolvedStyles = {}; + const finalStyles: ResolvedStyles = {}; + const important = new Set<string>(); for (const { rule } of matchedRules) { for (const decl of rule.declarations) { // !important handling - if (decl.important || !isImportant(finalStyles, decl.property)) { + if (decl.important || !isImportant(important, decl.property)) { finalStyles[decl.property] = resolveValue(decl.value, graph, mode); - if (decl.important) { - finalStyles[`__important_${decl.property}`] = 'true'; - } + if (decl.important) important.add(decl.property); } } } @@ -function isImportant(styles: ResolvedStyles, property: string): boolean { - return styles[`__important_${property}`] === 'true'; +function isImportant(important: Set<string>, property: string): boolean { + return important.has(property); }Also applies to: 98-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/css/resolver.ts` around lines 62 - 70, resolveStyles() is leaking internal !important sentinels into the returned finalStyles object by writing keys like __important_<property> alongside real declarations; instead track importance in a separate internal map (e.g., importantMap keyed by property) or keep the sentinel data in a local scope and never add __important_* keys to finalStyles, apply that map when checking isImportant/merging, and remove the writes at the locations that set finalStyles[`__important_${decl.property}`] (also fix the same pattern around the other occurrence referenced in the review).packages/bijou-tui/src/eventbus.ts-333-338 (1)
333-338:⚠️ Potential issue | 🟠 MajorA throwing
onPulsecallback can take down the heartbeat loop.Pulse listeners are invoked without a guard, so one exception escapes the timer callback instead of being reported like middleware and command failures. Wrap each handler in
try/catchand route failures throughsafeReport().Suggested fix
- for (const handler of pulseHandlers) { - handler(dt); - } + for (const handler of pulseHandlers) { + try { + handler(dt); + } catch (err) { + safeReport('[EventBus] Pulse handler threw:', err); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/eventbus.ts` around lines 333 - 338, The pulse loop currently calls each pulse handler directly (pulseHandlers iteration in the heartbeat code), so an exception from an onPulse callback can escape the timer; wrap each handler(dt) call in try/catch and forward any thrown error to safeReport (e.g., safeReport(err, { source: 'onPulse', handler })) before continuing to the next handler, ensuring the heartbeat continues and failures are reported the same way as middleware/command errors; keep the subsequent emit({ type: 'pulse', dt }) unchanged.packages/bijou-tui/src/driver.ts-116-118 (1)
116-118:⚠️ Potential issue | 🟠 MajorThe scripted driver becomes wall-clock dependent once the real pulse starts.
runScript()already has explicitpulsesteps, but startingbus.startPulse()here means anydelaycan inject extra pulse events based on scheduler timing. That makes frame counts and final model state nondeterministic in tests and CI.Also applies to: 184-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/driver.ts` around lines 116 - 118, The driver is starting the real heartbeat with bus.startPulse(), which makes scripted runs using runScript() nondeterministic; stop calling bus.startPulse() during scripted execution—remove or gate the calls at the sites referencing bus.startPulse() (the occurrences around the runScript usage at lines shown) and instead start the real pulse only when running in live/non-scripted mode (e.g., add a boolean flag or check like isScriptedRun or onlyStartPulseWhenLive and call bus.startPulse() when that flag is false, or move startPulse() to after runScript() completes); update the places where bus.startPulse() is invoked (the two occurrences you noted) to use this conditional so tests remain deterministic.packages/bijou-node/src/worker/worker.ts-63-66 (1)
63-66:⚠️ Potential issue | 🟠 MajorKeep
altScreenandhideCursorindependent.With
altScreen: falseandhideCursor: true, these branches still enter and exit the alternate buffer.🩹 Split the terminal modes
- if (useAltScreen || useHideCursor) { - ctx.io.write('\x1b[?1049h'); // ENTER_ALT_SCREEN - if (useHideCursor) ctx.io.write('\x1b[?25l'); // HIDE_CURSOR - } + if (useAltScreen) ctx.io.write('\x1b[?1049h'); // ENTER_ALT_SCREEN + if (useHideCursor) ctx.io.write('\x1b[?25l'); // HIDE_CURSOR ... - if (useAltScreen || useHideCursor) { - ctx.io.write('\x1b[?1049l'); // EXIT_ALT_SCREEN - if (useHideCursor) ctx.io.write('\x1b[?25h'); // SHOW_CURSOR - } + if (useHideCursor) ctx.io.write('\x1b[?25h'); // SHOW_CURSOR + if (useAltScreen) ctx.io.write('\x1b[?1049l'); // EXIT_ALT_SCREENAlso applies to: 117-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-node/src/worker/worker.ts` around lines 63 - 66, The code currently gates both alternate-screen and cursor-hide sequences behind a single condition (useAltScreen || useHideCursor) causing ENTER/EXIT_ALT_SCREEN to be sent even when only hideCursor is true; update the startup and teardown branches in worker.ts so each mode is handled independently: call ctx.io.write('\x1b[?1049h') only when useAltScreen is true and call ctx.io.write('\x1b[?25l') only when useHideCursor is true (and mirror this on exit by sending the corresponding exit sequences only when each flag is true). Locate the blocks referencing useAltScreen, useHideCursor and ctx.io.write and split the combined condition into two separate guarded writes for enter and two separate guarded writes for exit.packages/bijou-node/src/worker/worker.ts-88-97 (1)
88-97:⚠️ Potential issue | 🟠 MajorDo not force-terminate a graceful
quitpath.The worker sends
msg.type === 'quit'on successful completion. Callingworker.terminate()forces an exit code of 1, which is then rejected by line 106 (if (code !== 0) reject(...)), converting the success path into a failure. Remove theterminate()call and let the thread exit naturally with code 0, or mark the quit path as expected before evaluating the exit code at line 106.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-node/src/worker/worker.ts` around lines 88 - 97, The quit message handler currently forces a non-graceful exit by calling worker.terminate() when msg.type === 'quit'; remove that terminate() call in the worker.on('message', ...) handler so the worker can exit naturally with code 0 (or alternatively set a flag to treat a quit as expected before the exit-code check). Update the branch that checks msg.type === 'quit' (in the worker.on('message') callback) to either do nothing (allow natural exit) or set an explicit expected-quit marker that the exit-code evaluation (the rejection logic that checks code !== 0) will consider a clean shutdown.
🟡 Minor comments (7)
packages/bijou-tui/src/runtime.test.ts-76-77 (1)
76-77:⚠️ Potential issue | 🟡 MinorThis assertion is too weak to catch frame regressions.
A plain substring match still passes if the initial interactive write is missing the home/clear sequences. Keep the render contract in the assertion by matching
frame('count: 0')instead of only'count: 0'.🔧 Proposed fix
- const hasInitialRender = ctx.io.written.some((w) => w.includes('count: 0')); + const hasInitialRender = ctx.io.written.some((w) => w.includes(frame('count: 0'))); expect(hasInitialRender).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/runtime.test.ts` around lines 76 - 77, Replace the weak substring assertion that checks for 'count: 0' with the framed render assertion so the test validates the full render contract; specifically update the logic around hasInitialRender (or the expect) to search for frame('count: 0') (e.g., ctx.io.written.some(w => w.includes(frame('count: 0'))) or expect(ctx.io.written).toContain(frame('count: 0'))), ensuring you reference the frame helper instead of the plain string.examples/v3-motion/main.ts-45-45 (1)
45-45:⚠️ Potential issue | 🟡 MinorGuard the root size for non-TTY runs.
process.stdout.columnsandprocess.stdout.rowsareundefinedwhen stdout is not attached to a TTY (redirected or piped). This causes an invalid root rect in CI and non-interactive environments. Add the same?? 80/24fallback used by the runtime adapter.Proposed fix
- rect: { x: 0, y: 0, width: process.stdout.columns, height: process.stdout.rows }, + rect: { + x: 0, + y: 0, + width: process.stdout.columns ?? 80, + height: process.stdout.rows ?? 24 + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/v3-motion/main.ts` at line 45, The root rect uses process.stdout.columns/process.stdout.rows which are undefined when stdout is not a TTY; update the rect initializer (the object assigned to rect in main.ts) to guard these values with fallbacks like (process.stdout.columns ?? 80) for width and (process.stdout.rows ?? 24) for height so the root size is valid in CI/non-interactive runs.packages/bijou-node/src/runtime.ts-12-15 (1)
12-15:⚠️ Potential issue | 🟡 MinorUse
Number()instead ofparseInt()forBIJOU_FPSparsing.
parseInt()truncates decimal values (e.g.,59.94becomes59) and accepts junk suffixes (e.g.,120hzparses as120). Since refresh rates like 59.94 Hz and 119.88 Hz are common, this causes silent failures. UseNumber()andNumber.isFinite()instead.🔧 Proposed fix
export function detectRefreshRate(): number { const envFps = process.env['BIJOU_FPS']; if (envFps) { - const parsed = parseInt(envFps, 10); - if (!isNaN(parsed) && parsed > 0) return parsed; + const parsed = Number(envFps); + if (Number.isFinite(parsed) && parsed > 0) return parsed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-node/src/runtime.ts` around lines 12 - 15, The code uses parseInt to parse BIJOU_FPS (envFps -> parsed) which truncates decimals and accepts suffixes; replace parseInt(envFps, 10) with Number(envFps) (or Number(envFps.trim())) and validate with Number.isFinite(parsed) && parsed > 0 instead of isNaN, keeping the same early-return behavior in the function that reads BIJOU_FPS.packages/bijou-tui/src/subapp/mount.test.ts-49-55 (1)
49-55:⚠️ Potential issue | 🟡 MinorAssert that the mapper is skipped for
QUIT.With
(m) => m, this still passes even ifmapCmds()incorrectly invokes the mapper onQUIT. Use a spy and assert it was not called so the passthrough guarantee is actually covered.Suggested test tightening
- const mapped = mapCmds([cmd], (m) => m); + const mapper = vi.fn((m) => m); + const mapped = mapCmds([cmd], mapper); const result = await mapped[0]!(vi.fn(), { onPulse: vi.fn() }); expect(result).toBe(QUIT); + expect(mapper).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/subapp/mount.test.ts` around lines 49 - 55, The test currently uses a no-op mapper (m => m) so it won't detect if mapCmds wrongly invokes the mapper for QUIT; update the test to use a spy mapper (e.g., vi.fn()) passed to mapCmds and assert that the spy was not called, while still verifying the returned result is QUIT from the Cmd invocation (referencing mapCmds, the Cmd async function that returns QUIT, and the QUIT/ QuitSignal expectation).packages/bijou-tui/src/driver.test.ts-44-45 (1)
44-45:⚠️ Potential issue | 🟡 MinorStrengthen these frame assertions after switching to
surfaceToString().
toContain()and index-only checks can still pass when stale content remains on the surface or the callback receives the wrong frame. For these one-line views, normalize the rendered string and assert the full expected payload instead.Suggested assertion pattern
- expect(surfaceToString(result.frames[1]!, style)).toContain('Count: 1'); + expect(surfaceToString(result.frames[1]!, style).trim()).toBe('Count: 1'); ... expect(captured[0]!.index).toBe(0); expect(captured[1]!.index).toBe(1); + expect(captured[0]!.frame.trim()).toBe('Count: 0'); + expect(captured[1]!.frame.trim()).toBe('Count: 1');Based on learnings, the test adapter's
plainStyle()is a deliberate no-op, so exact text assertions here remain deterministic.Also applies to: 57-59, 73-80, 144-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/driver.test.ts` around lines 44 - 45, The test currently uses toContain() against surfaceToString(result.frames[0]!, style) which can hide stale or wrong-frame content; instead normalize the rendered string and assert the full expected payload exactly (e.g., the one-line view text plus newline) for surfaceToString(...) of result.frames[0]!, using the test adapter's plainStyle() behavior for determinism; update the assertions at the shown lines (and the other occurrences mentioned: 57-59, 73-80, 144-145) to compare the normalized surfaceToString(...) output to the exact expected string and keep the model assertions (result.model.count) as-is.docs/specs/snapshot-harness.spec.json-12-12 (1)
12-12:⚠️ Potential issue | 🟡 MinorClarify "DIND" acronym.
The term "DIND" in "with absolute certainty (DIND)" is unclear. If this refers to a specific testing methodology or pattern, consider spelling it out for readers unfamiliar with the term.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/snapshot-harness.spec.json` at line 12, The "soThat" string contains the unexplained acronym "DIND"; update the "soThat" value to either spell out the acronym or remove it so readers understand the intent—for example replace "I can catch visual regressions and breaking logic changes with absolute certainty (DIND)." with a clarified phrase like "I can catch visual regressions and breaking logic changes with absolute certainty (Do It, No Doubt)" or simply omit the parentheses; locate and edit the "soThat" JSON property in the spec to make this change.packages/bijou-tui/src/css/parser.ts-101-104 (1)
101-104:⚠️ Potential issue | 🟡 MinorHandle whitespace variants in
!importantparsing.Per the CSS specification, whitespace is allowed between
!andimportant(e.g.,color: blue ! important;). The currentendsWith('!important')check misses these cases.Suggested fix
- const important = value.endsWith('!important'); - if (important) { - value = value.replace('!important', '').trim(); - } + const importantMatch = /!\s*important\s*$/i.exec(value); + const important = importantMatch !== null; + if (important) { + value = value.slice(0, importantMatch.index).trim(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/css/parser.ts` around lines 101 - 104, The parser's current detection of !important using value.endsWith('!important') misses valid CSS like "! important" or other whitespace variants; update the logic around the important/value handling (the const important and subsequent value reassignment) to detect case-insensitive "!important" with optional whitespace between "!" and "important" (e.g., using a regex that matches /\!\s*important$/i), strip that matched token from value and trim the result, and preserve behavior of the important boolean and the cleaned value variables.
- Implement industrial-grade ANSI-to-Surface parser in core (parseAnsiToSurface) - Upgrade App Frame to support Surface-native rendering for lenses - Implement boxV3 component for color-pure Surface wrapping - Enhance background worker IPC with sendToMain() and bidirectional messaging - Move Help Line to footer and implement smart truncation with '?' indicator - Force 24-bit Truecolor in Node adapter by default - Upgrade canvas shader engine with smart style accumulation for high-res modes - Make EventBus async-safe for robust command execution
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bijou-tui/src/eventbus.ts (1)
276-282:⚠️ Potential issue | 🟡 MinorFormatting issue: missing newline.
Line 278 has the
ifstatement andforloop concatenated on the same line, which appears to be an accidental merge artifact.🔧 Proposed formatting fix
Promise.resolve(cmd(emit, caps)).then((result) => { if (disposed) return; - if (result === QUIT) { for (const handler of quitHandlers) { + if (result === QUIT) { + for (const handler of quitHandlers) { handler(); } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/eventbus.ts` around lines 276 - 282, The code around Promise.resolve(cmd(emit, caps)).then(...) has a formatting/merge artifact where the `if (result === QUIT)` and the following `for (const handler of quitHandlers) { handler(); }` are concatenated on the same line; fix by splitting into proper lines and indentation so the `if (result === QUIT) {` opens a new block and the `for (const handler of quitHandlers) { handler(); }` runs inside it, preserving the early return and keeping checks for `disposed`, `cmd`, `emit`, `caps`, and the QUIT handling logic intact.
♻️ Duplicate comments (1)
packages/bijou-node/src/worker/worker.ts (1)
99-103:⚠️ Potential issue | 🔴 Critical
workerDataincludes non-cloneable fields fromRunOptions.The spread
{ ...options, onMessage: undefined }copies the entire options object includingctx(BijouContext with function methods) andmiddlewares(function array) if provided by the caller. These cannot be structured-cloned and will causenew Worker()to throw synchronously—before any error handlers can catch it—potentially leaving the TTY in alt-screen mode.Filter to only serializable fields:
🐛 Proposed fix: pass only serializable options
const worker = new Worker(resolvePath(options.entry), { - workerData: { isBijouWorker: true, options: { ...options, onMessage: undefined } }, + workerData: { + isBijouWorker: true, + options: { + altScreen: options.altScreen, + hideCursor: options.hideCursor, + mouse: options.mouse, + css: options.css, + entry: options.entry, + }, + }, execArgv: options.execArgv, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-node/src/worker/worker.ts` around lines 99 - 103, The Worker constructor is being passed non-cloneable fields via workerData ({ ...options, onMessage: undefined }) which will throw when options contains functions like ctx methods or middlewares; update the worker creation in Worker(resolvePath(options.entry), ...) to build a sanitized serializableOptions object that explicitly picks only serializable RunOptions fields (e.g., entry, execArgv, env, any primitive flags) and excludes ctx, middlewares, and any functions before setting workerData: { isBijouWorker: true, options: serializableOptions }; ensure references to options.execArgv and resolvePath(options.entry) remain unchanged and remove onMessage from the payload rather than spreading the whole options object.
🧹 Nitpick comments (3)
packages/bijou/src/ports/io.ts (1)
96-101: Consider usingunknowninstead ofanyfor the payload type.The
payload: anytype bypasses TypeScript's type checking at the call site. Usingunknownwould require consumers to explicitly narrow or assert the type before use, which is safer for IPC data that could have arbitrary structure.🔧 Proposed type-safe alternative
/** * Register a callback invoked when arbitrary data is received from the host. * Useful for IPC in background workers. * `@returns` A handle whose `dispose()` removes the listener. */ - onData?(callback: (payload: any) => void): RawInputHandle; + onData?(callback: (payload: unknown) => void): RawInputHandle;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/ports/io.ts` around lines 96 - 101, Update the onData callback signature to use unknown instead of any: change onData?(callback: (payload: unknown) => void): RawInputHandle; and then update any call sites of onData (and implementations of RawInputHandle consumers) to explicitly narrow or assert the payload before use (e.g., type-guards or instanceof checks) so TypeScript enforces safety when handling IPC payloads; keep the RawInputHandle return type unchanged.packages/bijou-node/src/worker/worker.ts (1)
95-97: Asymmetric mouse mode sequences.Enable writes modes 1000, 1002, 1006 but disable includes 1003 which was never enabled. This works (terminals ignore disabling unset modes) but is asymmetric.
♻️ Suggested fix for symmetry
if (useMouse) { - ctx.io.write('\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l'); // DISABLE_MOUSE + ctx.io.write('\x1b[?1000l\x1b[?1002l\x1b[?1006l'); // DISABLE_MOUSE }Also applies to: 144-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-node/src/worker/worker.ts` around lines 95 - 97, The disable sequence is asymmetric: when useMouse is true the code writes enable sequences '\x1b[?1000h\x1b[?1002h\x1b[?1006h' but the corresponding disable uses '\x1b[?1003l' (1003 was never enabled). Update the disable sequence(s) to mirror the enabled modes (use '\x1b[?1000l\x1b[?1002l\x1b[?1006l') wherever ctx.io.write is used to turn mouse modes off (the paired blocks around useMouse in worker.ts, including the other occurrence around lines 144-146), ensuring symmetry between enable and disable calls.packages/bijou-tui/src/canvas.ts (1)
48-50: Unusedctxparameter in CanvasOptions.The
ctxproperty is declared inCanvasOptionsbut is never used in thecanvas()function. If mode detection is no longer needed, consider removing it to avoid confusion.♻️ Proposed fix to remove unused property
export interface CanvasOptions { /** Animation time value passed to the shader. Default: 0. */ time?: number; /** Plotting resolution. Default: 'cell'. */ resolution?: CanvasResolution; /** Custom data passed to the shader. */ uniforms?: Record<string, any>; - /** Bijou context for mode detection. */ - ctx?: BijouContext; }And update the import:
-import { createSurface, type Surface, type Cell, type BijouContext } from '@flyingrobots/bijou'; +import { createSurface, type Surface, type Cell } from '@flyingrobots/bijou';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/canvas.ts` around lines 48 - 50, The CanvasOptions interface declares an unused ctx?: BijouContext property that is never referenced in the canvas() function; remove this dead API surface by deleting the ctx field from CanvasOptions and any related type imports/usages, and update any files importing BijouContext (or the CanvasOptions type) to stop referencing ctx so the code and types reflect that mode detection is no longer used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bijou-node/package.json`:
- Around line 27-28: Update the package manifest so internal packages are locked
to the v3 release line: change the dependency "@flyingrobots/bijou-tui" from
"2.1.0" to the v3 version (e.g., "3.0.0") and ensure the package's own "version"
field (for `@flyingrobots/bijou-node` and any listed `@flyingrobots/bijou`) is also
set to the matching v3 value; confirm all internal dependency entries
referencing `@flyingrobots/bijou`, `@flyingrobots/bijou-node`, and
`@flyingrobots/bijou-tui` use the same v3 exact version to keep lock-step versions
across the monorepo.
In `@packages/bijou-tui/README.md`:
- Around line 24-50: The Quick Start example is not self-contained because
childAppA/childAppB are undefined and init() returns {} while view() accesses
model.left/model.right; update the snippet so it compiles by either (A) inlining
minimal Sub-App definitions named childAppA and childAppB (each providing
init/update/view) and changing the parent init() to return a model with left and
right keys, or (B) simplifying the example to a single mounted child so view()
only reads the existing model shape; reference the functions init, view, mount,
childAppA, childAppB and ensure the parent init() returns the model shape that
view() expects (e.g., { left, right }) before calling mount.
- Around line 18-22: Update the Installation section so the npm install command
installs all three internal packages with exact lock-step versions (match the
monorepo convention) instead of only `@flyingrobots/bijou-tui`; replace the
single-package install line with a single npm install that includes
`@flyingrobots/bijou`, `@flyingrobots/bijou-node`, and `@flyingrobots/bijou-tui`
pinned to the same version (e.g., 3.0.0) to keep package APIs in sync and ensure
the subsequent example imports work.
In `@packages/bijou-tui/src/canvas.ts`:
- Around line 24-36: The example shader still uses the old positional signature
(x,y,cols,rows,time) so when the runtime calls it with a single ShaderParams
object it yields NaN; update the shader implementation to the new ShaderFn
signature by accepting a single params: ShaderParams argument and replace uses
of x/y with params.u and params.v (normalized 0–1 coords), access params.cols,
params.rows and params.time for dimensions/time, and map any uses of absolute
pixel math to normalized math (or compute pixel coords from params.u/params.v if
needed); alternatively, if you need backward compatibility implement a small
wrapper that accepts (params: ShaderParams) and calls the old positional
function with reconstructed x,y,cols,rows,time before returning the Cell/string.
- Around line 61-85: canvas() now returns a Surface object (not a string), so
update the example view to return that Surface directly instead of interpolating
it into a template string; locate the example's view function that calls
canvas(...) and remove the `${art}` interpolation (or any string concatenation)
so it returns the Surface value produced by canvas(...) directly, ensuring the
view's return type matches the Surface type expected by the runtime and no
manual toString conversion is used.
In `@packages/bijou-tui/src/eventbus.ts`:
- Around line 197-213: The middleware chain can double-dispatch if a middleware
calls dispatch(next) and then throws; fix by tracking whether the middleware
already invoked the next dispatch: inside dispatch (the function that iterates
middlewares and uses index and mw), create a local boolean (e.g., nextCalled)
and pass a wrapper dispatch to mw that sets nextCalled = true before delegating
to the real dispatch; in the catch block only call dispatch(currentMsg) when
nextCalled is false so subscribers/middlewares are not invoked twice; keep
references to dispatch, middlewares, mw, index and subscribers when implementing
this guard.
In `@packages/bijou/src/core/components/box-v3.ts`:
- Around line 28-30: The inner forEach callback currently uses an expression
body which implicitly returns a value; change the gs.forEach callback from
(char, x) => contentSurf.set(x, y, { char }) to a block-bodied arrow (char, x)
=> { contentSurf.set(x, y, { char }); } so the callback does not return a value;
update the code around lines.forEach, segmentGraphemes, gs.forEach and
contentSurf.set accordingly.
In `@packages/bijou/src/core/components/box.ts`:
- Around line 11-12: The BoxOptions.title field is declared but unused; update
the box rendering to honor it by having box() and drawBox() read options.title
and render it centered (or left-aligned per existing style) in the top border
(e.g., "┌─ Title ─┐"). Modify drawBox() to compute the title's visual length,
trim/pad to fit inside the computed inner width, and insert the title between
the top-left and top-right corner characters while preserving border characters
and spacing; ensure width calculation (used by box() and drawBox()) accounts for
title length so content and borders don't overflow, and remove the title
property if you prefer not to support it. Reference: BoxOptions, box(),
drawBox().
In `@packages/bijou/src/core/render/differ.ts`:
- Around line 117-122: The code in the Token construction (the object assigned
to token used by style.styled in this file) is coercing an undefined foreground
into '#ffffff'; instead preserve undefined so "inherit terminal foreground"
stays undefined (do not hard-code white). Update the token creation used around
the style.styled(token, cell.char) call to set hex to cell.fg (or cell.fg ??
undefined) rather than cell.fg ?? '#ffffff', and make the identical change in
the other occurrence noted (the block around lines 230-236) so foreground-only/
bg-only/modifier-only cells keep their original undefined fg and round-trip
correctly with parseAnsiToSurface().
- Around line 64-85: parseAnsiToSurface currently never updates currentMods so
SGR text modifiers are lost; update the SGR parsing to iterate the numeric codes
(split the incoming code string by ';' or iterate parts when handling 38/48) and
add/remove modifier tokens in currentMods for standard SGR values (e.g. add
'bold' for 1, 'dim' for 2, 'italic' for 3, 'underline' for 4, 'inverse' for 7,
'strike' for 9; remove them on their explicit resets like 22/23/24/27/29 and
clear all on 0). Implement this in parseAnsiToSurface where currentFg/currentBg
are handled (the blocks using code.startsWith and the initial code === '0'
branch) so each cell written continues to receive the correct currentMods state.
Ensure modifier tokens are consistently named and that adding is idempotent
(avoid duplicates) and removals handle missing entries gracefully.
---
Outside diff comments:
In `@packages/bijou-tui/src/eventbus.ts`:
- Around line 276-282: The code around Promise.resolve(cmd(emit,
caps)).then(...) has a formatting/merge artifact where the `if (result ===
QUIT)` and the following `for (const handler of quitHandlers) { handler(); }`
are concatenated on the same line; fix by splitting into proper lines and
indentation so the `if (result === QUIT) {` opens a new block and the `for
(const handler of quitHandlers) { handler(); }` runs inside it, preserving the
early return and keeping checks for `disposed`, `cmd`, `emit`, `caps`, and the
QUIT handling logic intact.
---
Duplicate comments:
In `@packages/bijou-node/src/worker/worker.ts`:
- Around line 99-103: The Worker constructor is being passed non-cloneable
fields via workerData ({ ...options, onMessage: undefined }) which will throw
when options contains functions like ctx methods or middlewares; update the
worker creation in Worker(resolvePath(options.entry), ...) to build a sanitized
serializableOptions object that explicitly picks only serializable RunOptions
fields (e.g., entry, execArgv, env, any primitive flags) and excludes ctx,
middlewares, and any functions before setting workerData: { isBijouWorker: true,
options: serializableOptions }; ensure references to options.execArgv and
resolvePath(options.entry) remain unchanged and remove onMessage from the
payload rather than spreading the whole options object.
---
Nitpick comments:
In `@packages/bijou-node/src/worker/worker.ts`:
- Around line 95-97: The disable sequence is asymmetric: when useMouse is true
the code writes enable sequences '\x1b[?1000h\x1b[?1002h\x1b[?1006h' but the
corresponding disable uses '\x1b[?1003l' (1003 was never enabled). Update the
disable sequence(s) to mirror the enabled modes (use
'\x1b[?1000l\x1b[?1002l\x1b[?1006l') wherever ctx.io.write is used to turn mouse
modes off (the paired blocks around useMouse in worker.ts, including the other
occurrence around lines 144-146), ensuring symmetry between enable and disable
calls.
In `@packages/bijou-tui/src/canvas.ts`:
- Around line 48-50: The CanvasOptions interface declares an unused ctx?:
BijouContext property that is never referenced in the canvas() function; remove
this dead API surface by deleting the ctx field from CanvasOptions and any
related type imports/usages, and update any files importing BijouContext (or the
CanvasOptions type) to stop referencing ctx so the code and types reflect that
mode detection is no longer used.
In `@packages/bijou/src/ports/io.ts`:
- Around line 96-101: Update the onData callback signature to use unknown
instead of any: change onData?(callback: (payload: unknown) => void):
RawInputHandle; and then update any call sites of onData (and implementations of
RawInputHandle consumers) to explicitly narrow or assert the payload before use
(e.g., type-guards or instanceof checks) so TypeScript enforces safety when
handling IPC payloads; keep the RawInputHandle return type unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 149da3dd-72e2-4f2a-afc3-ac1d1dbd1d63
📒 Files selected for processing (16)
packages/bijou-node/README.mdpackages/bijou-node/package.jsonpackages/bijou-node/src/index.tspackages/bijou-node/src/worker/worker.tspackages/bijou-node/tsconfig.jsonpackages/bijou-tui-app/src/index.test.tspackages/bijou-tui-app/src/index.tspackages/bijou-tui/README.mdpackages/bijou-tui/src/canvas.tspackages/bijou-tui/src/eventbus.tspackages/bijou/README.mdpackages/bijou/src/core/components/box-v3.tspackages/bijou/src/core/components/box.tspackages/bijou/src/core/render/differ.tspackages/bijou/src/index.tspackages/bijou/src/ports/io.ts
✅ Files skipped from review due to trivial changes (1)
- packages/bijou/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bijou-tui-app/src/index.test.ts
flyingrobots
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bijou-tui/src/commands.test.ts (1)
15-39:⚠️ Potential issue | 🟡 MinorAlways restore fake timers in a
finally.If an assertion or awaited promise fails before the last line,
vi.useRealTimers()never runs and later tests can inherit fake timers.🛠️ Suggested cleanup guard
describe('tick', () => { it('resolves with the message after the delay', async () => { vi.useFakeTimers(); - const msg = { type: 'tick' as const }; - const cmd = tick(100, msg); - const promise = cmd(() => {}, { onPulse: () => ({ dispose() {} }) }); - vi.advanceTimersByTime(100); - const result = await promise; - expect(result).toBe(msg); - vi.useRealTimers(); + try { + const msg = { type: 'tick' as const }; + const cmd = tick(100, msg); + const promise = cmd(() => {}, { onPulse: () => ({ dispose() {} }) }); + vi.advanceTimersByTime(100); + const result = await promise; + expect(result).toBe(msg); + } finally { + vi.useRealTimers(); + } }); it('does not resolve before the delay', async () => { vi.useFakeTimers(); - const cmd = tick(500, 'done'); - let resolved = false; - void cmd(() => {}, { onPulse: () => ({ dispose() {} }) }).then(() => { - resolved = true; - }); - vi.advanceTimersByTime(499); - await vi.advanceTimersByTimeAsync(0); - expect(resolved).toBe(false); - vi.advanceTimersByTime(1); - await vi.advanceTimersByTimeAsync(0); - expect(resolved).toBe(true); - vi.useRealTimers(); + try { + const cmd = tick(500, 'done'); + let resolved = false; + void cmd(() => {}, { onPulse: () => ({ dispose() {} }) }).then(() => { + resolved = true; + }); + vi.advanceTimersByTime(499); + await vi.advanceTimersByTimeAsync(0); + expect(resolved).toBe(false); + vi.advanceTimersByTime(1); + await vi.advanceTimersByTimeAsync(0); + expect(resolved).toBe(true); + } finally { + vi.useRealTimers(); + } }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/commands.test.ts` around lines 15 - 39, Both tests call vi.useFakeTimers() but call vi.useRealTimers() only at the end, risking leaked fake timers if an assertion throws; wrap the fake timer usage in a try/finally (or add an afterEach that calls vi.useRealTimers()) so real timers are always restored; update the tests that call vi.useFakeTimers() (the cases invoking tick(100, msg) and tick(500, 'done') in commands.test.ts) to ensure vi.useRealTimers() runs in finally (or add a global afterEach cleanup) so timers are reliably restored.
🟠 Major comments (20)
scripts/pty-driver.py-28-40 (1)
28-40:⚠️ Potential issue | 🟠 MajorResource leak:
master_fdnot closed ifPopenfails.If
subprocess.Popen()raises an exception, thefinallyblock closesslave_fd, butmaster_fdremains open. Consider restructuring to ensure both fds are cleaned up on failure.Proposed fix
master_fd, slave_fd = os.openpty() try: proc = subprocess.Popen( argv, cwd=cwd, env=env, stdin=slave_fd, stdout=slave_fd, stderr=slave_fd, close_fds=True, ) - finally: - os.close(slave_fd) + except: + os.close(master_fd) + raise + finally: + os.close(slave_fd)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pty-driver.py` around lines 28 - 40, Popen failure currently leaves master_fd open; change the flow to initialize proc = None, call subprocess.Popen(...) inside a try/except, and on exception close both slave_fd and master_fd before re-raising; on success close only slave_fd (preserving master_fd for use). Refer to master_fd, slave_fd, proc, and subprocess.Popen in your changes.examples/chat/main.ts-37-42 (1)
37-42:⚠️ Potential issue | 🟠 MajorUse the available width instead of discarding it.
Line 38 turns
widthinto dead data, so long chat lines are rendered as a single row and get clipped in narrow terminals. Since this view only enables vertical scrolling, user-entered messages can become unreadable instead of wrapping to the viewport width.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/chat/main.ts` around lines 37 - 42, renderMessages currently ignores the width parameter (void width) causing long lines to be clipped; update renderMessages to wrap each Message.text to the supplied width (accounting for the tag produced by badgeText) and join wrapped lines with '\n\n', ensuring subsequent wrapped lines are indented to align under the message body (or start at column after the tag) so messages display correctly in narrow terminals; locate renderMessages and badgeText to compute tag length and apply a simple word-wrap on Message.text using width - tagLength for the first line and width - indent for subsequent lines.packages/bijou/src/core/components/box.ts-68-71 (1)
68-71:⚠️ Potential issue | 🟠 MajorAuto-width titled boxes are still sized too small.
When the title is wider than the content, this branch only reserves
graphemeWidth(title) + 2, but the top-border renderer later needs room for${title}plus one horizontal border cell on each side.contentWidthalso stays atmaxWidth, so content rows do not expand to the wider title-driven box. The result is a clipped title and misaligned side borders for cases likebox('', { title: 'Hello' }).Suggested fix
- const titleWidth = title ? graphemeWidth(title) + 2 : 0; + const titleInnerWidth = title ? graphemeWidth(` ${title} `) + 2 : 0; const maxWidth = contentLines.reduce((max, line) => Math.max(max, graphemeWidth(line)), 0); - contentWidth = maxWidth; - innerWidth = Math.max(titleWidth, maxWidth + padding.left + padding.right); + innerWidth = Math.max(titleInnerWidth, maxWidth + padding.left + padding.right); + contentWidth = Math.max(0, innerWidth - padding.left - padding.right);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/components/box.ts` around lines 68 - 71, The auto-width calculation underestimates space for titled boxes: update the titleWidth computation (where titleWidth is set using graphemeWidth(title)) to account for the surrounding spaces and the extra horizontal border cell on both sides (i.e., reserve space for " ${title} " plus one border cell each side), and then ensure contentWidth and innerWidth are derived so contentWidth expands when title drives the width (set contentWidth to the larger of maxWidth and the adjusted title-driven inner width minus padding.left/right), adjusting the assignments that touch titleWidth, contentWidth, innerWidth and using graphemeWidth and contentLines/padding to compute the final dimensions so the title is not clipped and side borders align.examples/v3-pipeline/main.ts-7-35 (1)
7-35:⚠️ Potential issue | 🟠 MajorDon’t bake a single context into the exported app.
viewcloses over the module-scopedctx, so any caller that runsappwith a different context will still center against the wrong dimensions. That makes the exported example render incorrectly in fixed-size recording/test contexts.♻️ Suggested refactor
-export const ctx = initDefaultContext(); - -export const app: App<Model> = { - init: () => [{ scanlineOn: true }, []], - update: (msg, model) => { - if (isKeyMsg(msg)) { - if (msg.key === 'q' || (msg.ctrl && msg.key === 'c')) return [model, [quit()]]; - if (msg.key === ' ') return [{ ...model, scanlineOn: !model.scanlineOn }, []]; - } - return [model, []]; - }, - view: (model) => centerSurface( - ctx, - boxV3( - vstackV3( - badge('Pipeline Hook', { variant: 'primary' }), - spacer(1, 1), - line('This app installs a custom PostProcess middleware.'), - line(`Scanlines: ${model.scanlineOn ? 'enabled' : 'disabled'}`), - spacer(1, 1), - line('Press SPACE to toggle the effect. Press Q to quit.'), - ), - { title: 'Render Pipeline', padding: { top: 1, bottom: 1, left: 2, right: 2 } }, - ), - ), -}; +export function createApp(ctx: ReturnType<typeof initDefaultContext>): App<Model> { + return { + init: () => [{ scanlineOn: true }, []], + update: (msg, model) => { + if (isKeyMsg(msg)) { + if (msg.key === 'q' || (msg.ctrl && msg.key === 'c')) return [model, [quit()]]; + if (msg.key === ' ') return [{ ...model, scanlineOn: !model.scanlineOn }, []]; + } + return [model, []]; + }, + view: (model) => centerSurface( + ctx, + boxV3( + vstackV3( + badge('Pipeline Hook', { variant: 'primary' }), + spacer(1, 1), + line('This app installs a custom PostProcess middleware.'), + line(`Scanlines: ${model.scanlineOn ? 'enabled' : 'disabled'}`), + spacer(1, 1), + line('Press SPACE to toggle the effect. Press Q to quit.'), + ), + { title: 'Render Pipeline', padding: { top: 1, bottom: 1, left: 2, right: 2 } }, + ), + ), + }; +} + +export const ctx = initDefaultContext(); +export const app = createApp(ctx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/v3-pipeline/main.ts` around lines 7 - 35, The view currently closes over the module-scoped ctx (created by initDefaultContext) which makes rendering use a fixed context; instead remove the exported ctx and put the context into app state: add a ctx field to Model, call initDefaultContext() in app.init to populate model.ctx, update app.view to call centerSurface(model.ctx, ...) (and remove references to the module ctx); alternatively, if the framework passes a rendering context into view, use that parameter (e.g., change view signature to use the provided ctx) rather than the module-scoped ctx (references: ctx, initDefaultContext, Model, app.init, app.view, centerSurface).packages/bijou/src/core/components/box-v3.ts-43-44 (1)
43-44:⚠️ Potential issue | 🟠 MajorInclude the title when auto-sizing the box.
Line 43 only derives
outerWfrom the content width. With something likeboxV3('x', { title: 'Details' }), the box ends up too narrow and the later title pass truncates the title away. Whenwidthis omitted, the minimum width needs to account fortitletoo.♻️ Suggested fix
- const outerW = fixedWidth ?? (innerW + 2); + const minTitleOuterW = title == null ? 0 : segmentGraphemes(` ${title} `).length + 4; + const outerW = fixedWidth ?? Math.max(innerW + 2, minTitleOuterW);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/components/box-v3.ts` around lines 43 - 44, When auto-sizing in boxV3, outerW is calculated only from innerW so a provided title can get truncated; update the logic that sets outerW (currently using fixedWidth, innerW, innerH) to also consider the title length by computing a minimum width from the title (e.g., titleLength + any needed padding/border) and use outerW = fixedWidth ?? Math.max(innerW + 2, titleMinWidth) so the title always fits; update references to fixedWidth/innerW/outerW in the boxV3 sizing path accordingly.packages/bijou-tui/src/css/install.ts-31-35 (1)
31-35:⚠️ Potential issue | 🟠 MajorDon't force Apple Terminal into
lightmode.Line 34 treats every
TERM_PROGRAM=Apple_Terminalsession as light, but Terminal.app supports dark profiles too. That will resolve BCSS theme rules against the wrong mode wheneverCOLORFGBGis absent.♻️ Suggested fix
function detectThemeMode(ctx: BijouContext): ThemeMode { const colorFgBg = ctx.runtime.env('COLORFGBG')?.split(';').pop(); if (colorFgBg === '15') return 'light'; - if (ctx.runtime.env('TERM_PROGRAM') === 'Apple_Terminal') return 'light'; + if (colorFgBg === '0') return 'dark'; return 'dark'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/css/install.ts` around lines 31 - 35, The detectThemeMode function currently forces TERM_PROGRAM === 'Apple_Terminal' to 'light' which is incorrect; remove that special-case check so theme detection only uses COLORFGBG (ctx.runtime.env('COLORFGBG')) and then falls back to the default ('dark') when COLORFGBG is absent; update detectThemeMode to stop treating ctx.runtime.env('TERM_PROGRAM') === 'Apple_Terminal' as authoritative for light mode.examples/canvas/main.ts-47-49 (1)
47-49:⚠️ Potential issue | 🟠 MajorReserve rows for the title before drawing the shader.
Lines 47-49 render the canvas at
model.rowsand then stack a 3-row title above it, so the composed surface is taller than the terminal on every frame. That will clip the animation and create unnecessary bottom-row churn in the differ.♻️ Suggested fix
- const art = canvas(model.cols, model.rows, shader, { time: model.time }); - const title = stringToSurface(' Plasma Shader (q to quit)', model.cols, 3); + const titleHeight = 3; + const title = stringToSurface(' Plasma Shader (q to quit)', model.cols, titleHeight); + const art = canvas(model.cols, Math.max(0, model.rows - titleHeight), shader, { time: model.time }); return vstackV3(title, art);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/canvas/main.ts` around lines 47 - 49, The shader canvas is being created at model.rows and then a 3-row title is stacked on top, making the combined surface too tall; change the call that builds the shader surface (canvas(...)) to reserve space for the title by using rows = Math.max(0, model.rows - 3) (or a TITLE_HEIGHT constant) when calling canvas(model.cols, ..., shader, { time: model.time }), then keep stringToSurface(' Plasma Shader (q to quit)', model.cols, 3) and vstackV3(title, art) as before; also ensure the reserved-rows computation handles small terminal heights so it never passes a negative row count to canvas.packages/bijou/src/ports/surface.ts-100-113 (1)
100-113:⚠️ Potential issue | 🟠 Major
interpolateColorsis exposed but never honored.The public
TransformOptionscontract advertises blended colors, buttransform()ignores the flag and always does nearest-neighbor sampling. Either implement the blended path or remove/defer the option before3.0.0ships.Also applies to: 316-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/ports/surface.ts` around lines 100 - 113, TransformOptions advertises interpolateColors but transform() currently ignores it; either implement color blending in the transform sampling path or remove the option. Locate the transform() implementation and the pixel sampling/resampling helpers it calls (e.g., any samplePixel/nearestNeighbor routines used during scaling/rotation) and add a blended-color codepath when opts.interpolateColors is true that performs proper hex/RGB interpolation (or, if you prefer to defer, remove interpolateColors from TransformOptions and any references in callers and docs so the flag is not exposed). Ensure the change touches TransformOptions, transform(), and the sampling helper functions so the option is honored or fully removed.packages/bijou/src/ports/surface.ts-211-223 (1)
211-223:⚠️ Potential issue | 🟠 Major
set()cannot restore transparency.Because
applyMask()returns immediately whensource.emptyis true,surface.set(x, y, { char: ' ', empty: true })is a no-op. Right now there is no way to clear a single populated cell back to transparent once it has content.Also applies to: 257-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/ports/surface.ts` around lines 211 - 223, The early return in applyMask prevents writing a transparent/empty cell because it exits when source.empty is true; remove that immediate return and instead let the mask logic run even if source.empty is true so fields like empty/opacity (driven by mask.alpha) and other masked properties can be applied from the source; update the same logic in the mirrored implementation at the other occurrence (lines referenced in the review) so surface.set(x,y, {char:' ', empty:true}) is honored when the mask (especially alpha) indicates the source should replace the target.packages/bijou/src/ports/surface.ts-263-267 (1)
263-267:⚠️ Potential issue | 🟠 Major
fill()overdraws when the origin is negative.The clipped end is computed from the clamped start instead of the requested start. For example,
fill(cell, -2, 0, 5, 1)currently paints 5 columns instead of the clipped 3.💡 Proposed fix
fill(cell, fx = 0, fy = 0, fw = w, fh = h, mask = FULL_MASK) { const xStart = Math.max(0, fx); const yStart = Math.max(0, fy); - const xEnd = Math.min(w, xStart + fw); - const yEnd = Math.min(h, yStart + fh); + const xEnd = Math.min(w, fx + fw); + const yEnd = Math.min(h, fy + fh); for (let y = yStart; y < yEnd; y++) { for (let x = xStart; x < xEnd; x++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/ports/surface.ts` around lines 263 - 267, The clip math in fill() is using the clamped xStart/yStart to compute xEnd/yEnd which causes overdraw when fx or fy are negative; change xEnd to Math.min(w, fx + fw) and yEnd to Math.min(h, fy + fh) while keeping xStart = Math.max(0, fx) and yStart = Math.max(0, fy) so the width/height are clipped against the original requested origin; update the bounds in the fill(cell, fx = 0, fy = 0, fw = w, fh = h, mask = FULL_MASK) implementation accordingly (function fill).packages/bijou/src/ports/surface.ts-343-360 (1)
343-360:⚠️ Potential issue | 🟠 MajorNegative row offsets are clipped against the wrong source index.
getRow()andsetRow()clamp the destination x, but neither skips the clipped cells from the requested span.getRow(0, -2, 3)currently returns three cells, andsetRow(..., -1)writesrowCells[0]into x=0 instead ofrowCells[1].💡 Proposed fix
getRow(y, rx = 0, rw = w - rx, mask = FULL_MASK) { if (y < 0 || y >= h) return []; const xStart = Math.max(0, rx); - const count = Math.min(w - xStart, rw); + const sourceStart = Math.max(0, -rx); + const count = Math.min(w - xStart, rw - sourceStart); if (count <= 0) return []; return cells .slice(y * w + xStart, y * w + xStart + count) .map((c) => maskCell(c!, mask)); }, setRow(y, rowCells, sx = 0, mask = FULL_MASK) { if (y < 0 || y >= h) return; const xStart = Math.max(0, sx); - const count = Math.min(w - xStart, rowCells.length); + const sourceStart = Math.max(0, -sx); + const count = Math.min(w - xStart, rowCells.length - sourceStart); for (let i = 0; i < count; i++) { const idx = y * w + xStart + i; - cells[idx] = applyMask(cells[idx]!, rowCells[i]!, mask); + cells[idx] = applyMask(cells[idx]!, rowCells[sourceStart + i]!, mask); } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/ports/surface.ts` around lines 343 - 360, getRow and setRow incorrectly handle negative source offsets (rx/sx): they clamp the destination xStart but do not advance the source index or reduce count, so a call like getRow(0, -2, 3) still returns 3 cells starting from rowCells[0] and setRow(..., -1) writes rowCells[0] into x=0 instead of rowCells[1]. Fix both functions by computing a sourceStart (e.g., srcOffset = Math.max(0, -rx) for getRow and Math.max(0, -sx) for setRow), advance the slice/map or rowCells index by that srcOffset, and reduce count accordingly so you skip clipped source cells; keep using xStart = Math.max(0, rx/sx) and ensure maskCell/applyMask still receive the aligned source cell values.packages/bijou/src/ports/surface.ts-192-223 (1)
192-223:⚠️ Potential issue | 🟠 MajorCell copies are still shallow around
modifiers.
modifiersis the only nested mutable field, and every copy path here reuses the same array reference. That meansget(),clone(), or a shared fill cell can leak laterpush()/mutation back into other cells, which contradicts theclone()deep-copy contract.💡 Proposed fix
+function cloneCell(cell: Cell): Cell { + return { + ...cell, + modifiers: cell.modifiers ? [...cell.modifiers] : undefined, + }; +} + function maskCell(cell: Cell, mask: CellMask): Cell { return { char: mask.char ? cell.char : ' ', fg: mask.fg ? cell.fg : undefined, bg: mask.bg ? cell.bg : undefined, - modifiers: mask.modifiers ? cell.modifiers : undefined, + modifiers: mask.modifiers ? (cell.modifiers ? [...cell.modifiers] : undefined) : undefined, empty: mask.alpha ? (cell.empty ?? false) : false, opacity: mask.alpha ? (cell.opacity ?? 1) : 1, }; } @@ return { char: mask.char ? source.char : target.char, fg: mask.fg ? source.fg : target.fg, bg: mask.bg ? source.bg : target.bg, - modifiers: mask.modifiers ? source.modifiers : target.modifiers, + modifiers: mask.modifiers ? (source.modifiers ? [...source.modifiers] : undefined) : target.modifiers, empty: mask.alpha ? (source.empty ?? false) : (target.empty ?? false), opacity: mask.alpha ? (source.opacity ?? 1) : (target.opacity ?? 1), }; } @@ - const cells: Cell[] = Array.from({ length: size }, () => ({ ...defaultCell })); + const cells: Cell[] = Array.from({ length: size }, () => cloneCell(defaultCell)); @@ - cells[i] = { ...defaultCell }; + cells[i] = cloneCell(defaultCell); @@ - s.cells[i] = { ...cells[i]! }; + s.cells[i] = cloneCell(cells[i]!);Also applies to: 233-248, 363-367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/ports/surface.ts` around lines 192 - 223, The code currently reuses the same modifiers array reference when building Cells (in maskCell and applyMask and the other copy/merge sites mentioned), causing shallow-copy leaks; change every place that assigns modifiers to always create a new array copy (e.g., when using source.modifiers or target.modifiers or cell.modifiers, set modifiers to a new array copy of that value) and do the same in all analogous functions (the other copy paths referenced) so no Cell ever reuses a modifiers reference from another Cell.examples/_shared/v3.ts-21-22 (1)
21-22:⚠️ Potential issue | 🟠 MajorThis regex literal will fail the Biome gate.
Biome's
noControlCharactersInRegexrule flags the\x1bliteral in the regex, sonpm run lintwill fail. The solution is to use aRegExpconstructor to avoid the rule violation, which is the established pattern throughout the codebase (seeclip.ts).💡 Proposed fix
+const ANSI_SGR_RE = new RegExp(String.raw`\x1b\[[0-9;]*m`, 'g'); + function visibleWidth(text: string): number { - return text.replace(/\x1b\[[0-9;]*m/g, '').length; + return text.replace(ANSI_SGR_RE, '').length; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/_shared/v3.ts` around lines 21 - 22, The regex literal in visibleWidth(text: string) uses the \x1b escape which trips Biome's noControlCharactersInRegex rule; update visibleWidth to use the RegExp constructor instead (e.g., replace the literal /\x1b\[[0-9;]*m/g with new RegExp('\\x1b\\[[0-9;]*m','g')) so linting passes while preserving the same replace behavior on the visibleWidth function.examples/v3-demo/record.ts-1-10 (1)
1-10:⚠️ Potential issue | 🟠 MajorUse
fileURLToPath()instead of.pathname.
URL.pathnamedoes not produce a valid filesystem path on Windows—it keeps percent-encoded characters intact (spaces remain as%20) and produces paths with an invalid leading/before the drive letter. This causes GIF recording to fail on non-POSIX systems. The pattern occurs across all sixrecord.tsexamples: v3-css, v3-demo, v3-motion, v3-subapp, v3-worker, and v3-pipeline.💡 Proposed fix
+import { fileURLToPath } from 'node:url'; import { recordDemoGif } from '@flyingrobots/bijou-node'; import { app, ctx } from './main.ts'; @@ - outputPath: new URL('./demo.gif', import.meta.url).pathname, + outputPath: fileURLToPath(new URL('./demo.gif', import.meta.url)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/v3-demo/record.ts` around lines 1 - 10, The outputPath is built using new URL(...).pathname which yields invalid Windows paths; update the record() implementation to convert the file URL to a platform path using fileURLToPath from 'url' (replace usage of new URL('./demo.gif', import.meta.url).pathname), so pass a correct filesystem path into recordDemoGif (refer to the record function and the outputPath argument passed to recordDemoGif); apply the same change across the other record.ts examples (v3-css, v3-motion, v3-subapp, v3-worker, v3-pipeline).packages/bijou-tui/src/motion/motion.ts-34-39 (1)
34-39:⚠️ Potential issue | 🟠 MajorDon't overwrite an existing
LayoutNode.idhere.
options.keyis already present insidemotion, but Lines 36-38 replace any caller-providedcontent.id. That drops the original node identity for anything else keyed offLayoutNode.idonce the node is wrapped. Preservecontent.id, or normalize both branches through a dedicated wrapper node and let reconciliation usemotion.key.🧩 Minimal safe change if the reconciler reads `motion.key`
return { ...content, - id: options.key, motion: options, } as MotionNode;If the reconciler currently keys on
id, prefer wrapping both branches in the same synthetic motion node instead of mutating the wrapped node's own id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/motion/motion.ts` around lines 34 - 39, The current return in create/wrap path overwrites the wrapped node's LayoutNode.id by setting id: options.key; instead preserve content.id and instead attach motion.key inside the MotionNode wrapper so you don't mutate the original node identity. Update the wrapping logic around MotionNode (the object returned from this file) to leave content.id untouched and ensure options.key is stored on the motion property (e.g., motion.key) or by returning a synthetic wrapper node that contains both the original content (with its id) and the motion object; make the change in the function that builds/returns the MotionNode so reconciliation can continue to key off LayoutNode.id while the reconciler can read motion.key when needed.examples/v3-subapp/main.ts-17-17 (1)
17-17:⚠️ Potential issue | 🟠 MajorResolve the context at render time, not import time.
Line 17 captures a
ctxonce during module evaluation and Line 96 reuses it for every render, while Line 125 runs the app without explicitly binding that same context. Any recorder/test/script that swaps the default context later can end up centering against stale dimensions. Use the current context insideview(), or thread one sharedctxthrough both rendering andrun(...).Also applies to: 80-97, 124-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/v3-subapp/main.ts` at line 17, The module currently calls initDefaultContext() once at import and exports ctx, causing a stale context to be reused; instead resolve the context at render/run time by removing the exported frozen ctx and either (a) call initDefaultContext() inside view() (or a new getCtx() accessor) so each render obtains the current context, or (b) thread a single context instance returned by initDefaultContext() through both render and run(...) so both use the same, up-to-date context; update usages of ctx, view(), and run(...) to obtain or accept the context at invocation rather than importing the module-level ctx.examples/v3-worker/main.ts-109-115 (1)
109-115:⚠️ Potential issue | 🟠 MajorDispose the delayed host-note timer on exit.
Lines 110-112 leave a live
setTimeoutbehind. If the user quits before 800ms, the process stays alive waiting for that timer and the callback can callhandle.send()after the worker has already exited.🧹 Cleanup sketch
- if (useMockContext) { - handle.send({ type: 'host-note', text: 'Main thread says hello via the data channel.' }); - } else { - setTimeout(() => { - handle.send({ type: 'host-note', text: 'Main thread says hello via the data channel.' }); - }, 800); - } - - await handle.onExit; + let hostNoteTimer: ReturnType<typeof setTimeout> | undefined; + if (useMockContext) { + handle.send({ type: 'host-note', text: 'Main thread says hello via the data channel.' }); + } else { + hostNoteTimer = setTimeout(() => { + handle.send({ type: 'host-note', text: 'Main thread says hello via the data channel.' }); + }, 800); + hostNoteTimer.unref?.(); + } + + try { + await handle.onExit; + } finally { + if (hostNoteTimer) clearTimeout(hostNoteTimer); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/v3-worker/main.ts` around lines 109 - 115, The deferred setTimeout used to call handle.send({ type: 'host-note', ... }) can keep the process alive and may run after the worker exits; capture the timer id when calling setTimeout in the else branch and ensure you clear it (clearTimeout) when the worker exits—e.g., register cleanup before awaiting handle.onExit or in an on-exit handler so the timer is cancelled if handle.onExit resolves earlier; refer to the setTimeout call, the timer id variable, handle.send, and handle.onExit when locating and changing the code.packages/bijou-tui/src/types.ts-135-144 (1)
135-144:⚠️ Potential issue | 🟠 MajorBreaking change:
Cmdsignature now requirescapabilitiesparameter.Existing commands with signature
(emit) => Promise<...>will fail TypeScript compilation. The migration guide atMIGRATING_TO_V3.mddoes not document this breaking change. Add a section with upgrade examples showing how to migrate:// Before const cmd: Cmd<Msg> = async (emit) => { ... }; // After const cmd: Cmd<Msg> = async (emit, capabilities) => { ... };Include a note that
capabilitiesprovidesonPulse(handler)for animation frame subscriptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/types.ts` around lines 135 - 144, Add a new "Commands signature change" section to MIGRATING_TO_V3.md documenting that the Cmd generic now requires a second parameter "capabilities" (type CmdCapabilities) so existing commands like const cmd: Cmd<Msg> = async (emit) => { ... } must be updated to const cmd: Cmd<Msg> = async (emit, capabilities) => { ... }; include a short before/after example and a note that capabilities exposes onPulse(handler: (dt:number)=>void): {dispose():void} for animation-frame subscriptions and must be used instead of any previous global pulse APIs.packages/bijou-tui/src/subapp/mount.ts-50-59 (1)
50-59:⚠️ Potential issue | 🟠 Major
mount()currently ignores its command-mapping contract.This implementation only renders
app.view(model)and always returns an empty command list, soMountOptions.onMsg/mapCmdare dead API here. That makes the exportedmount()surface misleading and prevents callers from receiving mapped sub-app commands through this entry point. Either makemount()strictly view-only and drop the command-related options/return value, or pass the pending sub-app cmds into this function and map them here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/subapp/mount.ts` around lines 50 - 59, mount currently renders app.view(model) but ignores command mapping; either remove the command-related API or wire it up: if you want mount to propagate sub-app commands, accept the pending sub-app Cmd[] (e.g., add options.pendingCmds or use the app init/update result), call options.mapCmd for each sub Cmd to convert SubMsg->ParentMsg (and/or wrap with options.onMsg), and return the mapped Cmd<ParentMsg>[] alongside the ViewOutput; alternatively, make mount strictly view-only by removing MountOptions.onMsg and mapCmd and change the return to just ViewOutput. Ensure you update the mount signature and use the unique symbols mount, app.view, MountOptions.onMsg, and MountOptions.mapCmd accordingly.packages/bijou-tui/src/subapp/mount.ts-130-132 (1)
130-132:⚠️ Potential issue | 🟠 MajorImport
QUITand compare directly instead of checkingtypeof result === 'symbol'.
mount.tsdoesn't import theQUITsentinel, so it can't distinguish between it and any other symbol. A sub-app legitimately returning a symbol-valued message will be incorrectly treated as a quit signal. Change line 131 fromif (typeof result === 'symbol')toif (result === QUIT)and addQUITto the imports from../types.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/subapp/mount.ts` around lines 130 - 132, Import the QUIT sentinel from ../types.js and replace the generic symbol check with a direct equality against QUIT: instead of using "typeof result === 'symbol'" in the mount handling (the block that currently returns QuitSignal), change it to "if (result === QUIT)" so only the sentinel triggers the quit path; keep the other branches (result === undefined returning undefined and the final "return mapper(result as SubMsg)") unchanged and ensure the returned type remains QuitSignal when result === QUIT.
🟡 Minor comments (11)
packages/bijou/src/core/components/bcss-style.ts-33-40 (1)
33-40:⚠️ Potential issue | 🟡 MinorUnderline variant handling may leave stale modifiers.
When
text-decorationincludes'underline', the function adds'underline'but doesn't clear existing variant modifiers (curly-underline,dotted-underline,dashed-underline). Ifbasealready has a variant underline, the result would contain both the variant and plain'underline'.If the intent is for
text-decoration: underlineto override any existing underline style, consider clearing variants before adding:Proposed fix
} else if (decoration) { if (decoration.includes('underline')) { + modifiers.delete('curly-underline'); + modifiers.delete('dotted-underline'); + modifiers.delete('dashed-underline'); modifiers.add('underline'); }If the additive behavior is intentional (allowing multiple underline styles), this can be disregarded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/components/bcss-style.ts` around lines 33 - 40, The underline handling in the decoration branch can leave stale variant modifiers: when decoration.includes('underline') add logic in the same block that currently manipulates the modifiers Set (the code that checks decoration and uses modifiers.add('underline') and modifiers.add('strikethrough')/modifiers.delete('strike')) to first remove any variant underline modifiers (e.g. 'curly-underline', 'dotted-underline', 'dashed-underline') from modifiers before adding the plain 'underline' so the plain underline overrides prior variants; update the code that references modifiers, decoration, and the underline branch accordingly.scripts/pty-driver.py-22-26 (1)
22-26:⚠️ Potential issue | 🟡 MinorInconsistent indentation: 2 spaces instead of 4.
This block uses 2-space indentation while the rest of the file uses 4-space indentation (PEP 8 standard).
Proposed fix
env = os.environ.copy() for key, value in spec.get("env", {}).items(): - if value is None: - env.pop(key, None) - else: - env[key] = str(value) + if value is None: + env.pop(key, None) + else: + env[key] = str(value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pty-driver.py` around lines 22 - 26, The for-loop over spec.get("env", {}) uses 2-space indentation which breaks PEP8 style—update the indentation to 4 spaces to match the rest of the file; locate the block that iterates "for key, value in spec.get(\"env\", {}).items():" and re-indent the body lines that reference env.pop(key, None) and env[key] = str(value) to 4 spaces so the file's indentation is consistent.scripts/pty-driver.py-53-55 (1)
53-55:⚠️ Potential issue | 🟡 MinorPotential
KeyErrorif step is missing"input"key.Unlike
delayMswhich uses.get("delayMs", 0)with a default, the"input"field is accessed directly and will raiseKeyErrorif missing. Consider using.get()with a default or adding validation.Proposed fix (option: default to empty string)
while next_step_index < len(steps) and now >= next_deadline: - payload = steps[next_step_index]["input"].encode("utf-8", "surrogatepass") + payload = steps[next_step_index].get("input", "").encode("utf-8", "surrogatepass") os.write(master_fd, payload)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pty-driver.py` around lines 53 - 55, The loop that builds payload from steps[next_step_index]["input"] can raise KeyError if the "input" key is missing; update the code in the loop that computes payload (the block referencing steps, next_step_index, payload, and os.write(master_fd, ...)) to safely access the field—either validate the step dict before use or replace the direct access with steps[next_step_index].get("input", "") (and encode that default) so missing keys become an empty payload instead of throwing, then proceed to os.write(master_fd, payload) as before.examples/split-editors/main.ts-142-143 (1)
142-143:⚠️ Potential issue | 🟡 MinorFocused header badges now make the title row spacing unstable.
badgeText()returns a styled string, but the header is still spaced with raw string padding at Line 147. That makes the escape sequences part of the length calculation, so the right-hand filename shifts when focus toggles. Keep plain-text labels for width math, or render the two header cells as separate layout items instead of padding a rendered ANSI string.💡 Minimal fix
- const leftLabel = model.focusLeft ? badgeText('app.ts', 'primary') : 'app.ts'; - const rightLabel = !model.focusLeft ? badgeText('app.test.ts', 'primary') : 'app.test.ts'; + const leftText = 'app.ts'; + const rightText = 'app.test.ts'; + const leftLabel = model.focusLeft ? badgeText(leftText, 'primary') : leftText; + const rightLabel = !model.focusLeft ? badgeText(rightText, 'primary') : rightText; + const headerGap = ''.padEnd(Math.max(1, paneWidth - leftText.length - 2));- { basis: 1, content: ` ${leftLabel}${''.padEnd(paneWidth - 10)}${rightLabel}` }, + { basis: 1, content: ` ${leftLabel}${headerGap}${rightLabel}` },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/split-editors/main.ts` around lines 142 - 143, The issue is that badgeText() injects ANSI styling into leftLabel/rightLabel so the header width math uses escaped lengths and shifts when model.focusLeft toggles; change the header width calculation to use plain-text labels (e.g., introduce leftLabelPlain = 'app.ts' and rightLabelPlain = 'app.test.ts') for all padding/width computations, then only apply badgeText(leftLabelPlain, 'primary') / badgeText(rightLabelPlain, 'primary') when actually rendering the header cell content (or alternatively render the two header cells as separate layout items instead of padding a single ANSI-styled string); update uses of leftLabel and rightLabel in the header layout code to use the plain-labels for length math and styled labels for display.examples/v3-pipeline/main.ts-38-40 (1)
38-40:⚠️ Potential issue | 🟡 MinorClarify the model contract instead of silently casting.
Line 40 assumes
state.modelis always aModelinstance. Although the library typesmodel: any, this helper should either document its model dependency or add a defensive check to fail gracefully if the model lacksscanlineOn. The current unchecked cast hides an implicit contract violation at runtime.Suggested defensive guard
export function configureScanlinePipeline(pipeline: RenderPipeline): void { pipeline.use('PostProcess', (state, next) => { - const enabled = (state.model as Model).scanlineOn; + const enabled = + typeof state.model === 'object' && + state.model !== null && + 'scanlineOn' in state.model && + state.model.scanlineOn === true; if (enabled) { for (let y = 1; y < state.targetSurface.height; y += 2) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/v3-pipeline/main.ts` around lines 38 - 40, In configureScanlinePipeline, avoid silently casting state.model to Model; add a defensive guard that checks state.model and the scanlineOn property (e.g., "if (!state.model || typeof state.model.scanlineOn !== 'boolean')") and either return early or throw a clear error describing the expected model contract, or update the function signature/docs to require a Model; reference state.model and the scanlineOn property so the check happens before using (state.model as Model).scanlineOn within the pipeline.use callback.scripts/smoke-all-examples.ts-269-281 (1)
269-281:⚠️ Potential issue | 🟡 MinorPython dependency required for smoke tests.
The interactive-tty mode test suite depends on an external
python3script (scripts/pty-driver.py). This dependency is not documented in the README, CI workflow, or contributing guidelines. Add a note to the development setup section or CI comments documenting that Python 3 must be available when runningnpm run smoke:examples:all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke-all-examples.ts` around lines 269 - 281, The test harness spawns an external Python script ('scripts/pty-driver.py') via the spawn call in smoke-all-examples.ts (where BIJOU_PTY_SPEC is set and python3 is invoked) but this dependency isn't documented; update the development setup docs and CI comments to state that Python 3 is required to run `npm run smoke:examples:all`, and either add a short CI workflow comment/check that verifies `python3 --version` (or fails with a clear message) before running the smoke tests, and add the same note to CONTRIBUTING/README under the development/setup section so maintainers and contributors know the requirement.packages/bijou/src/core/render/differ.ts-79-92 (1)
79-92:⚠️ Potential issue | 🟡 MinorConsider defensive handling for malformed truecolor sequences.
If the ANSI sequence is malformed (e.g.,
38;2;with missing RGB values),parseInt(undefined, 10)returnsNaN, andNaN.toString(16)produces"NaN", resulting in invalid hex like#NaNNaNNaN. This is an edge case but could cause downstream issues.🛡️ Proposed defensive fix
} else if (code === '38' && parts[i+1] === '2') { // Truecolor FG: 38;2;R;G;B - const r = parseInt(parts[i+2]!, 10).toString(16).padStart(2, '0'); - const g = parseInt(parts[i+3]!, 10).toString(16).padStart(2, '0'); - const b = parseInt(parts[i+4]!, 10).toString(16).padStart(2, '0'); - currentFg = '#' + r + g + b; + const r = parseInt(parts[i+2] ?? '', 10); + const g = parseInt(parts[i+3] ?? '', 10); + const b = parseInt(parts[i+4] ?? '', 10); + if (!Number.isNaN(r) && !Number.isNaN(g) && !Number.isNaN(b)) { + currentFg = '#' + r.toString(16).padStart(2, '0') + g.toString(16).padStart(2, '0') + b.toString(16).padStart(2, '0'); + } i += 4; } else if (code === '48' && parts[i+1] === '2') { // Truecolor BG: 48;2;R;G;B - const r = parseInt(parts[i+2]!, 10).toString(16).padStart(2, '0'); - const g = parseInt(parts[i+3]!, 10).toString(16).padStart(2, '0'); - const b = parseInt(parts[i+4]!, 10).toString(16).padStart(2, '0'); - currentBg = '#' + r + g + b; + const r = parseInt(parts[i+2] ?? '', 10); + const g = parseInt(parts[i+3] ?? '', 10); + const b = parseInt(parts[i+4] ?? '', 10); + if (!Number.isNaN(r) && !Number.isNaN(g) && !Number.isNaN(b)) { + currentBg = '#' + r.toString(16).padStart(2, '0') + g.toString(16).padStart(2, '0') + b.toString(16).padStart(2, '0'); + } i += 4;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/render/differ.ts` around lines 79 - 92, The truecolor parsing in render/differ.ts (handling codes '38;2' and '48;2') doesn't guard against missing or non-numeric RGB parts, which leads to "#NaNNaNNaN"; fix by validating that parts[i+2], parts[i+3], parts[i+4] exist and parse to finite numbers, clamp them to 0–255, and only then convert to two‑digit hex to set currentFg/currentBg (otherwise fall back to a safe value like '#000000' or skip updating the color); also ensure the index increment (i += 4) only occurs after successful consumption of the RGB components.packages/bijou/src/core/render/differ.ts-165-170 (1)
165-170:⚠️ Potential issue | 🟡 MinorModifier comparison is order-sensitive.
The current implementation compares modifiers by index, so
['bold', 'dim']and['dim', 'bold']would be considered different. If modifiers can be added in different orders (e.g., from different SGR sequences), this could cause unnecessary re-renders.💡 Set-based comparison alternative
const aMods = a.modifiers ?? []; const bMods = b.modifiers ?? []; if (aMods.length !== bMods.length) return false; - for (let i = 0; i < aMods.length; i++) { - if (aMods[i] !== bMods[i]) return false; - } + const aSet = new Set(aMods); + for (const mod of bMods) { + if (!aSet.has(mod)) return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/render/differ.ts` around lines 165 - 170, The modifier comparison is order-sensitive because it compares aMods and bMods by index; change it to an order-insensitive (set) comparison: after obtaining aMods and bMods, if lengths differ return false, then create a Set from aMods (or a frequency map if duplicates matter) and verify every entry in bMods exists in that Set (and sizes match) so ['bold','dim'] and ['dim','bold'] are treated equal; update the comparison logic where aMods and bMods are used in the differ (the aMods/bMods block) to perform this set-based containment check.packages/bijou-tui/src/eventbus.ts-264-267 (1)
264-267:⚠️ Potential issue | 🟡 MinorUnsafe cast of
io.onDatapayload toM.The payload from
io.onDatais cast directly toMwithout validation. If the data channel emits messages that don't conform toM, this could cause runtime issues in the application'supdatefunction.Consider either:
- Adding a type guard or validation callback in
connectIOoptions, or- Documenting that
io.onDatapayloads must conform toM.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/eventbus.ts` around lines 264 - 267, The code unsafely casts the payload from io.onData to generic M before emitting (io.onData, emit, M); add a runtime validation step: extend the connectIO/options to accept an optional type guard or validator (e.g., validateData: (raw: unknown) => raw is M or transformData: (raw: unknown) => M | undefined) and invoke it inside the onData handler, only calling emit(payload as M) when the validator returns true (otherwise log/ignore the message); update types for connectIO and any callers to accept the validator so payloads are verified before being passed to update.packages/bijou-tui/src/runtime.ts-268-273 (1)
268-273:⚠️ Potential issue | 🟡 MinorPending render flush may not wait for the actual render.
The
renderRequestedflag is set, and a 0ms timeout is awaited. However, the render itself is scheduled viasetTimeout(..., 0)in therender()function. If these timeouts race, the exit might occur before the render completes.Consider using a more deterministic approach, such as awaiting a render completion signal or using
requestAnimationFrame-style callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/runtime.ts` around lines 268 - 273, The current wait (await new Promise setTimeout 0) can race with render()'s own setTimeout; replace this ad-hoc wait with a deterministic render completion signal: add a module-scoped pendingRenderPromise and pendingRenderResolve that render() assigns/creates and resolves when the actual render work completes, then here (where renderRequested is checked) await that pendingRenderPromise (or create one if missing) instead of the 0ms timeout so the exit only proceeds after render() has finished.packages/bijou-tui/src/driver.ts-114-128 (1)
114-128:⚠️ Potential issue | 🟡 MinorRemove
startPulse()from the scripted driver—it's unnecessary overhead.The scripted driver feeds explicit steps (key, pulse, resize, msg) sequentially and doesn't need a background animation timer. Calling
bus.startPulse()starts a 60fps interval that continuously emitsPulseMsgevents throughout script execution, wasting resources with no benefit for deterministic testing.If animation testing is needed, use explicit
{ pulse: { dt } }steps instead. The interactive driver (runtime.ts) correctly usesstartPulse()with a refresh rate; the scripted driver should not.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/driver.ts` around lines 114 - 128, Remove the automatic animation heartbeat from the scripted driver by deleting the bus.startPulse() call (the line invoking startPulse on the bus) so the driver only processes explicit scripted steps (pulse, key, resize, msg); ensure no other logic depends on a background pulse in this flow (check surrounding initialization where ctx, installBCSSResolver, app.init(), model/currentSize are set) and leave the interactive driver's startPulse usage untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d6237ec-321c-428d-92b7-13b18d706744
⛔ Files ignored due to path filters (7)
examples/v3-css/demo.gifis excluded by!**/*.gifexamples/v3-demo/demo.gifis excluded by!**/*.gifexamples/v3-motion/demo.gifis excluded by!**/*.gifexamples/v3-pipeline/demo.gifis excluded by!**/*.gifexamples/v3-subapp/demo.gifis excluded by!**/*.gifexamples/v3-worker/demo.gifis excluded by!**/*.gifpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (127)
.github/workflows/ci.yml.github/workflows/publish.ymlREADME.mddemo.tsdocs/ARCHITECTURE.mddocs/CHANGELOG.mddocs/EXAMPLES.mddocs/MIGRATING_TO_V3.mdexamples/README.mdexamples/_shared/canonical-app.tsexamples/_shared/v3.tsexamples/badge/main.tsexamples/canvas/README.mdexamples/canvas/main.tsexamples/chat/main.tsexamples/composable/main.tsexamples/confirm/main.tsexamples/drawer/main.tsexamples/filter/main.tsexamples/flex-layout/main.tsexamples/focus-area/main.tsexamples/input/main.tsexamples/logo/main.tsexamples/modal/main.tsexamples/mouse/main.tsexamples/multiselect/main.tsexamples/package-manager/main.tsexamples/pipe/main.tsexamples/print-key/main.tsexamples/progress-animated/main.tsexamples/progress-download/main.tsexamples/select/main.tsexamples/showcase/app.tsexamples/showcase/registry.tsexamples/spinner/main.tsexamples/split-editors/main.tsexamples/spring/main.tsexamples/theme/main.tsexamples/timeline-anim/main.tsexamples/toast/main.tsexamples/tooltip/main.tsexamples/transitions/main.tsexamples/v3-css/README.mdexamples/v3-css/main.tsexamples/v3-css/record.tsexamples/v3-demo/README.mdexamples/v3-demo/main.tsexamples/v3-demo/record.tsexamples/v3-motion/README.mdexamples/v3-motion/main.tsexamples/v3-motion/record.tsexamples/v3-pipeline/README.mdexamples/v3-pipeline/main.tsexamples/v3-pipeline/record.tsexamples/v3-subapp/README.mdexamples/v3-subapp/main.tsexamples/v3-subapp/record.tsexamples/v3-worker/README.mdexamples/v3-worker/main.tsexamples/v3-worker/record.tspackage.jsonpackages/bijou-node/README.mdpackages/bijou-node/package.jsonpackages/bijou-node/src/gifenc.d.tspackages/bijou-node/src/index.tspackages/bijou-node/src/recorder.test.tspackages/bijou-node/src/recorder.tspackages/bijou-node/src/style.tspackages/bijou-node/src/worker/fixtures/echo-worker.tspackages/bijou-node/src/worker/worker.test.tspackages/bijou-node/src/worker/worker.tspackages/bijou-tui-app/README.mdpackages/bijou-tui-app/package.jsonpackages/bijou-tui-app/src/index.test.tspackages/bijou-tui/README.mdpackages/bijou-tui/package.jsonpackages/bijou-tui/src/animate.test.tspackages/bijou-tui/src/app-frame-actions.tspackages/bijou-tui/src/app-frame-render.tspackages/bijou-tui/src/app-frame.test.tspackages/bijou-tui/src/app-frame.tspackages/bijou-tui/src/browsable-list.test.tspackages/bijou-tui/src/command-palette.test.tspackages/bijou-tui/src/commands.test.tspackages/bijou-tui/src/css/install.tspackages/bijou-tui/src/css/text-style.tspackages/bijou-tui/src/driver.test.tspackages/bijou-tui/src/driver.tspackages/bijou-tui/src/eventbus.test.tspackages/bijou-tui/src/eventbus.tspackages/bijou-tui/src/focus-area.test.tspackages/bijou-tui/src/focus-area.tspackages/bijou-tui/src/index.tspackages/bijou-tui/src/layout-preset.test.tspackages/bijou-tui/src/motion/motion.tspackages/bijou-tui/src/motion/reconciler.test.tspackages/bijou-tui/src/motion/reconciler.tspackages/bijou-tui/src/motion/types.tspackages/bijou-tui/src/navigable-table.test.tspackages/bijou-tui/src/panel-dock.test.tspackages/bijou-tui/src/pipeline/pipeline.test.tspackages/bijou-tui/src/runtime.test.tspackages/bijou-tui/src/runtime.tspackages/bijou-tui/src/subapp/mount.test.tspackages/bijou-tui/src/subapp/mount.tspackages/bijou-tui/src/types.tspackages/bijou-tui/src/view-output.tspackages/bijou/README.mdpackages/bijou/package.jsonpackages/bijou/src/core/components/badge.tspackages/bijou/src/core/components/bcss-style.tspackages/bijou/src/core/components/box-v3.tspackages/bijou/src/core/components/box.tspackages/bijou/src/core/components/dag.test.tspackages/bijou/src/core/render/differ.test.tspackages/bijou/src/core/render/differ.tspackages/bijou/src/core/theme/dtcg.fuzz.test.tspackages/bijou/src/core/theme/tokens.test.tspackages/bijou/src/index.tspackages/bijou/src/ports/surface.tspackages/create-bijou-tui-app/README.mdpackages/create-bijou-tui-app/package.jsonscripts/pty-driver.pyscripts/record-gifs.tsscripts/smoke-all-examples.tsscripts/smoke-v3-examples.tstsconfig.tests.json
✅ Files skipped from review due to trivial changes (5)
- packages/bijou-tui-app/README.md
- examples/v3-pipeline/README.md
- examples/v3-subapp/README.md
- docs/CHANGELOG.md
- examples/v3-demo/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/bijou-tui/src/index.ts
- packages/bijou-tui/src/subapp/mount.test.ts
- packages/bijou/src/index.ts
- examples/v3-css/main.ts
|
@coderabbitai review please 🥕 |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/bijou-node/src/worker/worker.ts (1)
47-54:⚠️ Potential issue | 🟠 MajorNarrow
RunWorkerOptionsto the options worker mode actually honors.Line 47 still exposes the full
RunOptions<any>surface, but Lines 99-105 only serializealtScreen,hideCursor,mouse, andcss, and Lines 214-220 rebuildproxyOptionsfrom that subset. That meansctx,middlewares, andconfigurePipelineare currently accepted by the type system and then silently ignored in worker mode, sorunInWorker()does not behave likerun()for the same options.Please either make
RunWorkerOptionsa worker-safe subset or reject unsupported fields up front.Suggested API narrowing
-export interface RunWorkerOptions extends RunOptions<any> { +export interface RunWorkerOptions { + altScreen?: boolean; + hideCursor?: boolean; + mouse?: boolean; + css?: string; /** The absolute path to the file containing the worker entry point. */ entry: string; /** Optional callback for custom data messages sent from the worker via `sendToMain`. */ onMessage?: (payload: unknown) => void; /** Optional arguments passed to the Node.js worker process (e.g. ['--import', 'tsx']). */ execArgv?: string[]; }Also applies to: 99-105, 214-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-node/src/worker/worker.ts` around lines 47 - 54, The RunWorkerOptions type currently extends RunOptions<any> but the worker only serializes altScreen, hideCursor, mouse, and css and rebuilds proxyOptions in runInWorker(), so unsupported fields (ctx, middlewares, configurePipeline, etc.) are silently ignored; update RunWorkerOptions to be a worker-safe subset containing only entry, onMessage?, execArgv?, and the four serializable UI options (altScreen, hideCursor, mouse, css) OR add upfront validation inside runInWorker() to throw an error when unsupported keys (ctx, middlewares, configurePipeline and any others from RunOptions) are present before serialization, and update references to proxyOptions reconstruction (proxyOptions, buildProxyOptions) so they only depend on the allowed subset.
🧹 Nitpick comments (1)
packages/bijou/src/core/layout/flex.test.ts (1)
4-65: Good foundational coverage, but consider expanding edge cases.The tests cover core scenarios well. Consider adding tests for:
- Empty children array (returns bounds with empty children)
minSizeconstraint (onlymaxSizeis tested)- Non-zero starting position (
bounds.xandbounds.y> 0) to verify offset calculation- Unequal flex ratios (e.g.,
flex: 2vsflex: 1)These are optional additions that would strengthen the test suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou/src/core/layout/flex.test.ts` around lines 4 - 65, Add the suggested edge-case tests to the calculateFlex test suite: create new it blocks exercising calculateFlex with (1) an empty children array to assert it returns a root with empty children and original bounds, (2) a child using minSize to verify it respects minimum constraint (use both minSize and maxSize together), (3) non-zero bounds (bounds.x and bounds.y > 0) to ensure child rect positions are offset correctly, and (4) unequal flex ratios (e.g., flex: 2 vs flex: 1) to assert proportional allocation; reference the calculateFlex function and existing test patterns in flex.test.ts when adding these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bijou-node/src/worker/worker.ts`:
- Around line 99-108: The worker is not receiving the host terminal size so its
initial createNodeContext()/run() use process-global dimensions; update the
Worker creation (where workerData is set for options.serializableOptions) to
include the host columns and rows (e.g., workerData: { ..., options:
serializableOptions, columns, rows }), then in the worker bootstrap where
createNodeContext() and proxyCtx are initialized, hydrate proxyCtx.runtime with
those initial columns/rows before calling run() so the first render uses the
caller's surface size; also modify the resize bridge that currently only
forwards callbacks (proxy runtime update code around the
proxyCtx.runtime/resizer handling) to set proxyCtx.runtime.columns/rows when
resize events arrive so the proxy runtime stays in sync.
In `@packages/bijou-tui/src/runtime.ts`:
- Around line 100-103: The runtime resize handling is leaving
ctx.runtime.columns and ctx.runtime.rows stale while currentSurface is updated,
causing Layout (wrapViewOutputAsLayoutRoot) and Render to use old dimensions;
either update ctx.runtime.columns/rows inside the resize handler (where
currentSurface is set) to the new width/height from the resize message, or
change the Layout (wrapViewOutputAsLayoutRoot call) and Render timeout to accept
and use the width/height from the resize event instead of reading ctx.runtime;
also apply the same fix to any other consumers (e.g., the CSS resolver in
pipeline/middleware/css.ts) so all stages read the same source of truth.
- Around line 163-183: The scheduled callback inside render() currently runs
code (createSurface, pipeline.execute which may call
app.view/renderSurfaceFrame) without error handling; wrap the body of the
setTimeout callback in a try-catch so any thrown error calls shutdown(error) (or
shutdown() then rethrow/exit) to restore terminal state, while still ensuring
renderRequested is cleared; update references inside the callback that touch
createSurface(sanitizeDimension(ctx.runtime.columns),
sanitizeDimension(ctx.runtime.rows)), pipeline.execute(renderState), and any use
of app.view/renderSurfaceFrame to occur within the try block and route
exceptions to shutdown in the catch.
In `@packages/bijou/src/core/layout/flex.ts`:
- Around line 19-26: FlexChildProps defines an align property that calculateFlex
never uses, so implement cross-axis alignment inside calculateFlex: read
child.align (from FlexChildProps) and when computing each child's cross-axis
size/position in calculateFlex, apply alignment rules (e.g., Align.Start =>
position at cross start, Align.Center => center within containerCrossSize,
Align.End => position at cross end, Align.Stretch => fill containerCrossSize)
rather than always stretching; update calculateFlex to compute child.crossSize
and child.crossOffset based on containerCrossSize and the child's min/max if
needed, using the existing Align type/enum to drive behavior.
- Line 6: The barrel export is missing the Align type so consumers can't import
it alongside FlexOptions and FlexChildProps; update the module exports to
re-export Align (the exported type from flex.ts) together with FlexOptions and
FlexChildProps so callers can import Align from the same barrel (i.e., add Align
to the list of exported symbols in the index.ts export statement referencing
FlexOptions and FlexChildProps).
---
Duplicate comments:
In `@packages/bijou-node/src/worker/worker.ts`:
- Around line 47-54: The RunWorkerOptions type currently extends RunOptions<any>
but the worker only serializes altScreen, hideCursor, mouse, and css and
rebuilds proxyOptions in runInWorker(), so unsupported fields (ctx, middlewares,
configurePipeline, etc.) are silently ignored; update RunWorkerOptions to be a
worker-safe subset containing only entry, onMessage?, execArgv?, and the four
serializable UI options (altScreen, hideCursor, mouse, css) OR add upfront
validation inside runInWorker() to throw an error when unsupported keys (ctx,
middlewares, configurePipeline and any others from RunOptions) are present
before serialization, and update references to proxyOptions reconstruction
(proxyOptions, buildProxyOptions) so they only depend on the allowed subset.
---
Nitpick comments:
In `@packages/bijou/src/core/layout/flex.test.ts`:
- Around line 4-65: Add the suggested edge-case tests to the calculateFlex test
suite: create new it blocks exercising calculateFlex with (1) an empty children
array to assert it returns a root with empty children and original bounds, (2) a
child using minSize to verify it respects minimum constraint (use both minSize
and maxSize together), (3) non-zero bounds (bounds.x and bounds.y > 0) to ensure
child rect positions are offset correctly, and (4) unequal flex ratios (e.g.,
flex: 2 vs flex: 1) to assert proportional allocation; reference the
calculateFlex function and existing test patterns in flex.test.ts when adding
these cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 631711f9-dd7f-4755-bbb9-efc3bb8ab0d2
📒 Files selected for processing (11)
docs/CHANGELOG.mdpackages/bijou-node/src/worker/fixtures/echo-worker.mjspackages/bijou-node/src/worker/worker.test.tspackages/bijou-node/src/worker/worker.tspackages/bijou-tui/src/canvas.test.tspackages/bijou-tui/src/canvas.tspackages/bijou-tui/src/runtime.test.tspackages/bijou-tui/src/runtime.tspackages/bijou/src/core/layout/flex.test.tspackages/bijou/src/core/layout/flex.tspackages/bijou/src/ports/io.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bijou-node/src/worker/worker.test.ts
- packages/bijou-tui/src/runtime.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/bijou-node/src/recorder.ts (1)
312-320: Consider hardeningparseHexagainst invalid hex characters.If a 6-character string contains non-hex characters (e.g.,
'#gggggg'),parseIntreturnsNaN, which becomes0when written toUint8Array. This silently produces black instead of the intended white fallback. Given that inputs typically come from controlled sources (BCSS tokens, user config), this is low risk.♻️ Optional: Add NaN guard for defensive robustness
function parseHex(hex: string): Rgb { const normalized = hex.replace('#', ''); if (normalized.length !== 6) return { r: 255, g: 255, b: 255 }; - return { - r: parseInt(normalized.slice(0, 2), 16), - g: parseInt(normalized.slice(2, 4), 16), - b: parseInt(normalized.slice(4, 6), 16), - }; + const r = parseInt(normalized.slice(0, 2), 16); + const g = parseInt(normalized.slice(2, 4), 16); + const b = parseInt(normalized.slice(4, 6), 16); + if (Number.isNaN(r) || Number.isNaN(g) || Number.isNaN(b)) { + return { r: 255, g: 255, b: 255 }; + } + return { r, g, b }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-node/src/recorder.ts` around lines 312 - 320, The parseHex function can silently produce NaNs for non-hex characters; update parseHex to validate the normalized string contains only 6 hex digits (e.g., a regex like /^[0-9a-fA-F]{6}$/) before calling parseInt, and return the white fallback ({ r:255,g:255,b:255 }) when validation fails (or alternatively guard each parseInt result for NaN and use 255 on NaN). Ensure you update the parseHex function and its return path so invalid input never produces NaN/0 values in the resulting Rgb.scripts/smoke-all-examples.ts (2)
309-310: Print a short output tail for failures.Right now the gate logs mostly stop at
FAIL: <reason>. Sinceresult.outputis already available, dumping the last few sanitized lines here would make CI/publish failures debuggable without a local rerun.Suggested improvement
if (failures.length > 0) { process.stdout.write('\nFailures:\n'); for (const failure of failures) { process.stdout.write(`- ${failure.path} [${failure.mode}] ${failure.reason}\n`); + if (failure.output) { + const tail = failure.output.trimEnd().split('\n').slice(-20).join('\n'); + if (tail) process.stdout.write(`${tail}\n\n`); + } } process.exitCode = 1; }Also applies to: 313-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke-all-examples.ts` around lines 309 - 310, When recording a failure (where failures.push(result) and process.stdout.write(`FAIL: ${result.reason}\n`) is called), also print a short sanitized tail of result.output (e.g., last 5–10 non-empty lines) to aid debugging; implement a helper to sanitize/control length (strip ANSI, trim long lines) and call it to generate the tail string, then append it to the FAIL message (and replicate the same change for the other process.stdout.write occurrence that logs failures) so CI logs include a compact output snippet without dumping full logs.
14-20: The runner is still Unix-bound in discovery and PTY startup.
find,/usr/bin/script, andpython3meansmoke:examples:allcan still die before the smoke logic even starts on unsupported environments. Since this script now gates.github/workflows/ci.yml:29-31and.github/workflows/publish.yml:117-118, I’d keep target discovery inside Node and either use a platform-aware PTY wrapper or explicitly skip unsupported modes per platform.Also applies to: 235-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke-all-examples.ts` around lines 14 - 20, The EXAMPLES discovery is Unix-bound because it uses execSync('find ...') and the script later spawns platform-specific PTY helpers; replace the execSync('find ...') usage (symbol: EXAMPLES) with a Node-native discovery using fs.readdir (or a small glob library) to recursively find main.ts so paths work on Windows, macOS, and Linux, and update the PTY startup block (the current PTY/spawn logic) to use a cross-platform PTY library (e.g., node-pty) or use cross-spawn and explicit process.platform checks to skip or choose an alternate startup on unsupported platforms instead of invoking /usr/bin/script or python3; ensure any skipped runs are clearly logged and that exit codes reflect skips vs failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/CHANGELOG.md`:
- Around line 9-15: Move the four stabilization bullet points currently under
the [Unreleased] heading into the 3.0.0 release notes section so they appear as
part of the v3.0.0 changelog; specifically cut the bullets mentioning
back-buffer syncing, leftover flex units, worker IPC/canvas/mouse changes, and
GIF encoder normalization and paste them under the "3.0.0" heading, then remove
them from the [Unreleased] section (which should remain as v3.0.0...HEAD) so the
3.0.0 notes are complete and Unreleased only contains post-release commits.
In `@scripts/smoke-all-examples.ts`:
- Around line 5-7: The ROOT calculation using dirname(new
URL(import.meta.url).pathname) produces percent-encoded and Windows-drive-path
issues; replace it to use fileURLToPath(import.meta.url) from 'node:url'
instead: import fileURLToPath and call dirname(fileURLToPath(import.meta.url))
when building ROOT with resolve so paths are decoded correctly across platforms
(update the import list and the ROOT constant where dirname/new URL are used).
---
Nitpick comments:
In `@packages/bijou-node/src/recorder.ts`:
- Around line 312-320: The parseHex function can silently produce NaNs for
non-hex characters; update parseHex to validate the normalized string contains
only 6 hex digits (e.g., a regex like /^[0-9a-fA-F]{6}$/) before calling
parseInt, and return the white fallback ({ r:255,g:255,b:255 }) when validation
fails (or alternatively guard each parseInt result for NaN and use 255 on NaN).
Ensure you update the parseHex function and its return path so invalid input
never produces NaN/0 values in the resulting Rgb.
In `@scripts/smoke-all-examples.ts`:
- Around line 309-310: When recording a failure (where failures.push(result) and
process.stdout.write(`FAIL: ${result.reason}\n`) is called), also print a short
sanitized tail of result.output (e.g., last 5–10 non-empty lines) to aid
debugging; implement a helper to sanitize/control length (strip ANSI, trim long
lines) and call it to generate the tail string, then append it to the FAIL
message (and replicate the same change for the other process.stdout.write
occurrence that logs failures) so CI logs include a compact output snippet
without dumping full logs.
- Around line 14-20: The EXAMPLES discovery is Unix-bound because it uses
execSync('find ...') and the script later spawns platform-specific PTY helpers;
replace the execSync('find ...') usage (symbol: EXAMPLES) with a Node-native
discovery using fs.readdir (or a small glob library) to recursively find main.ts
so paths work on Windows, macOS, and Linux, and update the PTY startup block
(the current PTY/spawn logic) to use a cross-platform PTY library (e.g.,
node-pty) or use cross-spawn and explicit process.platform checks to skip or
choose an alternate startup on unsupported platforms instead of invoking
/usr/bin/script or python3; ensure any skipped runs are clearly logged and that
exit codes reflect skips vs failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 364fd32e-2aac-4f82-9f45-03c788d0a2de
📒 Files selected for processing (6)
docs/CHANGELOG.mdpackages/bijou-node/src/gifenc.d.tspackages/bijou-node/src/gifenc.test.tspackages/bijou-node/src/recorder.test.tspackages/bijou-node/src/recorder.tsscripts/smoke-all-examples.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bijou-node/src/recorder.test.ts
Phase A of the RE-017 execution plan. Migrates remaining components to the theme cache fast path (fgRGB/bgRGB), adds three new bench scenarios that cover realistic workloads, and captures the Phase A baseline for measuring Part II. Component migration (task #38): - notification.ts: CellTextStyle extended with fgRGB/bgRGB; tokenToCellStyle now threads them through. - overlay.ts: CellStyle type extended; styleFromToken and backgroundStyleFromToken updated. - transition-shaders.ts: tokenCell helper updated. - app-frame-render.ts: active header tab override path updated. - Skipped css/text-style.ts (local StyledTextToken has no pre-parsed RGB; hot path already caches via parseHex once); box-v3.ts fallback (already has setRGB fast path); theme/graph.ts (not a rendering hot path). New bench scenarios (tasks #39, #40, #41): - diff-sparse: ~10% dirty cells per frame, realistic interactive updates. P50 349 µs at baseline. - diff-static: identical surfaces, zero changes. Reference target for II-1 render-dirty bitmap optimization. P50 227 µs — should drop to near-zero post-II-1. - dogfood-realistic: multi-region composition (header+sidebar+ body+footer). Cross-component regression gate for Part II. P50 469 µs. Phase A baseline (task #43): bench/baselines/HEAD-f966c72-phase-a.json. 30 samples each, all CoVs under 5% except paint-rgb-fixed (7.4% due to its 145 µs floor where OS jitter dominates). Numbers are trustworthy. Obsolete tasks moved to docs/graveyard/: - I-3 (custom baseline format) — bench v2 already ships one. - I-0d (render pipeline instrumentation) — I-0e confirmed the hex-parse hypothesis without needing general instrumentation. Full findings and Part II targets documented in docs/perf/RE-017-byte-pipeline.md. 2,807 tests pass. Typecheck clean across all workspace packages. Part II targets unlocked by this baseline: - diff-static: 227 µs -> ~0 µs (II-1 render-dirty bitmap skip) - diff-gradient: 2.08 ms -> <1.5 ms (II-4 byte pipeline) - dogfood-realistic: 469 µs -> <350 µs (combined II-1 + II-4) - diff-sparse: 349 µs -> <200 µs (II-1 + II-4)
1 participant
Summary
This PR finishes Bijou v3.0.0 and prepares it for general release.
It includes the V3 runtime work, shell compatibility fixes, native demo recording, canonical V3 demos, example smoke coverage, CI/release gate wiring, and the final docs truth pass.
What changed
ViewOutputacrossApp.viewand framed shell panesinitSubApp(),updateSubApp(),mount(), andmapCmds()typecheck:testandsmoke:examples:allinto CI and release verificationRelease-facing docs
What's New in v3.0.0sectiondocs/ARCHITECTURE.mdrewritten to reflect the actual five-package monorepo and package splitdocs/CHANGELOG.mdupdated with the3.0.0release entrydocs/MIGRATING_TO_V3.mdadded for existing appsbijou,bijou-node,bijou-tui,bijou-tui-app, andcreate-bijou-tui-appVerification
npm run buildnpm run lintnpm test(120files,2120passing,12skipped)npm run typecheck:testnpm run smoke:examples:allnpm pack --dry-run --workspacesDownstream canary
warp-lensmanually checked and working well on this branchSummary by CodeRabbit
Release Notes
New Features
Breaking Changes
App.viewnow returnsViewOutput(Surface | LayoutNode | string)BijouNodeOptions