feat(app-settings): per‑vibe Libraries chooser + scoped prompt usage #229
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for fireproof-ai-builder ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
- Duplicated validation logic for allowed dependency names appears in multiple places; prefer the single exported
ALLOWED_DEPENDENCY_NAMESto avoid drift. - Persisting dependency arrays without de-duping or canonical order can cause noisy doc writes; normalize before saving.
- The prompt builder falls back to defaults even when an explicit empty array is provided, which may contradict user intent unless the UI enforces a minimum selection.
- Settings page shows a success message but immediately navigates away, making the feedback invisible; either delay navigation or remove the message.
Additional notes (2)
-
Maintainability |
app/hooks/useSystemPromptManager.ts:28-31
Merging the entirevibeDocinto the system prompt input can unintentionally override settings fields if future vibe attributes collide (e.g.,stylePromptoruserPromptadded to the vibe schema). SincemakeBaseSystemPromptonly needsdependenciesfrom the vibe, passing just that field is safer and clearer. -
Maintainability |
app/prompts.ts:69-72
You’re rebuilding the allowed-name set locally. SinceALLOWED_DEPENDENCY_NAMESis exported from the catalog, reusing it avoids duplication and keeps a single source of truth across the app.
Summary of changes
- ResultPreview now passes per‑vibe dependency settings to AppSettingsView by adding
selectedDependenciesandonUpdateDependenciesprops and pullsvibeDoc/updateDependenciesfromuseSession. - useSession adds
updateDependencies(deps: string[])that validates and persists dependency names on the vibe document. - useSimpleChat passes
vibeDocintouseSystemPromptManager. - useSystemPromptManager merges
vibeDocinto the prompt-building input somakeBaseSystemPromptcan readdependencies. - Introduced
app/llms/catalog.tswith eager JSON loading,llmsCatalog,ALLOWED_DEPENDENCY_NAMES, andDEFAULT_DEPENDENCIES. - prompts.ts refactors dependency selection to be deterministic based on
sessionDoc.dependencieswith a fallback toDEFAULT_DEPENDENCIES; removes AI/history-based selection; adapts import generation and preloading tollmsCatalog. - Settings page removes Libraries chooser; adds save success/error inline feedback.
- VibeDocument type extended with optional
dependencies?: string[]. - Added tests for per‑vibe App Settings UI and prompt dependency integration; updated prompt builder tests to use default dependencies.
Comment on lines
140
to
161
| const updateDependencies = useCallback( | ||
| async (deps: string[]) => { | ||
| const allowed = new Set(llmsCatalog.map((l) => l.name)); | ||
| const validDeps = (Array.isArray(deps) ? deps : []) | ||
| .filter((n): n is string => typeof n === 'string') | ||
| .filter((n) => allowed.has(n)); | ||
|
|
||
| const base = vibeRef.current; | ||
| const updatedDoc = { ...base, dependencies: validDeps } as VibeDocument; | ||
| mergeRef.current(updatedDoc); | ||
| await sessionDatabase.put(updatedDoc); | ||
| }, | ||
| [sessionDatabase] | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
Good move scoping dependencies to the vibe. One improvement: this persistence path currently allows duplicate names and stores them in whatever order the UI sends, which makes the stored doc nondeterministic. Since downstream prompt generation ignores order and de-duplicates imports anyway, storing duplicates is wasteful and unstable.
Recommend validating, de-duping, and canonicalizing the order by the catalog before persisting. Also, reuse the exported ALLOWED_DEPENDENCY_NAMES instead of rebuilding the set to avoid drift if the catalog source changes.
Suggestion
You can normalize and sort by catalog order and use the shared constant:
import { ALLOWED_DEPENDENCY_NAMES, llmsCatalog } from '../llms/catalog';
const updateDependencies = useCallback(
async (deps: string[]) => {
const input = Array.isArray(deps) ? deps.filter((n): n is string => typeof n === 'string') : [];
const deduped = Array.from(new Set(input.filter((n) => ALLOWED_DEPENDENCY_NAMES.has(n))));
const order = new Map(llmsCatalog.map((l, i) => [l.name, i] as const));
const validDeps = deduped.sort((a, b) => (order.get(a)! - order.get(b)!));
const base = vibeRef.current;
const updatedDoc = { ...base, dependencies: validDeps } as VibeDocument;
mergeRef.current(updatedDoc);
await sessionDatabase.put(updatedDoc);
},
[sessionDatabase]
);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion
app/prompts.ts
Outdated
Comment on lines
75
to
192
| if (!selectedNames || selectedNames.length === 0) { | ||
| selectedNames = [...DEFAULT_DEPENDENCIES]; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
The fallback currently treats an explicitly saved empty array as “use defaults,” which can surprise users who intentionally deselect everything. If the UI enforces “at least one selected,” then this is fine; otherwise, it would be clearer to distinguish between “no field provided” (use defaults) and “empty array provided” (use none).
Suggestion
Only fall back when dependencies is absent, not when it’s an empty array:
let selectedNames: string[] | undefined;
const hasDeps = Object.prototype.hasOwnProperty.call(sessionDoc ?? {}, 'dependencies');
if (Array.isArray(sessionDoc?.dependencies)) {
const allowed = new Set(llmsCatalog.map((l) => l.name));
selectedNames = (sessionDoc.dependencies as unknown[])
.filter((v): v is string => typeof v === 'string')
.filter((name) => allowed.has(name)); // may be []
} else if (!hasDeps) {
selectedNames = [...DEFAULT_DEPENDENCIES];
}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion
Comment on lines
+125
to
136
| setSaveError(null); | ||
| setSaveSuccess(false); | ||
| try { | ||
| await saveSettings({ ...settings }); | ||
| setHasUnsavedChanges(false); | ||
| setSaveSuccess(true); | ||
| setTimeout(() => setSaveSuccess(false), 2000); | ||
| navigate('/'); | ||
| } catch (err: any) { | ||
| setSaveError(err?.message || 'Failed to save settings'); | ||
| } | ||
| }, [saveSettings, settings, navigate]); |
Contributor
Author
There was a problem hiding this comment.
You added inline success feedback but immediately navigate away on success, so the message won’t be visible. Either keep the user on the page to see the confirmation, or delay navigation long enough for the feedback to be meaningful.
Suggestion
If you want the success message to be seen, delay navigation:
await saveSettings({ ...settings });
setHasUnsavedChanges(false);
setSaveSuccess(true);
setTimeout(() => {
setSaveSuccess(false);
navigate('/');
}, 1200);Alternatively, remove the success UI if you prefer immediate navigation to home. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion
Contributor
|
@necrodome I feel like this should just be a simple operation on the existing data structure from the existing llm catalog chooser. and then a flag on the vibe to say "user override" if it is one and then dont run the llm catalog picker prompt. @CharlieHelps can you try that simple way? |
Contributor
Author
Expand this to see my work.
|
Contributor
Author
|
I implemented the per‑vibe “user override” for Libraries and gated prompt module selection on it. What changed
Open questions
Touches
Verification
Commits
If you reply “yes, re‑enable the picker” I’ll wire the non‑override path back to the prior AI/history‑based picker and extend tests accordingly. Also tell me your preference on empty selection and I’ll adjust the UI save logic and tests. |
Contributor
|
@CharlieHelps main now has LLM pick the libraries based on a schema callAI query. So lets rebase to main and then stay close it it on that front. we can let the llm also choose about the 'instructional text' and 'demo data' options |
Contributor
Author
Expand this to see my work.
|
charliecreates
bot
force-pushed
the
ai-215-settings-page-dependency-chooser
branch
from
1ebe641 to
eed7851
Compare
Contributor
Author
|
Rebased onto main and aligned the PR with the schema callAI selector. Kept the per‑vibe override intact and extended the selector to decide “instructional text” and “demo data”. What changed
Files touched
Open question
Verification
|
Contributor
|
add them to the vibe document and app settings ui as options |
jchris
added a commit
that referenced
this pull request
Aug 13, 2025
Addresses the open question from PR #229 by implementing separate per-vibe override flags for instructional text and demo data in App Settings. ## Changes Made ### Core Implementation - **Types**: Added `instructionalTextOverride` and `demoDataOverride` fields to VibeDocument - **Session Management**: Extended useSession with `updateInstructionalTextOverride()` and `updateDemoDataOverride()` functions - **UI Controls**: Added "Prompt Options" section in AppSettingsView with radio button controls: - "Let LLM decide" (default - uses LLM decision) - "Always include [feature]" (force enable) - "Never include [feature]" (force disable) ### Logic Integration - **Component Wiring**: Connected new override props through ResultPreview → AppSettingsView - **Prompt Builder**: Modified `makeBaseSystemPrompt` to respect override flags: - `undefined` → use LLM decision from `selectLlmsAndOptions` - `true` → force include feature regardless of LLM decision - `false` → force exclude feature regardless of LLM decision ### Testing Coverage - **UI Tests**: Added comprehensive AppSettingsView tests for radio button interactions - **Prompt Tests**: Added prompt builder tests verifying override behavior in all combinations - **Integration**: Verified override flags work independently and together ## User Experience Users now have granular per-vibe control over instructional text and demo data inclusion, matching the level of control available for Libraries selection. Preserves existing LLM-driven defaults while enabling precise customization. ## Files Modified - app/types/chat.ts - VibeDocument type extension - app/hooks/useSession.ts - Added override update functions - app/components/ResultPreview/ - UI controls and prop wiring - app/prompts.ts - Override logic in prompt builder - tests/ - Comprehensive test coverage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Contributor
|
@CharlieHelps review for merge |
Contributor
Author
There was a problem hiding this comment.
- Success feedback on the Settings page is rendered and then immediately hidden due to navigation, making it useless; delay navigation or remove the banner.
- The
selectLlmsAndOptionsschema is too permissive; addtype,required, andadditionalProperties: false, and early-filterselectedagainstALLOWED_DEPENDENCY_NAMESto reduce garbage. - Prompt builder tests rely on APP_MODE being "test"; explicitly mocking env or the selector will make tests reliable across environments.
- UI tests use value-based queries for radios, which are brittle; use label- or role-based queries for robustness.
Additional notes (2)
-
Maintainability |
app/prompts.ts:104-111
The schema forselectLlmsAndOptionsis permissive and doesn’t constrain the output shape or values. Tightening the schema reduces garbage responses and simplifies parsing. Also, filter the parsedselectedlist against the allowed names immediately to avoid carrying invalid values further into the pipeline. -
Readability |
tests/appSettings-dependencies.test.tsx:95-116
The tests locate the "Let LLM decide" radio inputs byvalue="llm"and name inference viagetAllByDisplayValue('llm'), which is brittle. A label- or role-based query is more resilient to future refactors.
Summary of changes
- prompts.ts
- Replaced local LLM catalog globbing with a centralized
llms/catalogimport and types. - Added schema-based selection via
selectLlmsAndOptionsreturning{ selected, instructionalText, demoData }and integrated it intomakeBaseSystemPrompt. - Preserved per‑vibe overrides for dependencies and added new per‑vibe overrides for instructional text and demo data.
- Switched import/doc handling to use
llmsCatalog; added conditional instructional/demo guidance lines.
- Replaced local LLM catalog globbing with a centralized
- routes/settings.tsx
- Added basic save success/error UI states to Settings page (global), but still navigate immediately after save.
- types/chat.ts
- Extended
VibeDocumentwithdependencies,dependenciesUserOverride,instructionalTextOverride, anddemoDataOverridefields with docs.
- Extended
- tests
- Added per‑vibe App Settings UI tests (including new Prompt Options radios) and integration tests for dependency selection.
- Updated prompt builder tests to align with schema-driven selection and new conditional guidance.
Comment on lines
+125
to
+135
| setSaveError(null); | ||
| setSaveSuccess(false); | ||
| try { | ||
| await saveSettings({ ...settings }); | ||
| setHasUnsavedChanges(false); | ||
| setSaveSuccess(true); | ||
| setTimeout(() => setSaveSuccess(false), 2000); | ||
| navigate('/'); | ||
| } catch (err: any) { | ||
| setSaveError(err?.message || 'Failed to save settings'); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
The success message is set and then you navigate away immediately, so users won’t see it. This undermines the purpose of the inline feedback and can confuse users about whether their changes were saved.
Suggestion
Delay navigation long enough for the confirmation to be visible, or remove the success banner if immediate navigation is preferred. For example:
await saveSettings({ ...settings });
setHasUnsavedChanges(false);
setSaveSuccess(true);
setTimeout(() => {
setSaveSuccess(false);
navigate('/');
}, 1200);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change
Comment on lines
41
to
51
| generateImportStatements = (mod as any).generateImportStatements; | ||
| makeBaseSystemPrompt = mod.makeBaseSystemPrompt; | ||
| preloadLlmsText = mod.preloadLlmsText; | ||
| // ensure catalog loads for glob ordering | ||
| await import('../app/llms/catalog'); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
These tests implicitly rely on APP_MODE being "test" to avoid making a real networked callAI request. That coupling is fragile and can lead to flaky tests when environment setup changes.
Suggestion
Mock the env module (or the selector) explicitly so the tests are hermetic regardless of APP_MODE:
vi.resetModules();
vi.mock('../app/config/env', () => ({
APP_MODE: 'test',
CALLAI_ENDPOINT: 'http://localhost/test',
}));
const mod = await import('../app/prompts');Alternatively, mock selectLlmsAndOptions to return a fixed decision set. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change
Contributor
Author
There was a problem hiding this comment.
Expand this to see my work.
- Refactored
tests/prompt-builder.test.tsto callvi.resetModules()then mock../app/config/envwithAPP_MODE: 'test'and a stubCALLAI_ENDPOINT, before importing../app/promptsfor all symbol access. - Kept the top-level import of
../app/llms/catalogto ensure prompt glob ordering remains correct. - Confirmed by rerunning the test suite that no codepath uses ambient
process.envor real networkedcallAIside effects after the refactor. - Validated passing tests and deterministic behavior no matter the environment by using a full
pnpm check. - Committed as 378eafc: test(prompts): mock app/config/env in prompt-builder.test to force APP_MODE='test' and stub CALLAI_ENDPOINT, per review suggestion.
Contributor
Author
There was a problem hiding this comment.
Implemented the accepted review suggestion to make tests/prompt-builder.test.ts hermetic by explicitly mocking the env module before importing app/prompts. This guarantees APP_MODE='test' and stubs CALLAI_ENDPOINT so the test never attempts a real callAI request, regardless of the actual environment. Kept the explicit llms/catalog import for glob ordering.
Change applied
// tests/prompt-builder.test.ts
(vi as any).doUnmock?.('../app/prompts');
vi.unmock('../app/prompts');
// Reset the module registry and mock env before importing the module under test.
vi.resetModules();
vi.mock('../app/config/env', () => ({
APP_MODE: 'test',
CALLAI_ENDPOINT: 'http://localhost/test',
}));
// ...later in beforeAll
const mod = await import('../app/prompts');
// ensure catalog loads for glob ordering
await import('../app/llms/catalog');Verification
- pnpm format — ok
- pnpm typecheck — ok
- pnpm test — 67 files, 381 passed, 4 skipped
Requested review and assigned: @jchris
Contributor
|
@CharlieHelps rebase from main |
Contributor
Author
Expand this to see my work.
|
…make module selection deterministic via Settings\n\n- Add llms catalog module and default dependencies\n- Extend UserSettings with dependencies:string[]\n- Settings UI: checkbox list for libraries, validation, and save feedback\n- Prompts: honor Settings.dependencies or default; generate imports/docs accordingly\n- Tests: update prompt builder expectations; add integration + UI tests for dependencies
…nd scope persistence to Vibe document - Add per‑vibe Libraries (dependency) chooser to App Settings view - Persist selection on the session's vibe doc (dependencies: string[]) with validation - Use vibe dependencies in prompt composition via useSystemPromptManager - Remove Libraries chooser and global dependencies from /settings and UserSettings type - Update tests: replace global settings UI test with per‑vibe AppSettings test Closes #215
…Libraries chooser; gate prompt module selection on override\n\n- Extend VibeDocument with dependenciesUserOverride:boolean\n- AppSettings: saving Libraries sets override=true and persists via useSession\n- useSession.updateDependencies now normalizes (validate, de‑dupe, canonical order) and saves flag\n- prompts: when override=true use saved deps; otherwise use deterministic DEFAULT_DEPENDENCIES (picker bypass pending clarification)\n- tests: update UI + prompt tests for override gating and keep defaults for non‑override path\n- use ALLOWED_DEPENDENCY_NAMES as single source of truth
…ridden; add decisions for instructional text and demo data; update App Settings UI to reflect LLM‑driven mode\n\n- Non‑override path now uses schema callAI selector (keeps per‑vibe override via dependenciesUserOverride)\n- Extend selector schema to also decide instructionalText/demoData; prompt composition respects both\n- AppSettingsView shows LLM‑driven note when not overridden and no longer preselects defaults\n- Wire override flag to AppSettingsView via ResultPreview\n- Tests: update for schema path; add coverage for instructional/demo lines; keep override behavior
Addresses the open question from PR #229 by implementing separate per-vibe override flags for instructional text and demo data in App Settings. - **Types**: Added `instructionalTextOverride` and `demoDataOverride` fields to VibeDocument - **Session Management**: Extended useSession with `updateInstructionalTextOverride()` and `updateDemoDataOverride()` functions - **UI Controls**: Added "Prompt Options" section in AppSettingsView with radio button controls: - "Let LLM decide" (default - uses LLM decision) - "Always include [feature]" (force enable) - "Never include [feature]" (force disable) - **Component Wiring**: Connected new override props through ResultPreview → AppSettingsView - **Prompt Builder**: Modified `makeBaseSystemPrompt` to respect override flags: - `undefined` → use LLM decision from `selectLlmsAndOptions` - `true` → force include feature regardless of LLM decision - `false` → force exclude feature regardless of LLM decision - **UI Tests**: Added comprehensive AppSettingsView tests for radio button interactions - **Prompt Tests**: Added prompt builder tests verifying override behavior in all combinations - **Integration**: Verified override flags work independently and together Users now have granular per-vibe control over instructional text and demo data inclusion, matching the level of control available for Libraries selection. Preserves existing LLM-driven defaults while enabling precise customization. - app/types/chat.ts - VibeDocument type extension - app/hooks/useSession.ts - Added override update functions - app/components/ResultPreview/ - UI controls and prop wiring - app/prompts.ts - Override logic in prompt builder - tests/ - Comprehensive test coverage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…P_MODE='test' and stub CALLAI_ENDPOINT\n\n- Reset module registry and mock env before importing app/prompts\n- Keep explicit llms/catalog import for glob ordering\n- Remove implicit reliance on real APP_MODE so tests never call real callAI\n\nImplements review suggestion to make the test hermetic.
Incorporates PR 219's intelligent decision logic into PR 229's simpler architecture. The system now analyzes user prompts to determine if specific look & feel requirements are described, and only includes instructional text and demo data for plain CRUD apps without specified styling. This provides contextually appropriate guidance without the complex slot/catalog plumbing from PR 219. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
charliecreates
bot
force-pushed
the
ai-215-settings-page-dependency-chooser
branch
from
2d6b173 to
acdf168
Compare
…ibe + deps; make AppSettingsView handlers optional to avoid breaking unrelated tests after rebase
Contributor
Author
|
Rebased onto main and resolved all conflicts. The branch now includes main’s schema-driven selector and preserves per‑vibe overrides and types as requested. Summary of reconciliations
Branch
Verification
Housekeeping
|
Contributor
Author
There was a problem hiding this comment.
- Success feedback on the Settings page is immediately hidden by navigation; either delay navigation or remove the banner to avoid confusing UX.
- Tests in
appSettings-dependencies.test.tsxuse value-based radio queries, which are brittle; switch to role/label-based queries scoped to groups. prompts-dependencies.integration.test.tsdoesn’t mock env and can hit real network paths; mockAPP_MODE='test'like in the prompt-builder test to keep it hermetic.- No other correctness or performance issues observed in the modified code segments.
Summary of changes
- Settings page: added inline save success/error feedback states and updated
handleSubmitto set those states, but it still navigates to/immediately after saving. - VibeDocument: extended with
dependencies,dependenciesUserOverride,instructionalTextOverride, anddemoDataOverridefields with inline docs. - Removed a stray investigation doc
docs/flicker-investigation.md. - Tests:
- Added
tests/appSettings-dependencies.test.tsxcovering per‑vibe Libraries chooser and new Prompt Options radios. - Updated
tests/prompt-builder.test.tsto mock env (APP_MODE='test') and expanded coverage for overrides and guidance lines. - Added
tests/prompts-dependencies.integration.test.tsto exercise schema-driven vs override dependency selection.
- Added
Comment on lines
+125
to
+133
| setSaveError(null); | ||
| setSaveSuccess(false); | ||
| try { | ||
| await saveSettings({ ...settings }); | ||
| setHasUnsavedChanges(false); | ||
| setSaveSuccess(true); | ||
| setTimeout(() => setSaveSuccess(false), 2000); | ||
| navigate('/'); | ||
| } catch (err: any) { |
Contributor
Author
There was a problem hiding this comment.
The success banner is set and then you navigate away immediately, so users won’t see the confirmation. This makes the feedback ineffective and can confuse users about whether their changes were saved.
Suggestion
Delay navigation long enough for the success state to be visible, or remove the banner if you want immediate navigation. For example:
await saveSettings({ ...settings });
setHasUnsavedChanges(false);
setSaveSuccess(true);
setTimeout(() => {
setSaveSuccess(false);
navigate('/');
}, 1200);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change
Comment on lines
+97
to
+116
| const instructionalTextInputs = screen.getAllByDisplayValue('llm'); | ||
| const llmDecideInstructional = instructionalTextInputs.find( | ||
| (input) => (input as HTMLInputElement).name === 'instructionalText' | ||
| ); | ||
| const alwaysIncludeInstructional = screen.getByLabelText('Always include instructional text'); | ||
| const neverIncludeInstructional = screen.getByLabelText('Never include instructional text'); | ||
|
|
||
| // Default should be "Let LLM decide" | ||
| expect(llmDecideInstructional!).toBeChecked(); | ||
| expect(alwaysIncludeInstructional).not.toBeChecked(); | ||
| expect(neverIncludeInstructional).not.toBeChecked(); | ||
|
|
||
| // Check demo data controls | ||
| expect(screen.getByText('Demo Data')).toBeInTheDocument(); | ||
| const llmDecideDemo = instructionalTextInputs.find( | ||
| (input) => (input as HTMLInputElement).name === 'demoData' | ||
| ); | ||
|
|
||
| expect(llmDecideDemo!).toBeChecked(); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
These tests locate radios using getAllByDisplayValue('llm'), which is brittle and ties the test to internal values rather than the accessible UI. Prefer label- or role-based queries scoped to each fieldset/group for resilience.
Suggestion
Use role- and label-based queries scoped to the corresponding section to target the correct radio without relying on the value attribute. For example:
import { within } from '@testing-library/react';
// Instructional Text group
const instructionalGroup = screen.getByRole('group', { name: /Instructional Text/i });
const llmDecideInstructional = within(instructionalGroup).getByRole('radio', { name: /Let LLM decide/i });
const alwaysIncludeInstructional = within(instructionalGroup).getByRole('radio', { name: /Always include instructional text/i });
const neverIncludeInstructional = within(instructionalGroup).getByRole('radio', { name: /Never include instructional text/i });
// Demo Data group
const demoGroup = screen.getByRole('group', { name: /Demo Data/i });
const llmDecideDemo = within(demoGroup).getByRole('radio', { name: /Let LLM decide/i });Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor
Comment on lines
+1
to
+14
| import { describe, it, expect, beforeAll, vi } from 'vitest'; | ||
| // Ensure real implementation | ||
| (vi as any).doUnmock?.('../app/prompts'); | ||
| vi.unmock('../app/prompts'); | ||
| vi.resetModules(); | ||
|
|
||
| let makeBaseSystemPrompt: (model: string, sessionDoc?: any) => Promise<string>; | ||
| let preloadLlmsText: () => Promise<void>; | ||
|
|
||
| beforeAll(async () => { | ||
| const mod = await import('../app/prompts'); | ||
| makeBaseSystemPrompt = mod.makeBaseSystemPrompt; | ||
| preloadLlmsText = mod.preloadLlmsText; | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
This integration test doesn’t mock the env and may call the real callAI in non-test environments, making the test non-hermetic and potentially flaky. Your other test explicitly mocks APP_MODE='test'; do the same here.
Suggestion
Mock the env before importing app/prompts to guarantee test mode and prevent real network calls:
(vi as any).doUnmock?.('../app/prompts');
vi.unmock('../app/prompts');
vi.resetModules();
vi.mock('../app/config/env', () => ({
APP_MODE: 'test',
CALLAI_ENDPOINT: 'http://localhost/test',
}));
const mod = await import('../app/prompts');Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change
jchris
pushed a commit
that referenced
this pull request
Aug 15, 2025
…sh_url) (#265) * docs(llms): add Web Audio research llms-txt and catalog entry (no refresh_url); wire into prompts (#228, #229) * docs(llms): remove unused llmsTxtUrl from web-audio catalog; refine Web Audio examples (gesture-gated resume, robust IR loader, metronome cleanup) --------- Co-authored-by: CharlieHelps <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move the Libraries (dependency) chooser from the global Settings page to the per‑vibe App Settings view and scope persistence/usage per vibe.
Context
/settingswith global persistence. Per feedback in settings page dependency chooser #215 (comment), this needs to be per‑vibe and live on the App Settings page (introduced in c046dac…).Changes
app/llms/*.jsonviallmsCatalog, with defaulting and validation.VibeDocumentwithdependencies?: string[].vibedoc viauseSession.updateDependencies().useSystemPromptManagernow merges the active vibe’s doc when building prompts;makeBaseSystemPromptreadsdependenciesfrom that merged doc (fallback toDEFAULT_DEPENDENCIES)./settingsand removeddependenciesfromUserSettingstype.tests/appSettings-dependencies.test.tsxfor the new per‑vibe UI.dependenciesfield.Impact
Verification
Closes #215