Skip to content

Link to local file for permalinks in webview#8583

Open
Daniel-Aaron-Bloom wants to merge 4 commits intomicrosoft:mainfrom
Daniel-Aaron-Bloom:main
Open

Link to local file for permalinks in webview#8583
Daniel-Aaron-Bloom wants to merge 4 commits intomicrosoft:mainfrom
Daniel-Aaron-Bloom:main

Conversation

@Daniel-Aaron-Bloom
Copy link

This pull request adds the functionality to link to a file within the project in a comment on the PR/issue overview pages. This feature creates parity with #5558, so links appear and function identically in both locations. It also enables users to quickly jump around using a list of referenced file locations in the PR description or a comment.

Fixes #8571

This PR also adds a few await this._waitForReady; in _replyMessage and _throwError, which I think are the correct behavior, and also without them this extension sometimes fails to respond.

@Daniel-Aaron-Bloom
Copy link
Author

@microsoft-github-policy-service agree

seq: originalMessage.req,
res: message,
};
await this._waitForReady;
Copy link
Member

Choose a reason for hiding this comment

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

Why wait here?

Copy link
Author

Choose a reason for hiding this comment

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

For the same reason we wait in _postMessage above

protected async _postMessage(message: any) {
 	// Without the following ready check, we can end up in a state where the message handler in the webview
 	// isn't ready for any of the messages we post.
 	await this._waitForReady;
 	this._webview?.postMessage({
 		res: message,
 	});
 }

Copy link
Author

Choose a reason for hiding this comment

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

In practice without this, the extension simply fails to work about half the time in a dev container (never got a failure outside of the container). The await calls to get the file mapping simply never resolve (presumably because the message got dropped)

Copy link
Author

Choose a reason for hiding this comment

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

With the refactor, this became less of an issue, but it still seems like the right thing to do.

seq: originalMessage?.req,
err: error,
};
await this._waitForReady;
Copy link
Member

Choose a reason for hiding this comment

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

Why wait here?

Copy link
Author

Choose a reason for hiding this comment

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

Same as above

@Daniel-Aaron-Bloom
Copy link
Author

Yup. Much cleaner and more efficient. I've also added diff link support to open the diff view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Local-ized Links in PR and Issue Overviews

2 participants