feat: integrate sync eng #2924
feat: integrate sync eng #2924
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplace sandbox-centered file and template management with a branch-scoped CodeFileSystem and FS-driven sync; refactor the right-panel code editor to a per-file filesystem model with forwarded ref APIs; propagate branchId across models, tools, and preload/frame APIs; remove multiple legacy image/sync/template/preload subsystems and several test/demo pages; add path/filename utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as CodeTab (UI)
participant Tree as FileTree
participant FS as CodeFileSystem
participant Sync as SyncEngine
participant SB as Sandbox
UI->>Tree: open(filePath)
Tree->>FS: readFile(filePath)
FS-->>Tree: content
Tree-->>UI: render editor
UI->>FS: writeFile(path, content)
FS->>FS: update JSX index (if JSX/TSX)
FS-->>UI: ack
par background sync
Sync->>FS: identify modified files
Sync->>SB: pushModifiedFilesToSandbox(files)
SB-->>Sync: ack
end
sequenceDiagram
autonumber
participant Parent as App (Parent)
participant Child as IFrame (Preload)
participant State as Preload State
Parent->>Child: setFrameId(...)
Parent->>Child: setBranchId(branchId)
Child->>State: store frameId/branchId
Child->>Parent: getBranchId() (if missing)
Parent-->>Child: branchId
Note over Parent,Child: branchId flows into metadata lookups and tool ops
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(1 hunks)apps/web/client/src/app/test-sync-engine/page.tsx(1 hunks)apps/web/client/src/components/store/editor/sandbox/index.ts(7 hunks)apps/web/client/src/services/sync-engine/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/services/sync-engine/index.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_hooks/use-start-project.tsxapps/web/client/src/app/test-sync-engine/page.tsxapps/web/client/src/components/store/editor/sandbox/index.tsapps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/services/sync-engine/index.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_hooks/use-start-project.tsxapps/web/client/src/app/test-sync-engine/page.tsxapps/web/client/src/components/store/editor/sandbox/index.tsapps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/services/sync-engine/index.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_hooks/use-start-project.tsxapps/web/client/src/app/test-sync-engine/page.tsxapps/web/client/src/components/store/editor/sandbox/index.tsapps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_hooks/use-start-project.tsxapps/web/client/src/app/test-sync-engine/page.tsxapps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_hooks/use-start-project.tsxapps/web/client/src/app/test-sync-engine/page.tsxapps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_hooks/use-start-project.tsxapps/web/client/src/app/test-sync-engine/page.tsxapps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx
🧠 Learnings (3)
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Applied to files:
apps/web/client/src/services/sync-engine/index.tsapps/web/client/src/app/test-sync-engine/page.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Use path aliases @/* and ~/* for imports mapping to src/*
Applied to files:
apps/web/client/src/services/sync-engine/index.tsapps/web/client/src/app/test-sync-engine/page.tsx
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
🧬 Code graph analysis (1)
apps/web/client/src/components/store/editor/sandbox/index.ts (4)
apps/web/client/src/components/store/editor/sandbox/session.ts (1)
SessionManager(8-217)packages/file-system/src/fs.ts (1)
FileSystem(13-562)packages/models/src/sandbox/files.ts (1)
SandboxFile(17-17)packages/code-provider/src/types.ts (2)
ListFilesOutputFile(49-53)WatchEvent(102-105)
🔇 Additional comments (3)
apps/web/client/src/services/sync-engine/index.ts (1)
2-2: Re-export path update looks goodUsing the hyphenated module path keeps the barrel export aligned with the renamed file; no further changes needed.
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
56-60: Reconnection trigger hooked up cleanlyThe new effect ties the reconnect to tab reactivation while pulling from the centralized
sandbox.session, which lines up with the broader sync-engine migration.apps/web/client/src/app/test-sync-engine/page.tsx (1)
13-13: Import path update aligns with new module layoutThanks for updating the hook import to the hyphenated path; this keeps the test page tracking the sync-engine re-export structure.
apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/index.tsx
Outdated
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
Outdated
Show resolved
Hide resolved
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
|
|
||
| // File system | ||
| const rootDir = `/${editorEngine.projectId}/${editorEngine.branches.activeBranch.id}`; | ||
| // const { fs, isInitializing: fsInitializing, error: fsError } = useFS(rootDir); |
There was a problem hiding this comment.
There is commented-out code for the useFS hook. Remove unused code if it’s no longer required to reduce confusion.
| // const { fs, isInitializing: fsInitializing, error: fsError } = useFS(rootDir); |
| // Get file icon based on extension | ||
| const getFileIcon = () => { | ||
| const extension = node.data.extension?.toLowerCase(); | ||
| const extension = node.data.path.split('.').pop()?.toLowerCase(); |
There was a problem hiding this comment.
The file icon logic attempts to check for extensions starting with '.', but the extracted extension from 'split' does not include the dot. Adjust the case labels (e.g. use 'js' instead of '.js') or prepend the dot to the extracted extension.
| const extension = node.data.path.split('.').pop()?.toLowerCase(); | |
| const extension = '.' + node.data.path.split('.').pop()?.toLowerCase(); |
| {contextMenuItems.find(item => item.label === 'Create File') && ( | ||
| <button | ||
| onClick={() => { | ||
| const fileName = prompt('Enter file name:'); |
There was a problem hiding this comment.
User input is obtained via prompt for file and directory creation without sanitization. Consider adding basic input validation to avoid potential issues.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx (1)
47-79: Restore file-icon matching by fixing the extension switch
extensionis now justjs,tsx, etc., but the switch still looks for values like.js. Because the cases never match, every non-directory fallback renders the default icon. Update the switch (or reintroduce the dot prefix) so icons reflect actual file types again.Apply this diff to normalize the extension handling:
- const extension = node.data.path.split('.').pop()?.toLowerCase(); + const extension = node.data.path.split('.').pop()?.toLowerCase(); ... - switch (extension) { - case '.js': - case '.jsx': - case '.ts': - case '.tsx': + switch (`.${extension ?? ''}`) { + case '.js': + case '.jsx': + case '.ts': + case '.tsx':apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx (1)
435-438: Avoid treating directories as files in the tree selection
FileNodeno longer exposesisDirectory, so this check always treats directories as files. Selecting a folder now tries to open it and will error. Switch the predicate to the newtype === 'directory'shape before invokingloadFile.- if (nodes.length > 0 && !nodes[0].data.isDirectory) { + if (nodes.length > 0 && nodes[0].data.type !== 'directory') {apps/web/client/src/services/sync-engine/sync-engine.ts (1)
342-375: Restore exclude filtering during recursive directory syncThe new
syncDirectoryContentstraversal no longer callsshouldSyncon nested entries. If a sandbox change arrives for an allowed parent (e.g.src/), we now blindly recreate excluded descendants such assrc/node_modules/**. That defeats the exclusion list and can overwhelm the local FS with large folders we explicitly skip. Please reintroduce the filter before creating directories or reading files.- for (const item of dirContents.files) { + for (const item of dirContents.files) { const itemSandboxPath = `${sandboxPath}/${item.name}`; const itemLocalPath = `${localDirPath}/${item.name}`; + if (!this.shouldSync(itemLocalPath)) { + continue; + } + if (item.type === 'directory') { // Create subdirectory console.log(`[Sync] Creating subdirectory ${itemLocalPath}`);
🧹 Nitpick comments (4)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsx (1)
12-34: Localize the dialog copyThis component hardcodes user-facing strings. Per our web-client guidelines, route this through next-intl (or the established localization helper) so the dialog can be translated.
As per coding guidelines
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor-area.tsx (1)
33-45: Externalize the user-facing stringsStrings such as “Loading file...” and “Open a file or select an element on the page.” should be sourced from next-intl rather than hardcoded so that we comply with localization guidelines. Please move them into the message catalogs and pull them in via the intl hooks.
As per coding guidelines
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx (1)
27-238: Localize the sidebar stringsLabels like “Files”, “No files found”, “Search files”, and “Refresh files” are hardcoded. Please surface them through the next-intl messages so that the sidebar can be translated.
As per coding guidelines
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsx (1)
4-4: Prefer the configured path alias for local importsPer the client coding guidelines we should use the
@/*/~/*aliases for imports underapps/web/client/src. Please switch this relative import to the alias-based path.-import type { FileNode } from '../types'; +import type { FileNode } from '@/app/project/[id]/_components/right-panel/code-tab/types';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-block.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-diff.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/code-controls.tsx(0 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor-area.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/file-tab.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx(6 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx(3 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx(10 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/types.ts(1 hunks)apps/web/client/src/components/store/editor/ide/index.ts(1 hunks)apps/web/client/src/components/store/editor/sandbox/index.ts(7 hunks)apps/web/client/src/services/sync-engine/sync-engine.ts(6 hunks)packages/models/src/code/index.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/models/src/code/index.ts
- apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/code-controls.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/web/client/src/components/store/editor/ide/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/types.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/services/sync-engine/sync-engine.tsapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-diff.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor-area.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/components/store/editor/sandbox/index.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/types.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/services/sync-engine/sync-engine.tsapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-diff.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor-area.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/components/store/editor/sandbox/index.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/types.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-diff.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor-area.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/types.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/services/sync-engine/sync-engine.tsapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-diff.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor-area.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/components/store/editor/sandbox/index.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-diff.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor-area.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-diff.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor-area.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-block.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
🧠 Learnings (3)
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Use path aliases @/* and ~/* for imports mapping to src/*
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsx
🧬 Code graph analysis (6)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx (3)
apps/web/client/src/components/store/editor/ide/index.ts (1)
EditorFile(8-17)apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-mirror-config.ts (2)
getBasicSetup(254-281)getExtensions(309-326)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor-area.tsx (4)
apps/web/client/src/components/store/editor/ide/index.ts (1)
EditorFile(8-17)apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx (1)
CodeEditor(17-85)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsx (1)
UnsavedChangesDialog(9-39)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx (4)
apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/types.ts (1)
FileNode(1-6)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor-area.tsx (1)
CodeEditorArea(17-75)apps/web/client/src/components/store/editor/ide/index.ts (1)
updateFileContent(143-152)
apps/web/client/src/components/store/editor/sandbox/index.ts (3)
apps/web/client/src/components/store/editor/sandbox/session.ts (1)
SessionManager(8-217)packages/file-system/src/fs.ts (1)
FileSystem(13-562)apps/web/client/src/services/sync-engine/sync-engine.ts (1)
CodeProviderSync(27-631)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsx (1)
packages/ui/src/components/button.tsx (1)
Button(57-57)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx (1)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx (1)
FileTreeNode(23-174)
🔇 Additional comments (7)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-block.tsx (1)
1-1: Import realignment looks good.The updated alias matches the new file-content location for the CodeMirror config and keeps the module resolution consistent across the refactor.
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/code-display/code-diff.tsx (1)
4-4: Import path realignment looks good.Line 4 now points to the relocated CodeMirror config under
file-content, matching the refactor.apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx (1)
1-85: FixEditorSelectionimport to prevent build break
EditorSelectionis defined in@codemirror/state, not@uiw/react-codemirror. The current import will fail TypeScript/webpack resolution, breaking the build. Point the import back to@codemirror/state.-import { EditorView } from '@codemirror/view'; -import CodeMirror from '@uiw/react-codemirror'; +import { EditorView } from '@codemirror/view'; +import { EditorSelection } from '@codemirror/state'; +import CodeMirror from '@uiw/react-codemirror'; - import { EditorSelection } from '@uiw/react-codemirror';Likely an incorrect or invalid review comment.
apps/web/client/src/components/store/editor/sandbox/index.ts (2)
267-275: Restore functionalreadFileimplementation
readFilenow always returnsnull, so every call site (e.g.,editorEngine.activeSandbox.readFilein the code editor) will fail to load file contents, effectively bricking the editor. We need to wire this up to the new filesystem (or restore the previous provider-backed logic) before merging.
290-322:writeFilemust persist editor savesReturning
falseshort-circuits all writes. Callers such asprocessFileForMappingand user-initiated saves silently fail, so no edits ever reach the sandbox or local FS. Please re-implement this to write through the new FileSystem (and trigger the sync) or reinstate the prior provider-backed flow.apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/file-tab.tsx (1)
13-20: Plain-function export keeps typings sharp.The move away from
React.FCremoves implicit children and aligns with our preferred style while keeping thedata-activedestructuring nicely typed.apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/types.ts (1)
1-6: Local FileNode mirrors the new tree usage well.With name/path/type spelled out here, the sidebar components get a clean, recursive structure to rely on—looks solid.
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
Outdated
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
Outdated
Show resolved
Hide resolved
| async delete(path: string, recursive?: boolean): Promise<boolean> { | ||
| if (!this.session.provider) { | ||
| console.error('No provider found for delete file'); | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| const normalizedPath = normalizePath(path); | ||
|
|
||
| // Check if file exists before attempting to delete | ||
| const exists = await this.fileExists(normalizedPath); | ||
| if (!exists) { | ||
| console.error(`File ${normalizedPath} does not exist`); | ||
| return false; | ||
| } | ||
|
|
||
| // Delete the file using the filesystem API | ||
| await this.session.provider.deleteFiles({ | ||
| args: { | ||
| path: normalizedPath, | ||
| recursive, | ||
| }, | ||
| }); | ||
|
|
||
| // Clean up the file sync cache | ||
| await this.fileSync.delete(normalizedPath); | ||
|
|
||
| // Publish file deletion event | ||
| this.fileEventBus.publish({ | ||
| type: 'remove', | ||
| paths: [normalizedPath], | ||
| timestamp: Date.now(), | ||
| }); | ||
|
|
||
| console.log(`Successfully deleted file: ${normalizedPath}`); | ||
| return true; | ||
| } catch (error) { | ||
| console.error(`Error deleting file ${path}:`, error); | ||
| return false; | ||
| } | ||
| return false; | ||
| // if (!this.session.provider) { | ||
| // console.error('No provider found for delete file'); | ||
| // return false; | ||
| // } | ||
|
|
||
| // try { | ||
| // const normalizedPath = normalizePath(path); | ||
|
|
||
| // // Check if file exists before attempting to delete | ||
| // const exists = await this.fileExists(normalizedPath); | ||
| // if (!exists) { | ||
| // console.error(`File ${normalizedPath} does not exist`); | ||
| // return false; | ||
| // } | ||
|
|
||
| // // Delete the file using the filesystem API | ||
| // await this.session.provider.deleteFiles({ | ||
| // args: { | ||
| // path: normalizedPath, | ||
| // recursive, | ||
| // }, | ||
| // }); | ||
|
|
||
| // // Clean up the file sync cache | ||
| // await this.fileSync.delete(normalizedPath); | ||
|
|
||
| // // Publish file deletion event | ||
| // this.fileEventBus.publish({ | ||
| // type: 'remove', | ||
| // paths: [normalizedPath], | ||
| // timestamp: Date.now(), | ||
| // }); | ||
|
|
||
| // console.log(`Successfully deleted file: ${normalizedPath}`); | ||
| // return true; | ||
| // } catch (error) { | ||
| // console.error(`Error deleting file ${path}:`, error); | ||
| // return false; | ||
| // } | ||
| } |
There was a problem hiding this comment.
Implement file deletion against the new FS/provider
delete now just returns false, which means UI flows like the file-tree “Delete” action never remove anything. Please hook this up to the FileSystem (and let the sync propagate upstream) or call the provider API so deletions actually happen.
🤖 Prompt for AI Agents
In apps/web/client/src/components/store/editor/sandbox/index.ts around lines 702
to 743, the delete method currently returns false and is not wired to the new
FS/provider; replace that stub with logic that: verify this.session.provider
exists and throw or return false if absent, normalize the input path, optionally
check file existence via this.fileExists(normalizedPath), call the provider
filesystem delete API (e.g., this.session.provider.deleteFiles or the FS client)
with the normalized path and recursive flag, await completion, remove any local
cache via await this.fileSync.delete(normalizedPath), publish a 'remove' event
on this.fileEventBus with paths and timestamp, and wrap the flow in try/catch to
log errors and return true on success or false on failure.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/modals/file-modal.tsx (1)
21-22: Use the project path alias instead of a relative import.Per our client-side guidelines we should prefer the
@/*(or~/*) alias for anything underapps/web/client/src. Please update these two imports to use the alias so they stay resilient to directory reshuffles.-import { - createFileInSandbox, - doesFileExist, - validateFileName, -} from '../shared/file-operations'; -import { getFileTemplate } from '../shared/file-templates'; +import { + createFileInSandbox, + doesFileExist, + validateFileName, +} from '@/app/project/[id]/_components/right-panel/code-tab/shared/file-operations'; +import { getFileTemplate } from '@/app/project/[id]/_components/right-panel/code-tab/shared/file-templates';apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx (1)
64-68: Drop debug logging before mergePlease remove the stray
console.logso we don’t leak noisy debug output into production bundles.- console.log('localFiles', localFiles); - // _____________________________________________________ -apps/web/client/src/app/test-local-fs/page.tsx (1)
21-21: Switch FileExplorer import to the @ aliasPer our client-side guidelines we should reference modules under
apps/web/client/srcvia the configured@/(or~/) alias instead of relative paths, so let’s update this import accordingly.-import { FileExplorer, type FileNode } from '../test-sync-engine/_components/file-explorer'; +import { FileExplorer, type FileNode } from '@/app/test-sync-engine/_components/file-explorer';apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsx (1)
51-74: Externalize the dropdown/tooltip copyThe new UI strings (
'Collapse sidebar','Expand sidebar','Close file','Close all') should flow throughnext-intlrather than being inlined so we stay compliant with the app directory i18n rule. Please wire these throughuseTranslations(or the existing messages helper for this surface) before merging.apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsx (1)
4-24: Use the project path alias for importsThe shared type lives under
apps/web/client/src, so please swap this relative import for the configured@/…alias to stay aligned with our client-side import guidelines.Apply this diff:
-import type { FileNode } from '../shared/types'; +import type { FileNode } from '@/app/project/[id]/_components/right-panel/code-tab/shared/types';Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/file-tab.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx(5 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/modals/file-modal.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/modals/folder-modal.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx(1 hunks)apps/web/client/src/app/test-local-fs/page.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/modals/folder-modal.tsx
👮 Files not reviewed due to content moderation or server errors (1)
- apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/modals/file-modal.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/test-local-fs/page.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/modals/file-modal.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/test-local-fs/page.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/modals/file-modal.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/test-local-fs/page.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/modals/file-modal.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/test-local-fs/page.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/modals/file-modal.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/test-local-fs/page.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/modals/file-modal.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/test-local-fs/page.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/file-tab.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx
🧠 Learnings (10)
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsxapps/web/client/src/app/test-local-fs/page.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Use path aliases @/* and ~/* for imports mapping to src/*
Applied to files:
apps/web/client/src/app/test-local-fs/page.tsx
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to **/*.{ts,tsx} : Do not use the any type unless necessary
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to {apps,packages}/**/*.{ts,tsx} : Avoid using the any type unless absolutely necessary
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/**/*.tsx : observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/app/**/*.{ts,tsx} : Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Avoid hardcoded user-facing text; use next-intl messages/hooks
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/trpc/react.tsx : Keep tRPC React client provider(s) behind a client boundary (ensure this provider file is a client component)
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
🧬 Code graph analysis (5)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsx (2)
apps/web/client/src/components/store/editor/ide/index.ts (1)
EditorFile(8-17)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/file-tab.tsx (1)
FileTab(13-74)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx (6)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts (1)
FileNode(1-6)packages/ui/src/components/icons/index.tsx (1)
Icons(138-3667)packages/ui/src/components/tooltip.tsx (4)
Tooltip(72-72)TooltipTrigger(72-72)TooltipPortal(72-72)TooltipContent(72-72)packages/ui/src/components/button.tsx (1)
Button(57-57)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-row.tsx (1)
FileTreeRow(6-29)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx (1)
FileTreeNode(21-166)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx (4)
apps/web/client/src/components/store/editor/ide/index.ts (1)
EditorFile(8-17)apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx (1)
CodeEditor(17-85)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsx (1)
UnsavedChangesDialog(9-39)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx (6)
apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts (1)
FileNode(1-6)apps/web/client/src/components/store/editor/ide/index.ts (6)
getElementCodeRange(254-277)isSandboxReady(44-49)EditorFile(8-17)closeFile(195-203)closeAllFiles(205-209)updateFileContent(143-152)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx (1)
FileTree(22-284)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsx (1)
FileTabs(27-93)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx (1)
CodeEditorArea(17-75)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx (3)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts (1)
FileNode(1-6)packages/ui/src/components/icons/index.tsx (1)
Icons(138-3667)packages/ui/src/components/context-menu.tsx (3)
ContextMenuTrigger(226-226)ContextMenuContent(214-214)ContextMenuSeparator(221-221)
🪛 GitHub Actions: CI
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx
[error] 75-75: Variable 'menuItems' implicitly has type 'any[]' in some locations where its type cannot be determined.
[error] 148-148: Variable 'menuItems' implicitly has an 'any[]' type.
🔇 Additional comments (3)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx (1)
41-72: Fix extension matching so file icons resolve
split('.').pop()returns the extension without the leading dot, so none of thecase '.ts'style branches ever fire; every file falls through to the default icon. Restore the dot before switching.- const extension = node.data.path.split('.').pop()?.toLowerCase(); + const extension = (() => { + const lastDot = node.data.path.lastIndexOf('.'); + return lastDot >= 0 ? node.data.path.slice(lastDot).toLowerCase() : ''; + })();apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx (1)
22-31: Replaceanyon the forwarded tree refThe ref is a
TreeApi<FileNode>; keeping it typed avoidsanyleakage and lets callers use the API safely.-export const FileTree = memo( - forwardRef<any, FileTreeProps>(({ +export const FileTree = memo( + forwardRef<TreeApi<FileNode>, FileTreeProps>(({apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/file-tab.tsx (1)
13-74: Signature simplification looks goodThanks for inlining the prop typing and dropping
React.FC; the component reads cleaner and keeps the inferred return type intact.
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
Outdated
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
Show resolved
Hide resolved
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
Outdated
Show resolved
Hide resolved
.../web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx
Outdated
Show resolved
Hide resolved
| const getFileUrl = (file: BinaryEditorFile) => { | ||
| const mime = getMimeType(file.path.toLowerCase()); | ||
| // Convert Uint8Array to base64 string | ||
| const base64 = btoa(String.fromCharCode(...new Uint8Array(file.content))); |
There was a problem hiding this comment.
Spreading the Uint8Array into String.fromCharCode may hit argument limits for large files. Consider a more memory‐efficient conversion.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/client/src/components/store/editor/ide/index.ts (1)
17-18: Restore theEditorFiletype before using it.
EditorFileis no longer declared or imported in this module, yet we still typeopenedFiles(and other members) with it. TypeScript now fails to compile (Cannot find name 'EditorFile', see CI). Please reintroduce the interface (or import/extend the shared type) so the IDE store retains theid,filename,savedContent,language, andisBinaryfields it depends on.+export interface EditorFile { + id: string; + filename: string; + path: string; + content: string; + savedContent: string; + language: string; + isDirty: boolean; + isBinary: boolean; +}apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx (1)
121-126: AligneditorViewsRefkeys with the lookup.We store editor instances under
file.path(editorViewsRef.current.set(file.path, editor)), yet this lookup still useside.activeFile.id, so it always returnsundefined. Make both sides use the same key (path or id) to keep highlighting/search functioning.
🧹 Nitpick comments (1)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-search.tsx (1)
50-55: Set button type to avoid unintended form submission.Without
type="button", this clear control defaults tosubmit, which can break when the search lives inside a<form>. Explicitly set the type.{searchQuery && ( <button + type="button" className="absolute right-[1px] top-[1px] bottom-[1px] aspect-square hover:bg-background-onlook active:bg-transparent flex items-center justify-center rounded-r-[calc(theme(borderRadius.md)-1px)] group" onClick={clearSearch} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx(5 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-search.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx(1 hunks)apps/web/client/src/components/store/editor/ide/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts
- apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx
- apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
- apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsx
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/components/store/editor/ide/index.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-search.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/components/store/editor/ide/index.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-search.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/components/store/editor/ide/index.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-search.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-search.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-search.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-search.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
🧠 Learnings (6)
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/app/**/*.{ts,tsx} : Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/**/*.tsx : observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Avoid hardcoded user-facing text; use next-intl messages/hooks
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/trpc/react.tsx : Keep tRPC React client provider(s) behind a client boundary (ensure this provider file is a client component)
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
🧬 Code graph analysis (4)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx (2)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts (1)
FileNode(1-6)packages/ui/src/components/context-menu.tsx (3)
ContextMenuTrigger(226-226)ContextMenuContent(214-214)ContextMenuSeparator(221-221)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-search.tsx (3)
packages/ui/src/components/icons/index.tsx (1)
Icons(138-3667)packages/ui/src/components/tooltip.tsx (4)
Tooltip(72-72)TooltipTrigger(72-72)TooltipPortal(72-72)TooltipContent(72-72)packages/ui/src/components/button.tsx (1)
Button(57-57)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx (3)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts (1)
EditorFile(8-14)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx (1)
CodeEditor(18-93)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsx (1)
UnsavedChangesDialog(9-39)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx (6)
apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts (4)
EditorFile(8-14)FileNode(1-6)BinaryEditorFile(24-28)TextEditorFile(16-21)apps/web/client/src/components/store/editor/ide/index.ts (3)
getElementCodeRange(244-267)isSandboxReady(34-39)getFilePathFromOid(228-242)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx (1)
FileTree(19-235)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsx (1)
FileTabs(27-93)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx (1)
CodeEditorArea(17-62)
🪛 GitHub Actions: CI
apps/web/client/src/components/store/editor/ide/index.ts
[error] 17-18: Cannot find name 'EditorFile'.
[error] 18-18: Cannot find name 'EditorFile'.
[error] 57-95: Cannot find name 'EditorFile'.
[error] 98-98: Cannot find name 'EditorFile'.
[error] 221-221: Cannot find name 'EditorFile'.
🔇 Additional comments (3)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx (1)
41-72: Align the extension switch with the parsed valueReiterating the earlier feedback:
split('.').pop()returnsjsx,tsx, etc. (without the dot), so none of thesecase '.tsx'branches ever match. Every file falls through to the default icon. Update the switch (or prepend the dot toextension) so the right icon renders.- const extension = node.data.path.split('.').pop()?.toLowerCase(); + const extension = node.data.path.split('.').pop()?.toLowerCase() ?? ''; @@ - switch (extension) { - case '.js': - case '.jsx': - case '.ts': - case '.tsx': + switch (extension) { + case 'js': + case 'jsx': + case 'ts': + case 'tsx': return <Icons.Code className="w-4 h-4 mr-2" />; - case '.css': - case '.scss': - case '.sass': + case 'css': + case 'scss': + case 'sass': return <Icons.Box className="w-4 h-4 mr-2" />; - case '.html': + case 'html': return <Icons.Frame className="w-4 h-4 mr-2" />; - case '.json': + case 'json': return <Icons.Code className="w-4 h-4 mr-2" />; - case '.md': - case '.mdx': + case 'md': + case 'mdx': return <Icons.Text className="w-4 h-4 mr-2" />; - case '.jpg': - case '.jpeg': - case '.png': - case '.gif': - case '.svg': + case 'jpg': + case 'jpeg': + case 'png': + case 'gif': + case 'svg': return <Icons.Image className="w-4 h-4 mr-2" />; default: return <Icons.File className="w-4 h-4 mr-2" />; }apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx (2)
1-14: Add'use client';at the top of this component.This module invokes hooks (
useEditorEngine,useState,useEffect, …), but without the client directive Next.js will treat it as a Server Component and crash. Add'use client';before the imports.+'use client'; + import { useEditorEngine } from '@/components/store/editor';
505-521: Respectide.isFilesVisiblewhen rendering the sidebar.
onToggleFilesVisiblestill flips the flag, but we always render<FileTree />, so the collapse button no longer does anything. Gate the render (or width) onide.isFilesVisibleso the toggle actually hides the panel.- <FileTree - onFileSelect={handleFileTreeSelect} - fileNodes={localFiles} - isLoading={localDirLoading} - selectedFilePath={selectedFile} - /> + {ide.isFilesVisible && ( + <FileTree + onFileSelect={handleFileTreeSelect} + fileNodes={localFiles} + isLoading={localDirLoading} + selectedFilePath={selectedFile} + /> + )}
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
Show resolved
Hide resolved
| {activeFile?.isDirty && showUnsavedDialog && ( | ||
| <UnsavedChangesDialog | ||
| onSave={onSaveFile} | ||
| onDiscard={() => onDiscardChanges(activeFile.path)} | ||
| onCancel={() => { onCancelUnsaved(); }} |
There was a problem hiding this comment.
Pass the file id into onDiscardChanges.
onDiscardChanges ultimately calls ide.discardFileChanges, which looks up files by id. Forwarding activeFile.path means the lookup always fails and the discard flow silently does nothing. Please keep passing the file id (e.g. activeFile.id) once the type is restored.
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
around lines 53 to 57, the UnsavedChangesDialog is calling onDiscardChanges with
activeFile.path which fails because ide.discardFileChanges looks up files by id;
change the prop to pass activeFile.id (e.g. onDiscard={() =>
onDiscardChanges(activeFile.id)}) so the discard flow can find and remove the
file; ensure the value is only used once the activeFile type includes id.
| if (!selectedFile || !loadedContent) return; | ||
|
|
||
| const newLocalFile = createLocalEditorFile(selectedFile, loadedContent); | ||
|
|
||
| // Check if file is already open | ||
| const existingFileIndex = openedLocalFiles.findIndex(f => f.path === selectedFile); | ||
|
|
||
| if (existingFileIndex >= 0) { | ||
| // File already open, just set as active and update content | ||
| const existingFile = openedLocalFiles[existingFileIndex]; | ||
| if (existingFile) { | ||
| const updatedFile: EditorFile = { | ||
| path: existingFile.path, | ||
| isDirty: existingFile.isDirty, | ||
| type: existingFile.type, | ||
| originalContent: existingFile.originalContent, | ||
| content: newLocalFile.content | ||
| }; | ||
| const updatedFiles = [...openedLocalFiles]; | ||
| updatedFiles[existingFileIndex] = updatedFile; | ||
| setOpenedLocalFiles(updatedFiles); | ||
| setActiveLocalFile(updatedFile); | ||
| } | ||
| } else { | ||
| // Add new file to opened files | ||
| const updatedFiles = [...openedLocalFiles, newLocalFile]; | ||
| setOpenedLocalFiles(updatedFiles); | ||
| setActiveLocalFile(newLocalFile); | ||
| } | ||
| }, [selectedFile, loadedContent]); |
There was a problem hiding this comment.
Handle empty file contents correctly when opening.
The guard if (!selectedFile || !loadedContent) short-circuits when the file contents are an empty string, so blank files never load. Check explicitly for null/undefined instead.
- if (!selectedFile || !loadedContent) return;
+ if (!selectedFile || loadedContent === null || loadedContent === undefined) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!selectedFile || !loadedContent) return; | |
| const newLocalFile = createLocalEditorFile(selectedFile, loadedContent); | |
| // Check if file is already open | |
| const existingFileIndex = openedLocalFiles.findIndex(f => f.path === selectedFile); | |
| if (existingFileIndex >= 0) { | |
| // File already open, just set as active and update content | |
| const existingFile = openedLocalFiles[existingFileIndex]; | |
| if (existingFile) { | |
| const updatedFile: EditorFile = { | |
| path: existingFile.path, | |
| isDirty: existingFile.isDirty, | |
| type: existingFile.type, | |
| originalContent: existingFile.originalContent, | |
| content: newLocalFile.content | |
| }; | |
| const updatedFiles = [...openedLocalFiles]; | |
| updatedFiles[existingFileIndex] = updatedFile; | |
| setOpenedLocalFiles(updatedFiles); | |
| setActiveLocalFile(updatedFile); | |
| } | |
| } else { | |
| // Add new file to opened files | |
| const updatedFiles = [...openedLocalFiles, newLocalFile]; | |
| setOpenedLocalFiles(updatedFiles); | |
| setActiveLocalFile(newLocalFile); | |
| } | |
| }, [selectedFile, loadedContent]); | |
| if (!selectedFile || loadedContent === null || loadedContent === undefined) return; | |
| const newLocalFile = createLocalEditorFile(selectedFile, loadedContent); | |
| // Check if file is already open | |
| const existingFileIndex = openedLocalFiles.findIndex(f => f.path === selectedFile); | |
| if (existingFileIndex >= 0) { | |
| // File already open, just set as active and update content | |
| const existingFile = openedLocalFiles[existingFileIndex]; | |
| if (existingFile) { | |
| const updatedFile: EditorFile = { | |
| path: existingFile.path, | |
| isDirty: existingFile.isDirty, | |
| type: existingFile.type, | |
| originalContent: existingFile.originalContent, | |
| content: newLocalFile.content | |
| }; | |
| const updatedFiles = [...openedLocalFiles]; | |
| updatedFiles[existingFileIndex] = updatedFile; | |
| setOpenedLocalFiles(updatedFiles); | |
| setActiveLocalFile(updatedFile); | |
| } | |
| } else { | |
| // Add new file to opened files | |
| const updatedFiles = [...openedLocalFiles, newLocalFile]; | |
| setOpenedLocalFiles(updatedFiles); | |
| setActiveLocalFile(newLocalFile); | |
| } | |
| }, [selectedFile, loadedContent]); |
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
around lines 81 to 110, the current guard `if (!selectedFile || !loadedContent)`
prevents loading files whose content is an empty string; change the check to
only bail on null/undefined (e.g., `selectedFile == null || loadedContent ==
null`) so empty-string file contents are allowed, leaving the rest of the
open/update logic unchanged.
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
Outdated
Show resolved
Hide resolved
| const fileToClose = openedLocalFiles.find(f => f.path === filePath); | ||
| if (fileToClose?.isDirty) { | ||
| setShowLocalUnsavedDialog(true); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| return await ide.openFile(filePath, searchTerm, false); | ||
| } catch (error) { | ||
| console.error('Error loading file:', error); | ||
| return null; | ||
| const editorView = editorViewsRef.current.get(filePath); | ||
| if (editorView) { | ||
| editorView.destroy(); | ||
| editorViewsRef.current.delete(filePath); | ||
| } | ||
| }, [isSandboxReady]); | ||
|
|
||
| function handleFileSelect(file: EditorFile) { | ||
| ide.setHighlightRange(null); | ||
| ide.activeFile = file; | ||
| } | ||
| const updatedFiles = openedLocalFiles.filter(f => f.path !== filePath); | ||
| setOpenedLocalFiles(updatedFiles); | ||
|
|
||
| async function getFilePathFromOid(oid: string): Promise<string | null> { | ||
| if (!isSandboxReady) { | ||
| handleSandboxNotReady('get file path from OID'); | ||
| return null; | ||
| // Set new active file if we closed the active one | ||
| if (activeLocalFile?.path === filePath) { | ||
| const newActiveFile = updatedFiles.length > 0 ? updatedFiles[updatedFiles.length - 1] || null : null; | ||
| setActiveLocalFile(newActiveFile); |
There was a problem hiding this comment.
Track which dirty tab triggered the unsaved dialog.
When closing a dirty tab we only set showLocalUnsavedDialog and bail. We never record the target path/id, so the dialog either never appears (if the dirty tab isn’t active) or applies to the wrong file. Please cache the pending tab and drive the dialog from that state so confirmation targets the correct file.
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
around lines 330-348, the code only sets showLocalUnsavedDialog when
encountering a dirty file but does not record which file triggered it, causing
the dialog to target the wrong file or not appear for non-active tabs; add a new
piece of state (e.g., pendingLocalClosePath or pendingLocalUnsavedFile) and set
it to the filePath (or file object) before returning, update the unsaved dialog
to read from that pending state to show the correct filename/details, and on
dialog confirm/abort handlers use that pending state to either proceed to
destroy/delete the correct editorView and remove the file from openedLocalFiles
(then clear pending state) or simply clear pending state without closing. Ensure
you clear pending state on dialog close and when performing the actual close
path so state stays consistent.
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx
Show resolved
Hide resolved
.../web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-node.tsx
Show resolved
Hide resolved
| import { Button } from '@onlook/ui/button'; | ||
| import { Icons } from '@onlook/ui/icons'; | ||
| import { Input } from '@onlook/ui/input'; | ||
| import { Tooltip, TooltipContent, TooltipPortal, TooltipTrigger } from '@onlook/ui/tooltip'; | ||
| import { forwardRef } from 'react'; |
There was a problem hiding this comment.
Add 'use client' directive before using event handlers.
Components under app/ default to server mode. Because this module attaches onChange, onClick, and similar handlers, it must opt into the client boundary, otherwise Next.js will throw during build/runtime. Insert the directive ahead of the imports.
+'use client';
+
import { Button } from '@onlook/ui/button';
import { Icons } from '@onlook/ui/icons';
import { Input } from '@onlook/ui/input';
import { Tooltip, TooltipContent, TooltipPortal, TooltipTrigger } from '@onlook/ui/tooltip';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Button } from '@onlook/ui/button'; | |
| import { Icons } from '@onlook/ui/icons'; | |
| import { Input } from '@onlook/ui/input'; | |
| import { Tooltip, TooltipContent, TooltipPortal, TooltipTrigger } from '@onlook/ui/tooltip'; | |
| import { forwardRef } from 'react'; | |
| 'use client'; | |
| import { Button } from '@onlook/ui/button'; | |
| import { Icons } from '@onlook/ui/icons'; | |
| import { Input } from '@onlook/ui/input'; | |
| import { Tooltip, TooltipContent, TooltipPortal, TooltipTrigger } from '@onlook/ui/tooltip'; | |
| import { forwardRef } from 'react'; |
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree-search.tsx
around lines 1 to 5, this component uses event handlers but lacks the Next.js
client directive; add "use client" as the very first line of the file (before
any imports) to opt into the client boundary so onChange/onClick handlers work
correctly during build/runtime.
| placeholder="Search files" | ||
| value={searchQuery} | ||
| disabled={isLoading} | ||
| onChange={(e) => onSearchChange(e.target.value)} | ||
| onKeyDown={onKeyDown} | ||
| /> | ||
| {searchQuery && ( | ||
| <button | ||
| className="absolute right-[1px] top-[1px] bottom-[1px] aspect-square hover:bg-background-onlook active:bg-transparent flex items-center justify-center rounded-r-[calc(theme(borderRadius.md)-1px)] group" | ||
| onClick={clearSearch} | ||
| > | ||
| <Icons.CrossS className="h-3 w-3 text-foreground-primary/50 group-hover:text-foreground-primary" /> | ||
| </button> | ||
| )} | ||
| </div> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| variant={'default'} | ||
| size={'icon'} | ||
| className="p-2 w-fit h-8 text-foreground-tertiary hover:text-foreground-hover hover:border-border-onlook bg-background-none hover:bg-background-onlook" | ||
| disabled={isLoading} | ||
| onClick={handleRefresh} | ||
| > | ||
| {isLoading ? ( | ||
| <div className="animate-spin h-4 w-4 border-2 border-foreground-primary rounded-full border-t-transparent"></div> | ||
| ) : ( | ||
| <Icons.Reload /> | ||
| )} | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipPortal> | ||
| <TooltipContent> | ||
| <p>{isLoading ? 'Loading files...' : 'Refresh files'}</p> | ||
| </TooltipContent> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace hardcoded UI strings with localized messages.
Per the client coding guidelines, user-facing copy should come from next-intl (or our localization layer). Please move the placeholder and tooltip strings to translations and read them with useTranslations, rather than hardcoding English literals in the component.
Based on coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.ts (2)
49-66: Clear stale navigation target when OID, branch data, or metadata is missing.When any of these conditions fail (no OID at lines 51-52, no branchData at lines 58-59, or no metadata at lines 64-65), the function returns without clearing
navigationTarget. The previous target stays active, so the editor keeps highlighting the wrong element. ResetnavigationTargettonullbefore returning in each of these branches.Apply this diff:
const oid = selectedElement.instanceId ?? selectedElement.oid; if (!oid) { console.warn('[CodeNavigation] No OID found for selected element'); + setNavigationTarget(null); return; } try { const branchData = editorEngine.branches.getBranchDataById(selectedElement.branchId); if (!branchData) { console.warn(`[CodeNavigation] No branch data found for branchId: ${selectedElement.branchId}`); + setNavigationTarget(null); return; } const metadata = await branchData.codeEditor.getJsxElementMetadata(oid); if (!metadata) { console.warn(`[CodeNavigation] No metadata found for OID: ${oid}`); + setNavigationTarget(null); return; }
55-91: Guard against async stale updates.Because the reaction effect is async, a slower metadata lookup for an older selection can resolve after a newer selection and overwrite the current target. Before calling
setNavigationTarget, verify that the selection hasn't changed (e.g., compare againsteditorEngine.elements.selected[0]or track a request counter).apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx (2)
61-66: Replace hardcoded empty-state text with a localized message.The hardcoded string "Open a file or select an element on the page." at line 64 violates the coding guidelines. Use next-intl's translation hooks to source this text from the messages bundle.
As per coding guidelines
1-9: Add'use client'directive for hook usage.This module uses React hooks (
useEffect,useState) at line 3 and in the component starting at line 22, so Next.js requires the'use client'directive. Without it, the component will be treated as a Server Component and throw at runtime.As per coding guidelines
Apply this diff:
+'use client'; + import { EditorView } from '@codemirror/view'; import { pathsEqual } from '@onlook/utility'; import { type RefObject, useEffect, useState } from 'react';apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx (1)
1-8: Add'use client'directive for hook usage.This module uses
useEffect(imported at line 4 and used at line 43), so Next.js requires the'use client'directive. Without it, the component will be treated as a Server Component and throw at runtime.As per coding guidelines
Apply this diff:
+'use client'; + import { EditorView } from '@codemirror/view'; import { convertToBase64, getMimeType } from '@onlook/utility/src/file'; import CodeMirror from '@uiw/react-codemirror';apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx (3)
162-171: Fix the ineffective refresh mechanism.At line 169,
setSelectedFilePath(prev => prev)sets state to its previous value, which doesn't trigger a re-render in React. This refresh mechanism is ineffective. Consider using a refresh counter that increments and passing it as a key prop toFileTree, or implement a different refresh strategy.Apply this diff:
+const [refreshKey, setRefreshKey] = useState(0); + const refreshFileTree = () => { - // Force refresh of file entries - // This will cause the file tree to re-render with updated file list - // Note: The useDirectory hook automatically handles file watching, - // but this provides an explicit refresh mechanism for after file operations - setTimeout(() => { - // Simple state update to trigger re-render - setSelectedFilePath(prev => prev); - }, 100); + setRefreshKey(prev => prev + 1); };Then pass
key={refreshKey}to theFileTreecomponent at line 372.
294-302: Await file rename and update editor state.At line 297,
branchData.codeEditor.moveFileis called withoutawait. This misses errors and prevents updating local editor state after the rename. As a result, any opened files referencing the old path will continue writing to the wrong location. Make the handlerasync, await the move, then updateopenedEditorFiles,activeEditorFile, andselectedFilePathfor entries matching the old path.Apply this diff:
- const handleRenameFile = (oldPath: string, newName: string) => { + const handleRenameFile = async (oldPath: string, newName: string) => { if (!branchData?.codeEditor) return; try { - branchData.codeEditor.moveFile(oldPath, newName); + await branchData.codeEditor.moveFile(oldPath, newName); + + // Compute new path (assuming newName is the full new path or just filename) + const newPath = newName.includes('/') ? newName : oldPath.replace(/[^/]+$/, newName); + + // Update opened files + setOpenedEditorFiles(prev => prev.map(f => + pathsEqual(f.path, oldPath) ? { ...f, path: newPath } : f + )); + + // Update active file + if (activeEditorFile && pathsEqual(activeEditorFile.path, oldPath)) { + setActiveEditorFile({ ...activeEditorFile, path: newPath }); + } + + // Update selected path + if (selectedFilePath && pathsEqual(selectedFilePath, oldPath)) { + setSelectedFilePath(newPath); + } } catch (error) { console.error('Failed to rename file:', error); toast.error(`Failed to rename: ${error instanceof Error ? error.message : 'Unknown error'}`);
304-312: Await file deletion and close opened editors.At line 307,
branchData.codeEditor.deleteFileis called withoutawait. This misses errors and leaves stale editor tabs open. If the active file is deleted, it will resurrect on the next save. Make the handlerasync, await the delete, then close any opened files matching the deleted path usingcloseFileInternal.Apply this diff:
- const handleDeleteFile = (path: string) => { + const handleDeleteFile = async (path: string) => { if (!branchData?.codeEditor) return; try { - branchData.codeEditor.deleteFile(path); + await branchData.codeEditor.deleteFile(path); + + // Close any opened files matching the deleted path + closeFileInternal(path); } catch (error) { console.error('Failed to delete file:', error); toast.error(`Failed to delete: ${error instanceof Error ? error.message : 'Unknown error'}`);
🧹 Nitpick comments (1)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx (1)
43-50: RemoveeditorViewsRef.currentfrom dependency array.At line 50,
editorViewsRef.currentappears in the dependency array. Since this is a mutable ref object (Map), React only tracks its identity, not its contents. Including it doesn't trigger re-runs when the map changes, and it's unnecessary because the effect should run based onnavigationTarget,isActive, and other explicit dependencies. Removing it improves clarity.Apply this diff:
- }, [navigationTarget, isActive, file.originalHash, file.type, file.path, editorViewsRef.current]); + }, [navigationTarget, isActive, file.originalHash, file.type, file.path]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-mirror-config.ts(4 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.ts(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-mirror-config.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-mirror-config.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-mirror-config.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-mirror-config.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
🧠 Learnings (5)
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.tsapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/**/*.tsx : observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/app/**/*.{ts,tsx} : Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/app/**/*.tsx : Avoid hardcoded user-facing text; use next-intl messages/hooks
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Avoid hardcoded user-facing text; use next-intl messages/hooks instead
Applied to files:
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
🧬 Code graph analysis (5)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.ts (2)
packages/utility/src/path.ts (1)
pathsEqual(112-121)apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx (4)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts (2)
EditorFile(1-6)BinaryEditorFile(15-19)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.ts (1)
CodeNavigationTarget(18-21)packages/utility/src/file.ts (2)
getMimeType(85-99)convertToBase64(106-112)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-mirror-config.ts (4)
scrollToLineColumn(302-306)highlightElementRange(293-296)getBasicSetup(348-376)getExtensions(404-421)
apps/web/client/src/app/project/[id]/_components/right-panel/index.tsx (3)
apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx (2)
CodeTabRef(18-25)CodeTab(56-408)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/header-controls.tsx (1)
CodeControls(25-153)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/index.tsx (11)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts (3)
EditorFile(1-6)BinaryEditorFile(15-19)TextEditorFile(8-12)apps/web/client/src/services/sync-engine/sync-engine.ts (1)
hashContent(19-25)apps/web/client/src/components/store/editor/index.tsx (1)
useEditorEngine(10-14)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.ts (1)
useCodeNavigation(34-100)packages/file-system/src/hooks/use-dir.ts (1)
useDirectory(7-50)packages/file-system/src/hooks/use-file.ts (1)
useFile(6-49)packages/utility/src/path.ts (1)
pathsEqual(112-121)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/utils.ts (1)
isDirty(5-17)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx (1)
FileTree(20-240)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-tabs/index.tsx (1)
FileTabs(27-118)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx (1)
CodeEditorArea(22-92)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx (6)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/types.ts (1)
EditorFile(1-6)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/hooks/use-code-navigation.ts (1)
CodeNavigationTarget(18-21)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/shared/utils.ts (1)
isDirty(5-17)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx (1)
CodeEditor(18-102)packages/utility/src/path.ts (1)
pathsEqual(112-121)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/unsaved-changes-dialog.tsx (1)
UnsavedChangesDialog(9-39)
🔇 Additional comments (1)
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-mirror-config.ts (1)
242-291: LGTM: Element highlighting with proper bounds checking.The element highlighting implementation correctly clamps line numbers (lines 260-262) and column positions (lines 267-269) to valid ranges before computing document positions. The final position clamping (lines 274-276) provides additional safety. This defensive approach prevents exceptions from stale or truncated documents.
| const getFileUrl = (file: BinaryEditorFile) => { | ||
| const mime = getMimeType(file.path.toLowerCase()); | ||
| const base64 = convertToBase64(new Uint8Array(file.content)); | ||
| return `data:${mime};base64,${base64}`; | ||
| }; |
There was a problem hiding this comment.
Consider chunking for large binary files.
At line 28, convertToBase64 uses String.fromCharCode with array mapping. For large binary files (e.g., multi-megabyte images), this could be slow or hit memory limits. Consider chunking the conversion or providing a streaming alternative for large files.
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/code-editor.tsx
around lines 26 to 30, the current convertToBase64 call converts the entire
Uint8Array at once which can be slow or OOM for multi-megabyte binaries; change
to a size-aware approach: if the file exceeds a configured threshold (e.g.,
1–5MB) either (A) convert in chunks by slicing the Uint8Array into reasonably
sized pieces (e.g., 64KB–256KB), convert each chunk with String.fromCharCode and
btoa or TextDecoder+base64 encoder and concatenate results, or (B) avoid base64
entirely by creating a Blob from the Uint8Array and returning a blob URL via
URL.createObjectURL(blob) to use as the file URL; pick one approach, add the
threshold check, and ensure cleanup of blob URLs (revokeObjectURL) when no
longer needed.
| {activeFileIsDirty && showUnsavedDialog && ( | ||
| <UnsavedChangesDialog | ||
| onSave={onSaveFile} | ||
| onDiscard={() => onDiscardChanges(activeFile?.path || '')} | ||
| onCancel={() => { onCancelUnsaved(); }} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Guard against empty path in discard handler.
At line 86, onDiscardChanges(activeFile?.path || '') uses an empty string fallback when activeFile is null. This could cause the discard operation to fail silently or target the wrong file. Consider showing the dialog only when activeFile exists, or handle the null case explicitly in the parent.
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/file-content/index.tsx
around lines 83 to 89, the discard handler currently falls back to an empty
string (onDiscardChanges(activeFile?.path || '')) which can cause a silent/no-op
or target the wrong file; change the rendering/handler so the
UnsavedChangesDialog is only shown when activeFile is non-null (e.g., include
activeFile in the conditional) and pass activeFile.path directly, or
alternatively keep the dialog rendering but change the onDiscard prop to a small
guard that checks activeFile and only calls onDiscardChanges when
activeFile?.path is a non-empty string (otherwise no-op or surface an error).
Ensure the selected approach prevents calling onDiscardChanges with an empty
path.
| {selectedTab === EditorTabValue.CODE && codeTabRef.current && <CodeControls | ||
| isDirty={codeTabHasUnsavedChanges} | ||
| currentPath={codeTabRef.current.getCurrentPath()} | ||
| onSave={codeTabRef.current.handleSaveFile} | ||
| onRefresh={codeTabRef.current.refreshFileTree} | ||
| onCreateFile={codeTabRef.current.handleCreateFile} | ||
| onCreateFolder={codeTabRef.current.handleCreateFolder} | ||
| />} |
There was a problem hiding this comment.
Conditional ref check may cause initial render flash.
At line 80, codeTabRef.current is checked before rendering CodeControls. On the first render of the CODE tab, codeTabRef.current will be null until CodeTab mounts and sets the ref. This means CodeControls won't appear initially, potentially causing a brief layout shift or missing controls until the ref is populated. Consider rendering a placeholder or using a different gating mechanism to avoid the flash.
|
Generated with ❤️ by ellipsis.dev |
|
Generated with ❤️ by ellipsis.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
apps/web/client/src/components/store/editor/font/font-config-manager.ts (1)
197-204: Allow empty font config content.
ensureFontConfigFileExistswrites an empty string, but this guard treats''as unreadable, soaddFontToConfigfails on the very first add. Only reject non-text values; allow empty strings.Apply this diff:
- if (!file || typeof file !== 'string') { - console.error("Font config file is empty or doesn't exist"); + if (file == null || typeof file !== 'string') { + console.error('Font config file is not readable as text'); return; }apps/web/client/src/components/store/editor/sandbox/index.ts (4)
32-45: Handle async errors when initializing sync engine.The reaction calls
this.initializeSyncEngine(provider)at line 37 without handling rejections, which can cause unhandled promise rejections and leave the sandbox manager in an inconsistent state.Apply this diff:
if (provider) { - this.initializeSyncEngine(provider); + void this.initializeSyncEngine(provider).catch((error) => { + console.error('Failed to initialize sync engine', error); + // Clean up partial state + this.sync?.stop(); + this.sync = null; + }); } else if (this.sync) {
58-71: Add error handling to prevent inconsistent state.If
fs.initialize()orsync.start()fail, the method leavesthis.fsandthis.syncin an inconsistent state. The caller ininit()also doesn't handle errors, so failures will silently corrupt the manager.Apply this diff:
async initializeSyncEngine(provider: Provider) { + try { if (this.sync) { this.sync?.stop(); this.sync = null; } this.sync = new CodeProviderSync(provider, this.fs, { exclude: EXCLUDED_SYNC_PATHS, }); await this.sync.start(); await this.fs.rebuildIndex(); await this.ensurePreloadScriptExists(); + } catch (error) { + console.error('Failed to initialize sync engine:', error); + // Clean up partial state + this.sync?.stop(); + this.sync = null; + throw error; + } }
112-165: File operation methods fail when FileSystem isn't initialized.All file operation methods throw "File system not initialized" errors when
this.fsis null. This causes test failures and creates a race condition where methods may be called beforeinitializeSyncEnginecompletes.Consider implementing one of these approaches:
- Queue operations until FS is ready - Add a promise that resolves when FS is initialized and make methods await it
- Make
init()await initialization - Ensureinit()awaitsinitializeSyncEngineand tests wait for completion- Return safe defaults - For read operations, return empty values instead of throwing
Based on past review comments and static analysis hints showing test failures.
193-200: Call FileSystem cleanup before clearing.The
clear()method should callthis.fs?.cleanup()to ensure watchers are properly closed and timeouts are cleared, even thoughthis.fsis not being set tonullin the current implementation.Apply this diff:
clear() { this.providerReactionDisposer?.(); this.providerReactionDisposer = undefined; this.sync?.stop(); this.sync = null; + this.fs?.cleanup(); this.preloadScriptInjected = false; this.session.clear(); }Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/client/src/app/project/[id]/_components/left-panel/page-tab/index.tsx(3 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx(1 hunks)apps/web/client/src/components/store/editor/font/font-config-manager.ts(1 hunks)apps/web/client/src/components/store/editor/image/index.ts(5 hunks)apps/web/client/src/components/store/editor/pages/helper.ts(26 hunks)apps/web/client/src/components/store/editor/sandbox/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/client/src/app/project/[id]/_components/left-panel/page-tab/index.tsx
- apps/web/client/src/app/project/[id]/_components/right-panel/code-tab/sidebar/file-tree.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/components/store/editor/font/font-config-manager.tsapps/web/client/src/components/store/editor/image/index.tsapps/web/client/src/components/store/editor/pages/helper.tsapps/web/client/src/components/store/editor/sandbox/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/components/store/editor/font/font-config-manager.tsapps/web/client/src/components/store/editor/image/index.tsapps/web/client/src/components/store/editor/pages/helper.tsapps/web/client/src/components/store/editor/sandbox/index.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/components/store/editor/font/font-config-manager.tsapps/web/client/src/components/store/editor/image/index.tsapps/web/client/src/components/store/editor/pages/helper.tsapps/web/client/src/components/store/editor/sandbox/index.ts
🧠 Learnings (2)
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Applied to files:
apps/web/client/src/components/store/editor/font/font-config-manager.ts
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/**/*.{ts,tsx} : Use path aliases @/* and ~/* for imports mapping to src/*
Applied to files:
apps/web/client/src/components/store/editor/font/font-config-manager.ts
🧬 Code graph analysis (4)
apps/web/client/src/components/store/editor/font/font-config-manager.ts (8)
apps/web/client/src/components/store/editor/engine.ts (1)
EditorEngine(34-137)packages/fonts/src/helpers/font-extractors.ts (2)
parseFontDeclarations(15-75)migrateFontsFromLayout(172-283)apps/web/client/src/components/store/editor/sandbox/helpers.ts (1)
normalizePath(5-9)packages/fonts/src/helpers/validators.ts (3)
validateGoogleFontSetup(132-176)ExportNamedDeclaration(104-116)ExportNamedDeclaration(164-172)packages/fonts/src/helpers/ast-manipulators.ts (4)
ExportNamedDeclaration(194-225)ExportNamedDeclaration(415-449)addGoogleFontSpecifier(462-482)removeFontDeclaration(158-239)packages/fonts/src/helpers/ast-generators.ts (1)
generateFontVariableExport(93-109)apps/web/client/src/components/store/editor/sandbox/index.ts (1)
fileExists(137-140)packages/parser/src/parse.ts (1)
getAstFromContent(7-24)
apps/web/client/src/components/store/editor/image/index.ts (4)
packages/models/src/actions/action.ts (1)
ImageContentData(93-98)apps/web/client/src/components/store/editor/engine.ts (1)
EditorEngine(34-137)packages/utility/src/string.ts (1)
sanitizeFilename(72-88)packages/utility/src/file.ts (1)
convertToBase64(106-112)
apps/web/client/src/components/store/editor/pages/helper.ts (4)
packages/models/src/pages/index.ts (1)
PageMetadata(21-28)packages/models/src/next/index.ts (1)
RouterConfig(6-9)packages/utility/src/file.ts (2)
getDirName(74-78)getBaseName(80-83)apps/web/client/src/components/store/editor/sandbox/index.ts (1)
SandboxManager(14-201)
apps/web/client/src/components/store/editor/sandbox/index.ts (7)
apps/web/client/src/components/store/editor/sandbox/session.ts (1)
SessionManager(8-225)apps/web/client/src/services/sync-engine/sync-engine.ts (1)
CodeProviderSync(27-594)packages/file-system/src/code-fs.ts (1)
CodeFileSystem(25-253)apps/web/client/src/components/store/editor/pages/helper.ts (1)
detectRouterConfig(436-487)packages/constants/src/files.ts (1)
EXCLUDED_SYNC_PATHS(9-16)apps/web/client/src/components/store/editor/sandbox/preload-script.ts (1)
copyPreloadScriptToPublic(8-29)packages/file-system/src/types.ts (1)
FileEntry(1-8)
🔇 Additional comments (23)
apps/web/client/src/components/store/editor/image/index.ts (5)
1-4: LGTM!Import changes align well with the refactored functionality—added utilities support filename sanitization and content conversion, while removed imports reflect cleanup of unused dependencies.
17-17: LGTM!The empty
init()is intentional per the refactoring that removed reaction wiring and scanning logic.
134-134: LGTM!Adding
branchIdto each target aligns with the branch-aware architecture introduced in this PR.
207-212: Well done addressing the previous review feedback!The
clear()method now properly resets all state properties (_imagePaths,_selectedImage,_previewImage,_isSelectingImage), eliminating the stale state issue flagged in the previous review.
156-171: activeSandbox.readFile returnsstring | Uint8Array; both SVG text and binary branches are correctly handled.apps/web/client/src/components/store/editor/sandbox/index.ts (6)
1-11: LGTM!The import statements are correct and follow the project's path alias conventions.
22-30: LGTM!The constructor changes correctly integrate the new
CodeFileSystemparameter and initialize the required fields for the FS/Sync-based architecture.
47-56: LGTM!The
getRouterConfigmethod correctly implements caching and error handling for router configuration detection.
73-101: LGTM!The preload script initialization is well-implemented with proper error handling, concurrency guards, and appropriate checks for dependencies.
103-110: LGTM!Both
getLayoutPathand theerrorsgetter are implemented correctly.
167-191: LGTM!The
downloadFilesmethod correctly handles the provider check and error scenarios.apps/web/client/src/components/store/editor/pages/helper.ts (12)
1-7: LGTM!The import statements are correct and follow the project's conventions for importing from
@onlookpackages.
119-123: LGTM!The type guard correctly ensures that metadata extraction only proceeds with string content, which is appropriate since AST parsing requires text.
212-243: LGTM!The
scanAppDirectorymethod correctly uses theFileEntrytype and properly filters directories using theisDirectoryproperty.
314-415: LGTM!The
scanPagesDirectorymethod correctly uses theFileEntrytype and properly distinguishes between files and directories.
417-431: LGTM!The method correctly uses
awaitwhen callingsandboxManager.getRouterConfig()and properly handles the router configuration.
434-487: LGTM, but note the TODO comment.The
detectRouterConfigfunction is correctly implemented. However, there's a TODO comment at line 434 indicating thatgetRouterConfigis being called in many places before the provider is initialized, suggesting initialization should happen earlier during setup.Consider addressing the TODO to ensure the provider is initialized before
getRouterConfigis called throughout the codebase.
490-520: LGTM!The
pathExistsandcleanupEmptyFoldershelper functions are correctly implemented using the sandbox manager's file system methods.
545-580: LGTM!The
createPageInSandboxfunction correctly usesawaitfor the asyncgetRouterConfigcall and properly validates the router configuration.
582-626: LGTM!The
deletePageInSandboxfunction correctly handles both directory and file deletion with appropriate cleanup of empty folders.
628-669: LGTM!The
renamePageInSandboxfunction correctly validates the rename operation and uses the sandbox manager'srenamemethod.
671-739: LGTM!The
duplicatePageInSandboxfunction correctly handles both file and directory duplication. Line 732 properly callscopyDirectoryfor App Router pages, which addresses the previously flagged issue.
741-1092: LGTM!The metadata update functions and helper functions are correctly implemented:
- Proper file content type checking (string vs Uint8Array)
- Correct usage of the
FileEntrytype throughout- Appropriate async/await patterns for file operations
| // Sanitize filename from user upload | ||
| const sanitizedName = sanitizeFilename(file.name); | ||
| const filePath = path.join(destinationFolder, sanitizedName); | ||
| const uint8Array = new Uint8Array(await file.arrayBuffer()); | ||
| await this.editorEngine.activeSandbox.writeBinaryFile(path, uint8Array); | ||
| await this.scanImages(); | ||
| await this.editorEngine.activeSandbox.writeFile(filePath, uint8Array); |
There was a problem hiding this comment.
Path traversal vulnerability in filename sanitization.
sanitizeFilename() only normalizes unicode and whitespace; it does not remove path separators (/, \) or parent directory references (..). An attacker could upload a file named ../../etc/passwd, and path.join(destinationFolder, sanitizedName) would resolve the traversal, writing outside the intended directory.
Apply this diff to strip dangerous characters from the filename:
// Sanitize filename from user upload
const sanitizedName = sanitizeFilename(file.name);
+ // Remove path separators and parent directory references to prevent path traversal
+ const safeName = sanitizedName.replace(/[/\\]|\.\.+/g, '_');
- const filePath = path.join(destinationFolder, sanitizedName);
+ const filePath = path.join(destinationFolder, safeName);
const uint8Array = new Uint8Array(await file.arrayBuffer());Alternatively, use only path.basename(sanitizedName) to ensure no directory components remain:
// Sanitize filename from user upload
const sanitizedName = sanitizeFilename(file.name);
- const filePath = path.join(destinationFolder, sanitizedName);
+ // Use basename to strip any directory components
+ const filePath = path.join(destinationFolder, path.basename(sanitizedName));
const uint8Array = new Uint8Array(await file.arrayBuffer());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Sanitize filename from user upload | |
| const sanitizedName = sanitizeFilename(file.name); | |
| const filePath = path.join(destinationFolder, sanitizedName); | |
| const uint8Array = new Uint8Array(await file.arrayBuffer()); | |
| await this.editorEngine.activeSandbox.writeBinaryFile(path, uint8Array); | |
| await this.scanImages(); | |
| await this.editorEngine.activeSandbox.writeFile(filePath, uint8Array); | |
| // Sanitize filename from user upload | |
| const sanitizedName = sanitizeFilename(file.name); | |
| // Remove path separators and parent directory references to prevent path traversal | |
| const safeName = sanitizedName.replace(/[/\\]|\.\.+/g, '_'); | |
| const filePath = path.join(destinationFolder, safeName); | |
| const uint8Array = new Uint8Array(await file.arrayBuffer()); | |
| await this.editorEngine.activeSandbox.writeFile(filePath, uint8Array); |
🤖 Prompt for AI Agents
In apps/web/client/src/components/store/editor/image/index.ts around lines 108
to 112, the sanitized filename still allows path traversal because
sanitizeFilename() doesn't remove path separators or parent refs; change the
code to derive a safe filename by taking only the basename (e.g. use
path.basename(sanitizedName)) and additionally strip any remaining path
separators or null bytes, reject or fallback on empty results, and enforce a max
length (or append a safe unique suffix) before joining with destinationFolder so
writes cannot escape the intended directory.
|
Generated with ❤️ by ellipsis.dev |
|
Generated with ❤️ by ellipsis.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/web/client/src/components/store/editor/sandbox/preload-script.ts (3)
10-14: Consider checking directory existence before creation.The nested try-catch for directory creation works but is unconventional. Using an existence check before creation would be more explicit and easier to follow.
Apply this diff:
- try { - await provider.createDirectory({ args: { path: 'public' } }); - } catch { - // Directory might already exist, ignore error - } + const publicDirExists = await provider.exists({ args: { path: 'public' } }); + if (!publicDirExists) { + await provider.createDirectory({ args: { path: 'public' } }); + }Note: Verify that the
Providerinterface includes anexistsmethod. If not, the current pattern is acceptable.
32-34: Add defensive check forrouterConfig.basePath.The validation checks for
routerConfigexistence but doesn't verify thatbasePathis defined. This could lead to runtime errors when accessingrouterConfig.basePathon line 36.Apply this diff:
- if (!routerConfig) { + if (!routerConfig || !routerConfig.basePath) { throw new Error('Could not detect router type for script injection. This is required for iframe communication.'); }
45-45: Usepath.joinfor consistency.The path is constructed using a template literal here, but line 85 in
getLayoutPathusespath.join. For consistency and cross-platform compatibility, preferpath.jointhroughout.Apply this diff:
- const layoutPath = `${routerConfig.basePath}/${layoutFile.name}`; + const layoutPath = path.join(routerConfig.basePath, layoutFile.name);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/components/store/editor/sandbox/preload-script.ts(1 hunks)apps/web/client/src/server/api/routers/publish/manager.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/client/src/server/api/routers/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/server/api/routers/**/*.ts: Place tRPC routers under apps/web/client/src/server/api/routers/**
Use publicProcedure/protectedProcedure from apps/web/client/src/server/api/trpc.ts and validate inputs with Zod
Return plain objects/arrays; rely on SuperJSON for serialization in tRPC procedures
apps/web/client/src/server/api/routers/**/*.ts: Place tRPC routers under src/server/api/routers/**
Use publicProcedure/protectedProcedure from src/server/api/trpc.ts and validate inputs with Zod
Return plain objects/arrays; rely on SuperJSON for serialization
Files:
apps/web/client/src/server/api/routers/publish/manager.ts
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/server/api/routers/publish/manager.tsapps/web/client/src/components/store/editor/sandbox/preload-script.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/server/api/routers/publish/manager.tsapps/web/client/src/components/store/editor/sandbox/preload-script.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/server/api/routers/publish/manager.tsapps/web/client/src/components/store/editor/sandbox/preload-script.ts
🧬 Code graph analysis (2)
apps/web/client/src/server/api/routers/publish/manager.ts (1)
packages/constants/src/files.ts (1)
ONLOOK_PRELOAD_SCRIPT_FILE(8-8)
apps/web/client/src/components/store/editor/sandbox/preload-script.ts (7)
packages/models/src/next/index.ts (1)
RouterConfig(6-9)apps/web/preload/server/index.ts (1)
fetch(6-26)packages/constants/src/files.ts (3)
ONLOOK_DEV_PRELOAD_SCRIPT_SRC(10-10)ONLOOK_DEV_PRELOAD_SCRIPT_PATH(12-12)NEXT_JS_FILE_EXTENSIONS(47-47)packages/utility/src/path.ts (1)
isRootLayoutFile(90-101)packages/parser/src/parse.ts (2)
getAstFromContent(7-24)getContentFromAst(47-62)packages/parser/src/code-edit/layout.ts (1)
injectPreloadScript(6-58)apps/web/client/src/components/store/editor/sandbox/index.ts (2)
getLayoutPath(103-106)fileExists(137-140)
🔇 Additional comments (2)
apps/web/client/src/server/api/routers/publish/manager.ts (2)
6-7: LGTM! Good refactoring to centralized constant.The import change from
LOCAL_PRELOAD_SCRIPT_SRCtoONLOOK_PRELOAD_SCRIPT_FILEimproves maintainability by using a centralized constant from@onlook/constants. This aligns with the broader file system refactoring introduced in this PR.
345-345: LGTM! Improved specificity with path prefix.The updated logic correctly filters the preload script file by using
endsWith(\/${ONLOOK_PRELOAD_SCRIPT_FILE}`). The addition of the/` prefix makes the check more specific, ensuring it matches the filename rather than any path segment ending with the string. This is appropriate given that file paths are constructed with directory prefixes (line 316).
|
Generated with ❤️ by ellipsis.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/client/src/components/store/editor/chat/context.ts (1)
240-273: Align getAgentRuleContext with branchData.codeEditor file operations.Other methods in this file use branchData.codeEditor for file reads; getAgentRuleContext still calls this.editorEngine.activeSandbox. Refactor this method to use branchData.codeEditor.readFile (and existence checks) to ensure branch-specific, consistent file access.
♻️ Duplicate comments (5)
apps/web/client/src/components/store/editor/ast/index.ts (1)
88-109: Clear stale element types when metadata provides noneWe still bail out when
dynamicType/coreElementTypeare missing, so any previously assigned special type sticks around on the node and in the frame view. That leaves elements permanently flagged as Link (etc.) even after the source is edited back to a plain element. Please normalize both the node and view tonullwhen metadata drops the special type before continuing.- const hasSpecialType = metadata.dynamicType || metadata.coreElementType; - if (!hasSpecialType) { - this.findNodeInstance(frameId, node, node, metadata, branchData); - return; - } - - // Always update node types based on metadata - node.dynamicType = metadata.dynamicType || null; - node.coreElementType = metadata.coreElementType || null; - - if (frameData.view) { - frameData.view.setElementType( - node.domId, - metadata.dynamicType || null, - metadata.coreElementType || null, - ); - } else { - console.error('No frame view found'); - } + const dynamicType = metadata.dynamicType ?? null; + const coreElementType = metadata.coreElementType ?? null; + node.dynamicType = dynamicType; + node.coreElementType = coreElementType; + + if (frameData.view) { + frameData.view.setElementType(node.domId, dynamicType, coreElementType); + } else { + console.error('No frame view found'); + } - this.findNodeInstance(frameId, node, node, metadata, branchData); + void this.findNodeInstance(frameId, node, node, metadata, branchData); + return;apps/web/client/src/components/store/editor/sandbox/index.ts (3)
32-45: Wrap async call to prevent unhandled rejections.
initializeSyncEngineis async; calling it bare in the reaction means any rejection becomes an unhandled promise rejection. This can leave the sandbox manager in a half-initialized state.Apply this diff to handle errors:
if (provider) { - this.initializeSyncEngine(provider); + void this.initializeSyncEngine(provider).catch((error) => { + console.error('[SandboxManager] Failed to initialize sync engine:', error); + }); } else if (this.sync) {
58-71: Add error handling to prevent inconsistent state.If
sync.start(),fs.rebuildIndex(), orensurePreloadScriptExists()fail, the method leavesthis.syncin an inconsistent state. Since the caller ininit()doesn't handle errors either (as noted in a separate comment), failures will silently corrupt the manager.Apply this diff to add error handling:
async initializeSyncEngine(provider: Provider) { + try { if (this.sync) { this.sync?.stop(); this.sync = null; } this.sync = new CodeProviderSync(provider, this.fs, { exclude: EXCLUDED_SYNC_PATHS, }); await this.sync.start(); await this.fs.rebuildIndex(); await this.ensurePreloadScriptExists(); + } catch (error) { + console.error('[SandboxManager] Failed to initialize sync engine:', error); + // Clean up partial state + this.sync?.stop(); + this.sync = null; + throw error; // Re-throw so caller can handle + } }
196-203: Call FileSystem cleanup before nulling.The
clear()method should callthis.fs?.cleanup()before setting it tonullto ensure watchers are properly closed and timeouts are cleared. This was flagged in a previous review.Apply this diff:
clear() { this.providerReactionDisposer?.(); this.providerReactionDisposer = undefined; this.sync?.stop(); this.sync = null; this.preloadScriptInjected = false; + this.fs?.cleanup(); this.session.clear(); }apps/web/client/src/components/store/editor/chat/context.ts (1)
93-96: Binary file handling remains inconsistent.The inconsistency between
getRefreshedContext(which logs an error and returns the original context) andgetFileContext(which silently skips binary files withcontinueat line 142) persists. This was previously flagged but has not been addressed.
🧹 Nitpick comments (2)
apps/web/client/src/components/store/editor/sandbox/index.ts (2)
47-56: Simplify truthy check.The
!!operator on Line 48 is unnecessary. A simpleif (this.routerConfig)check is clearer and sufficient.Apply this diff:
async getRouterConfig(): Promise<RouterConfig | null> { - if (!!this.routerConfig) { + if (this.routerConfig) { return this.routerConfig; }
140-143: Remove redundant optional chaining.Line 142 uses
this.fs?.exists(path)but Line 141 already throws if!this.fs, making the optional chaining unnecessary.Apply this diff:
async fileExists(path: string): Promise<boolean> { if (!this.fs) throw new Error('File system not initialized'); - return this.fs?.exists(path); + return this.fs.exists(path); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/client/src/components/store/editor/ast/index.ts(3 hunks)apps/web/client/src/components/store/editor/chat/context.ts(8 hunks)apps/web/client/src/components/store/editor/sandbox/index.ts(2 hunks)apps/web/client/src/components/store/editor/sandbox/preload-script.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/client/src/components/store/editor/sandbox/preload-script.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/components/store/editor/ast/index.tsapps/web/client/src/components/store/editor/chat/context.tsapps/web/client/src/components/store/editor/sandbox/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/components/store/editor/ast/index.tsapps/web/client/src/components/store/editor/chat/context.tsapps/web/client/src/components/store/editor/sandbox/index.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/components/store/editor/ast/index.tsapps/web/client/src/components/store/editor/chat/context.tsapps/web/client/src/components/store/editor/sandbox/index.ts
🧬 Code graph analysis (3)
apps/web/client/src/components/store/editor/ast/index.ts (3)
packages/models/src/element/layers.ts (1)
LayerNode(12-25)packages/file-system/src/code-fs.ts (1)
JsxElementMetadata(16-19)apps/web/client/src/components/store/editor/branch/manager.ts (1)
BranchData(12-18)
apps/web/client/src/components/store/editor/chat/context.ts (1)
packages/models/src/chat/message/context.ts (4)
FileMessageContext(23-27)HighlightMessageContext(29-36)MessageContext(53-59)AgentRuleMessageContext(48-51)
apps/web/client/src/components/store/editor/sandbox/index.ts (8)
apps/web/client/src/components/store/editor/sandbox/session.ts (1)
SessionManager(8-225)apps/web/client/src/services/sync-engine/sync-engine.ts (1)
CodeProviderSync(27-594)packages/models/src/next/index.ts (1)
RouterConfig(6-9)packages/file-system/src/code-fs.ts (1)
CodeFileSystem(25-254)apps/web/client/src/components/store/editor/pages/helper.ts (1)
detectRouterConfig(436-487)packages/constants/src/files.ts (1)
EXCLUDED_SYNC_PATHS(29-36)apps/web/client/src/components/store/editor/sandbox/preload-script.ts (1)
copyPreloadScriptToPublic(8-29)packages/file-system/src/types.ts (1)
FileEntry(1-8)
🔇 Additional comments (5)
apps/web/client/src/components/store/editor/sandbox/index.ts (1)
73-101: LGTM!The preload script injection logic is well-implemented with proper:
- Duplicate prevention via
preloadScriptInjectedflag- Concurrent access protection via
preloadScriptLoadingflag- Error handling with try-catch-finally
- Clear error logging
apps/web/client/src/components/store/editor/chat/context.ts (4)
14-15: LGTM!The import style change to
import { type ... }is a valid TypeScript pattern and aligns with modern conventions.
134-151: LGTM!The refactoring to use
branchData.codeEditoris correct and aligns with the branch-aware architecture. The branch validation logic properly handles missing branch data.
209-238: LGTM!The signature extension to include
branchIdand the refactoring to usebranchData.codeEditor.getJsxElementMetadatacorrectly implement the branch-aware architecture. Branch validation and error handling are appropriate.
314-354: LGTM!The refactoring to use
branchData.codeEditor.readFileis correct. The method includes appropriate validation for active branch presence, branch data availability, and proper type checking for file content. Error handling is comprehensive.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Integrates a new file system and sync engine, refactoring file operations and tools for improved efficiency and reliability.
FileSystemclass for file operations, replacing sandbox-based methods.useFileanduseDirectoryhooks to useFileSystem.getFileSystem()inhelpers/files.tsfor consistent file system access.CodeProviderSyncfor file synchronization with code providers.SandboxManager.WriteFileTool,ReadFileTool, andSearchReplaceEditToolto useFileSystem.helpers/files.ts.modelsto reflect new file handling structures.This description was created by
for 14cebae. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor