Skip to content

fix: stricter file caching #2844

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Kitenite merged 4 commits into from
Sep 15, 2025
Merged

fix: stricter file caching #2844

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f74eab
fix: stricter caching
Kitenite Sep 15, 2025
8273d32
cache on copy file
Kitenite Sep 15, 2025
67c3018
update images
Kitenite Sep 15, 2025
c60429f
update images
Kitenite Sep 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 21 additions & 36 deletions apps/web/client/src/components/store/editor/sandbox/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,42 +549,20 @@ export class SandboxManager {
async handleFileChangedEvent(normalizedPath: string) {
const cachedFile = this.fileSync.readCache(normalizedPath);

if (isImageFile(normalizedPath)) {
if (!cachedFile || cachedFile.content === null) {
// If the file was not cached, we need to write an empty file
this.fileSync.writeEmptyFile(normalizedPath, 'binary');
} else {
// If the file was already cached, we need to read the remote file and update the cache
const remoteFile = await this.readRemoteFile(normalizedPath);
if (!remoteFile || remoteFile.content === null) {
console.error(`File content for ${normalizedPath} not found in remote`);
return;
}
this.fileSync.updateCache(remoteFile);
}
} else {
// If the file is not an image, we need to read the remote file and update the cache
const remoteFile = await this.readRemoteFile(normalizedPath);
if (!remoteFile || remoteFile.content === null) {
console.error(`File content for ${normalizedPath} not found in remote`);
return;
}
if (remoteFile.type === 'text') {
// If the file is a text file, we need to process it for mapping
this.fileSync.updateCache({
type: 'text',
path: normalizedPath,
content: remoteFile.content,
});
if (remoteFile.content !== cachedFile?.content) {
await this.processFileForMapping(remoteFile);
}
} else {
this.fileSync.updateCache({
type: 'binary',
path: normalizedPath,
content: remoteFile.content,
});
// Always read the remote file and update the cache, regardless of file type
const remoteFile = await this.readRemoteFile(normalizedPath);
if (!remoteFile) {
console.error(`File content for ${normalizedPath} not found in remote`);
return;
}
Comment on lines +553 to +557
Copy link
Contributor

@graphite-app graphite-app bot Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic error: The code returns early if remoteFile is falsy, but the comment on line 552 states it should 'always update cache regardless of file type'. This creates inconsistent behavior where some file change events are silently ignored. The function should either handle the null case appropriately (perhaps by removing the file from cache) or the early return should be reconsidered to match the stated behavior.

Suggested change
const remoteFile = await this.readRemoteFile(normalizedPath);
if (!remoteFile) {
console.error(`File content for ${normalizedPath} not found in remote`);
return;
}
const remoteFile = await this.readRemoteFile(normalizedPath);
if (!remoteFile) {
console.error(`File content for ${normalizedPath} not found in remote`);
// Remove file from cache instead of returning early
this.removeFromCache(normalizedPath);
return;
}

Spotted by Diamond

Fix in Graphite

Is this helpful? React 👍 or 👎 to let us know.


// Always update the cache with the fresh remote file content
this.fileSync.updateCache(remoteFile);

// For text files, also process for mapping if content has changed
if (remoteFile.type === 'text' && this.isJsxFile(normalizedPath)) {
if (remoteFile.content !== cachedFile?.content) {
await this.processFileForMapping(remoteFile);
}
}
}
Expand Down Expand Up @@ -678,6 +656,12 @@ export class SandboxManager {
},
});

// Read and cache the copied file
const copiedFile = await this.readRemoteFile(normalizedTargetPath);
if (copiedFile) {
this.fileSync.updateCache(copiedFile);
}

Comment on lines +659 to +664
Copy link

@coderabbitai coderabbitai bot Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Copying directories leaves cache stale; handle directory targets

After copyFiles, attempting to read a directory via readRemoteFile returns null and the directory cache isn’t updated. This desynchronizes the cache on directory copies (common when recursive is true).

Apply this diff:

-            // Read and cache the copied file
-            const copiedFile = await this.readRemoteFile(normalizedTargetPath);
-            if (copiedFile) {
-                this.fileSync.updateCache(copiedFile);
-            }
+            // Sync cache for the copy target (file or directory)
+            const stat = await this.session.provider.statFile({
+                args: { path: normalizedTargetPath },
+            });
+            if (stat?.type === 'directory') {
+                this.fileSync.updateDirectoryCache(normalizedTargetPath);
+                // Optional: list + seed child entries if provider doesn't emit add events for children.
+                // (Rely on watcher by default.)
+            } else {
+                const copiedFile = await this.readRemoteFile(normalizedTargetPath);
+                if (copiedFile) {
+                    this.fileSync.updateCache(copiedFile);
+                }
+            }
📝 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.

Suggested change
// Read and cache the copied file
const copiedFile = await this.readRemoteFile(normalizedTargetPath);
if (copiedFile) {
this.fileSync.updateCache(copiedFile);
}
// Sync cache for the copy target (file or directory)
const stat = await this.session.provider.statFile({
args: { path: normalizedTargetPath },
});
if (stat?.type === 'directory') {
this.fileSync.updateDirectoryCache(normalizedTargetPath);
// Optional: list + seed child entries if provider doesn't emit add events for children.
// (Rely on watcher by default.)
} else {
const copiedFile = await this.readRemoteFile(normalizedTargetPath);
if (copiedFile) {
this.fileSync.updateCache(copiedFile);
}
}
🤖 Prompt for AI Agents 
In apps/web/client/src/components/store/editor/sandbox/index.ts around lines 659
to 664, the code calls readRemoteFile after copyFiles and skips cache updates
when readRemoteFile returns null, which leaves the cache stale for directory
targets; change the logic so that if readRemoteFile returns null you detect
whether the normalizedTargetPath is a directory (e.g., via readRemoteDirectory
or a metadata/stat call), and when it is update the fileSync cache with a
directory entry (and for recursive copies, fetch and update the directory
contents recursively or refresh the parent directory listing) so the cache
reflects the newly copied directory structure.

return true;
} catch (error) {
console.error(`Error copying ${path} to ${targetPath}:`, error);
Expand Down Expand Up @@ -744,6 +728,7 @@ export class SandboxManager {
},
});

// Note: Cache update handled by file watcher rename event
return true;
} catch (error) {
console.error(`Error renaming file ${oldPath} to ${newPath}:`, error);
Expand Down