builder files tab #2569
builder files tab #2569
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR adds file-management UI to the builder: a visible "Files" tab and enhancements to BuilderFilesTab (drag-and-drop upload, 5 MB size validation, base64 upload helper, context menu). It introduces RenameFileModal and DeleteFileModal and registers/exports them via modal registry and builder-apppanel. Backend changes replace WaveChatOpts.AppBuildStatus with AppStaticFiles and add generateBuilderAppData plus StaticFileInfo to return app.go contents and a JSON list of static files. openai-convertmessage.go centralizes appending message fragments. New utilities: ReadStaticFile/OpenStaticFile and frontend.util.arrayToBase64. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tsunami/app/defaultclient.go (1)
191-221: Consider extracting common validation logic.Both
ReadStaticFileandOpenStaticFileduplicate the path validation logic (lines 196-201 and 212-217). Extract this into a helper to reduce duplication and improve maintainability.Apply this refactor:
+// validateStaticPath validates and strips the "static/" prefix from the path. +// Returns the relative path and an error if validation fails. +func validateStaticPath(client *engine.ClientImpl, path string) (string, error) { + if client.StaticFS == nil { + return "", fs.ErrNotExist + } + if !strings.HasPrefix(path, "static/") { + return "", fs.ErrNotExist + } + return strings.TrimPrefix(path, "static/"), nil +} + // ReadStaticFile reads a file from the embedded static filesystem. // The path MUST start with "static/" (e.g., "static/config.json"). // Returns the file contents or an error if the file doesn't exist or can't be read. func ReadStaticFile(path string) ([]byte, error) { client := engine.GetDefaultClient() - if client.StaticFS == nil { - return nil, fs.ErrNotExist - } - if !strings.HasPrefix(path, "static/") { - return nil, fs.ErrNotExist + relativePath, err := validateStaticPath(client, path) + if err != nil { + return nil, err } - // Strip "static/" prefix since the FS is already sub'd to the static directory - relativePath := strings.TrimPrefix(path, "static/") return fs.ReadFile(client.StaticFS, relativePath) } // OpenStaticFile opens a file from the embedded static filesystem. // The path MUST start with "static/" (e.g., "static/config.json"). // Returns an fs.File or an error if the file doesn't exist or can't be opened. func OpenStaticFile(path string) (fs.File, error) { client := engine.GetDefaultClient() - if client.StaticFS == nil { - return nil, fs.ErrNotExist - } - if !strings.HasPrefix(path, "static/") { - return nil, fs.ErrNotExist + relativePath, err := validateStaticPath(client, path) + if err != nil { + return nil, err } - // Strip "static/" prefix since the FS is already sub'd to the static directory - relativePath := strings.TrimPrefix(path, "static/") return client.StaticFS.Open(relativePath) }pkg/aiusechat/usechat.go (1)
876-907: Consider surfacing errors or documenting silent failure behavior.The
generateBuilderAppDatafunction always returnsnilerror (line 906) and silently ignores failures fromReadAppFileandListAllAppFiles. This makes the error check at line 439 (if appErr == nil) always true, which is redundant.Either:
- Return actual errors and handle them in the caller, or
- Document that this function treats app data as optional context and never fails
If errors should be surfaced, apply this diff:
func generateBuilderAppData(appId string) (string, string, error) { appGoFile := "" fileData, err := waveappstore.ReadAppFile(appId, "app.go") - if err == nil { + if err != nil && !os.IsNotExist(err) { + return "", "", fmt.Errorf("failed to read app.go: %w", err) + } + if err == nil { appGoFile = string(fileData.Contents) } staticFilesJSON := "" allFiles, err := waveappstore.ListAllAppFiles(appId) + if err != nil { + return "", "", fmt.Errorf("failed to list app files: %w", err) + } - if err == nil { var staticFiles []StaticFileInfo for _, entry := range allFiles.Entries { if strings.HasPrefix(entry.Name, "static/") { staticFiles = append(staticFiles, StaticFileInfo{ Name: entry.Name, Size: entry.Size, Modified: entry.Modified, ModifiedTime: entry.ModifiedTime, }) } } if len(staticFiles) > 0 { staticFilesBytes, marshalErr := json.Marshal(staticFiles) - if marshalErr == nil { + if marshalErr != nil { + return "", "", fmt.Errorf("failed to marshal static files: %w", marshalErr) + } - staticFilesJSON = string(staticFilesBytes) - } + staticFilesJSON = string(staticFilesBytes) } - } return appGoFile, staticFilesJSON, nil }Or, if silent failure is intentional, add documentation:
+// generateBuilderAppData reads the app.go file and generates a JSON list of static files. +// Returns empty strings for missing data rather than errors, treating all builder data as optional context. func generateBuilderAppData(appId string) (string, string, error) {frontend/builder/tabs/builder-filestab.tsx (5)
12-12: Document the frontend file size limit and its relationship to the backend limit.The frontend enforces a 5 MB limit while the backend allows up to 50 MB (from relevant snippet in pkg/wshrpc/wshrpctypes.go). Consider:
- Adding a comment explaining why the frontend limit is more restrictive
- Verifying this limit is appropriate for the builder use case
-const MaxFileSize = 5 * 1024 * 1024; // 5MB +// MaxFileSize is the frontend limit for builder static files. +// This is more restrictive than the backend limit (50MB) to ensure +// reasonable builder app sizes and faster uploads. +const MaxFileSize = 5 * 1024 * 1024; // 5MB
21-27: Consider reusing the existing formatFileSize utility.A formatFileSize utility already exists in frontend/app/aipanel/ai-utils.ts (from relevant snippets) that supports up to GB. Consider importing and reusing it instead of defining a local version.
+import { formatFileSize } from "@/app/aipanel/ai-utils"; - -function formatFileSize(bytes: number): string { - if (bytes === 0) return "0 B"; - const k = 1024; - const sizes = ["B", "KB", "MB"]; - const i = Math.floor(Math.log(bytes) / Math.log(k)); - return `${(bytes / Math.pow(k, i)).toFixed(1)} ${sizes[i]}`; -}
125-125: Consider making the read-only file list configurable.The hardcoded check
entry.name === "static/tw.css"could be brittle if more framework-generated files are added in the future. Consider:
- Defining a constant array of read-only files, or
- Using a pattern-based check (e.g., files starting with "static/tw.")
+// Framework-generated files that should not be modified by users +const READ_ONLY_FILES = ["static/tw.css"]; + const fileEntries: FileEntry[] = result.entries .filter((entry) => !entry.dir && entry.name.startsWith("static/")) .map((entry) => ({ name: entry.name, size: entry.size || 0, modified: entry.modified, - isReadOnly: entry.name === "static/tw.css", + isReadOnly: READ_ONLY_FILES.includes(entry.name), })) .sort((a, b) => a.name.localeCompare(b.name));
161-169: Simplify base64 encoding.The manual conversion through Uint8Array and String.fromCharCode is more complex than needed. Modern browsers support direct base64 encoding.
const arrayBuffer = await file.arrayBuffer(); -const uint8Array = new Uint8Array(arrayBuffer); -const base64 = btoa(String.fromCharCode(...uint8Array)); +const base64 = btoa(String.fromCharCode(...new Uint8Array(arrayBuffer)));
302-328: Context menu positioning may clip at viewport edges.The context menu uses absolute positioning (
left: contextMenu.x, top: contextMenu.y) without checking viewport boundaries. This could cause the menu to render off-screen near window edges. Consider adding edge detection and adjustment logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
frontend/builder/builder-apppanel.tsx(3 hunks)frontend/builder/tabs/builder-filestab.tsx(1 hunks)pkg/aiusechat/openai/openai-convertmessage.go(2 hunks)pkg/aiusechat/uctypes/usechat-types.go(1 hunks)pkg/aiusechat/usechat.go(3 hunks)tsunami/app/defaultclient.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T03:21:02.229Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: pkg/aiusechat/tools_readfile.go:197-197
Timestamp: 2025-10-15T03:21:02.229Z
Learning: In Wave Terminal's AI tool definitions (pkg/aiusechat/tools_*.go), the Description field should not mention approval requirements even when ToolApproval returns ApprovalNeedsApproval. This prevents the LLM from asking users for approval before calling the tool, avoiding redundant double-approval prompts since the runtime will enforce approval anyway.
Applied to files:
pkg/aiusechat/uctypes/usechat-types.go
🧬 Code graph analysis (4)
pkg/aiusechat/openai/openai-convertmessage.go (1)
pkg/aiusechat/openai/openai-backend.go (2)
OpenAIMessage(54-57)OpenAIMessageContent(82-94)
tsunami/app/defaultclient.go (1)
tsunami/engine/clientimpl.go (1)
GetDefaultClient(116-118)
pkg/aiusechat/usechat.go (1)
pkg/waveappstore/waveappstore.go (2)
ReadAppFile(289-318)ListAllAppFiles(441-456)
frontend/builder/tabs/builder-filestab.tsx (9)
pkg/wshrpc/wshrpctypes.go (1)
MaxFileSize(27-27)frontend/app/aipanel/ai-utils.ts (1)
formatFileSize(250-256)frontend/builder/builder-apppanel.tsx (1)
RenameFileModal(361-361)frontend/app/view/preview/preview-directory-utils.tsx (1)
handleRename(87-134)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)frontend/app/store/wshclientapi.ts (1)
RpcApi(677-677)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/modals/modal.tsx (1)
Modal(116-116)frontend/app/store/global.ts (1)
atoms(816-816)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
pkg/aiusechat/uctypes/usechat-types.go (1)
464-464: LGTM!The field rename from
AppBuildStatustoAppStaticFilesis clear and aligns with the updated functionality throughout the PR.frontend/builder/builder-apppanel.tsx (1)
12-12: LGTM!The integration of the Files tab and RenameFileModal is clean and follows the existing tab pattern consistently.
Also applies to: 294-300, 361-361
pkg/aiusechat/usechat.go (1)
869-874: LGTM!The
StaticFileInfostruct is well-defined and provides appropriate metadata for static files.frontend/builder/tabs/builder-filestab.tsx (1)
29-99: LGTM!The RenameFileModal component is well-implemented with proper validation, error handling, and keyboard support (Enter key handling with composition check).
pkg/aiusechat/openai/openai-convertmessage.go (1)
73-86: Verify that silent failure when no user message exists is acceptable behavior.The
appendToLastUserMessagefunction silently does nothing if theinputsarray contains no user messages. This can occur in scenarios with only function calls or tool results—the optional context fields (TabState,AppStaticFiles,AppGoFile) would be ignored without warning.Confirm whether this is intentional or if the code should:
- Validate a user message exists before attempting to append context
- Log a warning when context cannot be appended
- Handle cases where no user message is available
No description provided.