feat: open ./agent-chat as its own door, and put the agent layer under test - #43
Conversation
… all src/agent/** and src/agent-chat/** shipped with zero tests because there was nowhere to put them: no happy-dom, no @testing-library/*, no bunfig.toml. The two most intricate modules in the package had no assertions between them. Four pieces, each of which is wrong in a way that fails silently: - The registrator is a separate package from happy-dom (@happy-dom/global-registrator), and @testing-library/dom has been a peer of @testing-library/react since v16, so both install explicitly. - RTL's auto-cleanup no-ops under bun test. It registers with `typeof afterEach === 'function'` against the global scope, and Bun exposes afterEach only through bun:test, so the branch never fires and every test leaks its DOM into the next. Wired by hand in the preload, with a comment saying why — it reads as redundant otherwise. - GlobalRegistrator.register() overwrites TransformStream and WritableStream with Stream.Transform and Stream.Writable — Node's classic streams, which share a name with the web-streams API and nothing else. The ai package constructs TransformStream at runtime inside its SSE parsing, so every streaming test breaks with an error naming neither cause. The native classes are restored from node:stream/web after registration. - Restoring those three then leaves AbortController and AbortSignal as happy-dom's, and a native stream refuses a foreign signal: pipeTo with one throws `options.signal must be AbortSignal`. Restoring half a family of globals creates a mismatch that did not exist before, so the abort pair is captured before registration and put back after. Nothing crosses that seam today only because every test injects its own fetch, and the agent layer is abort-based end to end. Root devDependencies only; the published package's manifest is untouched, so its empty dependencies and its optional-peer story are unchanged. The lefthook isolation allowlist gains bunfig.toml, package.json and bun.lock. The root test surface is as much the package's own infrastructure as tests/ already was, and without this the harness and the tests it enables could never be staged in the same commit.
📦 basalt-ui package modifiedTrigger the Make Release workflow after merging to publish to npm. |
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (37)
WalkthroughThe PR adds the ChangesAgent Chat platform
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (10)
apps/playground/src/demo/AgentWedgeDemoPage.tsx (1)
121-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type to the exported component.
The coding guidelines require explicit types on public exports.
AgentWedgeDemoPageis exported without a return type annotation.♻️ Proposed change
-export function AgentWedgeDemoPage() { +export function AgentWedgeDemoPage(): JSX.Element {As per coding guidelines: "Use strict TypeScript, avoid
any, prefer type inference, and provide explicit types on public exports."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/demo/AgentWedgeDemoPage.tsx` at line 121, Update the exported AgentWedgeDemoPage component declaration with an explicit React component return type, preserving its existing implementation and behavior.Source: Coding guidelines
packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx (1)
348-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink the skipped test to a tracking issue.
The comment documents a real defect in
consumeAndFinalize: afterawait resolveOutcome(...), the function writessetOutcomeandsetStatuswithout re-checking the supersede and abort guards it applied before the await. Astop()during a slowresolveOutcomeis therefore overwritten. The analysis matches the current implementation, becausestop()deletes the controller entry, so a re-check ofcontrollersRef.current.get(threadId) !== controllerwould suppress the late write.The PR defers the fix by design. A
test.skipwith no tracked reference can be lost. Add an issue link next to thetest.skip.Do you want me to open an issue that captures this reproduction and the proposed guard?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx` around lines 348 - 356, Add a tracking issue link immediately next to the skipped test declaration for consumeAndFinalize, referencing the issue that documents the stop() versus slow resolveOutcome race and proposed post-await guard. Keep the test skipped and do not implement the fix in this change.packages/basalt-ui/src/agent/use-agent-thread-runs.wedge.test.tsx (1)
69-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
createTestThreadsStoredouble into one shared module. Three test files each define a near-identical in-memoryThreadsStoreimplementation of roughly seventy lines. The shared root cause is the absence of a single test double for theThreadsStorecontract. Any future change to that interface must then be applied in three places, and the copies can drift apart silently.
packages/basalt-ui/src/agent/use-agent-thread-runs.wedge.test.tsx#L69-L138: move this implementation into a new shared test helper, for examplepackages/basalt-ui/src/agent/test-threads-store.ts, and keep the explanatory rationale comment with it. Accept an optionalonCallhook so the variant inuse-agent-thread-runs.test.tsxis covered.packages/basalt-ui/src/agent/use-agent-thread-runs.resume.test.tsx#L23-L92: delete the local copy and import the shared helper.packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx#L22-L102: delete the local copy and import the shared helper, passingonCallfor the finalize-order test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/basalt-ui/src/agent/use-agent-thread-runs.wedge.test.tsx` around lines 69 - 138, Extract the duplicated createTestThreadsStore implementation into packages/basalt-ui/src/agent/test-threads-store.ts, preserving the explanatory rationale comment and adding an optional onCall hook for finalize-order tracking. In packages/basalt-ui/src/agent/use-agent-thread-runs.wedge.test.tsx lines 69-138, replace the local implementation with an import; do the same in packages/basalt-ui/src/agent/use-agent-thread-runs.resume.test.tsx lines 23-92. In packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx lines 22-102, import the shared helper and pass onCall to the finalize-order test.apps/playground/src/demo/AgentChatSubpathDemoPage.tsx (1)
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type to the public component.
AgentChatSubpathDemoPageis a public export. Declare its return type.As per coding guidelines, "
**/*.{ts,tsx}: Use strict TypeScript, avoidany, prefer type inference, and provide explicit types on public exports."Proposed fix
+import type { ReactElement } from 'react' + -export function AgentChatSubpathDemoPage() { +export function AgentChatSubpathDemoPage(): ReactElement {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/demo/AgentChatSubpathDemoPage.tsx` at line 75, Update the public AgentChatSubpathDemoPage component declaration to include an explicit React JSX return type, preserving its existing implementation and behavior.Source: Coding guidelines
packages/basalt-ui/scripts/pack-test.sh (1)
369-374: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the complete
agent-chatexport contract.
export-surface.jsondeclares seven runtime exports, but this scratch consumer checks only four. A missingComposer,ThreadOutcomeCard, orthreadPartRenderersexport would still pass. Add assertions for the remaining three exports.Suggested assertions
const agentChat = await import('basalt-ui/agent-chat') +if (typeof agentChat.Composer !== 'function') throw new Error('agent-chat.Composer missing') +if (typeof agentChat.ThreadOutcomeCard !== 'function') throw new Error('agent-chat.ThreadOutcomeCard missing') +if (!agentChat.threadPartRenderers || typeof agentChat.threadPartRenderers !== 'object') { + throw new Error('agent-chat.threadPartRenderers missing') +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/basalt-ui/scripts/pack-test.sh` around lines 369 - 374, Extend the `agentChat` export assertions in the scratch consumer to cover the three missing runtime exports declared by `export-surface.json`: `Composer`, `ThreadOutcomeCard`, and `threadPartRenderers`. Preserve the existing checks and validate each added export against its expected runtime type.packages/basalt-ui/src/cli/index.ts (1)
1971-1984: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA malformed
aiMajorSkewReasonis silent while the majors agree.
resolveAiMajorSkewReasoncomputespresentButInvalidat Line 1949, but only the skew branch consults it. In the agreeing branch, the code testsaiMajorSkewReason !== null, which is also null for a malformed value. A consumer who writesaiMajorSkewReason: truetherefore gets no output at all until a real skew appears, and only then sees the "present but is not a non-empty string" failure.The stale-exemption warning exists for the same reason: an exemption nobody re-checks hides a later skew. A malformed exemption hides it just as effectively, and it is detectable now.
Warn on the malformed key in this branch too.
♻️ Proposed change
} else { const [major] = distinctMajors if (aiMajorSkewReason !== null) { warn( `basalt.aiMajorSkewReason ("${aiMajorSkewReason}") is declared but the ai package major ` + `already matches across all ${aiMajors.length} workspace package(s) declaring it ` + `(ai@${major}) — the exemption is no longer needed and can be deleted.`, ) + } else if (aiMajorSkewReasonInvalid) { + warn( + 'basalt.aiMajorSkewReason is present but is not a non-empty string (a bare `true` is ' + + 'not accepted) — it would NOT exempt a skew. The ai package major currently matches ' + + `across all ${aiMajors.length} workspace package(s) declaring it (ai@${major}); ` + + 'delete the key or give it a written reason.', + ) } else { pass( `ai package major matches across ${aiMajors.length} workspace package(s) declaring it (ai@${major})`, ) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/basalt-ui/src/cli/index.ts` around lines 1971 - 1984, Update the agreeing-majors branch near the existing aiMajorSkewReason warning to consult presentButInvalid before checking aiMajorSkewReason !== null. Emit the same malformed-value warning used by the skew branch when presentButInvalid is true; otherwise preserve the existing stale-exemption warning and pass behavior.packages/basalt-ui/tests/doctor.test.ts (2)
208-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the duplicated
runDoctorhelper.Lines 209-225 and Lines 348-364 define byte-identical helpers, including the same JSDoc line. Both are new in this change. Move one copy to module scope and delete the other.
This also resolves the
no-consolewarning that CI reports at Line 210 once instead of twice. Theconsole.logandconsole.errorreferences here save and restore the originals, which is a legitimate test-harness use of the rule's target, so a scoped disable on the hoisted helper is appropriate.♻️ Proposed refactor — one module-scope helper
+/** + * Run doctor, capturing stdout/stderr so the emitted lines can be asserted on either way. + * Swapping the console methods IS the mechanism here, not a stray debug statement. + */ +// oxlint-disable-next-line no-console +function runDoctor(): { code: number; out: string } { + const originalLog = console.log + const originalError = console.error + let out = '' + console.log = (...args: unknown[]) => { + out += `${args.join(' ')}\n` + } + console.error = (...args: unknown[]) => { + out += `${args.join(' ')}\n` + } + try { + return { code: doctor(tmpDir), out } + } finally { + console.log = originalLog + console.error = originalError + } +} + describe('basalt doctor — ai-major-parity', () => { - /** Run doctor, capturing stdout/stderr so the emitted lines can be asserted on either way. */ - function runDoctor(): { code: number; out: string } { - const originalLog = console.log - const originalError = console.error - let out = '' - console.log = (...args: unknown[]) => { - out += `${args.join(' ')}\n` - } - console.error = (...args: unknown[]) => { - out += `${args.join(' ')}\n` - } - try { - return { code: doctor(tmpDir), out } - } finally { - console.log = originalLog - console.error = originalError - } - } - it('hard-fails and names both packages when workspace packages disagree on the ai major', () => {Apply the same deletion to the second copy at Lines 347-364.
Also applies to: 347-364
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/basalt-ui/tests/doctor.test.ts` around lines 208 - 225, Hoist the duplicated runDoctor helper to module scope, preserving its existing JSDoc and behavior, then remove both in-test copies around the doctor test cases. Add a narrowly scoped no-console suppression for the hoisted helper because it intentionally captures and restores console.log and console.error.Source: Linters/SAST tools
267-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining
workspacesshapes.This case covers an absent
workspacesfield. Two other branches added in this change have no fixture:
- The object form
{ workspaces: { packages: [...] } }, handled atpackages/basalt-ui/src/cli/index.tsLines 1676-1680. The check must find the packages.- An unrecognized shape, for example
workspaces: 'packages/*'orworkspaces: {}, handled atpackages/basalt-ui/src/cli/index.tsLine 1681. The check must stay silent rather than throw.The second branch protects a documented invariant: the doc at
packages/basalt-ui/src/cli/index.tsLines 1660-1661 states the walk must never throw doctor over an unrecognizedworkspacesshape.💚 Proposed additional tests
it('walks the object form of the workspaces field', () => { setupPassingLayout() writeFixture( 'package.json', JSON.stringify({ name: 'consumer-monorepo', workspaces: { packages: ['packages/*'] }, }), ) writeFixture( 'packages/api/package.json', JSON.stringify({ name: 'api', dependencies: { ai: '5.0.196' } }), ) writeFixture( 'packages/dashboard/package.json', JSON.stringify({ name: 'dashboard', dependencies: { ai: '^7.0.18' } }), ) const { code, out } = runDoctor() expect(code).toBe(1) expect(out).toContain('api@ai5') }) it('stays silent for an unrecognized workspaces shape instead of throwing', () => { setupPassingLayout() writeFixture( 'package.json', JSON.stringify({ name: 'consumer-monorepo', workspaces: 'packages/*' }), ) const { code, out } = runDoctor() expect(code).toBe(0) expect(out).not.toContain('ai package major') })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/basalt-ui/tests/doctor.test.ts` around lines 267 - 272, Extend the doctor tests around the existing no-workspaces case to cover both remaining workspaces shapes: verify `{ packages: [...] }` discovers package dependencies and reports the expected major mismatch, and verify an unrecognized shape such as a string remains silent with exit code 0 instead of throwing. Use the existing `setupPassingLayout`, `writeFixture`, and `runDoctor` helpers.packages/basalt-ui/configs/oxlint-plugin.test.ts (1)
303-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fixture for the per-specifier type-only branch.
agentNoRawUseChathas two independent type-only escapes: the whole-declaration check (node.importKind === 'type', plugin Line 647) and the per-specifier check (specifier.importKind === 'type', plugin Line 652). This fixture exercises only the first. The second branch has no coverage.This file already applies that standard elsewhere, at Lines 321-327 and Lines 383-390, where a second fixture pins the other half of a two-branch matcher.
💚 Proposed additional test
it('does NOT flag a type-only import of UIMessage from the same module', () => { const { code, rules } = run(`import type { UIMessage } from '`@ai-sdk/react`'\n`, 'lib.ts') expect(code).toBe(0) expect(rules).not.toContain('agent-no-raw-usechat') }) + + // The matcher has TWO type-only escapes — the whole declaration (`importKind === 'type'` on the + // node) and the individual specifier; the fixture above only pins the first. + it('does NOT flag an inline per-specifier type-only useChat', () => { + const { code, rules } = run(`import { type useChat } from '`@ai-sdk/react`'\n`, 'lib.ts') + expect(code).toBe(0) + expect(rules).not.toContain('agent-no-raw-usechat') + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/basalt-ui/configs/oxlint-plugin.test.ts` around lines 303 - 307, Add a separate test fixture in the agentNoRawUseChat tests for a value import declaration containing a type-only UIMessage specifier, so it exercises specifier.importKind === 'type' rather than the whole-declaration importKind check. Assert the code remains valid and agent-no-raw-usechat is not reported, following the paired-fixture pattern used around the existing tests.packages/basalt-ui/configs/oxlint-plugin.js (1)
664-678: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a shared drift check for the duplicated
majorOfbehavior.
configs/oxlint-plugin.jsandsrc/cli/index.tseach keep their ownmajorOffor structural reasons, but no test pins both implementations against shared edge cases. A shared fixture or cross-copy assertion can catch missed parsing changes beforeoxlintanddoctordisagree on the same repository.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/basalt-ui/configs/oxlint-plugin.js` around lines 664 - 678, Add a shared test fixture or cross-copy assertion covering identical edge cases for the duplicated majorOf implementations in configs/oxlint-plugin.js and src/cli/index.ts. Ensure both copies are exercised against the same inputs and their results are asserted equal, without merging the implementations or changing the import-free structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/AGENT-CHAT-SPEC.md`:
- Line 6: Replace the `/tmp/hermes-synthesis.md` reference in the gap-analysis
source note with a repository-tracked path or durable external link; if neither
is available, remove the reference while preserving the surrounding
documentation.
In `@packages/basalt-ui/agent/rules/basalt-agent.md`:
- Around line 234-243: Update the earlier workspace ownership statement to
identify the Mantine chrome as shipping from the basalt-ui/agent-chat subpath
rather than only the root basalt-ui entry, keeping the documentation consistent
with the agent-chat section. Rename the “Ready-built UI” heading to “Prebuilt
UI” to satisfy the static-analysis requirement.
In `@packages/basalt-ui/configs/oxlint-plugin.js`:
- Around line 693-709: Update the BASALT_AI_MAJOR initialization to read the
declared ai version specifically from peerDependencies rather than using
aiMajorFromPkg’s dependencies/devDependencies fallback. Preserve the null result
when the package is unreadable or has no peer declaration, and keep the existing
peer-major naming and report wording consistent.
In `@packages/basalt-ui/llms.txt`:
- Around line 122-128: Update the version metadata in llms.txt from 1.9.0 to
1.10.0 in both the line-1 version declaration and the “Version: 1.9.0” field
near the package metadata, leaving the agent-chat documentation unchanged.
- Around line 122-128: Update the basalt-ui/agent-chat surface metadata to
represent remend and motion as required peers rather than optional, adding the
per-subpath required-peer field or list used by the project. Adjust SURFACES and
pack consumers to honor that required-peer data while preserving other optional
peers and ensuring the generated machine-readable surface map matches the
documented requirements.
In `@packages/basalt-ui/src/agent/use-agent-stream.test.tsx`:
- Around line 86-89: Move the result.current.status assertion into the existing
waitFor callback alongside the parts assertion, so waitFor retries until both
the expected final part and 'done' status are observed. Keep the current
expectations and streaming setup otherwise unchanged.
In `@packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx`:
- Around line 180-190: Update the async assertions in the use-agent-thread-runs
tests around start() and retry() so they yield to the stream() generator before
checking calls. Await a microtask or otherwise wait for the generator body after
each relevant start()/retry() invocation, including the assertions near the
first start, busy second start, and line 336, while preserving the existing
expected call sequences.
In `@packages/basalt-ui/src/agent/use-agent-thread-runs.wedge.test.tsx`:
- Around line 242-284: Replace the setTimeout-based delay inside the transport’s
resume() generator with a caller-controlled deferred gate, using the existing
deferred() helper pattern from sibling tests. Keep the first resume attempt
suspended through the render and hide/show cycle, then resolve the gate
immediately afterward before the final waitFor, preserving the assertion that
resumeCalls equals 2.
In `@packages/basalt-ui/src/cli/index.ts`:
- Around line 1591-1631: Update packages/basalt-ui/src/cli/index.ts:1591-1631 in
subdirNames to exclude node_modules and dot-directories from traversal. Update
packages/basalt-ui/src/cli/index.ts:1641-1650 in expandWorkspaceGlobs to remove
!-prefixed patterns from includes and subtract their matched directories from
the results. Add fixtures in packages/basalt-ui/tests/doctor.test.ts near the
existing packages/** case covering both exclusions.
- Around line 1641-1650: Update expandWorkspaceGlobs to recognize patterns
beginning with “!” as exclusions instead of passing them to
expandPatternSegments as literal path segments. Collect directories from
positive patterns, resolve matching directories for negated patterns, and
subtract those directories from the collected results while preserving
package.json filtering and existing positive-pattern behavior.
In `@packages/basalt-ui/tests/required-peers.test.ts`:
- Around line 4-13: Update the root-entry required-peer invariant in
required-peers.test.ts: change the documented required-peer count from six to
seven, add motion to RUNTIME_REQUIRED_PEERS, and revise the root-entry
requirement documentation to include motion alongside the existing peers. Keep
subpath optionality and the existing boundary checks unchanged.
---
Nitpick comments:
In `@apps/playground/src/demo/AgentChatSubpathDemoPage.tsx`:
- Line 75: Update the public AgentChatSubpathDemoPage component declaration to
include an explicit React JSX return type, preserving its existing
implementation and behavior.
In `@apps/playground/src/demo/AgentWedgeDemoPage.tsx`:
- Line 121: Update the exported AgentWedgeDemoPage component declaration with an
explicit React component return type, preserving its existing implementation and
behavior.
In `@packages/basalt-ui/configs/oxlint-plugin.js`:
- Around line 664-678: Add a shared test fixture or cross-copy assertion
covering identical edge cases for the duplicated majorOf implementations in
configs/oxlint-plugin.js and src/cli/index.ts. Ensure both copies are exercised
against the same inputs and their results are asserted equal, without merging
the implementations or changing the import-free structure.
In `@packages/basalt-ui/configs/oxlint-plugin.test.ts`:
- Around line 303-307: Add a separate test fixture in the agentNoRawUseChat
tests for a value import declaration containing a type-only UIMessage specifier,
so it exercises specifier.importKind === 'type' rather than the
whole-declaration importKind check. Assert the code remains valid and
agent-no-raw-usechat is not reported, following the paired-fixture pattern used
around the existing tests.
In `@packages/basalt-ui/scripts/pack-test.sh`:
- Around line 369-374: Extend the `agentChat` export assertions in the scratch
consumer to cover the three missing runtime exports declared by
`export-surface.json`: `Composer`, `ThreadOutcomeCard`, and
`threadPartRenderers`. Preserve the existing checks and validate each added
export against its expected runtime type.
In `@packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx`:
- Around line 348-356: Add a tracking issue link immediately next to the skipped
test declaration for consumeAndFinalize, referencing the issue that documents
the stop() versus slow resolveOutcome race and proposed post-await guard. Keep
the test skipped and do not implement the fix in this change.
In `@packages/basalt-ui/src/agent/use-agent-thread-runs.wedge.test.tsx`:
- Around line 69-138: Extract the duplicated createTestThreadsStore
implementation into packages/basalt-ui/src/agent/test-threads-store.ts,
preserving the explanatory rationale comment and adding an optional onCall hook
for finalize-order tracking. In
packages/basalt-ui/src/agent/use-agent-thread-runs.wedge.test.tsx lines 69-138,
replace the local implementation with an import; do the same in
packages/basalt-ui/src/agent/use-agent-thread-runs.resume.test.tsx lines 23-92.
In packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx lines 22-102,
import the shared helper and pass onCall to the finalize-order test.
In `@packages/basalt-ui/src/cli/index.ts`:
- Around line 1971-1984: Update the agreeing-majors branch near the existing
aiMajorSkewReason warning to consult presentButInvalid before checking
aiMajorSkewReason !== null. Emit the same malformed-value warning used by the
skew branch when presentButInvalid is true; otherwise preserve the existing
stale-exemption warning and pass behavior.
In `@packages/basalt-ui/tests/doctor.test.ts`:
- Around line 208-225: Hoist the duplicated runDoctor helper to module scope,
preserving its existing JSDoc and behavior, then remove both in-test copies
around the doctor test cases. Add a narrowly scoped no-console suppression for
the hoisted helper because it intentionally captures and restores console.log
and console.error.
- Around line 267-272: Extend the doctor tests around the existing no-workspaces
case to cover both remaining workspaces shapes: verify `{ packages: [...] }`
discovers package dependencies and reports the expected major mismatch, and
verify an unrecognized shape such as a string remains silent with exit code 0
instead of throwing. Use the existing `setupPassingLayout`, `writeFixture`, and
`runDoctor` helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 117a7574-0a47-4362-8e04-b8e788224d70
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.oxlintrc.jsonapps/playground/src/demo/AgentChatSubpathDemoPage.tsxapps/playground/src/demo/AgentWedgeDemoPage.tsxapps/playground/src/demo/nav-model.tsxapps/playground/src/routes/agent-chat-subpath.tsxapps/playground/src/routes/agent-wedge.tsxbunfig.tomldocs/AGENT-CHAT-SPEC.mdlefthook.ymlpackage.jsonpackages/basalt-ui/AGENTS.mdpackages/basalt-ui/CLAUDE.mdpackages/basalt-ui/README.mdpackages/basalt-ui/agent/rules/basalt-agent.mdpackages/basalt-ui/configs/oxlint-plugin.jspackages/basalt-ui/configs/oxlint-plugin.test.tspackages/basalt-ui/configs/oxlint.jsonpackages/basalt-ui/llms.txtpackages/basalt-ui/package.jsonpackages/basalt-ui/scripts/export-surface.jsonpackages/basalt-ui/scripts/pack-test.shpackages/basalt-ui/src/agent-chat/index.tspackages/basalt-ui/src/agent/ai-sdk-transport.test.tspackages/basalt-ui/src/agent/parts.test.tspackages/basalt-ui/src/agent/thread.test.tspackages/basalt-ui/src/agent/use-agent-stream.test.tsxpackages/basalt-ui/src/agent/use-agent-thread-runs.resume.test.tsxpackages/basalt-ui/src/agent/use-agent-thread-runs.test.tsxpackages/basalt-ui/src/agent/use-agent-thread-runs.tspackages/basalt-ui/src/agent/use-agent-thread-runs.wedge.test.tsxpackages/basalt-ui/src/charts/primitives/ChartFrame.test.tsxpackages/basalt-ui/src/cli/index.tspackages/basalt-ui/src/dashboard/stat-card.test.tsxpackages/basalt-ui/src/provider/index.test.tsxpackages/basalt-ui/src/surfaces.tspackages/basalt-ui/src/theme/use-basalt-spacing.test.tsxpackages/basalt-ui/src/tokens/build-fonts-css.test.tspackages/basalt-ui/tests/doctor.test.tspackages/basalt-ui/tests/lefthook-preset.test.tspackages/basalt-ui/tests/required-peers.test.tstests/setup/dom.ts
| ### Ready-built UI (`basalt-ui/agent-chat`, Mantine) | ||
|
|
||
| The Mantine-coupled chrome (`ThreadWorkspace` and the lower-level pieces below it) ships its own | ||
| subpath, `basalt-ui/agent-chat` — added in 1.10.0. The root `basalt-ui` entry still re-exports the | ||
| same components (nothing that imported from `basalt-ui` before 1.10.0 needs to change), but a | ||
| consumer who only needs the thread-chat UI can import from `./agent-chat` directly, without pulling | ||
| in `BasaltProvider`, `BasaltShell`, the dashboard composites, or `basalt-ui/connectivity`: | ||
|
|
||
| ```tsx | ||
| import { ThreadWorkspace } from 'basalt-ui' | ||
| import { ThreadWorkspace } from 'basalt-ui/agent-chat' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the agent-chat ownership documentation.
The earlier workspace section at Line 200-203 still says that the Mantine chrome ships from the root basalt-ui entry. This section now says that it ships from basalt-ui/agent-chat. Update the earlier statement so the guide has one ownership model.
Use Prebuilt instead of Ready-built. The static-analysis hint flags the current wording.
🧰 Tools
🪛 LanguageTool
[grammar] ~234-~234: Ensure spelling is correct
Context: ...Ready-built UI (basalt-ui/agent-chat, Mantine) The Mantine-coupled chrome (`ThreadWorks...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/basalt-ui/agent/rules/basalt-agent.md` around lines 234 - 243,
Update the earlier workspace ownership statement to identify the Mantine chrome
as shipping from the basalt-ui/agent-chat subpath rather than only the root
basalt-ui entry, keeping the documentation consistent with the agent-chat
section. Rename the “Ready-built UI” heading to “Prebuilt UI” to satisfy the
static-analysis requirement.
Source: Linters/SAST tools
| await waitFor(() => { | ||
| expect(result.current.parts).toEqual([{ type: 'text', text: 'second-1' }]) | ||
| }) | ||
| expect(result.current.status).toBe('done') |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Move the status assertion inside waitFor to remove a race.
waitFor returns as soon as parts equals [{ type: 'text', text: 'second-1' }]. The hook sets that value while the second generator is still running, before the loop ends and the status becomes 'done'. The assertion on line 89 therefore reads the status at an unspecified point and can observe 'streaming'.
The same pattern is safe on line 44, because the gate keeps that stream open. Here nothing blocks completion.
💚 Proposed fix
await waitFor(() => {
expect(result.current.parts).toEqual([{ type: 'text', text: 'second-1' }])
+ expect(result.current.status).toBe('done')
})
- expect(result.current.status).toBe('done')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await waitFor(() => { | |
| expect(result.current.parts).toEqual([{ type: 'text', text: 'second-1' }]) | |
| }) | |
| expect(result.current.status).toBe('done') | |
| await waitFor(() => { | |
| expect(result.current.parts).toEqual([{ type: 'text', text: 'second-1' }]) | |
| expect(result.current.status).toBe('done') | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/basalt-ui/src/agent/use-agent-stream.test.tsx` around lines 86 - 89,
Move the result.current.status assertion into the existing waitFor callback
alongside the parts assertion, so waitFor retries until both the expected final
part and 'done' status are observed. Keep the current expectations and streaming
setup otherwise unchanged.
| act(() => { | ||
| result.current.start(threadId, 'first') | ||
| }) | ||
| expect(calls).toEqual(['first']) | ||
|
|
||
| // Busy: the SAME thread already has a controller. This must no-op — no second stream() call, | ||
| // and (critically) must NOT clobber the cached input retry() will replay. | ||
| act(() => { | ||
| result.current.start(threadId, 'second') | ||
| }) | ||
| expect(calls).toEqual(['first']) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare synchronous vs awaited act() usage around transport call-count assertions.
set -euo pipefail
fd -e tsx -e ts . packages/basalt-ui/src --exec rg -n -C4 'act\(\(\) => \{' {} \; | rg -n -C4 'calls' || true
# Show every awaited act() for contrast.
rg -n -C2 'await act\(' packages/basalt-ui/src || trueRepository: jkrumm/basalt-ui
Length of output: 4473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx"
impl="packages/basalt-ui/src/agent/use-agent-thread-runs.ts"
echo "== test file outline/size =="
wc -l "$target"
echo
echo "== relevant test sections =="
sed -n '150,210p' "$target" | nl -ba -v150
echo
sed -n '300,345p' "$target" | nl -ba -v300
echo
echo "== implementation start/call locations =="
rg -n "stream\\(|consumeAndFinalize|start\\(" "$impl" "$target"
echo
echo "== implementation relevant sections =="
sed -n '1,220p' "$impl" | nl -ba -v1Repository: jkrumm/basalt-ui
Length of output: 318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx"
impl="packages/basalt-ui/src/agent/use-agent-thread-runs.ts"
echo "== test file size =="
wc -l "$target" "$impl"
echo
echo "== relevant test sections =="
python3 - <<'PY'
from pathlib import Path
for path, ranges in [("packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx", [(150,210),(300,345)])]:
print(f"\n-- {path} --")
lines = Path(path).read_text().splitlines()
for start,end in ranges:
print(f"\n## {start}-{end}")
for i in range(max(1,start-1), min(end, len(lines))):
print(f"{i+1}: {lines[i]}")
PY
echo
echo "== source references =="
rg -n "stream\(|consumeAndFinalize|start\(" "$impl" "$target" || true
echo
echo "== implementation relevant sections =="
python3 - <<'PY'
from pathlib import Path
path="packages/basalt-ui/src/agent/use-agent-thread-runs.ts"
lines=Path(path).read_text().splitlines()
show=True
for i,line in enumerate(lines, start=1):
if "consumeAndFinalize" in line or "start(" in line or "stream(" in line or i < 180 or i > 180 and i <= 260:
print(f"{i}: {line}")
PY
echo
echo "== microtask ordering probe (standalone, no repo code executed) =="
node - <<'JS'
const calls = []
async function yieldStream() {
calls.push('next');
}
function start() {
void (async function consumed() {
await yieldStream();
})();
}
start();
console.log("sync-first-sink", JSON.stringify(calls));
await Promise.resolve();
console.log("microtask-flush", JSON.stringify(calls));
JSRepository: jkrumm/basalt-ui
Length of output: 21504
Make the calls assertions wait for the async stream() generator.
start() non-awaited invokes consumeAndFinalize, whose first generator yield can happen in a microtask. The synchronous expect(calls) assertions at lines 183, 190, and 336 can therefore pass before the transport side effect runs. Await the microtask between start()/retry() and the calls expectations, or wait for the generator body directly.
[low_effort và high_reward]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/basalt-ui/src/agent/use-agent-thread-runs.test.tsx` around lines 180
- 190, Update the async assertions in the use-agent-thread-runs tests around
start() and retry() so they yield to the stream() generator before checking
calls. Await a microtask or otherwise wait for the generator body after each
relevant start()/retry() invocation, including the assertions near the first
start, busy second start, and line 336, while preserving the existing expected
call sequences.
…r test The Mantine chrome over ./agent was built and shipped but reachable only through the root barrel, so taking the transcript meant taking BasaltProvider, the shell, the dashboard composites and ./connectivity along with it. Importing ThreadTranscript from basalt-ui/agent-chat now costs none of those. The root barrel's re-export is unchanged: this adds a door without closing one. What the subpath does NOT shed is remend and motion. Both are reached through static top-level imports — thread-message.tsx pulls content/markdown, and thread-feed.tsx and thread-detail-panel.tsx pull motion/react — so under unbundled ESM, importing anything from ./agent-chat evaluates them and the subpath fails to resolve without either. They are optional peers in the manifest because npm expresses optionality per package and never per subpath; the description string, the README and a test carry the truth instead. The dist gate could not have caught that. Its scratch consumer installs every optional peer at once, so a peer that is secretly required resolves fine. There is now a second, deliberately minimal install that takes only what ./agent-chat truly needs, in the same shape as the existing charts/tokens-only step. Behind the door the layer is finally verifiable. src/agent/** had zero tests; it now pins the abort/supersede/resume/finalize lifecycle, the threads store's ring buffers and resume-token deletion, the part parser, and the AI SDK transport's snapshot-to-delta diffing. Which surfaced F3, and it is not the dev-only bug the register called it. useAgentThreadRuns' unmount cleanup aborted every controller but never cleared the map, so the mount reconcile then saw has(id) as true, computed orphaned as false, and skipped — leaving the thread wedged in 'streaming' with no consumer, permanently. React reuses the fiber under StrictMode's doubleInvokeEffectsOnFiber, and React 19.2's Activity does the same thing in production: effects destroyed and re-created, refs preserved. The fix is controllersRef.current.clear(), matching what stopAll() already did. Reverting that one line fails two of the wedge file's three cases and nothing else in the suite, which is the property that makes it a guard rather than a decoration. The test drives Activity directly rather than through Mantine's Collapse. The version pinned here has no keepMounted default, so a bare Collapse keeps its children mounted and cannot wedge — but a consumer writing Collapse with keepMounted does get the Activity path on this same version, so the hazard is live here and not only on later Mantine. Three guards ship at warn, error repo-local, per the grace minor. agent-resume-guard flags useChat with resume true, and bare resumeStream calls, where nothing owns single-consumer discipline. agent-no-raw-usechat routes consumers onto useAgentStream and useAgentThreadRuns, which abort on unmount; it reports the import specifier, so pulling types from the same module stays clean. ai-sdk-major flags a declared ai major that disagrees with the peer basalt declares. All three honour basalt-agent-allow, deliberately not theme-allow — a colour exemption must never be able to switch off a streaming guard. ai-sdk-major cannot catch the case it was written for on its own: a lint run resolves the nearest package.json, so a workspace streaming on ai 5 into a client parsing it on ai 7 looks fine from inside either package. doctor gains a third axis that walks every workspace manifest and fails on the skew. That skew is sometimes correct, though, and a guard that permanently fails a correct configuration gets switched off. So the intentional case is declarable through the existing basalt config block, and the declaration must carry a written reason. An undeclared skew still fails. A declared one passes with both the skew and the reason echoed. A declaration left behind once the skew is gone warns that it is stale. The complaint this guard answers was that nothing pins the pairing, and a required reason is that pin. raw-scroll-container moves from off to warn in the shipped preset. The root Requirements table now says remend is a root-entry requirement rather than a ./content one, pinned by a test that fails when that import goes lazy, so the claim cannot rot quietly ahead of the change that makes it false.
…yground Two pages, each standing in for a claim the release makes that a test cannot observe. The subpath page imports only from basalt-ui/agent-chat and basalt-ui/agent — no root barrel, no BasaltProvider — so its module graph is the evidence that taking the transcript no longer costs the whole framework. A header comment says so, because one casual root-barrel import would destroy the proof while leaving the page working. The wedge page mounts a persisted streaming thread that resolves instead of hanging. That is the visible form of the F3 fix, and it is worth having next to the test: a thread stuck in 'streaming' with no consumer looks identical to a slow one until you wait forever.
The API specification for 1.10.0 through 1.13.0 — every signature, the type-level invariants, the guard authoring, and the test plan. It was written before this release and has been carried untracked since; committing it here means the phases that follow have a fixed reference rather than a file in someone's working tree. Two claims in it were corrected against source while implementing this release and are worth knowing before reading it: the proposed ./agent-chat description advertised ToolChip and ThreadFeedRow, which do not exist until 1.11.0 and 1.13.0 and would have published a surface listing exports the tarball does not carry; and its closing server-side section still needs reconciling against the program spec, which wins on any conflict. The ladder shifted by one after the spec was written. 1.9.0 released from master carrying only the chart-layer batch while this work sat on its branch, so the door lands as 1.10.0 and every phase after it moves up one.
|
Verified all ten findings against source before acting. Seven applied, three refuted: Applied — the Refuted, with evidence:
Two notes on severity. Gates green after the fixes: |
a387cfa to
573578c
Compare
1 participant
Opens
./agent-chatas its own subpath, stands up the DOM test harness the repo never had, puts the agent layer under test for the first time, and ships the streaming/resume guards.What lands
./agent-chatsubpath export + the matchingSURFACESentry, withllms.txtandAGENTS.mdregenerated. The root barrel's re-export is unchanged, so this adds a door without closing one.tests/setup/dom.ts+ rootbunfig.toml).src/agent/**andsrc/agent-chat/**had zero tests before this and there was nothing to add them to.src/agent/**suite — parts, thread, ai-sdk-transport, use-agent-stream, and the threeuse-agent-thread-runsfiles.useAgentThreadRuns's unmount cleanup now clearscontrollersRef, so a thread persisted asstreamingresolves on the next mount instead of hanging forever with no consumer behind it. Reproduces under React 19 StrictMode and under an<Activity>hide/show boundary, both of which preserve the fiber's refs across effect teardown.agent-resume-guard,agent-no-raw-usechat,ai-sdk-major— atwarnshipped /errorrepo-local, per the grace-minor doctrine.raw-scroll-containerpromoted towarn.ai-major-parityindoctor, declarable throughbasalt.aiMajorSkewReasonwith a mandatory reason string. Undeclared skew still hard-fails; a declaration left behind after the skew is gone warns that it is stale.Notes for review
remendandmotionare hard requirements of the new subpath, not optional peers.agent-chat/index.tsstatically re-exportsThreadTranscript→thread-message.tsx→../content/markdown, whoseimport remend from 'remend'is top-level, so under unbundled ESM importing anything frombasalt-ui/agent-chatevaluates that chain. This is documented, pinned inrequired-peers.test.ts, and gated by a new minimal-peer step inpack-test.sh— the existing scratch consumer installs every optional peer at once, so it structurally could not detect a peer that is secretly required. Makingremendlazy is queued for a later minor.happy-dom replaces
TransformStreamandWritableStreamwith Node classic stream classes — same names, different semantics — which breaks everyaistreaming path with an error that names neither cause.tests/setup/dom.tsrestores the natives fromnode:stream/web, includingReadableStreamand theAbortControllerfamily, because restoring a family of globals by halves is its own bug.The four-commit split is forced by lefthook's
isolated-basalt-uihook and cannot be collapsed.Version: this was written as 1.9.0. 1.9.0 released from
mastercarrying only the chart-layer batch while this sat on its branch, so it lands as 1.10.0 and the documented ladder after it moves up one. The version references inside the commits were corrected rather than left to publish a false claim.Gates
fmt:check,lint,typecheck,build,check-theme,check-coverage(8/8), both generator drift checks, andbun testall green.pack-test.shgreen, includingresolved basalt-ui/agent-chatand the newagent-chat minimal-peer resolutionstep — the only evidence the new door resolves from the published tarball, since the playground exercisessrc/and neverdist/.Browser gate walked on
/agent-chat-subpathand/agent-wedge: the subpath renders a transcript with no provider or shell in its import graph, and a seeded stuck thread resolves both on an explicit reconcile and on a plain page reload.One
skipis deliberate — a verified-failing reproduction ofconsumeAndFinalizeoverwriting astop()that lands whileresolveOutcomeis awaited. It is owned by the next phase, which rewritesstop().Summary by CodeRabbit
basalt-ui/agent-chatexport for thread-based chat interfaces.