Skip to content

Commit 52bbce8

Browse files
elkaixelkaix
committed
fix(web): address the review findings on the batched web UI branch
Keyboard and ARIA: the capability panel is a labelled dialog that takes focus when it opens (it is teleported to the end of <body>, so a keyboard user would otherwise tab through the whole page to reach it), and Retry is back in the tab order — it undoes a turn and resends the prompt, and nothing else reaches it. Capability writes now queue per field. A slow [A] could land after [A, B] and drop B, and a stale failure could roll a newer toggle back. Retry keeps the prompt's attachments: undo reports the fileIds it removed, so a retried prompt resends its images and clips instead of the text alone. Config patches keep provider and model ids as written. Camel-casing them renamed the provider while every reference to it kept the original spelling, so a model alias could no longer resolve its provider. Also: the Pythinker theme defines --r-xl (the composer inherited the 24px root value against its documented 16px scale), and the switch declarations get the blank line Stylelint expects.
1 parent 005f5f6 commit 52bbce8

18 files changed

Lines changed: 350 additions & 109 deletions

‎.changeset/preserve-provider-ids.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pymodel/pythinker-code": patch
3+
---
4+
5+
Keep provider and model ids exactly as written when a config patch is saved, so an id containing an underscore still resolves.

‎.changeset/web-capability-menu-a11y.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pymodel/pythinker-code": patch
3+
---
4+
5+
Fix web capability and retry controls: the capability panel takes keyboard focus when it opens, Retry stays reachable with Tab, rapid capability toggles reach the daemon in order, and retrying a prompt keeps its attachments.

‎apps/pythinker-web/src/App.vue‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -753,9 +753,12 @@ async function handleEditMessage(text: string): Promise<void> {
753753
// Retry the last assistant reply: undo the exchange, then send its original
754754
// user prompt as a new prompt. Undo reports any failure and returns null.
755755
async function handleRegenerate(): Promise<void> {
756-
const text = await client.undo(1);
757-
if (text === null) return;
758-
await client.sendPrompt(text);
756+
const prompt = await client.undo(1);
757+
if (prompt === null) return;
758+
await client.sendPrompt(
759+
prompt.text,
760+
prompt.attachments.length > 0 ? prompt.attachments : undefined,
761+
);
759762
}
760763
761764
// Handler for slash commands emitted by Composer (via ConversationPane)

‎apps/pythinker-web/src/components/CapabilityMenu.vue‎

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<script setup lang="ts">
2-
import { computed, ref, watch } from 'vue';
2+
import { computed, ref, watch, type Ref } from 'vue';
33
import { useI18n } from 'vue-i18n';
44
import ActivitySpinner from './ActivitySpinner.vue';
55
import Chip from './ui/Chip.vue';
@@ -115,30 +115,48 @@ function close(): void {
115115
view.value = 'root';
116116
}
117117
118-
async function setToolEnabled(name: string, enabled: boolean): Promise<void> {
118+
// Each capability field owns one write chain. A snapshot is read when the write
119+
// leaves the chain, so the daemon sees the toggles in the order the user made
120+
// them: a slow `[A]` can no longer land after `[A, B]` and drop B. A failed
121+
// write only rolls the selection back while it is still the newest one —
122+
// otherwise a stale failure would discard a newer toggle.
123+
type CapabilityField = 'tools' | 'mcpServers';
124+
125+
const writeChain: Record<CapabilityField, Promise<void>> = {
126+
tools: Promise.resolve(),
127+
mcpServers: Promise.resolve(),
128+
};
129+
const writeCount: Record<CapabilityField, number> = { tools: 0, mcpServers: 0 };
130+
131+
function queueWrite(field: CapabilityField, selection: Ref<string[]>, previous: string[]): Promise<void> {
132+
const seq = ++writeCount[field];
133+
const write = writeChain[field].then(async () => {
134+
try {
135+
await client.updateCapabilities({ [field]: [...selection.value] });
136+
} catch {
137+
if (seq === writeCount[field]) selection.value = previous;
138+
}
139+
});
140+
writeChain[field] = write;
141+
return write;
142+
}
143+
144+
function setToolEnabled(name: string, enabled: boolean): Promise<void> {
119145
const previous = [...selectedTools.value];
120146
const next = new Set(previous);
121147
if (enabled) next.add(name);
122148
else next.delete(name);
123149
selectedTools.value = [...next];
124-
try {
125-
await client.updateCapabilities({ tools: selectedTools.value });
126-
} catch {
127-
selectedTools.value = previous;
128-
}
150+
return queueWrite('tools', selectedTools, previous);
129151
}
130152
131-
async function setMcpServerEnabled(id: string, enabled: boolean): Promise<void> {
153+
function setMcpServerEnabled(id: string, enabled: boolean): Promise<void> {
132154
const previous = [...selectedMcpServers.value];
133155
const next = new Set(previous);
134156
if (enabled) next.add(id);
135157
else next.delete(id);
136158
selectedMcpServers.value = [...next];
137-
try {
138-
await client.updateCapabilities({ mcpServers: selectedMcpServers.value });
139-
} catch {
140-
selectedMcpServers.value = previous;
141-
}
159+
return queueWrite('mcpServers', selectedMcpServers, previous);
142160
}
143161
144162
function setPluginEnabled(id: string, enabled: boolean): void {
@@ -154,7 +172,7 @@ function setPluginEnabled(id: string, enabled: boolean): void {
154172
class="capability-trigger"
155173
:class="{ open }"
156174
:aria-expanded="open"
157-
aria-haspopup="menu"
175+
aria-haspopup="dialog"
158176
:aria-label="t('capabilityMenu.triggerLabel')"
159177
@click.stop="toggleOpen"
160178
>
@@ -190,7 +208,7 @@ function setPluginEnabled(id: string, enabled: boolean): void {
190208
/>
191209
</div>
192210

193-
<Popover :anchor="triggerRef" :open="open" @close="close">
211+
<Popover :anchor="triggerRef" :open="open" :label="t('capabilityMenu.triggerLabel')" @close="close">
194212
<div class="capability-panel">
195213
<div class="capability-viewport">
196214
<div class="capability-track" :class="{ 'is-drilled': view !== 'root' }">

‎apps/pythinker-web/src/components/ChatPane.vue‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -638,7 +638,6 @@ function renderBlockKey(block: AssistantRenderBlock, index: number): string {
638638
type="button"
639639
class="a-cpbtn retry-btn"
640640
:aria-label="t('conversation.retry')"
641-
tabindex="-1"
642641
@click="confirmingRetryTurnId = turn.id"
643642
>
644643
<svg viewBox="0 0 16 16" width="12" height="12" fill="none" stroke="currentColor" stroke-width="1.6" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true">
@@ -751,7 +750,6 @@ function renderBlockKey(block: AssistantRenderBlock, index: number): string {
751750
class="cpbtn retry-btn"
752751
:aria-label="t('conversation.retry')"
753752
@click="confirmingRetryTurnId = turn.id"
754-
tabindex="-1"
755753
>
756754
<svg viewBox="0 0 16 16" width="12" height="12" fill="none" stroke="currentColor" stroke-width="1.6" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true">
757755
<path d="M3 7a5 5 0 1 1 1.5 3.6"/>

‎apps/pythinker-web/src/components/settings/settings.css‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
.act.signin:hover { background: var(--blue2); }
6363
.switch {
6464
--switch-knob-size: calc(var(--ui-font-size) + 4px);
65+
6566
position: relative;
6667
flex: none;
6768
width: calc(var(--switch-knob-size) * 2 + 4px);
@@ -89,6 +90,7 @@
8990
.switch.on .knob { transform: translateX(var(--switch-knob-size)); }
9091
.switch.sm {
9192
--switch-knob-size: calc(var(--ui-font-size) - 1px);
93+
9294
width: calc(var(--switch-knob-size) * 2 + 4px);
9395
height: calc(var(--switch-knob-size) + 4px);
9496
}

‎apps/pythinker-web/src/components/ui/Popover.vue‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,12 @@ const props = withDefaults(defineProps<{
77
anchor: HTMLElement | null;
88
open: boolean;
99
align?: Alignment;
10+
/** Accessible name for the panel. The panel is a dialog, not a menu: the slot
11+
holds plain buttons and switches, not `menuitem` children. */
12+
label?: string;
1013
}>(), {
1114
align: 'start',
15+
label: undefined,
1216
});
1317
1418
const emit = defineEmits<{
@@ -82,6 +86,15 @@ function detachListeners(): void {
8286
listenersAttached = false;
8387
}
8488
89+
function focusPanel(): void {
90+
const panel = panelRef.value;
91+
if (!panel) return;
92+
const first = panel.querySelector<HTMLElement>(
93+
'button:not([disabled]), [href], input, select, textarea, [tabindex]:not([tabindex="-1"])',
94+
);
95+
(first ?? panel).focus();
96+
}
97+
8598
function restoreFocusIfNeeded(): void {
8699
const panel = panelRef.value;
87100
const activeElement = document.activeElement;
@@ -100,6 +113,9 @@ watch(() => props.open, (open, wasOpen) => {
100113
if (!props.open) return;
101114
positionPanel();
102115
attachListeners();
116+
// The panel is teleported to the end of <body>, so a keyboard user would
117+
// otherwise have to tab through the rest of the page to reach it.
118+
focusPanel();
103119
});
104120
return;
105121
}
@@ -126,7 +142,8 @@ onBeforeUnmount(() => {
126142
ref="panelRef"
127143
class="popover"
128144
:style="panelStyle"
129-
role="menu"
145+
role="dialog"
146+
:aria-label="label"
130147
tabindex="-1"
131148
>
132149
<slot />

‎apps/pythinker-web/src/composables/usePythinkerWebClient.ts‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,12 @@ interface GitStatusEntry {
433433
type (image vs video) so a still and a clip resolve to the right wire shape. */
434434
type PromptAttachment = { fileId: string; kind: 'image' | 'video' };
435435

436+
/** The prompt an undo removed, so a caller can edit or resend it unchanged. */
437+
export interface UndonePrompt {
438+
text: string;
439+
attachments: PromptAttachment[];
440+
}
441+
436442
/** A prompt waiting for the session to go idle. Keeps the uploaded
437443
fileIds so attachments survive queueing (not just the text). */
438444
interface QueuedPrompt {
@@ -4158,27 +4164,35 @@ async function forkSession(sessionId?: string): Promise<void> {
41584164
* Returns the text of the most-recent user message that was undone, so the UI
41594165
* can offer "edit + resend" (load it back into the composer).
41604166
*/
4161-
async function undo(count = 1): Promise<string | null> {
4167+
async function undo(count = 1): Promise<UndonePrompt | null> {
41624168
const sid = rawState.activeSessionId;
41634169
if (!sid) return null;
4164-
// Capture the last user message text BEFORE the undo removes it.
4165-
const lastUserText = (() => {
4170+
// Capture the last user prompt BEFORE the undo removes it. The attachments
4171+
// come along so a retry resends the same images and clips: undo only trims
4172+
// the history, so the uploaded fileIds stay valid.
4173+
const lastUserPrompt = ((): UndonePrompt | null => {
41664174
const msgs = rawState.messagesBySession[sid] ?? [];
41674175
for (let i = msgs.length - 1; i >= 0; i--) {
41684176
const m = msgs[i]!;
41694177
if (m.role !== 'user') continue;
41704178
if (m.metadata?.['origin'] && (m.metadata['origin'] as { kind?: string }).kind !== 'user') continue;
4171-
return m.content
4179+
const text = m.content
41724180
.filter((c): c is { type: 'text'; text: string } => c.type === 'text')
41734181
.map((c) => c.text)
41744182
.join('\n');
4183+
const attachments = m.content.flatMap<PromptAttachment>((c) =>
4184+
(c.type === 'image' || c.type === 'video') && c.source.kind === 'file'
4185+
? [{ fileId: c.source.fileId, kind: c.type }]
4186+
: [],
4187+
);
4188+
return { text, attachments };
41754189
}
41764190
return null;
41774191
})();
41784192
try {
41794193
await getPythinkerWebApi().undoSession(sid, count);
41804194
await syncSessionFromSnapshot(sid);
4181-
return lastUserText;
4195+
return lastUserPrompt;
41824196
} catch (error) {
41834197
pushOperationFailure('undo', error, { sessionId: sid });
41844198
return null;

‎apps/pythinker-web/src/style.css‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -784,6 +784,7 @@ html[data-theme="pythinker"] {
784784
--hover: rgba(0, 0, 0, 0.03); /* color.fills.f1 */
785785
/* radius scale snaps to radius.xxs/sm/lg/xl = 4/8/12/16 */
786786
--r-xs: 4px;
787+
--r-xl: 16px;
787788
/* flat by default; --sh (floating menus only) = effect.shadow.small */
788789
--sh: 0 4px 16.4px 0 rgba(0, 0, 0, 0.1);
789790
--shc: none;

‎apps/pythinker-web/test/capability-menu.test.ts‎

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { join } from 'node:path';
33
import { flushPromises, mount } from '@vue/test-utils';
44
import { createI18n } from 'vue-i18n';
55
import { afterEach, describe, expect, it, vi } from 'vitest';
6-
import type { Ref } from 'vue';
6+
import { nextTick, type Ref } from 'vue';
77

88
import { DaemonPythinkerWebApi } from '../src/api/daemon/client';
99
import CapabilityMenu from '../src/components/CapabilityMenu.vue';
@@ -233,6 +233,17 @@ describe('daemon capability contracts', () => {
233233
});
234234
});
235235

236+
/** A promise the test settles by hand, so two writes can finish out of order. */
237+
function deferred(): { promise: Promise<void>; resolve: () => void; reject: (error: Error) => void } {
238+
let resolve!: () => void;
239+
let reject!: (error: Error) => void;
240+
const promise = new Promise<void>((res, rej) => {
241+
resolve = res;
242+
reject = rej;
243+
});
244+
return { promise, resolve, reject };
245+
}
246+
236247
describe('CapabilityMenu', () => {
237248
it('omits a capability group with no items', async () => {
238249
const current = client();
@@ -270,11 +281,52 @@ describe('CapabilityMenu', () => {
270281
const toggle = document.body.querySelector('.mcp-row .switch-toggle') as HTMLButtonElement;
271282

272283
toggle.click();
284+
await nextTick();
285+
// The optimistic state has to be observed first: asserting only the final
286+
// value would also pass if the toggle never moved.
287+
expect(toggle.getAttribute('aria-checked')).toBe('false');
273288
await flushPromises();
274289

275290
expect(toggle.getAttribute('aria-checked')).toBe('true');
276291
});
277292

293+
it('sends capability writes in toggle order and ignores a stale rollback', async () => {
294+
const current = client();
295+
current.activeSessionCapabilities.value = { tools: ['Read'], mcpServers: [] };
296+
// Two writes held open, settled in the order the test chooses.
297+
const first = deferred();
298+
const second = deferred();
299+
current.updateCapabilities.mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise);
300+
301+
const wrapper = mountMenu();
302+
await wrapper.get('.capability-trigger').trigger('click');
303+
await flushPromises();
304+
const toggles = document.body.querySelectorAll('.mcp-row .switch-toggle');
305+
const one = toggles[0] as HTMLButtonElement;
306+
const two = toggles[1] as HTMLButtonElement;
307+
308+
one.click();
309+
await nextTick();
310+
two.click();
311+
await nextTick();
312+
313+
// The chain holds the second write until the first one settles, so the
314+
// daemon can never see [mcp_1] after [mcp_1, mcp_2].
315+
expect(current.updateCapabilities).toHaveBeenCalledTimes(1);
316+
first.reject(new Error('daemon unreachable'));
317+
await flushPromises();
318+
second.resolve();
319+
await flushPromises();
320+
321+
expect(current.updateCapabilities.mock.calls.map((call) => call[0])).toEqual([
322+
{ mcpServers: ['mcp_1'] },
323+
{ mcpServers: ['mcp_1', 'mcp_2'] },
324+
]);
325+
// The stale failure must not discard the newer selection.
326+
expect(one.getAttribute('aria-checked')).toBe('true');
327+
expect(two.getAttribute('aria-checked')).toBe('true');
328+
});
329+
278330
it('renders selected tools and MCP servers as chips', () => {
279331
const wrapper = mountMenu();
280332

@@ -283,9 +335,8 @@ describe('CapabilityMenu', () => {
283335

284336
it('keeps CapabilityMenu.vue free of dark utilities and color literals', () => {
285337
const source = readFileSync(join(import.meta.dirname, '../src/components/CapabilityMenu.vue'), 'utf8');
286-
expect(source).not.toMatch(/dark:/);
287-
expect(source).not.toMatch(/#[0-9a-f]{3,8}\b/i);
288-
expect(source).not.toContain(['r', 'gb('].join(''));
289-
expect(source).not.toContain(['r', 'gba('].join(''));
338+
expect(source).not.toMatch(/\bdark:/u);
339+
expect(source).not.toMatch(/#[\da-f]{3,8}\b/iu);
340+
expect(source).not.toMatch(/\brgba?\s*\(/iu);
290341
});
291342
});

0 commit comments

Comments
 (0)