-
Notifications
You must be signed in to change notification settings - Fork 725
Link to local file for permalinks in webview #8583
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,7 @@ export class WebviewBase extends Disposable { | |
| seq: originalMessage.req, | ||
| res: message, | ||
| }; | ||
| await this._waitForReady; | ||
| this._webview?.postMessage(reply); | ||
| } | ||
|
|
||
|
|
@@ -82,6 +83,7 @@ export class WebviewBase extends Disposable { | |
| seq: originalMessage?.req, | ||
| err: error, | ||
| }; | ||
| await this._waitForReady; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why wait here?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
| this._webview?.postMessage(reply); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,86 @@ import { PullRequest } from '../../src/github/views'; | |
| import { COMMENT_TEXTAREA_ID } from '../common/constants'; | ||
| import PullRequestContext from '../common/context'; | ||
|
|
||
| const PROCESSED_MARKER = 'data-permalink-processed'; | ||
|
|
||
| interface PermalinkAnchor { | ||
| element: HTMLAnchorElement; | ||
| url: string; | ||
| file: string; | ||
| startLine: number; | ||
| endLine: number; | ||
| } | ||
|
|
||
| function findUnprocessedPermalinks( | ||
| root: Document | Element, | ||
|
Comment on lines
+24
to
+25
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The approach of finding the permalinks in the webview involves multiple back-and-forths with the extension. I would suggest a different approach:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can give that a shot. |
||
| repoName: string, | ||
| ): PermalinkAnchor[] { | ||
| const anchors: PermalinkAnchor[] = []; | ||
| const urlPattern = new RegExp( | ||
| `^https://github\\.com/[^/]+/${repoName}/blob/[0-9a-f]{40}/([^#]+)#L([0-9]+)(?:-L([0-9]+))?$`, | ||
| ); | ||
|
|
||
| // Find all unprocessed anchor elements | ||
| const allAnchors = root.querySelectorAll( | ||
| `a[href^="https://github.com/"]:not([${PROCESSED_MARKER}])`, | ||
| ); | ||
|
|
||
| allAnchors.forEach((anchor: Element) => { | ||
| const htmlAnchor = anchor as HTMLAnchorElement; | ||
|
|
||
| const href = htmlAnchor.getAttribute('href'); | ||
| if (!href) return; | ||
|
|
||
| const match = href.match(urlPattern); | ||
| if (match) { | ||
| const file = match[1]; | ||
| const startLine = parseInt(match[2]); | ||
| const endLine = match[3] ? parseInt(match[3]) : startLine; | ||
|
|
||
| anchors.push({ | ||
| element: htmlAnchor, | ||
| url: href, | ||
| file, | ||
| startLine, | ||
| endLine, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| return anchors; | ||
| } | ||
|
|
||
|
|
||
| function updatePermalinks( | ||
| anchors: PermalinkAnchor[], | ||
| fileExistenceMap: Record<string, boolean>, | ||
| ): void { | ||
| anchors.forEach(({ element, url, file, startLine, endLine }) => { | ||
| const exists = fileExistenceMap[file]; | ||
| if (!exists) { | ||
| return; | ||
| } | ||
|
|
||
| element.setAttribute('data-local-file', file); | ||
| element.setAttribute('data-start-line', startLine.toString()); | ||
| element.setAttribute('data-end-line', endLine.toString()); | ||
|
|
||
| // Add "(view on GitHub)" link after this anchor | ||
| const githubLink = document.createElement('a'); | ||
| githubLink.href = url; | ||
| githubLink.textContent = 'view on GitHub'; | ||
| githubLink.setAttribute(PROCESSED_MARKER, 'true'); | ||
| if (element.className) { | ||
| githubLink.className = element.className; | ||
| } | ||
| element.after( | ||
| document.createTextNode(' ('), | ||
| githubLink, | ||
| document.createTextNode(')'), | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| export function main() { | ||
| render(<Root>{pr => <Overview {...pr} />}</Root>, document.getElementById('app')); | ||
| } | ||
|
|
@@ -41,6 +121,72 @@ export function Root({ children }) { | |
| return () => window.removeEventListener('focus', handleWindowFocus); | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| const handleLinkClick = (event: MouseEvent) => { | ||
| const target = event.target as HTMLElement; | ||
| const anchor = target.closest('a[data-local-file]'); | ||
| if (anchor) { | ||
| const file = anchor.getAttribute('data-local-file'); | ||
| const startLine = anchor.getAttribute('data-start-line'); | ||
| const endLine = anchor.getAttribute('data-end-line'); | ||
| if (file && startLine && endLine) { | ||
| // Swallow the event and open the file | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| ctx.openLocalFile(file, parseInt(startLine), parseInt(endLine)); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('click', handleLinkClick, true); | ||
| return () => document.removeEventListener('click', handleLinkClick, true); | ||
| }, [ctx]); | ||
|
|
||
| // Process GitHub permalinks | ||
| useEffect(() => { | ||
| if (!pr) return; | ||
|
|
||
| const processPermalinks = debounce(async () => { | ||
| try { | ||
| const anchors = findUnprocessedPermalinks(document.body, pr.repo); | ||
| anchors.forEach(({ element }) => { | ||
| element.setAttribute(PROCESSED_MARKER, 'true'); | ||
| }); | ||
|
|
||
| if (anchors.length > 0) { | ||
| const uniqueFiles = Array.from(new Set(anchors.map((a) => a.file))); | ||
| const fileExistenceMap = await ctx.checkFilesExist(uniqueFiles); | ||
| updatePermalinks(anchors, fileExistenceMap); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error processing permalinks:', error); | ||
| } | ||
| }, 100); | ||
|
|
||
| // Start observing the document body for changes | ||
| const observer = new MutationObserver((mutations) => { | ||
| const hasNewNodes = mutations.some( | ||
| ({ addedNodes }) => addedNodes.length > 0, | ||
| ); | ||
|
|
||
| if (hasNewNodes) { | ||
| processPermalinks(); | ||
| } | ||
| }); | ||
| observer.observe(document.body, { | ||
| childList: true, | ||
| subtree: true, | ||
| }); | ||
|
|
||
| // Process the initial set of links | ||
| processPermalinks(); | ||
|
|
||
| return () => { | ||
| observer.disconnect(); | ||
| processPermalinks.clear(); | ||
| }; | ||
| }, [pr, ctx]); | ||
|
|
||
| window.onscroll = debounce(() => { | ||
| ctx.postMessage({ | ||
| command: 'scroll', | ||
|
|
||
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.
Why wait here?
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.
For the same reason we wait in
_postMessageaboveThere 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.
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)