feat: Webcontainer package - #374

examples/app-builder/src/tools/updateFileContent/updateFileContentTool.ts (1)

1-4: Same deep-import concern as in createListFilesTool.

Consider importing WebContainerSliceState from @sqlrooms/webcontainer to avoid coupling to the build output path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/app-builder/src/tools/updateFileContent/updateFileContentTool.ts`
around lines 1 - 4, The import of WebContainerSliceState uses a deep
build-output path which couples code to the dist layout; replace the deep import
in updateFileContentTool.ts by importing WebContainerSliceState from the package
entry (e.g., '@sqlrooms/webcontainer') instead of
'../../../../../packages/webcontainer/dist' and update any references in the
file (WebContainerSliceState) accordingly so the module resolution uses the
package boundary rather than the build output path.

examples/app-builder/src/tools/getFileContent/getFileContentTool.ts (1)

2-4: Same deep-import concern as in the other tool files.

Consider importing WebContainerSliceState from @sqlrooms/webcontainer to avoid coupling to the build output path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/app-builder/src/tools/getFileContent/getFileContentTool.ts` around
lines 2 - 4, The import of the type WebContainerSliceState is using a deep
build-output path which couples code to the repo layout; replace the deep import
with a package-level type import (e.g. change import type {
WebContainerSliceState } from '../../../../../packages/webcontainer/dist' to
import type { WebContainerSliceState } from '@sqlrooms/webcontainer') so the
file (getFileContentTool.ts) depends on the published package API rather than
the build output.

examples/app-builder/src/tools/listFiles/createListFilesTool.ts (1)

1-5: Prefer public package entry over deep dist import.

Importing from packages/webcontainer/dist couples to build output and can be brittle in dev. Prefer the package export.

♻️ Proposed change

-import type {WebContainerSliceState} from '../../../../../packages/webcontainer/dist';
+import type {WebContainerSliceState} from '@sqlrooms/webcontainer';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/app-builder/src/tools/listFiles/createListFilesTool.ts` around lines
1 - 5, The file currently imports the type WebContainerSliceState via a deep
build output path ('../../../../../packages/webcontainer/dist'); replace that
with an import from the package's public entry (e.g. import type {
WebContainerSliceState } from 'webcontainer' or the package's actual npm scope
name) to avoid coupling to dist output—update the import in
createListFilesTool.ts to reference the package's exported entry and ensure
WebContainerSliceState is exported there (adjust the package name to the correct
public package if different).

Configuration used: defaults

Review profile: CHILL

Plan: Pro

examples/app-builder/src/tools/listFiles/createListFilesTool.ts (1)

37-42: Reuse the existing schema constant to avoid duplication.

The parameters schema here is identical to the module-level ListFilesToolParameters defined on lines 12-17. Reusing the constant avoids duplication and ensures consistency.

♻️ Proposed fix

   return {
     name: 'listFiles',
     description: 'List project files',
-    parameters: z.object({
-      basePath: z
-        .string()
-        .describe('Optional base path to list files from')
-        .default('/'),
-    }),
+    parameters: ListFilesToolParameters,
     execute: async ({basePath = '/'}: ListFilesToolParameters) => {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/app-builder/src/tools/listFiles/createListFilesTool.ts` around lines
37 - 42, Replace the inline parameters schema with the existing module-level
constant to avoid duplication: use the ListFilesToolParameters constant in place
of the inline parameters object in createListFilesTool (replace the current
"parameters: z.object({...})" block with a reference to ListFilesToolParameters)
so both the tool and the module share the same schema definition.

packages/webcontainer/src/setFileContentInTree.ts (1)

42-44: Defensive guard is reasonable but appears unreachable.

Given that pathToSegments filters out empty segments and line 65 returns early when segments.length === 0, this guard should never trigger in normal execution. If it's intended as defensive coding against future refactoring, consider adding a comment explaining the intent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/setFileContentInTree.ts` around lines 42 - 44, The
guard `if (!segment) return subtree;` inside setFileContentInTree is unreachable
in normal execution because pathToSegments filters out empty segments and
there's an early return when `segments.length === 0`; either remove the guard or
(preferred) keep it but add a one-line comment above it explaining it's a
defensive check against future refactors or unexpected inputs (mentioning that
pathToSegments filters empties and segments.length === 0 is handled earlier) so
readers know it's intentional rather than dead code.

examples/app-builder/src/tools/updateFileContent/updateFileContentTool.ts (1)

35-37: Unnecessary await on synchronous method.

updateFileContent is synchronous (returns void), so the await on line 36 is unnecessary. However, it's harmless and may serve as defensive coding if the method becomes async in the future.

♻️ Optional: Remove unnecessary await

     execute: async ({path, content}: UpdateFileContentToolParameters) => {
-      await store.getState().webcontainer.updateFileContent(path, content);
+      store.getState().webcontainer.updateFileContent(path, content);
       await store.getState().webcontainer.saveAllOpenFiles();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/app-builder/src/tools/updateFileContent/updateFileContentTool.ts`
around lines 35 - 37, The call to
store.getState().webcontainer.updateFileContent in the execute function is
synchronous (returns void) so the await before it is unnecessary; remove the
redundant await in execute (leave the subsequent await on saveAllOpenFiles,
which is async) by updating the execute implementation to call
updateFileContent(path, content) without await while keeping await on
saveAllOpenFiles() to preserve async behavior.

packages/webcontainer/src/WebContainerSlice.ts (2)

208-240: startDevServer resolves before server is actually ready.

The function returns (resolves) immediately after spawning npm run dev and setting state to starting-dev. The actual "ready" state is set asynchronously in the server-ready callback. Callers awaiting this function may incorrectly assume the server is ready when it's not.

Consider returning a Promise that resolves only when server-ready fires or an error occurs:

♻️ Suggested approach

       async startDevServer() {
         const instance = get().webcontainer.instance;
         if (!instance) {
           throw new Error('WebContainer instance not found');
         }
-        // Run `npm run dev` to start the Vite dev server
-        await instance.spawn('npm', ['run', 'dev']);
-        set((state) =>
-          produce(state, (draft) => {
-            draft.webcontainer.serverStatus = {type: 'starting-dev'};
-          }),
-        );
-
-        // Wait for `server-ready` event
-        instance.on('server-ready', (port, url) => {
-          console.log(`Server ready on port ${port} at ${url}`);
-          setCachedWebContainerServerUrl(url);
-          set((state) =>
-            produce(state, (draft) => {
-              draft.webcontainer.serverStatus = {type: 'ready', url};
-            }),
-          );
-        });
-
-        instance.on('error', (error) => {
-          console.error('Server error', error);
-          set((state) =>
-            produce(state, (draft) => {
-              draft.webcontainer.serverStatus = {type: 'error', error};
-            }),
-          );
-        });
+        return new Promise<void>((resolve, reject) => {
+          instance.on('server-ready', (port, url) => {
+            console.log(`Server ready on port ${port} at ${url}`);
+            setCachedWebContainerServerUrl(url);
+            set((state) =>
+              produce(state, (draft) => {
+                draft.webcontainer.serverStatus = {type: 'ready', url};
+              }),
+            );
+            resolve();
+          });
+
+          instance.on('error', (error) => {
+            console.error('Server error', error);
+            set((state) =>
+              produce(state, (draft) => {
+                draft.webcontainer.serverStatus = {type: 'error', error};
+              }),
+            );
+            reject(error);
+          });
+
+          // Start the dev server after listeners are attached
+          instance.spawn('npm', ['run', 'dev']);
+          set((state) =>
+            produce(state, (draft) => {
+              draft.webcontainer.serverStatus = {type: 'starting-dev'};
+            }),
+          );
+        });
       },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/WebContainerSlice.ts` around lines 208 - 240,
startDevServer currently spawns the dev server and returns immediately, so
callers may think the server is ready while it isn’t; change startDevServer to
return a Promise that only resolves when the instance emits 'server-ready' (and
sets setCachedWebContainerServerUrl and draft.webcontainer.serverStatus to
ready) and rejects if the instance emits 'error' (and sets serverStatus to
error). Attach one-time listeners to instance.on('server-ready', ...) and
instance.on('error', ...), resolve/reject the Promise inside those callbacks,
and remove the listeners after they fire to avoid leaks; keep existing state
updates (produce/set) in those callbacks so behavior is unchanged except for the
Promise lifecycle.

---

347-357: Unreachable TextDecoder branch when using 'utf-8' encoding.

When readFile is called with 'utf-8' encoding, @webcontainer/api returns a string directly. The Uint8Array fallback on line 352 is unreachable.

♻️ Simplify the type handling

       async getFileContent(path) {
         const state = get();
         const opened = state.webcontainer.config.openedFiles.find(
           (f) => f.path === path,
         );
         if (opened) return opened.content;
         const instance = state.webcontainer.instance;
         try {
           if (instance) {
-            const data = await instance.fs.readFile(path, 'utf-8');
-            return typeof data === 'string'
-              ? data
-              : new TextDecoder().decode(data as Uint8Array);
+            return await instance.fs.readFile(path, 'utf-8');
           }
         } catch (_e) {
           // Swallow and return empty string
         }
         return '';
       },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/WebContainerSlice.ts` around lines 347 - 357, The
code calls instance.fs.readFile(path, 'utf-8') and then checks for a Uint8Array
fallback with new TextDecoder(), but `@webcontainer/api` returns a string when
'utf-8' is passed so the Uint8Array branch is unreachable; simplify by treating
the result as a string (remove the typeof check and TextDecoder fallback) and
return the data directly, or if you need binary support instead call
instance.fs.readFile(path) without the 'utf-8' option and handle a Uint8Array
there (adjust logic in the read helper around instance.fs.readFile and remove
the TextDecoder path).

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.cursor/plans/sqlrooms_cli_app-builder_plan_(adjusted)_174b3a95.plan.md (1)

224-257: Add an explicit build prerequisite before example/UI validation steps.

Given the milestone and success-criteria flow, add a short prerequisite note that package builds must run first; otherwise example app flows can fail due to unresolved workspace:* links.

Based on learnings: Run pnpm build before running any example app, as examples depend on built sqlrooms/* packages via workspace:* links.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.cursor/plans/sqlrooms_cli_app-builder_plan_(adjusted)_174b3a95.plan.md
around lines 224 - 257, Add an explicit build prerequisite sentence/note before
the example/UI validation steps (e.g., right before "Milestone 1: Artifact MVP"
or the Example/App run instructions) stating that packages must be built first
and providing the exact command to run: "Run pnpm build before running any
example app" to ensure workspace:* linked packages are resolved; reference the
workspace linking issue by mentioning "workspace:* links" so readers know why
the prerequisite is required.

packages/webcontainer/src/setFileContentInTree.ts (1)

6-8: Edge case: paths starting with multiple slashes are not fully normalized.

If the input starts with // (e.g., //a//b), slice(1) removes only one slash, leaving /a//b. After the replace, you get /a/b — still with a leading slash, which doesn't match the docstring's "removing a leading slash."

Since pathToSegments filters empty strings after split, this bug doesn't affect setFileContentInTree. However, normalizePath is exported and could be used directly elsewhere.

Proposed fix to handle multiple leading slashes

 export function normalizePath(path: string): string {
-  return (path.startsWith('/') ? path.slice(1) : path).replace(/\/+/g, '/');
+  return path.replace(/^\/+/, '').replace(/\/+/g, '/');
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/setFileContentInTree.ts` around lines 6 - 8,
normalizePath currently removes only a single leading slash then collapses
repeated slashes, so inputs like "//a//b" still end up with a leading slash;
update normalizePath to strip all leading slashes (e.g., remove prefix matching
one-or-more '/' characters) before collapsing consecutive slashes so the
function always returns a path without any leading '/' as documented; reference
normalizePath (and note callers like pathToSegments / setFileContentInTree rely
on normalized segments) and ensure the new implementation is exported the same
way.

packages/webcontainer/src/components/CodeView.tsx (2)

23-25: Selector calls a method, which may defeat memoization.

Calling hasDirtyFiles() inside the selector could cause re-renders on every state change if this method performs computation. Consider selecting the underlying openedFiles state and deriving the dirty check in the component:

-  const hasDirty = useStoreWithWebContainer((s) =>
-    s.webContainer.hasDirtyFiles(),
-  );
+  const hasDirty = useMemo(
+    () => openedFiles.some((f) => f.dirty),
+    [openedFiles],
+  );

This leverages the already-selected openedFiles and avoids potential memoization issues with method calls in selectors. As per coding guidelines: "Use Zustand selectors with memoization to avoid unnecessary re-renders."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/components/CodeView.tsx` around lines 23 - 25, The
selector currently calls webContainer.hasDirtyFiles() via
useStoreWithWebContainer which can defeat selector memoization; instead select
the underlying state slice (e.g. webContainer.openedFiles or
webContainer.openedFilesMap) using useStoreWithWebContainer and compute hasDirty
locally in CodeView (replace hasDirty usage with a derived value computed from
the selected openedFiles), updating references to hasDirty,
useStoreWithWebContainer, and webContainer.hasDirtyFiles accordingly so the
selector only returns raw state and not a method call.

---

77-86: Hardcoded language won't highlight non-JavaScript files correctly.

The language="javascript" prop means TypeScript, JSON, CSS, HTML, and other file types won't get proper syntax highlighting. Consider deriving the language from the file extension:

♻️ Suggested approach

const getLanguageFromPath = (path: string): string => {
  const ext = path.split('.').pop()?.toLowerCase();
  const langMap: Record<string, string> = {
    js: 'javascript',
    jsx: 'javascript',
    ts: 'typescript',
    tsx: 'typescript',
    json: 'json',
    css: 'css',
    html: 'html',
    md: 'markdown',
  };
  return langMap[ext ?? ''] ?? 'plaintext';
};

// In the component:
<MonacoEditor
  className="h-full w-full"
  language={activeFilePath ? getLanguageFromPath(activeFilePath) : 'plaintext'}
  value={activeFile?.content ?? ''}
  // ...
/>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/components/CodeView.tsx` around lines 77 - 86, The
MonacoEditor currently hardcodes language="javascript", so derive the editor
language from the active file path: add a helper (e.g., getLanguageFromPath)
that maps file extensions (js, jsx -> javascript; ts, tsx -> typescript; json ->
json; css -> css; html -> html; md -> markdown; default -> plaintext), then pass
language={activeFilePath ? getLanguageFromPath(activeFilePath) : 'plaintext'} to
the MonacoEditor; update references to MonacoEditor, activeFilePath, activeFile,
and updateFileContent accordingly so other behavior is unchanged.

apps/sqlrooms-cli-ui/plugins/scaffolds.ts (2)

21-23: Missing error handling could crash dev server or fail build silently.

If generateScaffoldsModule throws (e.g., file read error), the error propagates unhandled. Consider wrapping in try-catch:

🛡️ Proposed fix

     async buildStart() {
-      await runGenerate();
+      try {
+        await runGenerate();
+      } catch (err) {
+        console.error('[scaffolds plugin] Failed to generate scaffolds:', err);
+        throw err; // Re-throw to fail the build
+      }
     },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/plugins/scaffolds.ts` around lines 21 - 23, Wrap the
await runGenerate() call inside buildStart() with a try-catch to prevent
unhandled exceptions from generateScaffoldsModule/runGenerate from crashing the
dev server or failing silently; in the catch block, log the error (use
this.error(err) when available in the plugin context, otherwise console.error)
and rethrow or call this.error to surface the failure to the build system so the
error is visible.

---

27-34: Rapid file changes may cause concurrent generation races.

Multiple rapid file system events (e.g., saving multiple files, IDE auto-formatting) will trigger concurrent runGenerate() calls without debouncing. Consider adding a simple debounce:

♻️ Optional debounce pattern

+  let debounceTimer: ReturnType<typeof setTimeout> | null = null;
+
   return {
     name: 'sqlrooms-cli-scaffolds',
     async buildStart() {
       await runGenerate();
     },
     configureServer(server) {
       const watcher = server.watcher;
       watcher.add(scaffoldsRoot);
       const onFsEvent = async (changedPath: string) => {
         if (!changedPath.startsWith(scaffoldsRoot)) return;
         if (changedPath === outputFile) return;
-        await runGenerate();
+        if (debounceTimer) clearTimeout(debounceTimer);
+        debounceTimer = setTimeout(async () => {
+          try {
+            await runGenerate();
+          } catch (err) {
+            console.error('[scaffolds plugin] Generation failed:', err);
+          }
+        }, 100);
       };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/plugins/scaffolds.ts` around lines 27 - 34, The
onFsEvent handler currently calls runGenerate() for every matching file event
which can cause concurrent runs; modify the watcher callbacks
(watcher.on('add'/'change'/'unlink') -> onFsEvent) to debounce/coalesce rapid
events by adding a short timer (e.g., 100–300ms) and clearing previous timers on
new events, and ensure you avoid overlapping executions by using a running flag
or awaiting the single scheduled run; update the symbols onFsEvent and
runGenerate to use a shared debounceTimer and optionally an isRunning flag so
multiple events within the debounce window only schedule one runGenerate and
concurrent runs are prevented while respecting scaffoldsRoot and outputFile
checks.

apps/sqlrooms-cli-ui/plugins/generateScaffolds.ts (1)

73-76: Binary files will be corrupted when read as UTF-8.

Reading all files with 'utf-8' encoding will corrupt binary files (images, fonts, etc.). Consider either:

1. Filtering out binary files by extension
2. Detecting binary content and using base64 encoding
3. Documenting that scaffolds should only contain text files

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/plugins/generateScaffolds.ts` around lines 73 - 76, The
current code in generateScaffolds.ts always calls fs.readFile(abs, 'utf-8') and
writes to result[entry.name].file.contents, which will corrupt binary files;
change the logic that builds result[entry.name] so that you detect whether a
file is binary (e.g., by checking extension or probing contents after reading as
a Buffer) and branch: for text files keep reading as UTF-8 and set file.contents
to the string, for binary files read as a Buffer and store base64-encoded
contents plus an explicit encoding flag (e.g., file.encoding = 'base64') so
consumers can decode; update the code paths that consume result[...].file to
respect the new encoding field.

packages/webcontainer/src/WebContainerSlice.ts (1)

550-556: Use produce() for nested state update consistency.

This state update uses object spread instead of produce(), which is inconsistent with the rest of the file. Per coding guidelines, use produce() from Immer when updating nested state in Zustand store.

♻️ Proposed fix

-                set((state) => ({
-                  webContainer: {
-                    ...state.webContainer,
-                    output: state.webContainer.output + text,
-                  },
-                }));
+                set((state) =>
+                  produce(state, (draft) => {
+                    draft.webContainer.output += text;
+                  }),
+                );

Based on learnings: "Always use produce() from Immer when updating nested state in Zustand store to ensure immutable updates."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/WebContainerSlice.ts` around lines 550 - 556, The
nested Zustand state update for webContainer.output in the setter should use
produce() from Immer instead of object spread to remain consistent and safe;
locate the set call that currently does set((state) => ({ webContainer: {
...state.webContainer, output: state.webContainer.output + text } })), replace
it with a produce-based update that mutates draft.webContainer.output (appending
text) inside produce, ensuring you import/ use the existing produce symbol and
keep other webContainer fields untouched.

packages/webcontainer/src/components/TerminalView.tsx (1)

18-42: Potential unnecessary terminal recreation when output changes.

The first useEffect has output in its dependency array, but the cleanup disposes the terminal and sets terminalRef.current = null. When output changes, this causes the terminal to be destroyed and recreated, which is inefficient since the second useEffect already handles incremental output updates.

Consider removing output from the dependency array and relying solely on the second effect for output synchronization:

♻️ Proposed fix

   useEffect(() => {
     if (!containerRef.current || terminalRef.current) {
       return;
     }
     const term = new Terminal({
       convertEol: true,
       disableStdin: true,
       fontSize: 12,
       cursorBlink: false,
       scrollback: 5000,
     });
     term.open(containerRef.current);
     terminalRef.current = term;

-    if (output && output.length > 0) {
-      term.write(output);
-      lastIndexRef.current = output.length;
-    }
-
     return () => {
       term.dispose();
       terminalRef.current = null;
       lastIndexRef.current = 0;
     };
-  }, [output]);
+  }, []);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/components/TerminalView.tsx` around lines 18 - 42,
The effect that creates and disposes the xterm instance (uses Terminal,
containerRef, terminalRef, lastIndexRef) should only run once on mount/unmount —
remove output from the dependency array so it becomes [] — so the terminal is
not destroyed/recreated whenever output changes; keep the existing cleanup
(term.dispose(), terminalRef.current = null, lastIndexRef.current = 0) and rely
on the other useEffect that incrementally writes new output to synchronize
terminal contents.

apps/sqlrooms-cli-ui/src/components/AppBuilderSheet.tsx (3)

183-183: Consider using a proper type instead of any.

Using any for the root object loses type safety. A recursive type or Record<string, unknown> would be safer.

Suggested change

 function toFileSystemTree(
   files: Array<{path: string; content: string}>,
 ): FileSystemTree {
-  const root: any = {};
+  const root: Record<string, any> = {};

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/src/components/AppBuilderSheet.tsx` at line 183, The
declaration "const root: any = {}" in AppBuilderSheet.tsx loses type safety;
replace the any with a precise type such as Record<string, unknown> or define a
recursive interface (e.g., type Node = { [key: string]: Node | unknown }) and
use that for the root variable so consumers of root (and functions interacting
with it) get proper type checks; locate the root binding in AppBuilderSheet.tsx
and update its type, adjusting any downstream code with narrow typings or type
guards as needed.

---

60-68: Potential redundant effect triggers due to appState?.files in dependencies.

Including appState?.files as a dependency alongside appState?.updatedAt may cause redundant triggers since both change together. Additionally, if loadArtifactRuntime modifies files via updateSheetAppFiles (line 51), this could trigger re-runs, though the patched.length > 0 guard should prevent infinite loops.

Consider relying solely on appState?.updatedAt as the change indicator:

Suggested change

   React.useEffect(() => {
     if (!currentSheetId || !appState?.files) return;
     void loadArtifactRuntime(currentSheetId, appState.files);
   }, [
     currentSheetId,
     appState?.updatedAt,
-    appState?.files,
     loadArtifactRuntime,
   ]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/src/components/AppBuilderSheet.tsx` around lines 60 -
68, The effect dependencies include appState?.files which can redundantly
retrigger the effect alongside appState?.updatedAt and may re-run when
loadArtifactRuntime calls updateSheetAppFiles (even if patched.length > 0
prevents loops); remove appState?.files from the dependency array and depend
only on currentSheetId and appState?.updatedAt and a stable loadArtifactRuntime
(ensure loadArtifactRuntime is memoized with useCallback or otherwise stable) so
the effect only runs when the sheet or its update timestamp changes.

---

293-325: Retry loop never actually retries different logic.

The while (attempts < 3) loop always produces the same output on each iteration since ensureRunnableViteScaffold deterministically adds all required files. The missing array will always be empty after the first call, making the retry mechanism ineffective.

If this is placeholder code for future AI-based generation that might fail, consider adding a comment to clarify intent. Otherwise, the loop can be simplified.

Suggested simplification if retry is not needed

 function generateAppFromPromptLocal(input: {
   prompt: string;
   template: string;
 }): {
   status: 'ok' | 'error';
   attempts: number;
   errors: string[];
   files: Array<{path: string; content: string}>;
 } {
-  const errors: string[] = [];
-  let attempts = 0;
-  let files: Array<{path: string; content: string}> = [];
-  while (attempts < 3) {
-    attempts += 1;
-    const appTitle =
-      input.template === 'basic-dashboard'
-        ? 'Analytics App'
-        : 'Mosaic Analytics App';
-    const appFile = `import React from 'react';
+  const appTitle =
+    input.template === 'basic-dashboard'
+      ? 'Analytics App'
+      : 'Mosaic Analytics App';
+  const appFile = `import React from 'react';
 
 export default function App() {
   return (
     <main style={{padding: 16}}>
       <h1>${appTitle}</h1>
       <p>Prompt: ${input.prompt.replace(/</g, '&lt;')}</p>
       <p>Template: ${input.template}</p>
     </main>
   );
 }
 `;
-    files = ensureRunnableViteScaffold([
-      {path: '/src/App.jsx', content: appFile},
-    ]).files;
-    const required = new Set(files.map((f) => f.path));
-    const missing = [
-      '/index.html',
-      '/src/main.jsx',
-      '/src/App.jsx',
-      '/package.json',
-    ].filter((p) => !required.has(p));
-    if (missing.length === 0) {
-      return {status: 'ok', attempts, errors, files};
-    }
-    errors.push(...missing.map((m) => `missing required file: ${m}`));
-  }
-  return {status: 'error', attempts, errors, files};
+  const files = ensureRunnableViteScaffold([
+    {path: '/src/App.jsx', content: appFile},
+  ]).files;
+  return {status: 'ok', attempts: 1, errors: [], files};
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/src/components/AppBuilderSheet.tsx` around lines 293 -
325, The retry while loop in AppBuilderSheet.tsx around
ensureRunnableViteScaffold is ineffective because ensureRunnableViteScaffold
deterministically supplies the required files so `missing` will be empty after
the first call; remove the loop and call ensureRunnableViteScaffold once (or if
you intend future non-deterministic generation, add a clear TODO comment and
break/return after the first failure), then compute `required`, `missing`, and
return {status: 'ok'...} or push errors accordingly; specifically edit the block
that uses attempts, ensureRunnableViteScaffold, required/missing and errors so
it performs a single attempt (or documents retry intent) instead of looping.

apps/sqlrooms-cli-ui/src/store.ts (1)

248-248: Type assertion on scaffolds may fail silently.

Casting scaffolds to Record<string, any> bypasses type checking. If the JSON structure changes, this could cause runtime errors.

Consider adding validation or a more specific type

-          filesTree: (scaffolds as Record<string, any>)['get-started'] ?? {},
+          filesTree: scaffolds['get-started'] ?? {},

If the import typing from the JSON is incorrect, consider defining a proper type or using Zod to validate the structure at runtime.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/src/store.ts` at line 248, The cast of scaffolds to
Record<string, any> for initializing filesTree is unsafe; update the code around
filesTree and scaffolds (the import/variable named scaffolds and the property
'get-started') to use a proper typed interface or runtime validation: define a
specific type (e.g., ScaffoldsMap with a 'get-started' entry shape) or add a Zod
schema to validate scaffolds after import, then use the validated value (or a
safe fallback {}) when assigning filesTree so you don't bypass type checks and
avoid silent runtime failures.

python/sqlrooms-cli/tests/test_cli.py (1)

11-14: Use a behavior-level CLI assertion instead of only checking registration.

This test can pass even if the command is broken at invocation time. Prefer invoking export --help (and consider renaming the test from test_cli_version to match intent).

Proposed refactor

-def test_cli_version():
-    # `--version` is not configured; assert expected command registration instead.
-    command_names = [command.name for command in app.registered_commands]
-    assert "export" in command_names
+def test_cli_export_command_registered():
+    result = runner.invoke(app, ["export", "--help"])
+    assert result.exit_code == 0
+    assert "Output directory for exported artifacts." in result.stdout

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/tests/test_cli.py` around lines 11 - 14, The test
test_cli_version only checks app.registered_commands which can miss invocation
failures; update it to actually invoke the CLI for the "export" command (e.g.,
use click.testing.CliRunner to run "export --help"), assert the invocation
exit_code is 0 and that the output contains expected help text (like "Usage" or
"export"), and rename the test to something like test_cli_export_help to reflect
intent; update references to app.registered_commands only if still needed.

python/sqlrooms-cli/sqlrooms/web/launcher.py (1)

376-376: Consider reducing SQL logging sensitivity.

Line 376 logs user SQL at info level; this can capture literals/secrets. Prefer debug and/or literal redaction.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/sqlrooms/web/launcher.py` at line 376, Change the logging
of user SQL from info to a lower-sensitivity level and ensure literals are
redacted: replace the logger.info call that logs sql (the line invoking
logger.info("project_query sql=%s", _normalize_sql_for_policy(sql)[:2000])) with
a logger.debug call and/or pass a redacted version of sql (use or extend
_normalize_sql_for_policy to strip or mask string/number literals before
truncation) so SQL is not recorded at info level with sensitive literals.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

packages/webcontainer/src/WebContainerSlice.ts (1)

194-230: ⚠️ Potential issue | 🟠 Major

Set serverStatus to error on initialization failures.

If bootWebContainer()/mount()/install/start fails, initialize() throws but state may stay at initializing/install-deps. Persisting failure state makes recovery and UX consistent.

🛠️ Suggested fix

       initialize: async (opts) => {
@@
-        set((state) =>
-          produce(state, (draft) => {
-            draft.webContainer.serverStatus = {type: 'initializing'};
-          }),
-        );
-        const instance = await bootWebContainer();
-        await instance.mount(get().webContainer.config.filesTree);
-        set((state) =>
-          produce(state, (draft) => {
-            draft.webContainer.instance = instance;
-          }),
-        );
+        set((state) =>
+          produce(state, (draft) => {
+            draft.webContainer.serverStatus = {type: 'initializing'};
+          }),
+        );
+        try {
+          const instance = await bootWebContainer();
+          await instance.mount(get().webContainer.config.filesTree);
+          set((state) =>
+            produce(state, (draft) => {
+              draft.webContainer.instance = instance;
+            }),
+          );
@@
-        try {
-          const exitCode = await get().webContainer.installDependencies();
-          if (exitCode !== 0) {
-            throw new Error('Installation failed');
-          }
-        } catch (error) {
-          throw new Error(
-            `WebContainer initialization failed during dependency installation: ${String(error)}`,
-          );
-        }
-
-        try {
-          await get().webContainer.startDevServer();
-        } catch (error) {
-          throw new Error(
-            `WebContainer initialization failed while starting dev server: ${String(error)}`,
-          );
-        }
+          const exitCode = await get().webContainer.installDependencies();
+          if (exitCode !== 0) {
+            throw new Error('Installation failed');
+          }
+          await get().webContainer.startDevServer();
+        } catch (error) {
+          set((state) =>
+            produce(state, (draft) => {
+              draft.webContainer.serverStatus = {type: 'error', error};
+            }),
+          );
+          throw new Error(`WebContainer initialization failed: ${String(error)}`);
+        }
       },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/WebContainerSlice.ts` around lines 194 - 230, In
initialize() (WebContainerSlice.ts) ensure any failure updates
webContainer.serverStatus to an error state before rethrowing: in the try/catch
around bootWebContainer()/mount(), in the installDependencies() catch, and in
the startDevServer() catch call set(state => produce(...)) to set
draft.webContainer.serverStatus = { type: 'error', message: String(error) } (or
include stage info like 'install-deps'/'start-dev' in the message) and persist
draft.webContainer.instance = undefined if appropriate, then rethrow the error.

packages/webcontainer/src/components/TerminalView.tsx (1)

18-42: Consider suppressing lint warning for intentional stale closure.

The empty dependency array [] is intentional here—you want to capture output's initial value once on mount, with subsequent updates handled by the second effect. However, eslint-plugin-react-hooks will flag this as a missing dependency.

Consider adding an eslint-disable comment to make the intent explicit:

📝 Suggested clarification

+    // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally capture initial output only on mount
   }, []);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/components/TerminalView.tsx` around lines 18 - 42,
The useEffect that initializes Terminal (creating term, calling term.open on
containerRef.current, setting terminalRef.current and lastIndexRef.current based
on output) intentionally has an empty dependency array and should suppress the
eslint warning; add an eslint disable comment immediately above that useEffect
(// eslint-disable-next-line react-hooks/exhaustive-deps) and include a brief
inline comment explaining the intent (e.g., "intentional stale closure:
initialize once on mount; updates handled by separate effect") so the linter
won't flag the missing dependency on output while preserving the current
behavior.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

python/sqlrooms-cli/sqlrooms/web/launcher.py (1)

406-416: Error response structure is inconsistent with other endpoints.

Other endpoints like execute_query and fetch_arrow return {"error": str(exc)} directly. This endpoint wraps errors in {"type": "json", "data": {"error": ..., "message": ...}}. Additionally, the type wrapper appears on both success and error responses, which may complicate client handling.

Consider aligning with the existing pattern:

♻️ Proposed fix for consistent error format

             try:
                 data = await db_async.run_db_task(_run)
             except Exception as exc:
+                logger.debug("project_query failed: %s", exc)
                 return JSONResponse(
-                    {
-                        "type": "json",
-                        "data": {"error": "query_error", "message": str(exc)},
-                    },
+                    {"error": str(exc)},
                     status_code=400,
                 )
-            return {"type": "json", "data": data}
+            return data

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/sqlrooms/web/launcher.py` around lines 406 - 416, The
error response in the try/except around db_async.run_db_task(_run) uses a nested
{"type":"json","data":{...}} format while other handlers (e.g. execute_query,
fetch_arrow) return errors as {"error": str(exc)} and success payloads directly;
change the except block to return JSONResponse({"error": str(exc)},
status_code=400) and adjust the success return to return the data payload
directly (remove the {"type":"json","data":...} wrapper), updating the
JSONResponse use and the final return statement that currently returns
{"type":"json","data": data}.

python/sqlrooms-cli/sqlrooms/cli.py (2)

48-50: Redundant resolve() call.

out is already resolved at line 48 via .resolve(). The out_resolved assignment at line 50 is redundant—you can use out directly in the validation at line 107.

♻️ Suggested simplification

     out = Path(out_dir).expanduser().resolve()
     out.mkdir(parents=True, exist_ok=True)
-    out_resolved = out.resolve()

Then replace out_resolved with out at line 107:

-                root_resolved.relative_to(out_resolved)
+                root_resolved.relative_to(out)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/sqlrooms/cli.py` around lines 48 - 50, The code resolves
the output path twice: remove the redundant out_resolved variable by keeping out
= Path(out_dir).expanduser().resolve() and delete the out_resolved =
out.resolve() line; then update any uses of out_resolved (e.g., the validation
at line 107) to use out instead so all checks operate on the already-resolved
Path object (references: out, out_resolved, out_dir, Path).

---

87-87: Loop variable app shadows global app.

The loop variable shadows the module-level app = typer.Typer(...) at line 21. Consider renaming to sheet_app or app_data to avoid confusion during future maintenance.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/sqlrooms/cli.py` at line 87, The loop variable name "app"
shadows the module-level Typer instance "app" and should be renamed (e.g., to
"sheet_app" or "app_data") to avoid confusion; update the for-loop over
"apps_by_sheet.items()" and all references inside that loop to use the new name,
ensuring any uses of the module-level "app" elsewhere remain unchanged (refer to
the module-level "app = typer.Typer(...)" and the for loop "for sheet_id, app in
apps_by_sheet.items()" to locate the change).

packages/webcontainer/src/WebContainerSlice.ts (2)

547-551: Hardcoded fallback path may not exist in all project structures.

The fallback '/src/App.jsx' assumes a specific project structure. If the project uses TypeScript (.tsx) or a different entry point, this could result in attempting to open a non-existent file.

Consider using getFirstFilePathFromTree as a final fallback, which already exists in this file:

♻️ Suggested improvement

         const nextActive =
           nextActiveFromRequest ??
           nextActiveFromPrev ??
           preservedOpened[0]?.path ??
-          '/src/App.jsx';
+          getFirstFilePathFromTree(filesTree) ??
+          null;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/WebContainerSlice.ts` around lines 547 - 551, The
current fallback for nextActive uses a hardcoded '/src/App.jsx' which may not
exist; change the final fallback to call getFirstFilePathFromTree(...) instead
so nextActive is set to nextActiveFromRequest ?? nextActiveFromPrev ??
preservedOpened[0]?.path ?? getFirstFilePathFromTree(tree) (or appropriate
tree/roots value available in this scope), referencing nextActive,
nextActiveFromRequest, nextActiveFromPrev, preservedOpened and
getFirstFilePathFromTree to locate and update the line.

---

578-579: TODO: stderr capture is not implemented.

The function signature promises stderr in the return value but always returns an empty string. Consider either implementing stderr capture or documenting this limitation in the function's JSDoc.

Note: WebContainer's spawn output stream combines stdout and stderr by default. If separate streams are needed, you may need to explore terminal mode options.

,

Would you like me to help implement stderr capture or open an issue to track this?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/webcontainer/src/WebContainerSlice.ts` around lines 578 - 579, The
code currently declares const stderr = '' and never captures error output;
update the implementation around the WebContainer spawn handling in
WebContainerSlice.ts to actually collect stderr by (a) calling spawn with
terminal: false (or disabling combined terminal mode) so the returned
process.output emits objects with a type field, then append chunks where
chunk.type === 'stderr' to a mutable stderr string (similar to how stdout is
accumulated into stdout) and return it; alternatively, if you choose not to
change spawn/terminal mode, update the function JSDoc for the enclosing function
to document that stderr is not separately captured because WebContainer combines
streams by default.

apps/sqlrooms-cli-ui/src/components/AppBuilderSheet.tsx (1)

322-333: Validation logic appears redundant.

The missing check will never find any files because ensureRunnableViteScaffold (called on line 319) guarantees that /index.html, /src/main.jsx, /src/App.jsx, and /package.json all exist in the returned files array.

This validation could be useful if ensureRunnableViteScaffold behavior changes, but currently it adds complexity without providing value. Consider either:

1. Removing this dead code path, or
2. Moving the validation inside ensureRunnableViteScaffold where it would be meaningful

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/src/components/AppBuilderSheet.tsx` around lines 322 -
333, The block computing required/missing and erroring is redundant because
ensureRunnableViteScaffold already guarantees those paths; remove the
missing-files validation (the const required = new Set(...), const missing =
[...].filter(...), the missing.length check and errors.push/return error) from
AppBuilderSheet.tsx so the function simply returns {status: 'ok', attempts,
errors, files} after ensureRunnableViteScaffold, or alternatively move
equivalent validation into ensureRunnableViteScaffold if you prefer the check to
live with the scaffold creation logic; adjust any callers of
ensureRunnableViteScaffold if you choose to move the validation.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

python/sqlrooms-server/sqlrooms/server/server.py (1)

39-41: Duplicate _quote_ident implementation.

This function is identical to _quote_ident in python/sqlrooms-server/sqlrooms/server/db_async.py (lines 227-229). Consider importing from db_async to avoid duplication.

♻️ Proposed fix

-def _quote_ident(ident: str) -> str:
-    return '"' + ident.replace('"', '""') + '"'
+from .db_async import _quote_ident

Note: You may need to export _quote_ident from db_async.py if it's not already public.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-server/sqlrooms/server/server.py` around lines 39 - 41, The
_quote_ident implementation in server.py duplicates the identical function in
db_async.py; remove the duplicate by importing the single implementation from
db_async (exporting it there if necessary). Replace the local _quote_ident
definition in server.py with an import of _quote_ident from
sqlrooms.server.db_async (or adjust the export in db_async.py to make
_quote_ident public), and ensure all callers in server.py use the imported
symbol to avoid duplication.

Configuration used: defaults

Review profile: CHILL

Plan: Pro