Fix: Camera becomes Perspective in sketch mode - #9200
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I thought about this a bit more, the main question is what we want to do when the file changes while we're in sketch edit mode:
|
Just killer debugging. |
|
@andrewvarga I definitely see the connection to the work I'm trying to do over in #8885 and #8880. And I agree with your assessment that this file system listening is creating this uncontrolled flow, and we've had a policy of automatically accepting these updates without really squaring that with our internal state. I think I could go for alerting the user instead, or building a more robust system for responsibly updating underneath the user, but probably not for outright ignoring edits made to the user's file system underneath. Regardless of whether which behavior we align on, I've recently learned more about the tools available in CodeMirror to handle updates more responsibly, which I think would be good for us to employ with @Irev-Dev and I are gonna talk about CodeMirror later today if you want to join! |
|
@andrewvarga here's a little proof-of-concept CodeMirror extension that fires a basic window alert whenever a transaction is marked as originating from disk. I guess my suggestion is that once we decide on what behavior we want we use these sort of mechanisms to implement it. DemoScreenshare.-.2025-12-09.10_46_08.AM-compressed.mp4 |
|
@franknoirot thanks for that demo, I agree annotations look useful for us! Wish there was a way to mark the file system change caused by us writing to disk to be able to differentiate it from external apps doing that but I think that's pretty obviously not going to be possible (file system doesn't keep track of the process which made the last edit).
For valid external edits, I think we could introduce an alert in that case but we have to make sure it's not producing false alarms (edits made by ZDS), which would be super annoying. I think this should come from our users needs. Are they going to work on version tracked .kcl files (synced via git / google drive / etc)? If yes, then it can definitely happen that they pull in a new sync while a file is open..I'm not sure they would like to have their work automatically getting lost..maybe undo would actually help in this case so they can recover it, but better to show an alert if it works reliably. |
Interested in that, let me know! |
File entries have a |
Yeah I'd support that. Definitely not a full window alert like I showed in my demo, but we could "freeze" the UI and provide a small confirmation dialog, basically just a designed version of that. Plugging that into my branch shouldn't be too hard if you want to have a go. |
… by ZDS in file watcher
@franknoirot That's a great idea! I digged in a bit more, we have a writeCausedByAppCheckedInFileTreeFileSystemWatcher flag too aimed to ignore the filewatcher for updates initiated by ZDS. In other times it only triggers once as expected: I have now implemented a check for last updated metadata of the file which seems to be working well, I couldn't get the perspective camera bug to appear anymore. I fixed the return type of Let me know what you think! Not sure if I should add a test, the timing is a bit racy, so even if I can manage to nail the timing for a failure (without the fix in this PR), it will be very fragile.. |
| // Re execute the file you are in because an imported file was changed | ||
| await kclManager.executeAst() | ||
| } else { | ||
| const fileNameWithExtension = getStringAfterLastSeparator(path) |
There was a problem hiding this comment.
Just moved these down to the else branch from above, since this is the only place they're used
| lastUpdatedMs < kclManager.lastWrite.time + 50 | ||
| ) { | ||
| // Ignore this change event, last update of the file was likely caused by us | ||
| return |
There was a problem hiding this comment.
This is the actual fix which ignores the change even if we're close to the file's last updated timestamp.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (window.electron) { | ||
| try { | ||
| const stat = await window.electron.stat(path) | ||
| const lastUpdatedMs = stat?.mtimeMs | ||
| if (kclManager.lastWrite && typeof lastUpdatedMs === 'number') { | ||
| // If last write happened shortly before the file was updated, it means the file was updated by us | ||
| // Typically the delay is 2-4 ms, so we allow a 50ms margin after the write and a 2ms margin | ||
| // before the write (just for inaccuracies for the timestamp). | ||
| if ( | ||
| kclManager.lastWrite.time - 2 < lastUpdatedMs && | ||
| lastUpdatedMs < kclManager.lastWrite.time + 50 | ||
| ) { | ||
| // Ignore this change event, last update of the file was likely caused by us | ||
| return | ||
| } | ||
| } | ||
| } catch (e) { | ||
| console.warn('stat failed for change event', e) | ||
| } | ||
|
|
||
| return false | ||
| }) | ||
| } |
There was a problem hiding this comment.
The timestamp check is applied to ALL file changes (including imported files), but kclManager.lastWrite only tracks writes to the current file. This causes incorrect behavior when imported files are modified.
Bug scenario: If the current file is written at time T, and within 50ms an imported file is modified externally, the imported file change will be incorrectly ignored because the timestamp check compares the imported file's mtime against the current file's write time.
Fix: Move the timestamp check inside the if (isCurrentFile) block (after line 99) so it only applies to changes in the current file:
const isCurrentFile = loadedProject?.file?.path === path
if (isCurrentFile) {
if (window.electron) {
// Timestamp check here
try {
const stat = await window.electron.stat(path)
const lastUpdatedMs = stat?.mtimeMs
if (kclManager.lastWrite && typeof lastUpdatedMs === 'number') {
if (
kclManager.lastWrite.time - 2 < lastUpdatedMs &&
lastUpdatedMs < kclManager.lastWrite.time + 50
) {
return
}
}
} catch (e) {
console.warn('stat failed for change event', e)
}
// Rest of current file handling...
}
}Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
this was actually valid
| // Wait one event loop to give a chance for params to be set | ||
| // Save the file to disk | ||
| this.lastWrite = { | ||
| code: this.code ?? '', |
There was a problem hiding this comment.
code is not used yet, I was thinking of comparing the file's current content in the file watcher agains the last written code, instead of the current code in memory, as that makes more sense.
jtran
left a comment
jtran
left a comment
There was a problem hiding this comment.
I tested this with new sketch mode, i.e. sketch solve, and it seems to fix the issue for me.
franknoirot
left a comment
franknoirot
left a comment
There was a problem hiding this comment.
Nice thanks for the thoughtful fix @andrewvarga! We should just make sure @lee-at-zoo-corp has a heads up to your changes to electron.stat since he's working on the FS with his web parity work, so he keeps the spirit of your fixes here when he rebases.
3 participants
See video in #9186
No code fixes in the PR yet, just starting a discussion.
Sketch mode should never use a perspective camera, yet in some cases it does switch to that. It's not too easy to reproduce it, definitely race conditions going on.
Here is the normal flow of events when dragging a segment in sketch mode:
RouteProvideris constantly detecting file changes inuseFileSystemWatcherand if there is actual change in the code it updates and re-executes it (this is what's causing the bug, the code IS different)What I found when I reproduced the issue was that the switch to perspective camera comes from here in RouteProvider: this piece of code listens for the current file's changes on the system level, and updates + executes the new code if it changed.
I couldn't find a way to reproduce this 100%, but it does happen when I'm dragging a segment, wait about a second and then edit it again: this is when the first edit's writeToFile kicks in, which triggers the file watcher, but the second edit changed the code again, so
RouterProviderthinks it has actually changed externally and updates the code <- this is the actual problem, it also callsresetCameraPositionbut we shouldn't detect this as external code change in the first place.Is this code here to support having an open ZDS and still allow people to edit files "underneath" it via other means (manual file edits, etc)? This situation in itself sounds very prone for races.
Wouldn't it be simpler and safer to ignore edits during the file being open in ZDS, or at least when in sketch mode?
Most text editors / programming IDEs also keep the in-memory state intact and if they detect external changes coming in they ask you which version you want to keep.
If we want to keep the support of editing files underneath ZDS, it's still possible to fix this issue but it's a bit more complicated so I just wanted to kick off the discussion first. We'd have to synchronize execution better, which can either be a local change that covers this case, or it might need a more architectural refactor regarding execution in general. I think @franknoirot was huddling about this recently.
Related PR, cc @nadr0 : #8115