Fix flaky open-connect-content e2e test #3677
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import { | |
| Disposable, | ||
| EventEmitter, | ||
| FileChangeEvent, | ||
| FileChangeType, | ||
| FileStat, | ||
| FileSystemError, | ||
| FileSystemProvider, | ||
|
|
@@ -109,12 +110,36 @@ export class ConnectContentFileSystemProvider implements FileSystemProvider { | |
|
|
||
| async stat(uri: Uri): Promise<FileStat> { | ||
| logger.info(`connect-content stat ${uri.toString()}`); | ||
| // For root connect-content URIs (e.g. /{guid}), return a directory stat | ||
| // immediately so that updateWorkspaceFolders / openFolder can validate the | ||
| // workspace folder without waiting for the (potentially slow) bundle fetch. | ||
| // The actual bundle is fetched lazily when readDirectory() is called. | ||
| if (isRootContentUri(uri)) { | ||
| // Kick off the bundle fetch in the background so readDirectory() is faster | ||
| void this.ensureBundleForUri(uri); | ||
|
Collaborator
There was a problem hiding this comment. what does
Collaborator
Author
There was a problem hiding this comment. claude is indicating that this is to avoid linting errors because this is an asynchronous promise that we are explicitly not awaiting on.
Collaborator
There was a problem hiding this comment. Interesting we don't use
Collaborator
There was a problem hiding this comment. It discards the return value of |
||
| const cached = contentRoots.get(uri.toString()); | ||
| return statFromEntry(cached ?? createDirectoryEntry()); | ||
| } | ||
| const entry = await this.resolveEntry(uri); | ||
| return statFromEntry(entry); | ||
| } | ||
|
|
||
| async readDirectory(uri: Uri): Promise<[string, FileType][]> { | ||
| logger.info(`connect-content readDirectory ${uri.toString()}`); | ||
| // For root URIs, return whatever is cached immediately. If the bundle | ||
| // hasn't been fetched yet, return an empty list and notify VS Code to | ||
| // refresh the tree once the fetch completes. | ||
| if (isRootContentUri(uri)) { | ||
| const cached = contentRoots.get(uri.toString()); | ||
| if (cached) { | ||
| return listDirectory(cached); | ||
| } | ||
| // Bundle not ready yet — start fetching and notify when done | ||
| this.ensureBundleForUri(uri).then(() => { | ||
| this.fileChangeEmitter.fire([{ type: FileChangeType.Changed, uri }]); | ||
| }); | ||
| return []; | ||
| } | ||
| const entry = await this.resolveEntry(uri); | ||
| return listDirectory(entry); | ||
| } | ||
|
|
@@ -157,14 +182,13 @@ export class ConnectContentFileSystemProvider implements FileSystemProvider { | |
| logger.warn( | ||
| `No credentials for ${normalizedServer}. Opening credential flow.`, | ||
| ); | ||
| // Launch the credential dialog without awaiting it. Blocking here would | ||
| // stall the filesystem provider's stat() call, preventing the explorer | ||
| // from rendering anything until the user completes the dialog. | ||
| void commands.executeCommand( | ||
| Commands.HomeView.AddCredential, | ||
| normalizedServer, | ||
| ); | ||
|
Comment on lines
+185
to
-167
Collaborator
There was a problem hiding this comment. I don't think this is right. This means we would throw an error immediately while trying to add a credential doesn't it? You can see the bad behavior if you try to open connect content on a server that you don't have credentials for yet. It leaves you in a bad state where you have an error for no valid credentials, and an empty temporary workspace, which isn't great.
Collaborator
There was a problem hiding this comment. I changed this to and that worked much better. I didn't get the "no valid credentials" error, but it take a bit of time for the files to load in. I don't think that is a blocker, but something that could be improved in the future. I thought it wasn't working for a second. |
||
| throw new Error(`No valid credentials available for ${normalizedServer}`); | ||
| } | ||
|
|
||
|
|
@@ -229,10 +253,14 @@ export class ConnectContentFileSystemProvider implements FileSystemProvider { | |
| logger.error( | ||
| `Unable to fetch bundle ${contentGuid} for ${normalizedServerUrl}: ${message}`, | ||
| ); | ||
| // Populate the cache with an empty directory BEFORE showing the error | ||
| // dialog. This unblocks the filesystem provider's stat() call so the | ||
| // explorer can render the (empty) folder immediately instead of hanging | ||
| // until the user dismisses the notification. | ||
| contentRoots.set(rootKey, createDirectoryEntry()); | ||
| void window.showErrorMessage( | ||
| `Unable to open Connect content ${contentGuid}: ${message}`, | ||
| ); | ||
| return; | ||
| } | ||
| } | ||
|
|
@@ -318,6 +346,14 @@ function decodeAuthorityAsServerUrl(authority: string): string | null { | |
| return `https://${authority}`; | ||
| } | ||
|
|
||
| // Check whether a URI points to the root of a content GUID | ||
| // (i.e. path is "/{guid}" with no sub-path segments). | ||
| function isRootContentUri(uri: Uri): boolean { | ||
| const trimmed = uri.path.replace(/^\/+/, "").replace(/\/+$/, ""); | ||
| // Root URIs have exactly one segment (the GUID) with no slashes | ||
| return trimmed.length > 0 && !trimmed.includes("/"); | ||
| } | ||
|
|
||
| function parseConnectContentUri(uri: Uri) { | ||
| if (uri.scheme !== CONNECT_CONTENT_SCHEME) { | ||
| return null; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand why we only returned right away if we were looking at a root content
Uri. I did a bit of digging and VS Code stats the root URI during workspace folder validation. Since other sub-paths are accessed after the explorer is rendered we only need the special handling here.