Conversation
There was a problem hiding this comment.
Pull request overview
Adds a “fishing” easter-egg scene to the Sessions (Agents) window UI by mounting a small interactive boat strip above the new-session chat input.
Changes:
- Register a new Sessions workbench contribution that mounts/unmounts the fishing scene into
.new-chat-input-container. - Implement the fishing scene DOM/keyboard interaction logic and styling.
- Add sprite-sheet image assets and wire up i18n + stylelint variable allowlist updates.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/sessions.common.main.ts | Loads the new fishing contribution in the Sessions app entrypoint. |
| src/vs/sessions/contrib/fishing/browser/fishing.contribution.ts | Observes layout DOM and mounts the fishing scene host into the new-chat input container. |
| src/vs/sessions/contrib/fishing/browser/fishingScene.ts | Implements the interactive boat (reveal, activate, arrow-key movement, “catch” animation). |
| src/vs/sessions/contrib/fishing/browser/media/fishing.css | Styles/animates the scene host, entry button, boat sprites, and preloads sheets. |
| src/vs/sessions/contrib/fishing/browser/media/*.png | Adds sprite sheets used by the fishing scene animations. |
| build/lib/stylelint/vscode-known-variables.json | Allows the new --boat-x CSS custom property under stylelint rules. |
| build/lib/i18n.resources.json | Adds the fishing contrib folder to the Sessions i18n project resources. |
Copilot's findings
Comments suppressed due to low confidence (5)
src/vs/sessions/contrib/fishing/browser/fishingScene.ts:75
boatGroupactivation usesaddDisposableListener(..., EventType.MOUSE_DOWN, ...). In the Sessions window we need the generic mouse-down helper (pointer-event backed) for touch/iOS support, and the boat control should also respond to tap events (gesture system) rather than relying on mouse events.
this._register(addDisposableListener(this.boatGroup, EventType.MOUSE_DOWN, e => {
if (!this.revealed) {
return;
}
e.preventDefault();
e.stopPropagation();
this.activate();
}));
src/vs/sessions/contrib/fishing/browser/fishingScene.ts:86
- Deactivation is wired to
EventType.MOUSE_DOWNondocument. This will not fire on some touch platforms; use the generic mouse-down listener (pointer-event backed) and consider handling tap as well so users can reliably release control on touch devices.
const doc = parent.ownerDocument;
this._register(addDisposableListener(doc, EventType.MOUSE_DOWN, e => {
if (!this.active) {
return;
}
if (e.target instanceof Node && this.boatGroup.contains(e.target)) {
return;
}
this.deactivate();
}, true));
src/vs/sessions/contrib/fishing/browser/fishingScene.ts:75
boatGroupis focusable (tabIndex = 0) but there is no keyboard activation path (e.g. Enter/Space when focused) to callactivate(). This makes the control difficult/impossible to use without a mouse/touch; add a keydown handler on the boat element to activate/deactivate appropriately.
this.boatGroup = append(this.sceneHost, $('div.boat-group'));
this.boatGroup.setAttribute('role', 'button');
this.boatGroup.tabIndex = 0;
this.boatGroup.title = localize('fishing.boat.tooltip', "Click to control with arrow keys; press Space to cast");
this.boatGroup.setAttribute('aria-label', localize('fishing.boat.aria', "Fishing boat"));
append(this.boatGroup, $('.row-sprite'));
this.applyBoatPosition();
this._register(addDisposableListener(this.entryButton, EventType.CLICK, e => {
e.preventDefault();
e.stopPropagation();
this.reveal();
}));
this._register(addDisposableListener(this.boatGroup, EventType.MOUSE_DOWN, e => {
if (!this.revealed) {
return;
}
e.preventDefault();
e.stopPropagation();
this.activate();
}));
src/vs/sessions/contrib/fishing/browser/media/fishing.css:95
.boat-groupis a custom clickable element, but it doesn't settouch-action: manipulation;. Adding this reduces tap delay and improves responsiveness on touch devices (especially iOS) for non-native button elements.
.fishing-scene-host .boat-group {
position: absolute;
left: 12px;
bottom: -12px;
width: 132px;
height: 90px;
transform: translateX(var(--boat-x, 0px));
cursor: pointer;
outline: none;
opacity: 1;
transition: opacity 360ms ease-out 80ms;
}
src/vs/sessions/contrib/fishing/browser/media/fishing.css:117
- The CSS explicitly removes focus outlines for
.boat-group(:focus/:focus-visible { outline: none; }) but the element is keyboard-focusable and is focused onactivate(). Please provide a visible focus indicator (e.g. outline/box-shadow using--vscode-focusBorder) rather than suppressing it.
.fishing-scene-host .boat-group:focus,
.fishing-scene-host .boat-group:focus-visible {
outline: none;
}
- Files reviewed: 6/14 changed files
- Comments generated: 3
| this.entryButton.type = 'button'; | ||
| this.entryButton.title = localize('fishing.entry.tooltip', "Go fishing"); | ||
| this.entryButton.setAttribute('aria-label', localize('fishing.entry.aria', "Go fishing")); | ||
| append(this.entryButton, $('span.codicon.codicon-sparkle')); |
There was a problem hiding this comment.
The icon inside the entry button is purely decorative, but it isn't marked as such. Consider setting aria-hidden="true" on the codicon span so screen readers only announce the button label.
This issue also appears in the following locations of the same file:
- line 53
- line 68
- line 77
| append(this.entryButton, $('span.codicon.codicon-sparkle')); | |
| append(this.entryButton, $('span.codicon.codicon-sparkle', { 'aria-hidden': 'true' })); |
| const observer = new MutationObserver(() => this.sync()); | ||
| // childList only on the root — we don't need to react to every keystroke | ||
| // or focus change inside the chat input. The input container is added / | ||
| // removed at the workbench level, not deep inside it. | ||
| observer.observe(this.layoutService.mainContainer, { childList: true, subtree: true }); | ||
| this._register(toDisposable(() => observer.disconnect())); |
There was a problem hiding this comment.
This MutationObserver watches layoutService.mainContainer with subtree: true, which can generate a large volume of mutation callbacks (Monaco/editor DOM churn), even though sync() usually early-returns. Consider narrowing the observed subtree, or debouncing/throttling sync() to avoid unnecessary querySelector work on frequent DOM mutations.
| * - .face-left : flips the sprite horizontally (boat faces left). | ||
| * | ||
| * Source sheets are 83px-per-frame (height 57). Idle/rowing have 4 frames | ||
| * (332×57 source). Caught has 6 frames (498×57 source). All are displayed |
There was a problem hiding this comment.
The header comment says “Caught has 6 frames (498×57 source)”, but the implementation uses the long caught sheets with 13 frames (see fishing-frames-13-once and the 1716px background size). Please update the comment to match the actual assets/animation so future changes don’t rely on incorrect frame counts.
This issue also appears in the following locations of the same file:
- line 84
- line 114
| * (332×57 source). Caught has 6 frames (498×57 source). All are displayed | |
| * (332×57 source). Caught has 13 frames (1079×57 source). All are displayed |
some open source sprites from https://free-game-assets.itch.io/free-fishing-pixel-art-pack
proof of concept for adding a game or something to the agents application new session screen