Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
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. Caution Review failedThe pull request is closed. WalkthroughProject- and branch-scoped caching and initialization were added across the editor: several managers (FileCacheManager, FileSyncManager, TemplateNodeManager) now take projectId/branchId to produce per-branch cache names; pages and font managers gained opt-in/init/reactive behavior; indexing/file-processing moved to cache-first and parallel batch handling; minor test and import fixes applied. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI
participant Sandbox as SandboxManager
participant Sync as FileSyncManager
participant Cache as FileCacheManager
note right of Sandbox #DDEBF7: Initialization with projectId, branchId
UI->>Sandbox: new SandboxManager(projectId, branchId)
Sandbox->>Sync: new FileSyncManager(projectId, branchId)
Sync->>Cache: new FileCacheManager(projectId, branchId)
Cache-->>Sync: initialized (per-branch cache names)
Sync-->>Sandbox: ready
sequenceDiagram
autonumber
participant Processor as BatchProcessor
participant Cache as FileCacheManager
participant Remote as Remote FS
Note over Processor,Cache: For each file (non-image)
Processor->>Cache: readCache(filePath)
alt cached and valid JSX
Cache-->>Processor: cached JSX
Processor->>Processor: process cached JSX
else no valid cache
Processor->>Remote: readOrFetch(filePath) (readRemoteFile fallback)
alt remote is JSX
Remote-->>Processor: JSX content
Processor->>Cache: optionally cache result
Processor->>Processor: process JSX
else not JSX
Remote-->>Processor: non-JSX
Processor-->>Processor: skip processing
end
end
Note over Processor: Batches run with Promise.all, small yield between batches
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (12)
✨ Finishing touches
🧪 Generate unit tests
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 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| maxItems: 1000, | ||
| maxSizeBytes: 5 * 1024 * 1024, | ||
| ttlMs: 1000 * 60 * 60, | ||
| persistent: false, |
There was a problem hiding this comment.
In static 'clearPersistentForBranch', the directory cache is configured with 'persistent: false' yet 'clearPersistent()' is called. Verify if this is intentional.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| maxItems: 1000, | ||
| maxSizeBytes: 5 * 1024 * 1024, | ||
| ttlMs: 1000 * 60 * 60, | ||
| persistent: false, |
There was a problem hiding this comment.
Configuration inconsistency bug: The directoryCache is created with persistent: false but then clearPersistent() is called on it. This is inconsistent - if the cache is not persistent, calling clearPersistent() is meaningless and could cause errors. This should be persistent: true to match the fileCache configuration and the intent of clearing persistent storage.
| persistent: false, | |
| persistent: true, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/client/src/components/store/editor/cache/file-cache.ts (1)
219-253: Don’t init/clear a non-persistent directory cache in clearPersistentForBranch (dup of prior review).directoryCache is created with persistent: false but clearPersistent() is invoked. Remove the directory cache from this method and scope docs to “file cache”.
Apply:
- /** - * Static method to clear persistent cache for a specific project/branch - * without creating an instance - */ + /** + * Clear persistent FILE cache for a specific project/branch without creating an instance. + */ static async clearPersistentForBranch(projectId: string, branchId: string): Promise<void> { try { - const fileCache = new UnifiedCacheManager({ + const fileCache = new UnifiedCacheManager({ name: `${projectId}-${branchId}-sandbox-files`, maxItems: 500, maxSizeBytes: 50 * 1024 * 1024, ttlMs: 1000 * 60 * 30, persistent: true, }); - - const directoryCache = new UnifiedCacheManager({ - name: `${projectId}-${branchId}-sandbox-directories`, - maxItems: 1000, - maxSizeBytes: 5 * 1024 * 1024, - ttlMs: 1000 * 60 * 60, - persistent: false, - }); await Promise.all([ fileCache.init(), - directoryCache.init(), ]); await Promise.all([ fileCache.clearPersistent(), - directoryCache.clearPersistent(), ]); } catch (error) { console.error(`Error clearing persistent cache for project ${projectId}, branch ${branchId}:`, error); } }Run to confirm UnifiedCacheManager behavior before changing:
#!/bin/bash # Locate UnifiedCacheManager and inspect clearPersistent/persistent handling fd -a 'unified-cache.*\.(ts|tsx|js)' | xargs -I{} sh -c 'echo "==> {}"; rg -n "class\s+UnifiedCacheManager|clearPersistent|persistent" -n {} -C3'
🧹 Nitpick comments (1)
apps/web/client/src/components/store/editor/cache/file-cache.ts (1)
10-26: Sanitize and centralize cache key construction to avoid invalid names and duplication.IDs may contain spaces/slashes. Build a safe, reusable prefix once and use it for both caches.
Apply:
- constructor(projectId: string, branchId: string) { - this.fileCache = new UnifiedCacheManager({ - name: `${projectId}-${branchId}-sandbox-files`, + constructor(projectId: string, branchId: string) { + const prefix = FileCacheManager.keyPrefix(projectId, branchId); + this.fileCache = new UnifiedCacheManager({ + name: `${prefix}-sandbox-files`, maxItems: 500, maxSizeBytes: 50 * 1024 * 1024, // 50MB ttlMs: 1000 * 60 * 30, // 30 minutes persistent: true, }); - this.directoryCache = new UnifiedCacheManager({ - name: `${projectId}-${branchId}-sandbox-directories`, + this.directoryCache = new UnifiedCacheManager({ + name: `${prefix}-sandbox-directories`, maxItems: 1000, maxSizeBytes: 5 * 1024 * 1024, // 5MB ttlMs: 1000 * 60 * 60, // 1 hour persistent: false, // Directories are lightweight, no need to persist }); }Add (outside the range, in the class):
private static keyPrefix(projectId: string, branchId: string): string { const safe = (s: string) => encodeURIComponent(String(s).trim()); return `${safe(projectId)}-${safe(branchId)}`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/client/src/components/store/editor/cache/file-cache.ts(2 hunks)apps/web/client/src/components/store/editor/sandbox/file-sync.ts(1 hunks)apps/web/client/src/components/store/editor/sandbox/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/client/src/components/store/editor/sandbox/index.ts (1)
apps/web/client/src/components/store/editor/sandbox/file-sync.ts (1)
FileSyncManager(6-101)
apps/web/client/src/components/store/editor/sandbox/file-sync.ts (1)
apps/web/client/src/components/store/editor/cache/file-cache.ts (1)
FileCacheManager(6-254)
🔇 Additional comments (1)
apps/web/client/src/components/store/editor/sandbox/file-sync.ts (1)
9-11: LGTM: per-project/branch cache is correctly injected.Constructor change and pass-through look correct.
| this.errorManager | ||
| ); | ||
| this.fileSync = new FileSyncManager(); | ||
| this.fileSync = new FileSyncManager(this.branch.projectId, this.branch.id); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pass stable identifiers; audit call sites after signature change.
Looks good. Verify all constructors were updated and that branch.projectId/branch.id are immutable over the SandboxManager lifecycle.
To verify:
🏁 Script executed:
#!/bin/bash
# Find remaining old-API usages (zero-arg) of FileSyncManager
rg -nP "new\s+FileSyncManager\s*\(" -C2 | rg -nPv "new\s+FileSyncManager\s*\(\s*[^,]+\s*,\s*[^)]+\)"
# (Optional) consider an options object to prevent arg-order mistakes:
# new FileSyncManager({ projectId: branch.projectId, branchId: branch.id })Length of output: 1158
Pass stable identifiers; update remaining FileSyncManager call sites.
Mixed constructor usages detected — update call sites or restore backward-compatible constructor.
- apps/web/client/test/sandbox/file-sync.test.ts:42 — new FileSyncManager() (zero-arg) needs updating.
- apps/web/client/test/sandbox/sandbox.test.ts:329 — new FileSyncManager({ ... }) uses options-object (keep or standardize).
- apps/web/client/src/components/store/editor/sandbox/index.ts:57 — new FileSyncManager(this.branch.projectId, this.branch.id) — ensure branch.projectId and branch.id remain stable for the SandboxManager lifecycle (capture into consts if they can change).
Consider switching FileSyncManager to an options object to avoid arg-order mistakes (optional).
🤖 Prompt for AI Agents
In apps/web/client/src/components/store/editor/sandbox/index.ts around line 57,
the FileSyncManager is constructed with positional args new
FileSyncManager(this.branch.projectId, this.branch.id) while other call sites
use zero-arg or options-object forms; capture stable identifiers into consts
(e.g., const projectId = this.branch.projectId; const branchId = this.branch.id)
before creating the manager to ensure values don't change during SandboxManager
lifecycle, and then update all FileSyncManager usages to a single consistent
constructor shape (prefer an options object like new FileSyncManager({
projectId, branchId }) or update tests and call sites to pass two explicit args)
so all call sites (apps/web/client/test/sandbox/file-sync.test.ts:42,
apps/web/client/test/sandbox/sandbox.test.ts:329, and this file) are consistent.
| init() { | ||
| reaction( | ||
| () => this.editorEngine.activeSandbox.routerConfig, | ||
| () => { | ||
| if (this.editorEngine.state.leftPanelTab !== LeftPanelTabValue.PAGES) { | ||
| return; | ||
| } | ||
| this.scanPages(); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Async setup race condition bug: The init() method sets up a reaction but is never called, meaning the reaction will never be established. The reaction setup should either be moved to the constructor or init() must be called during PageManager initialization. Without this, the automatic page scanning on router config changes will never occur.
| init() { | |
| reaction( | |
| () => this.editorEngine.activeSandbox.routerConfig, | |
| () => { | |
| if (this.editorEngine.state.leftPanelTab !== LeftPanelTabValue.PAGES) { | |
| return; | |
| } | |
| this.scanPages(); | |
| }, | |
| ); | |
| } | |
| constructor() { | |
| makeObservable(this, { | |
| pages: observable, | |
| scanPages: action, | |
| }); | |
| reaction( | |
| () => this.editorEngine.activeSandbox.routerConfig, | |
| () => { | |
| if (this.editorEngine.state.leftPanelTab !== LeftPanelTabValue.PAGES) { | |
| return; | |
| } | |
| this.scanPages(); | |
| }, | |
| ); | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| // Check cache first | ||
| const cachedFile = this.fileSync.readCache(filePath); | ||
| if (cachedFile && cachedFile.content !== null) { | ||
| if (this.isJsxFile(filePath)) { | ||
| await this.processFileForMapping(remoteFile); | ||
| await this.processFileForMapping(cachedFile); | ||
| } | ||
| } else { | ||
| const file = await this.fileSync.readOrFetch(filePath, this.readRemoteFile.bind(this)); | ||
| if (file && this.isJsxFile(filePath)) { | ||
| await this.processFileForMapping(file); | ||
| } |
There was a problem hiding this comment.
Logic error in cache validation: The code checks if cachedFile.content !== null but doesn't validate if the cached content is actually valid or up-to-date. This could lead to using stale cached data when the file has been modified. The cache check should include content hash validation or timestamp comparison to ensure data freshness.
| // Check cache first | |
| const cachedFile = this.fileSync.readCache(filePath); | |
| if (cachedFile && cachedFile.content !== null) { | |
| if (this.isJsxFile(filePath)) { | |
| await this.processFileForMapping(remoteFile); | |
| await this.processFileForMapping(cachedFile); | |
| } | |
| } else { | |
| const file = await this.fileSync.readOrFetch(filePath, this.readRemoteFile.bind(this)); | |
| if (file && this.isJsxFile(filePath)) { | |
| await this.processFileForMapping(file); | |
| } | |
| // Check cache first | |
| const cachedFile = this.fileSync.readCache(filePath); | |
| if (cachedFile && cachedFile.content !== null && this.fileSync.isCacheValid(filePath)) { | |
| if (this.isJsxFile(filePath)) { | |
| await this.processFileForMapping(cachedFile); | |
| } | |
| } else { | |
| const file = await this.fileSync.readOrFetch(filePath, this.readRemoteFile.bind(this)); | |
| if (file && this.isJsxFile(filePath)) { | |
| await this.processFileForMapping(file); | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Improve index caching by scoping it per project/branch and adding cache clearing, with updates to initialization and tests.
file-cache.tsandunified-cache.ts.file-cache.ts.EditorEngineinengine.tsto require project and branch context for file synchronization and caching.file-sync.test.tsandindex.test.tsto test new caching behavior and ensure correctness.useEffectinpage-selector.tsxandindex.tsxto improve performance.This description was created by
for 888436d. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Chores