feat(ai): add local RAG vector store and interactive RAGChat widget#779
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds a local retrieval-augmented generation (RAG) system to TermUI by extending the AIAdapter with embeddings, implementing a local vector store with document chunking and cosine similarity search, and creating an interactive RAGChat terminal widget that indexes documents, handles queries, and streams AI responses. ChangesRAG Vector Store and Chat Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/adapters/src/commander/index.ts (1)
14-14: ⚡ Quick winAdd an inline justification for the type assertion on Line 14.
as Tcurrently violates the repo rule requiring an inline explanation for type assertions.Suggested patch
- options: program.opts() as T, + options: program.opts() as T, // Commander returns an untyped options bag; caller-provided generic `T` defines the expected shape.As per coding guidelines, "No type assertions without an inline comment explaining why."
🤖 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/adapters/src/commander/index.ts` at line 14, The type assertion on the options assignment (options: program.opts() as T) needs an inline comment explaining why the assertion is safe; update the line to keep the assertion but add a brief justification comment after it (e.g., stating that commander validates/returns the expected shape or that upstream type inference cannot express the generic T but runtime checks cover it), referencing the exact expression "options: program.opts() as T" so future readers know why the assertion is allowed.packages/ui/src/RAGChat.ts (1)
98-133: ⚡ Quick winRemove
console.errorfrom library source code.Line 124 logs errors directly to the console. Library code should propagate errors to the caller (e.g., via a callback or event) so consumers control logging behavior.
As per coding guidelines:
packages/**/src/**/*.{ts,tsx}: Noconsole.login source files.🤖 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/ui/src/RAGChat.ts` around lines 98 - 133, The catch block in _submitQuery currently calls console.error('RAG query failed:', error) which violates the no-console rule; remove that console call and instead propagate the error to the consumer by invoking a dedicated error-handling hook (e.g., call this._handleError(error) or emit an 'error' event via this.emit('error', error)) and keep the existing push of an assistant error message; update or add the handler (e.g., _handleError or an EventEmitter hookup) so callers can subscribe and control logging behavior and ensure _submitQuery still sets _loading = false and markDirty() in finally.packages/adapters/src/ai/vectorStore.ts (1)
98-150: ⚡ Quick winRemove
console.errorcalls from library source code.Lines 90 and 124 use
console.errorto log initialization and query failures. Library code should avoid writing directly to console; instead, propagate errors to the caller or emit events/callbacks so consumers can handle logging.As per coding guidelines:
packages/**/src/**/*.{ts,tsx}: Noconsole.login source files.🤖 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/adapters/src/ai/vectorStore.ts` around lines 98 - 150, Remove any console.error calls in LocalVectorStore (e.g., in the constructor, load(), query() or any other methods) and stop swallowing errors there; instead either rethrow the caught error or let promise rejections propagate to the caller so callers can handle logging. Concretely, in load() keep the ENOENT branch that sets this._documents = [], but for other errors remove console.error and throw the error (or do not catch it). In query() and addDocuments() remove any console.error around AI calls and allow embed() failures to surface (or emit an error via an EventEmitter you add to LocalVectorStore) so consumers control logging. Ensure no console.* remains in LocalVectorStore, and preserve existing behavior for ENOENT only.
🤖 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 `@packages/adapters/src/ai/index.ts`:
- Around line 118-129: The embed method currently returns an empty array when
OpenAI's response lacks an embedding, which yields a zero-magnitude vector and
corrupts cosineSimilarity; modify the embed function (the async embed(text:
string) implementation for provider === 'openai') to detect missing
response.data[0]?.embedding and throw a descriptive Error (or rethrow the
provider error) instead of returning [] so callers can surface the failure
rather than treat it as a valid zero vector.
In `@packages/adapters/src/ai/vectorStore.ts`:
- Around line 74-96: The for-loop inside indexDirectory is shadowing the
chunkText function by using the loop variable name `chunkText`; rename that loop
variable (e.g., to `chunk`, `chunkStr`, or `chunkTextSlice`) and update its
usage in the docs.push call so it no longer conflicts with the `chunkText`
function name defined earlier, leaving the rest of indexDirectory and the call
to store.addDocuments(ai) unchanged.
In `@packages/ui/src/RAGChat.ts`:
- Around line 45-96: The constructor uses a type coercion for borderClr when
assigning this._borderColor (see RAGChat constructor and the borderClr ->
this._borderColor code) — replace the bare "as any" with a short inline comment
explaining why the runtime value cannot be typed more precisely (e.g., it can be
a named color string or a Style['fg'] object from upstream libs) and keep the
cast only with that justification; and in _initIndex replace the silent
console.error('Failed to initialize RAG index:', error) with error propagation
(either rethrow the caught error or rethrow after logging) so callers can handle
initialization failures instead of swallowing them (modify _initIndex's catch to
rethrow or remove console.error and throw the original error).
- Around line 187-293: In _renderSelf add short inline comments next to every
"as any" explaining why the cast is required and why it is safe: annotate the ba
assignment (ba = { ... } as any) to state the border color union mismatch and
why runtime value is valid; annotate fgColor (fgColor = ... as any) to explain
the intended CellAttr shape for user messages; annotate the two places in the
indexing/loading screen.writeString calls where fg is cast as any to say the
color literal is intentionally used despite TypeScript mismatch; keep each
comment one short sentence referencing the variable (ba, fgColor,
indexing/loading writeString fg) and the reason (third‑party type mismatch or
known runtime shape).
---
Nitpick comments:
In `@packages/adapters/src/ai/vectorStore.ts`:
- Around line 98-150: Remove any console.error calls in LocalVectorStore (e.g.,
in the constructor, load(), query() or any other methods) and stop swallowing
errors there; instead either rethrow the caught error or let promise rejections
propagate to the caller so callers can handle logging. Concretely, in load()
keep the ENOENT branch that sets this._documents = [], but for other errors
remove console.error and throw the error (or do not catch it). In query() and
addDocuments() remove any console.error around AI calls and allow embed()
failures to surface (or emit an error via an EventEmitter you add to
LocalVectorStore) so consumers control logging. Ensure no console.* remains in
LocalVectorStore, and preserve existing behavior for ENOENT only.
In `@packages/adapters/src/commander/index.ts`:
- Line 14: The type assertion on the options assignment (options: program.opts()
as T) needs an inline comment explaining why the assertion is safe; update the
line to keep the assertion but add a brief justification comment after it (e.g.,
stating that commander validates/returns the expected shape or that upstream
type inference cannot express the generic T but runtime checks cover it),
referencing the exact expression "options: program.opts() as T" so future
readers know why the assertion is allowed.
In `@packages/ui/src/RAGChat.ts`:
- Around line 98-133: The catch block in _submitQuery currently calls
console.error('RAG query failed:', error) which violates the no-console rule;
remove that console call and instead propagate the error to the consumer by
invoking a dedicated error-handling hook (e.g., call this._handleError(error) or
emit an 'error' event via this.emit('error', error)) and keep the existing push
of an assistant error message; update or add the handler (e.g., _handleError or
an EventEmitter hookup) so callers can subscribe and control logging behavior
and ensure _submitQuery still sets _loading = false and markDirty() in finally.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f3111de-6329-41bf-ab60-dfae7340e8a1
📒 Files selected for processing (9)
packages/adapters/src/ai/index.tspackages/adapters/src/ai/vectorStore.test.tspackages/adapters/src/ai/vectorStore.tspackages/adapters/src/commander/index.tspackages/adapters/src/index.tspackages/ui/package.jsonpackages/ui/src/RAGChat.test.tspackages/ui/src/RAGChat.tspackages/ui/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ui/src/RAGChat.ts (1)
87-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an inline justification for the event emitter
as anycast.Line 92 uses
as anywithout an inline rationale. Add a short inline comment (or a typed wrapper) to justify why this cast is required.Suggested minimal fix
- (this.events as any).emit('error', err); + // Cast needed: Widget event typings do not include custom 'error', but runtime emitter supports it. + (this.events as any).emit('error', err);As per coding guidelines:
**/*.{ts,tsx}: Use TypeScript strict mode. Noanywithout an inline comment explaining why. No type assertions without an inline comment explaining why.🤖 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/ui/src/RAGChat.ts` around lines 87 - 93, In _handleError, the cast (this.events as any) is used to access emit but lacks justification; either add a short inline comment explaining why a typed cast is necessary (e.g., events uses a dynamic/emitter API not expressible in current Events type) or introduce a small typed wrapper/override that exposes emit (e.g., a local variable typed as EventEmitter-like) and use that instead; update references in _handleError to use the justified cast or wrapper and mention the matching reason and link to the underlying type limitation (keep comment concise and adjacent to the cast/wrapper).
🤖 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 `@packages/ui/src/RAGChat.test.ts`:
- Line 145: Add short inline comments next to each use of `any`/`as any` in
RAGChat.test.ts to justify the cast: explain that the test is wiring a
lightweight mock that doesn't implement the full EventEmitter/interface so `} as
any` (around the mocked props) is used to bypass strict typing, that `store:
any` is a minimal partial store used only for test setup, and that `('error' as
any, ...)`/similar casts are to satisfy the event API signature on a mocked
emitter rather than the real typed EventEmitter; place each comment directly
after the cast (near the symbols in the file) so future readers know these are
deliberate test-only loosenings.
---
Duplicate comments:
In `@packages/ui/src/RAGChat.ts`:
- Around line 87-93: In _handleError, the cast (this.events as any) is used to
access emit but lacks justification; either add a short inline comment
explaining why a typed cast is necessary (e.g., events uses a dynamic/emitter
API not expressible in current Events type) or introduce a small typed
wrapper/override that exposes emit (e.g., a local variable typed as
EventEmitter-like) and use that instead; update references in _handleError to
use the justified cast or wrapper and mention the matching reason and link to
the underlying type limitation (keep comment concise and adjacent to the
cast/wrapper).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0de60ec6-6a7c-4e25-a249-97b81f1891a2
📒 Files selected for processing (5)
packages/adapters/src/ai/index.tspackages/adapters/src/ai/vectorStore.tspackages/adapters/src/commander/index.tspackages/ui/src/RAGChat.test.tspackages/ui/src/RAGChat.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/adapters/src/commander/index.ts
- packages/adapters/src/ai/vectorStore.ts
|
Hi @Tejas9406, The RAG vector store concept is interesting, but there are two architectural issues that must be resolved before this can be approved: Breaking change to The fix is to make it optional: Dependency inversion: Move Please address both issues and push a new commit. |
|
Hi @Tejas9406, thanks for iterating on this. CI passes and the test coverage looks solid. However, the two architectural blockers from the earlier review are still present. Please address both before requesting another review. Issue 1: embed() is a required method on AIAdapter The interface still reads: embed(text: string): Promise<number[]> This breaks every existing implementation of AIAdapter across the codebase and in downstream consumer code. TypeScript will error for anyone who already implements the interface. Fix: make it optional or extract it into a separate interface. Option A (minimal change): embed?: (text: string) => Promise<number[]> Then update LocalVectorStore.addDocuments and LocalVectorStore.query to throw a descriptive error when embed is undefined rather than calling it blindly. Option B (cleaner): define a separate EmbeddingAdapter interface that extends AIAdapter and add embed there. LocalVectorStore accepts EmbeddingAdapter instead of AIAdapter. Issue 2: packages/ui depends on packages/adapters packages/ui/package.json still contains: "@termuijs/adapters": "workspace:*" This inverts the package layering. The established dependency graph flows ui -> core/jsx/widgets. Adapters is a leaf package. If ui imports from adapters and adapters ever needs something from ui, the repo gets a circular dependency that breaks the build. Fix: move RAGChat.ts and RAGChat.test.ts out of packages/ui. Place them in packages/adapters/src/ai/ (or a new packages/ai/ package if preferred). Export from the adapters barrel. Remove the @termuijs/adapters dependency from packages/ui/package.json. Minor items (fix alongside the above):
The concept and test suite are solid. Once the two architectural issues are resolved, this is close to mergeable. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/adapters/src/ai/RAGChat.ts (1)
1-1:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Dependency inversion—adapters should not import from widgets.
RAGChatextendsWidgetfrom@termuijs/widgets, which meanspackages/adaptersnow depends onpackages/widgets. Adapters are intended to be low-level, thin wrappers around external libraries; widgets are higher-level UI components. This inverts the expected package layering.The PR objectives mention the reviewer suggested either moving
RAGChattopackages/adaptersor creating a new@termuijs/aipackage. Moving it to adapters solves thepackages/ui→packages/adaptersdependency issue but introduces a newpackages/adapters→packages/widgetsdependency.Recommended fix: Create a new
@termuijs/aipackage (or moveRAGChattopackages/uiand address the original dependency issue differently). Adapters should remain dependency-light.Based on learnings: "Follow
src/commander/index.tsas the reference shape for adapters: a thin function that takes the external object and returns TermUI-friendly data"—adapters should not extend Widget or depend on the widgets package.🤖 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/adapters/src/ai/RAGChat.ts` at line 1, RAGChat currently extends Widget (importing Widget from '`@termuijs/widgets`'), which creates an unwanted packages/adapters → packages/widgets dependency; refactor RAGChat into a thin adapter function (following the shape of src/commander/index.ts) that accepts the external AI object and returns TermUI-friendly data rather than extending Widget, or move the component into a new package `@termuijs/ai` (or into packages/ui) so adapters remain dependency-light; update any exports and references to use the new adapter function name (RAGChat adapter) or the new package, and remove the import of Widget from RAGChat to eliminate the inverse dependency.Source: Learnings
🧹 Nitpick comments (1)
packages/adapters/src/ai/RAGChat.test.ts (1)
73-88: ⚡ Quick winConsider asserting rendered output, not just mock calls.
The test verifies that
loadandsavewere called, but doesn't verify what was actually rendered to the screen. While this isn't a placeholder test, asserting onscreen.getCell()or similar would better test observable behavior.💡 Example enhanced assertion
expect(mockVectorStore.load).toHaveBeenCalled(); expect(mockVectorStore.save).toHaveBeenCalled(); + // Verify placeholder is rendered when not focused + const cell = screen.getCell(2, inputY); + expect(cell?.char).toBeTruthy();🤖 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/adapters/src/ai/RAGChat.test.ts` around lines 73 - 88, The test currently only asserts that mockVectorStore.load/save were called; instead assert rendered output from the Screen after RAGChat.render to verify visible UI. After calling chat.render(screen) and awaiting awaitIndex(mockVectorStore), use Screen methods (e.g., screen.getCell or screen.getRow/getText) to check that the chat history area and input area contain expected markers/labels or placeholder text produced by RAGChat (look for strings rendered by RAGChat such as the chat panel title, input prompt, or column separators). Keep the existing assertions for mockVectorStore.load/save but add assertions that screen.getCell/row/text at the coordinates RAGChat uses contains the expected characters so the test verifies observable rendering as well.
🤖 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 `@packages/adapters/src/ai/index.ts`:
- Around line 32-33: Add an inline comment explaining why the "as any" cast is
required on the dynamic require result: annotate the declaration "const mod =
req('openai') as any" with a short comment such as "Cast needed:
createRequire/require returns an opaque module whose shape varies by export
style (CJS vs ESM), so we treat it as any to access exported members safely."
This satisfies the ESLint rule justification and documents why we cannot use a
stricter type for mod.
- Around line 48-49: Add a brief inline comment explaining why the
eslint-disable and the `as any` cast are required for the dynamic require of the
Anthropic SDK: annotate the line with `const mod = req('`@anthropic-ai/sdk`') as
any` to state that the module has no TypeScript typings (or typings are
incompatible) and explain that a dynamic runtime require is used to avoid
conditional import/side-effects, so a temporary `any` cast is intentional until
proper typings are added; reference the `mod` variable and the
`req('`@anthropic-ai/sdk`')` call so reviewers can see the rationale.
---
Outside diff comments:
In `@packages/adapters/src/ai/RAGChat.ts`:
- Line 1: RAGChat currently extends Widget (importing Widget from
'`@termuijs/widgets`'), which creates an unwanted packages/adapters →
packages/widgets dependency; refactor RAGChat into a thin adapter function
(following the shape of src/commander/index.ts) that accepts the external AI
object and returns TermUI-friendly data rather than extending Widget, or move
the component into a new package `@termuijs/ai` (or into packages/ui) so adapters
remain dependency-light; update any exports and references to use the new
adapter function name (RAGChat adapter) or the new package, and remove the
import of Widget from RAGChat to eliminate the inverse dependency.
---
Nitpick comments:
In `@packages/adapters/src/ai/RAGChat.test.ts`:
- Around line 73-88: The test currently only asserts that
mockVectorStore.load/save were called; instead assert rendered output from the
Screen after RAGChat.render to verify visible UI. After calling
chat.render(screen) and awaiting awaitIndex(mockVectorStore), use Screen methods
(e.g., screen.getCell or screen.getRow/getText) to check that the chat history
area and input area contain expected markers/labels or placeholder text produced
by RAGChat (look for strings rendered by RAGChat such as the chat panel title,
input prompt, or column separators). Keep the existing assertions for
mockVectorStore.load/save but add assertions that screen.getCell/row/text at the
coordinates RAGChat uses contains the expected characters so the test verifies
observable rendering as well.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 283916eb-25a6-446c-b4bc-0b1df8b98cd1
📒 Files selected for processing (8)
packages/adapters/package.jsonpackages/adapters/src/ai/RAGChat.test.tspackages/adapters/src/ai/RAGChat.tspackages/adapters/src/ai/index.tspackages/adapters/src/ai/vectorStore.tspackages/adapters/src/commander/index.tspackages/adapters/src/index.tspackages/ui/src/index.ts
💤 Files with no reviewable changes (1)
- packages/ui/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/adapters/src/commander/index.ts
- packages/adapters/src/ai/vectorStore.ts
|
Hell @Karanjot786 , Sorry for the delay—I am currently quite busy with my ongoing exams. I have resolved the requested issues and pushed the updates. Please take a look when you get a chance. Thanks! |
|
Hi @Tejas9406, thanks for the updates. Making Issue 2 is still present in a different form. Here is what remains: Issue 2:
|
Karanjot786
left a comment
There was a problem hiding this comment.
Re-review: CI is green and the implementation looks comprehensive with tests. However, this adds a new AI/RAG feature to @termuijs/adapters which is a significant scope addition. Two questions:
- Is there a linked issue approving this feature direction?
- The
LocalVectorStoreuses cosine similarity with in-memory vectors — is this the intended approach for a terminal UI framework?
The code quality itself is good (proper types, tests, node: prefixed imports). Waiting on direction confirmation before approving.
|
Hello @Karanjot786 , Thank you for the re-review,To answer your questions: 2.Regarding the in-memory LocalVectorStore: Yes, this is the intended approach for a Terminal UI framework. |
|
Thanks for the contribution! Reviewed the RAGChat widget + LocalVectorStore. Several issues need to be fixed before this can merge. BLOCKING1. Constructor signature breaks widget conventions // Current — style first, opts optional:
constructor(style: Partial<Style> = {}, opts?: RAGChatOptions)But constructor(opts: RAGChatOptions)Callers who need custom style can include it in 2. Adding Per
MINOR3. Polling loop in const awaitIndex = async (store: MockVectorStore) => {
await new Promise<void>(resolve => {
const timer = setInterval(() => {
if (store.save.mock.calls.length > 0) {Polling with 4. No linked issue The PR body does not link a GitHub issue with Please fix the constructor API and remove the new deps (or get explicit maintainer approval for them on the issue) before re-requesting review. |
…checks, type assertions, and error hooks
…inversion and make AIAdapter.embed optional
…ui using abstract interfaces
ebd62ef to
40d7316
Compare
Description
This PR implements a lightweight, local, zero-dependency RAG (Retrieval-Augmented Generation) system. It introduces a pure-TypeScript vector database (
LocalVectorStore) utilizing in-memory cosine similarity search, an automated directory indexer (indexDirectory) that chunks markdown and text files, and an interactive TUI<RAGChat>assistant widget supporting scrollable histories, multiline query input, and real-time streaming answers.Related Issue
Closes #777
Which package(s)?
@termuijs/adapters, @termuijs/ui
Type of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/666f63ca-ed6a-4d0f-8ac8-1518b06ec839Screenshots / Recordings (UI changes)
(Terminal screenshots / recordings can be dropped here if needed)
Notes for the Reviewer
@termuijs/adaptersand@termuijs/uipass.packages/adapters/src/commander/index.tsso the workspace TypeScript declarations compile cleanly.Summary by CodeRabbit
Release Notes