Skip to content

Commit 8304066

Browse files
refactor based on feedback
1 parent 66bbd57 commit 8304066

7 files changed

Lines changed: 242 additions & 121 deletions

File tree

common/views.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,7 @@ export interface OpenLocalFileArgs {
184184
file: string;
185185
startLine: number;
186186
endLine: number;
187+
href: string;
187188
}
188189

189190
// #endregion

src/common/utils.ts

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,4 +1014,165 @@ export function truncate(value: string, maxLength: number, suffix = '...'): stri
10141014
return value;
10151015
}
10161016
return `${value.substr(0, maxLength)}${suffix}`;
1017+
}
1018+
1019+
/**
1020+
* Metadata extracted from code reference link data attributes.
1021+
* This interface defines the contract between the extension (which creates the attributes)
1022+
* and the webview (which reads them).
1023+
*/
1024+
export interface CodeReferenceLinkMetadata {
1025+
localFile: string;
1026+
startLine: number;
1027+
endLine: number;
1028+
linkType: 'blob' | 'diff';
1029+
href: string;
1030+
}
1031+
1032+
/**
1033+
* Extracts code reference link metadata from an anchor element's data attributes.
1034+
* Returns null if any required attributes are missing.
1035+
*/
1036+
export function extractCodeReferenceLinkMetadata(anchor: Element): CodeReferenceLinkMetadata | null {
1037+
const localFile = anchor.getAttribute('data-local-file');
1038+
const startLine = anchor.getAttribute('data-start-line');
1039+
const endLine = anchor.getAttribute('data-end-line');
1040+
const linkType = anchor.getAttribute('data-link-type');
1041+
const href = anchor.getAttribute('href');
1042+
1043+
if (!localFile || !startLine || !endLine || !linkType || !href) {
1044+
return null;
1045+
}
1046+
1047+
return {
1048+
localFile,
1049+
startLine: parseInt(startLine),
1050+
endLine: parseInt(endLine),
1051+
linkType: linkType as 'blob' | 'diff',
1052+
href
1053+
};
1054+
}
1055+
1056+
/**
1057+
* Process GitHub blob permalinks in HTML and add data attributes for local file handling.
1058+
* Finds blob permalinks (e.g., /blob/[sha]/file.ts#L10), checks if files exist locally,
1059+
* and adds data attributes to enable clicking to open local files.
1060+
*
1061+
* @param bodyHTML - The HTML content to process
1062+
* @param repoOwner - GitHub repository owner
1063+
* @param repoName - GitHub repository name
1064+
* @param authority - Git protocol URL authority (e.g., 'github.com')
1065+
* @param fileExistsCheck - Async function that checks if a file exists locally given its relative path
1066+
* @returns Promise resolving to processed HTML
1067+
*/
1068+
export async function processPermalinks(
1069+
bodyHTML: string,
1070+
repoOwner: string,
1071+
repoName: string,
1072+
authority: string,
1073+
fileExistsCheck: (filePath: string) => Promise<boolean>
1074+
): Promise<string> {
1075+
try {
1076+
const escapedRepoName = escapeRegExp(repoName);
1077+
const escapedRepoOwner = escapeRegExp(repoOwner);
1078+
const escapedAuthority = escapeRegExp(authority);
1079+
1080+
// Process blob permalinks (exclude already processed links)
1081+
const blobPattern = new RegExp(
1082+
`<a\\s+(?![^>]*data-permalink-processed)([^>]*?href="https?:\/\/${escapedAuthority}\/${escapedRepoOwner}\/${escapedRepoName}\/blob\/[0-9a-f]{40}\/(?<filePath>[^"#]+)#L(?<startLine>\\d+)(?:-L(?<endLine>\\d+))?"[^>]*?)>(?<linkText>[^<]*?)<\/a>`,
1083+
'g'
1084+
);
1085+
1086+
return await stringReplaceAsync(bodyHTML, blobPattern, async (
1087+
fullMatch: string,
1088+
attributes: string,
1089+
filePath: string,
1090+
startLine: string,
1091+
endLine: string | undefined,
1092+
linkText: string
1093+
) => {
1094+
try {
1095+
// Extract the original URL from attributes
1096+
const hrefMatch = attributes.match(/ href="([^"]+)"/);
1097+
const originalUrl = hrefMatch ? hrefMatch[1] : '';
1098+
1099+
// Check if file exists locally
1100+
const exists = await fileExistsCheck(filePath);
1101+
if (exists) {
1102+
// File exists - add data attributes for local handling and "(view on GitHub)" suffix
1103+
const endLineValue = endLine || startLine;
1104+
return `<a data-permalink-processed="true" ${attributes} data-local-file="${filePath}" data-start-line="${startLine}" data-end-line="${endLineValue}" data-link-type="blob">${linkText}</a> (<a data-permalink-processed="true" href="${originalUrl}">view on GitHub</a>)`;
1105+
}
1106+
} catch (error) {
1107+
// File doesn't exist or check failed - keep original link
1108+
}
1109+
return fullMatch;
1110+
});
1111+
} catch (error) {
1112+
// Return original HTML if processing fails
1113+
return bodyHTML;
1114+
}
1115+
}
1116+
1117+
/**
1118+
* Process GitHub diff permalinks in HTML and add data attributes for local file handling.
1119+
* Finds diff permalinks (e.g., /pull/123/files#diff-[hash]R10), maps hashes to filenames,
1120+
* and adds data attributes to enable clicking to open diff views.
1121+
*
1122+
* @param bodyHTML - The HTML content to process
1123+
* @param repoOwner - GitHub repository owner
1124+
* @param repoName - GitHub repository name
1125+
* @param authority - Git protocol URL authority (e.g., 'github.com')
1126+
* @param hashMap - Map of diff hashes to file paths
1127+
* @param prNumber - Pull request number
1128+
* @returns Promise resolving to processed HTML
1129+
*/
1130+
export async function processDiffLinks(
1131+
bodyHTML: string,
1132+
repoOwner: string,
1133+
repoName: string,
1134+
authority: string,
1135+
hashMap: Record<string, string>,
1136+
prNumber: number
1137+
): Promise<string> {
1138+
try {
1139+
const escapedRepoName = escapeRegExp(repoName);
1140+
const escapedRepoOwner = escapeRegExp(repoOwner);
1141+
const escapedAuthority = escapeRegExp(authority);
1142+
1143+
const diffPattern = new RegExp(
1144+
`<a\\s+(?![^>]*data-permalink-processed)([^>]*?href="https?:\/\/${escapedAuthority}\/${escapedRepoOwner}\/${escapedRepoName}\/pull\/${prNumber}\/(?:files|changes)#diff-(?<diffHash>[a-f0-9]{64})(?:R(?<startLine>\\d+)(?:-R(?<endLine>\\d+))?)?"[^>]*?)>(?<linkText>[^<]*?)<\/a>`,
1145+
'g'
1146+
);
1147+
1148+
return await stringReplaceAsync(bodyHTML, diffPattern, async (
1149+
fullMatch: string,
1150+
attributes: string,
1151+
diffHash: string,
1152+
startLine: string | undefined,
1153+
endLine: string | undefined,
1154+
linkText: string
1155+
) => {
1156+
try {
1157+
// Extract the original URL from attributes
1158+
const hrefMatch = attributes.match(/ href="([^"]+)"/);
1159+
const originalUrl = hrefMatch ? hrefMatch[1] : '';
1160+
1161+
// Look up filename from hash
1162+
const fileName = hashMap[diffHash];
1163+
if (fileName) {
1164+
// Hash found - add data attributes for diff handling and "(view on GitHub)" suffix
1165+
const startLineValue = startLine || '1';
1166+
const endLineValue = endLine || startLineValue;
1167+
return `<a data-permalink-processed="true" ${attributes} data-local-file="${fileName}" data-start-line="${startLineValue}" data-end-line="${endLineValue}" data-link-type="diff">${linkText}</a> (<a data-permalink-processed="true" href="${originalUrl}">view on GitHub</a>)`;
1168+
}
1169+
} catch (error) {
1170+
// Failed to process - keep original link
1171+
}
1172+
return fullMatch;
1173+
});
1174+
} catch (error) {
1175+
// Return original HTML if processing fails
1176+
return bodyHTML;
1177+
}
10171178
}

src/github/issueOverview.ts

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -322,12 +322,15 @@ export class IssueOverviewPanel<TItem extends IssueModel = IssueModel> extends W
322322
this.setPanelTitle(this.buildPanelTitle(issueModel.number, issueModel.title));
323323

324324
// Process permalinks in bodyHTML before sending to webview
325-
issue.bodyHTML = await this.processLinksInBodyHtml(issue.bodyHTML);
325+
const processedBodyHTML = await this.processLinksInBodyHtml(issue.bodyHTML);
326+
const context = this.getInitializeContext(currentUser, issue, await this.processTimelineEvents(timelineEvents), repositoryAccess, viewerCanEdit, assignableUsers[this._item.remote.remoteName] ?? []);
327+
// Override bodyHTML with processed version without mutating original issue
328+
context.bodyHTML = processedBodyHTML;
326329

327330
Logger.debug('pr.initialize', IssueOverviewPanel.ID);
328331
this._postMessage({
329332
command: 'pr.initialize',
330-
pullrequest: this.getInitializeContext(currentUser, issue, await this.processTimelineEvents(timelineEvents), repositoryAccess, viewerCanEdit, assignableUsers[this._item.remote.remoteName] ?? []),
333+
pullrequest: context,
331334
});
332335

333336
} catch (e) {
@@ -574,7 +577,7 @@ export class IssueOverviewPanel<TItem extends IssueModel = IssueModel> extends W
574577
}
575578

576579
/**
577-
* Process permalinks in bodyHTML. Can be overridden by subclasses (e.g., PullRequestOverviewPanel)
580+
* Process code reference links in bodyHTML. Can be overridden by subclasses (e.g., PullRequestOverviewPanel)
578581
* to provide custom processing logic for different item types.
579582
* Returns undefined if bodyHTML is undefined.
580583
*/
@@ -590,22 +593,25 @@ export class IssueOverviewPanel<TItem extends IssueModel = IssueModel> extends W
590593
}
591594

592595
/**
593-
* Process permalinks in timeline events (comments, reviews, commits).
596+
* Process code reference links in timeline events (comments, reviews, commits).
594597
* Updates bodyHTML fields for all events that contain them.
595598
*/
596599
protected async processTimelineEvents(events: TimelineEvent[]): Promise<TimelineEvent[]> {
597600
return Promise.all(events.map(async (event) => {
598-
if (event.event === EventType.Commented || event.event === EventType.Reviewed || event.event === EventType.Committed) {
599-
event.bodyHTML = await this.processLinksInBodyHtml(event.bodyHTML);
601+
// Create a shallow copy to avoid mutating the original
602+
const processedEvent = { ...event };
603+
604+
if (processedEvent.event === EventType.Commented || processedEvent.event === EventType.Reviewed || processedEvent.event === EventType.Committed) {
605+
processedEvent.bodyHTML = await this.processLinksInBodyHtml(processedEvent.bodyHTML);
600606
// ReviewEvent also has comments array
601-
if (event.event === EventType.Reviewed && event.comments) {
602-
event.comments = await Promise.all(event.comments.map(async (comment: IComment) => {
603-
comment.bodyHTML = await this.processLinksInBodyHtml(comment.bodyHTML);
604-
return comment;
605-
}));
607+
if (processedEvent.event === EventType.Reviewed && processedEvent.comments) {
608+
processedEvent.comments = await Promise.all(processedEvent.comments.map(async (comment: IComment) => ({
609+
...comment,
610+
bodyHTML: await this.processLinksInBodyHtml(comment.bodyHTML)
611+
})));
606612
}
607613
}
608-
return event;
614+
return processedEvent;
609615
}));
610616
}
611617

@@ -631,8 +637,9 @@ export class IssueOverviewPanel<TItem extends IssueModel = IssueModel> extends W
631637
});
632638
}
633639

634-
protected _getTimeline(): Promise<TimelineEvent[]> {
635-
return this._item.getIssueTimelineEvents();
640+
protected async _getTimeline(): Promise<TimelineEvent[]> {
641+
const events = await this._item.getIssueTimelineEvents();
642+
return this.processTimelineEvents(events);
636643
}
637644

638645
private async changeAssignees(message: IRequestMessage<void>): Promise<void> {
@@ -658,7 +665,7 @@ export class IssueOverviewPanel<TItem extends IssueModel = IssueModel> extends W
658665
if (allAssignees) {
659666
const newAssignees: IAccount[] = allAssignees.map(item => item.user);
660667
await this._item.replaceAssignees(newAssignees);
661-
const events = await this.processTimelineEvents(await this._getTimeline());
668+
const events = await this._getTimeline();
662669
const reply: ChangeAssigneesReply = {
663670
assignees: newAssignees,
664671
events
@@ -725,7 +732,7 @@ export class IssueOverviewPanel<TItem extends IssueModel = IssueModel> extends W
725732
const newAssignees = (this._item.assignees ?? []).concat(currentUser);
726733
await this._item.replaceAssignees(newAssignees);
727734
}
728-
const events = await this.processTimelineEvents(await this._getTimeline());
735+
const events = await this._getTimeline();
729736
const reply: ChangeAssigneesReply = {
730737
assignees: this._item.assignees ?? [],
731738
events
@@ -743,7 +750,7 @@ export class IssueOverviewPanel<TItem extends IssueModel = IssueModel> extends W
743750
const newAssignees = (this._item.assignees ?? []).concat(copilotUser);
744751
await this._item.replaceAssignees(newAssignees);
745752
}
746-
const events = await this.processTimelineEvents(await this._getTimeline());
753+
const events = await this._getTimeline();
747754
const reply: ChangeAssigneesReply = {
748755
assignees: this._item.assignees ?? [],
749756
events
@@ -817,6 +824,8 @@ export class IssueOverviewPanel<TItem extends IssueModel = IssueModel> extends W
817824
});
818825
} catch (e) {
819826
Logger.error(`Open local file failed: ${formatError(e)}`, IssueOverviewPanel.ID);
827+
// Fallback to opening external URL
828+
await vscode.env.openExternal(vscode.Uri.parse(message.args.href));
820829
}
821830
}
822831

src/github/pullRequestOverview.ts

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
404404
this.setPanelTitle(this.buildPanelTitle(pullRequestModel.number, pullRequestModel.title));
405405

406406
// Process permalinks in bodyHTML before sending to webview
407-
pullRequest.bodyHTML = await this.processLinksInBodyHtml(pullRequest.bodyHTML);
407+
const processedBodyHTML = await this.processLinksInBodyHtml(pullRequest.bodyHTML);
408408

409409
const isCurrentlyCheckedOut = pullRequestModel.equals(this._folderRepositoryManager.activePullRequest);
410410
const mergeMethodsAvailability = repositoryAccess!.mergeMethodsAvailability;
@@ -420,6 +420,8 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
420420
const copilotUser = users.find(user => COPILOT_ACCOUNTS[user.login]);
421421
const isCopilotAlreadyReviewer = this._existingReviewers.some(reviewer => !isITeam(reviewer.reviewer) && reviewer.reviewer.login === COPILOT_REVIEWER);
422422
const baseContext = this.getInitializeContext(currentUser, pullRequest, await this.processTimelineEvents(timelineEvents), repositoryAccess, viewerCanEdit, users);
423+
// Override bodyHTML with processed version without mutating original pullRequest
424+
baseContext.bodyHTML = processedBodyHTML;
423425

424426
this.preLoadInfoNotRequiredForOverview(pullRequest);
425427

@@ -663,8 +665,9 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
663665
}
664666
}
665667

666-
protected override _getTimeline(): Promise<TimelineEvent[]> {
667-
return this._item.getTimelineEvents();
668+
protected override async _getTimeline(): Promise<TimelineEvent[]> {
669+
const events = await this._item.getTimelineEvents();
670+
return this.processTimelineEvents(events);
668671
}
669672

670673
private async openDiff(message: IRequestMessage<{ comment: IComment }>): Promise<void> {
@@ -676,31 +679,34 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
676679
}
677680
}
678681

679-
private async openDiffFromLink(message: IRequestMessage<{ file: string; startLine: number; endLine: number }>): Promise<void> {
682+
private async openDiffFromLink(message: IRequestMessage<{ file: string; startLine: number; endLine: number; href: string }>): Promise<void> {
680683
try {
681684
const { file, startLine } = message.args;
682685
const fileChanges = await this._item.getFileChangesInfo();
683686
const change = fileChanges.find(
684687
fileChange => fileChange.fileName === file || fileChange.previousFileName === file,
685688
);
686689

687-
if (!change) {
688-
Logger.warn(`Could not find file ${file} in PR changes`, PullRequestOverviewPanel.ID);
690+
if (change) {
691+
692+
const pathSegments = file.split('/');
693+
// GitHub line numbers are 1-indexed, VSCode selection API is 0-indexed
694+
await PullRequestModel.openDiff(
695+
this._folderRepositoryManager,
696+
this._item,
697+
change,
698+
pathSegments[pathSegments.length - 1],
699+
startLine - 1,
700+
);
689701
return;
690702
}
691-
692-
const pathSegments = file.split('/');
693-
// GitHub line numbers are 1-indexed, VSCode selection API is 0-indexed
694-
return PullRequestModel.openDiff(
695-
this._folderRepositoryManager,
696-
this._item,
697-
change,
698-
pathSegments[pathSegments.length - 1],
699-
startLine - 1,
700-
);
703+
Logger.warn(`Could not find file ${file} in PR changes`, PullRequestOverviewPanel.ID);
701704
} catch (e) {
702705
Logger.error(`Open diff from link failed: ${formatError(e)}`, PullRequestOverviewPanel.ID);
703706
}
707+
708+
// Fallback to opening external URL
709+
await vscode.env.openExternal(vscode.Uri.parse(message.args.href));
704710
}
705711

706712
private async openSessionLog(message: IRequestMessage<{ link: SessionLinkInfo }>): Promise<void> {
@@ -793,7 +799,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
793799
await this._item.unresolveReviewThread(message.args.threadId);
794800
}
795801
const timelineEvents = await this._getTimeline();
796-
this._replyMessage(message, await this.processTimelineEvents(timelineEvents));
802+
this._replyMessage(message, timelineEvents);
797803
} catch (e) {
798804
vscode.window.showErrorMessage(e);
799805
this._replyMessage(message, undefined);

0 commit comments

Comments
 (0)