sessions: add rich GitHub reference previews - #335583
sessions: add rich GitHub reference previews#335583Cherry Wang (chryw) wants to merge 14 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Several moderate review findings remain around focus behavior, event propagation, and regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This pull request adds rich GitHub issue and pull-request previews to Sessions pills, with keyboard-accessible detail cards.
Changes:
- Adds lifecycle metadata, descriptions, links, and branch-copy controls.
- Integrates ActionList focus traversal and padding ownership.
- Adds styling, tests, accessibility guidance, and fixtures.
File summaries
| File | Description |
|---|---|
src/vs/workbench/test/browser/componentFixtures/sessions/githubReferenceHoverLayouts.fixture.ts |
Adds themed GitHub hover layout fixtures. |
src/vs/workbench/contrib/chat/browser/chatInputPills.ts |
Exposes chat pill focus behavior. |
src/vs/workbench/browser/chatPills.ts |
Supports enriched entries and first-pill focusing. |
src/vs/sessions/contrib/github/common/types.ts |
Extends GitHub models with closure metadata. |
src/vs/sessions/contrib/github/browser/pullRequestHover.ts |
Renders PR details and branch-copy controls. |
src/vs/sessions/contrib/github/browser/media/pullRequestHover.css |
Styles PR cards and branch pills. |
src/vs/sessions/contrib/github/browser/media/issueHover.css |
Styles issue cards. |
src/vs/sessions/contrib/github/browser/issueHover.ts |
Renders issue preview cards. |
src/vs/sessions/contrib/github/browser/githubHover.ts |
Normalizes and bounds descriptions. |
src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts |
Maps GitHub closure dates. |
src/vs/sessions/contrib/chat/test/browser/sessionChatInputToolbar.test.ts |
Tests rich preview behavior and controls. |
src/vs/sessions/contrib/chat/browser/sessionsChatAccessibilityHelp.ts |
Documents pill keyboard navigation. |
src/vs/sessions/contrib/chat/browser/sessionChatInputToolbar.ts |
Builds rich GitHub pill entries. |
src/vs/sessions/contrib/chat/browser/chatView.ts |
Routes Shift+Tab to visible pills. |
src/vs/platform/actionWidget/test/browser/actionList.test.ts |
Tests panel keyboard navigation. |
src/vs/platform/actionWidget/browser/actionWidget.css |
Styles content-owned panel padding. |
src/vs/platform/actionWidget/browser/actionList.ts |
Adds panel focus and padding contracts. |
Review details
Suppressed comments (5)
src/vs/platform/actionWidget/browser/actionList.ts:2286
- When a row has both Copy URL and Remove (multi-entry pull-request pills append
removeActionaftertoolbarActions), this always focuses the last toolbar action. Shift+Tab from the detail card therefore lands on Remove instead of Copy URL, contradicting the documented focus order. Please define the intended return action for this traversal (or reorder/exclude the removal action) and cover the multi-action case.
if (inPanel) {
if (controls.toolbar?.length()) {
dom.EventHelper.stop(event, true);
controls.toolbar.focus(controls.toolbar.length() - 1);
src/vs/sessions/contrib/chat/browser/chatView.ts:351
- This handler routes only Shift+Tab from the chat input into the pills, but the existing
sessionsChat.inputPillsaccessibility-help entry still tells users to press Tab to reach the pills above the input. Normal Tab is not intercepted here, so that instruction does not reach the intended control; update the help text to match this Shift+Tab behavior.
if (event.keyCode === KeyCode.Tab && event.shiftKey && !event.ctrlKey && !event.metaKey && !event.altKey && this._chatPills.focusFirst()) {
src/vs/sessions/contrib/chat/browser/chatView.ts:354
- This adds a new global Shift+Tab focus transition, but the Sessions ChatView tests do not dispatch this handler. A regression in the modifier filtering,
focusFirst()result, or cancellation behavior could silently break keyboard access to the status pills; add a focused test for the matching and non-matching paths.
this._register(this._widget.inputEditor.onKeyDown(event => {
if (event.keyCode === KeyCode.Tab && event.shiftKey && !event.ctrlKey && !event.metaKey && !event.altKey && this._chatPills.focusFirst()) {
event.preventDefault();
event.stopPropagation();
}
src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts:352
- The new
closedAtmapping drives the card's closed-PR transition date, but the fetcher tests never includeclosed_atinmakePRResponseor assertpr.data?.closedAt. A regression in this wire-to-domain mapping would therefore leave the UI showing the updated date while all current tests still pass; add a response fixture and assertion for the closed timestamp.
...(data.closed_at ? { closedAt: data.closed_at } : {}),
src/vs/sessions/contrib/github/browser/pullRequestHover.ts:143
- Activating either branch button bubbles a click to the ActionList root. Unlike the link handler above, this callback does not stop propagation, so
List.onViewPointersees a click outside a row and clears the focused row. After copying a branch, Up/Down and the panel Tab sequence lose their list context; stop propagation before invoking the copy callback.
branch.onclick = onDidClick;
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Screenshot ChangesBase: 3 insignificant change(s) omitted (≤20 px, Δ≤2). See CI logs for details. Added (12) |
c00e6c5 to
a4c1c0c
Compare
|
Addressed the remaining review findings from the latest Copilot review pass (
Also rebased onto latest |
ad39018 to
5123a6d
Compare
8ea390f to
c849093
Compare
Reuse the existing issue and pull request hover components for dropdown details, with compact metadata, explicit spacing ownership, and bounded readable descriptions. Integrate the pill, row action, detail link, and branch-copy controls into one accessible keyboard flow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use existing relative-time, title, spacing, and popup patterns consistently across standalone and collection references. Bound long titles, keep transient content concise, and make HTML hover padding ownership explicit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the existing 8px SessionSummaryHover rhythm between the title and description in both standalone and collection cards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Let the card root own a uniform 8px gap between metadata, title, description, and branch rows instead of splitting relationship spacing across child padding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follow GitHub hover cards for provenance, title and ID, state, context, routing, and author order while preserving VS Code styling and the bounded transient content budget. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Group the final visible title word with the linked reference ID, bound pathological titles safely, and allow narrow cards to wrap without clipping focusable content. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use measured submenu height for positioning and reveal the full bounded GitHub title when keyboard focus reaches its linked reference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Render shared hover actions as centered inline flex rows so codicons and text use the same vertical center. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- actionList: Shift+Tab from a hover panel now returns to the last non-removal toolbar action instead of a trailing Remove control, covered by a new multi-action regression test. - pullRequestHover: stop branch-pill clicks from bubbling to the ActionList row so copying a branch no longer clears list focus. - chatView: extract the Shift+Tab-to-pills predicate into a testable helper and add regression tests for modifier filtering and preventDefault/stopPropagation cancellation; align the accessibility help text with the Shift+Tab shortcut. - githubPRFetcher: cover the closed_at -> closedAt mapping with a dedicated fixture and assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Surface the shared overall checks state alongside PR lifecycle metadata, including draft pull requests, while keeping no-check and completed PR cards quiet. Preserve the same status in single-pill and collection-row accessibility descriptions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the familiar static pending dot and aggregate 'Checks pending' label rather than a sync metaphor that implies active execution or refresh. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Traverse rich-hover controls and row actions symmetrically with Shift+Tab, and preserve GitHub's distinct Duplicate issue terminology. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c849093 to
95e0035
Compare
Summary
Exploratory follow-up to #335448, building on the first-level title work from #335387.
This proposal adds a compact issue/PR preview to Agents Window reference pills so users can identify a GitHub object and decide whether to open it without leaving their current context.
Design rationale
The resulting card order is repository/date → title/ID → lifecycle and CI state → bounded description → PR branches → author. Open and draft pull requests show the shared overall checks state as
Checks pending,Checks failed, orChecks passed; closed, merged, and no-check states stay quiet. Pending uses GitHub's familiar attention-colored static dot rather than a spinner or sync metaphor because the aggregate may include both queued and running checks. Both status pills use neutral text and surfaces, while their icons carry restrained semantic color.Descriptions are normalized from Markdown and bounded to 200 Unicode code points. Visible titles are bounded to 80 Unicode code points. The final visible title word and linked ID are grouped so the number cannot interrupt the sentence or become clipped at narrow widths.
#335448 also explores a full Markdown reader across broader GitHub-reference surfaces; this iteration focuses on the compact pill-hover treatment.
What inspired the design
Those examples drove the current boundary: enough content to understand and route the reference, with individual checks, labels, reactions, comments, assignees, and full Markdown left to richer or persistent surfaces.
Collection item interactions
Pull request collection
github-hover-collection-pr.mp4
Starts on the focused Pull Requests parent pill, opens the first-level list, moves across PR states and the live overall CI result, then traverses Copy URL, repository, ID, and base/head branches. The clip follows the complete reverse Shift+Tab sequence back through every card control and row action.
Issue collection
github-hover-collection-issue.mp4
Starts on the focused Issues parent pill, opens the first-level list, covers long-title/closed/open rows, then traverses Copy URL, repository, and ID before reversing through each control back to the list.
Single-reference interactions
A single reference skips the list but keeps the same information budget.
Single pull request
github-hover-single-pr.mp4
Uses Shift+Tab and ArrowRight to focus the direct PR chip, then opens its hover with the live overall CI result and moves through repository, linked ID, and base/head branch controls.
Single issue
github-hover-single-issue.mp4
Uses Shift+Tab and ArrowRight to focus the direct issue chip, then opens its hover and moves through repository and linked ID.
Keyboard and screen-reader behavior
Implementation
ChatDropdownPillActionViewItem,ActionListWidget,createIssueHoverElement, andcreatePullRequestHoverElementDuplicateandNot plannedissue terminologyValidation
npm run compilenpm run typecheck-clientRelated