Turn requestfilesystem into a single layer providing project.FileHandle lookups so it can be used as the top layer above editor overlays - #64263
Conversation
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
There was a problem hiding this comment.
🟡 Changes recommended
Shared API sessions can switch layers without invalidating reused programs, producing inconsistent snapshot and program contents.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
| // The legacy updateSnapshot API restarts from the host when no base snapshot | ||
| // handle is supplied, rather than inheriting the canonical snapshot's layer. | ||
| // This workaround is removed by the snapshot state redesign in #64154. | ||
| fileSystemLayer, layerChanges, err := requestfilesystem.NewForUpdate(params.FileSystem, baseFileSystemLayer, baseSD == nil, s.fileSystem(), s.currentDirectory()) |
There was a problem hiding this comment.
Don't care, this is going away in the next PR in the stack
| export interface CreateFileSystemOptions { | ||
| /** Complete directory listings. Full filesystems derive these from `files` when omitted. */ | ||
| /** | ||
| * Complete `getAccessibleEntries` results. These do not constrain direct descendant lookups. |
There was a problem hiding this comment.
I'm not clear on what this means.
There was a problem hiding this comment.
Like
Subsequent lookups are not limited to the entries provided. Files may be "opened", or a file may exist on the host file system.
?
| ) | ||
|
|
||
| func TestFileChangesIncludeDirectoryTombstones(t *testing.T) { | ||
| func changeMap(changes []project.FileSourceLayerChange) map[string][2]bool { |
There was a problem hiding this comment.
Are you just doing this to statically avoid duplicates? You could just do a list of FileSourceLayerChange and make it clear which fields are explicitly true.
| result := make([]project.FileSourceLayerChange, 0, len(changes)) | ||
| for _, change := range changes { | ||
| result = append(result, change) | ||
| } |
There was a problem hiding this comment.
| result := make([]project.FileSourceLayerChange, 0, len(changes)) | |
| for _, change := range changes { | |
| result = append(result, change) | |
| } | |
| result := slices.Clone(changes) | |
| } |
| for _, alias := range baseRequestFS.aliasesForPath(fileName) { | ||
| addChange(alias, deleted) | ||
| } | ||
| shadowsRequestSymlinks := func(fileName string) bool { |
There was a problem hiding this comment.
What does this name of this mean? Is requestShadowsSymlinks a better name? pathShadowsHostSymlink?
There was a problem hiding this comment.
Okay, sorry, I didn't notice this was in requestfilesystem so I guess this is like shadowsReqFsSymlink?
| if shadowsRequestSymlinks(absolutePath) { | ||
| return project.FileSourceLayerChanges{InvalidateAll: true} | ||
| } | ||
| addRequestDescendants(absolutePath) | ||
| addWithAliases(absolutePath, true, true) |
There was a problem hiding this comment.
There's really nothing else to be done for a "removed" file? Maybe I'm out of my depth, but how is removing a host file done with these recursive calls?
After #64115 was merged, I realized its approach had a surprising interaction with editor overlays when using the LSP-connected API. If you get an LSP-based snapshot and then do
you would clearly expect the
newSnapshotto have the layer-provided contents formain.ts, but that will only be true ifmain.tsis not open in the editor.#64115 works by creating a full
vfs.FSview of the layer contents falling back to the host filesystem (if the layer is configured to fall back) which it passes to the project system, whereSnapshotFSdoes its own layering: editor overlays on top of its cached files on top of thevfs.FScreated by #64115, passed in by the API request. That's not the desired layer ordering, and avfs.FSdoesn’t provide enough info to split the actual API-provided layer apart from the host filesystem so the layers can be re-stacked as desired.So, this PR refactors
requestfilesystemto return a single layer providing the newproject.FileSourceLayerinterface that givesSnapshotFSenough info upon lookup to be able to stack that layer on top of the other layers it owns.