From e2d3243863814e53ca3b5767b4f081f469b953fc Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 4 Nov 2026 22:42:07 +0800 Subject: [PATCH] fix(web-shell): express the PR watch events dependency in the channel editor (#16236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web-shell): express the PR watch events dependency in the channel editor The GitHub adapter rejects a non-empty prWatchEvents unless deliverToBoundSessions is enabled, but the editor never expressed that dependency: operators could tick watch events, press Save, and receive a raw English daemon refusal that read as "the selection cannot be saved". Group the bound-delivery switch and the watch-event checkboxes into one framed panel, disable the checkboxes with an inline hint until the switch is on, and mirror the runtime rule as a client-side validation backstop for hand-written stored configs that carry events without delivery. * fix(web-shell): keep stored watch selections clearable while delivery is off Review follow-ups on the bound-session panel: - A stored selection stays uncheckable-while-gated no more: with delivery off, selected events (including unknown stored values) can be cleared directly, while new selections remain disabled, so turning delivery off never strands the operator between a dependency error and disabled boxes. - Turning the delivery switch on clears the stale requiresBoundDelivery error instead of leaving the instruction under the now-on switch. - The disabled hint moves into FieldShell's existing hint slot, restoring the "description is the first paragraph" invariant; the hint gets an id, aria-describedby on the checkbox group and a persistent polite live region so screen readers hear why the boxes are gated. - The render gate now carries the same github scope as the validation mirror, and checked-but-disabled rows keep the selected border. - Tests: shutdown path, stale-error retirement, aria association, and a stored delivery-on mount; visuals scenario seeds the bound-session fields and captures the gated and enabled states. * fix(web-shell): scope dependency-error retirement to the dependency message The switch-on clear deleted the whole prWatchEvents error slot, wiping the higher-precedence invalidOption message while an unknown stored value was still ticked; and unticking the last selected event — the other remedy — left the dependency alert over an empty, valid selection. Retire only the dependency message, from both remedies, and pin the live region's persistence (empty region mounted before the hint, same node across the transition) plus the hint-less rendering of the pre-existing hint slots. * fix(web-shell): retire watch-events errors by re-deriving validity Round-4 review follow-ups, all in this PR: - R3-1: replace the string-matched dependency clear with a per-field re-validation on edit, so unticking an unknown stored value retires the invalidOption instruction it satisfies (and a remaining valid event swaps the slot to the still-unsatisfied dependency message). - R3-3: submit now scrolls the first erroring field into view even when it is not an advanced field; a stored watch-events refusal below the fold is no longer invisible. - R3-2: pin the empty-panel guard with EN/zh absence assertions on a non-GitHub descriptor. - R3-4: table-driven parity test feeds the plugin's own descriptor and config through both the editor mirror and the daemon validator (test-only devDependency, never bundled); a lone change on either side now fails the table. Each fix carries a mutation witness; 286 channel tests green. * fix(web-shell): re-derive only shown errors, in both switch directions Round-5 review follow-ups, all in this PR: - R4-1: refreshErrors re-derives only the error keys already on screen, so a remedy retires exactly the instruction it satisfies without surfacing validation the operator never submitted (submit stays the only population site), and the delivery switch re-derives in both directions so a shown instruction is never silently dropped. New tests pin pre-submit silence and shown-message survival across switch toggles. - R4-2: narrow plugin.management once at module scope in the parity test so the optional contract member stays TS18048-clean if test files ever join the package typecheck, with a clear error instead of a load-time TypeError. - Deferred note from the same round: pin the data-disabled styling hook on gated rows (dimmed) versus clearable stored rows (selected styling). Each fix carries a mutation witness; 288 channel tests green. --------- Co-authored-by: verify-round --- .../channels/ChannelEditorDialog.module.css | 15 + .../channels/ChannelEditorDialog.test.tsx | 539 ++++++++++++++++++ .../channels/ChannelEditorDialog.tsx | 138 ++++- .../channels/channel-editor-parity.test.ts | 103 ++++ .../channels/channel-editor-state.test.ts | 60 +- .../channels/channel-editor-state.ts | 13 +- .../client/e2e/visuals/screenshots.spec.ts | 39 ++ packages/web-shell/client/i18n.tsx | 14 + packages/web-shell/package.json | 1 + pnpm-lock.yaml | 3 + 10 files changed, 903 insertions(+), 22 deletions(-) create mode 100644 packages/web-shell/client/components/channels/channel-editor-parity.test.ts diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.module.css b/packages/web-shell/client/components/channels/ChannelEditorDialog.module.css index 882128f7ea5..e9804616362 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.module.css +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.module.css @@ -192,6 +192,21 @@ background: color-mix(in srgb, var(--muted) 62%, transparent); } +.optionRow[data-disabled='true'] { + cursor: not-allowed; + opacity: 0.55; +} + +/* A checked-but-disabled row keeps the selected border; only unselected + disabled rows lose the hover emphasis. */ +.optionRow[data-disabled='true']:not([data-selected='true']):hover { + border-color: var(--border); +} + +.optionCheckbox:disabled { + cursor: not-allowed; +} + .optionCheckbox { width: 14px; height: 14px; diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx index 02c34e3356f..7dc536a9bac 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx @@ -866,6 +866,540 @@ describe('ChannelEditorDialog', () => { ); }); + it('frames both bound-session fields in one panel', async () => { + await renderDialog({ descriptor: GITHUB_BOUND }); + + const delivery = fieldByLabel('Deliver To PR-Bound Sessions'); + const watch = fieldByLabel('PR Watch Events'); + expect(delivery).not.toBeNull(); + expect(watch).not.toBeNull(); + expect(delivery!.closest('section')).toBe(watch!.closest('section')); + expect(sectionHeadingOf(delivery)).toBe('PR-bound sessions'); + }); + + it('disables the watch boxes with an inline hint until delivery is on', async () => { + await renderDialog({ descriptor: GITHUB_BOUND }); + + const boxes = () => + Array.from( + document.querySelectorAll( + '[role="group"][aria-label="PR Watch Events"] input[type="checkbox"]', + ), + ); + expect(boxes().length).toBeGreaterThan(0); + expect(boxes().every((box) => box.disabled)).toBe(true); + // The dimmed-row styling hook must track the gate, not just the input. + expect( + boxes().every( + (box) => box.closest('label')?.getAttribute('data-disabled') === 'true', + ), + ).toBe(true); + expect(document.body.textContent).toContain( + 'Enable "Deliver To PR-Bound Sessions" above to select watch events.', + ); + + const toggle = document.querySelector( + 'button[role="switch"]', + ); + expect(toggle).not.toBeNull(); + await act(async () => { + toggle!.click(); + }); + + expect(boxes().every((box) => box.disabled)).toBe(false); + expect(document.body.textContent).not.toContain( + 'above to select watch events.', + ); + }); + + it('shows the disabled hint in zh-CN', async () => { + await renderDialog({ descriptor: GITHUB_BOUND, language: 'zh-CN' }); + + expect(document.body.textContent).toContain( + '开启上方「投递到 PR 绑定会话」后即可选择监视事件。', + ); + }); + + it('saves the selected watch events once delivery is enabled', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ descriptor: GITHUB_BOUND, onSave }); + + await act(async () => { + setInputValue(inputByLabel('Instance name')!, 'github-bot'); + setInputValue(inputByLabel('Personal Access Token')!, 'ghp_test'); + document + .querySelector('button[role="switch"]')! + .click(); + }); + const box = Array.from( + document.querySelectorAll( + '[role="group"][aria-label="PR Watch Events"] input[type="checkbox"]', + ), + ).find((b) => b.closest('label')?.textContent?.includes('ci_failure')); + expect(box).toBeDefined(); + await act(async () => { + box!.click(); + }); + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(onSave).toHaveBeenCalledWith( + 'github-bot', + expect.objectContaining({ + config: expect.objectContaining({ + deliverToBoundSessions: true, + prWatchEvents: ['ci_failure'], + }), + }), + ); + }); + + it('names the delivery dependency when a stored selection lacks it', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + prWatchEvents: ['ci_failure'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_BOUND, instance, onSave }); + + const box = document.querySelector( + '[role="group"][aria-label="PR Watch Events"] input[type="checkbox"]', + ); + expect(box!.checked).toBe(true); + // The stored selection stays clearable even while the gate is closed. + expect(box!.disabled).toBe(false); + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(onSave).not.toHaveBeenCalled(); + expect(document.body.textContent).toContain( + 'Enable "Deliver To PR-Bound Sessions" to save the selected watch events.', + ); + + // Following the instruction must retire the instruction: turning the + // switch on clears the stale dependency error without another Save. + await act(async () => { + document + .querySelector('button[role="switch"]')! + .click(); + }); + expect(document.body.textContent).not.toContain( + 'to save the selected watch events.', + ); + }); + + it('lets a stored selection be cleared while delivery is off', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + prWatchEvents: ['ci_failure'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_BOUND, instance, onSave }); + + const boxes = Array.from( + document.querySelectorAll( + '[role="group"][aria-label="PR Watch Events"] input[type="checkbox"]', + ), + ); + const selected = boxes.find((b) => + b.closest('label')?.textContent?.includes('ci_failure'), + )!; + expect(selected.checked).toBe(true); + expect(selected.disabled).toBe(false); + expect(boxes.filter((b) => b !== selected).every((b) => b.disabled)).toBe( + true, + ); + // A clearable stored row keeps the selected styling, not the dimmed one. + expect(selected.closest('label')?.getAttribute('data-disabled')).toBe( + 'false', + ); + expect( + boxes + .filter((b) => b !== selected) + .every( + (b) => b.closest('label')?.getAttribute('data-disabled') === 'true', + ), + ).toBe(true); + + await act(async () => { + selected.click(); + }); + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(onSave).toHaveBeenCalledTimes(1); + const request = onSave.mock.calls[0][1] as { + config: Record; + }; + expect(request.config['prWatchEvents']).toBeUndefined(); + expect(request.config['deliverToBoundSessions']).toBe(false); + }); + + it('keeps the invalid-option message when delivery is switched on', async () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + prWatchEvents: ['bogus_event'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_BOUND, instance }); + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + expect(document.body.textContent).toContain( + "Remove values that aren't in the allowed list.", + ); + + // Switching delivery on remedies the dependency, not the unknown value: + // the higher-precedence invalidOption message must survive. + await act(async () => { + document + .querySelector('button[role="switch"]')! + .click(); + }); + expect(document.body.textContent).toContain( + "Remove values that aren't in the allowed list.", + ); + expect(document.body.textContent).not.toContain( + 'to save the selected watch events.', + ); + }); + + it('clears the dependency message when the last event is unticked', async () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + prWatchEvents: ['ci_failure'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_BOUND, instance }); + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + expect(document.body.textContent).toContain( + 'to save the selected watch events.', + ); + + // Unticking the last event is the other remedy; the alert must retire + // without another Save. + const box = Array.from( + document.querySelectorAll( + '[role="group"][aria-label="PR Watch Events"] input[type="checkbox"]', + ), + ).find((b) => b.closest('label')?.textContent?.includes('ci_failure'))!; + await act(async () => { + box.click(); + }); + expect(document.body.textContent).not.toContain( + 'to save the selected watch events.', + ); + }); + + it('retires the invalid-option message when the offending value is unticked', async () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + prWatchEvents: ['bogus_event'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_BOUND, instance }); + + await act(async () => { + ( + Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ) as HTMLButtonElement + ).click(); + }); + expect(document.body.textContent).toContain( + "Remove values that aren't in the allowed list.", + ); + + // Unticking the offending value is the remedy the message asks for; it + // must retire the message without a second Save. + const bogus = Array.from( + document.querySelectorAll( + '[role="group"][aria-label="PR Watch Events"] input[type="checkbox"]', + ), + ).find((b) => b.closest('label')?.textContent?.includes('bogus_event'))!; + await act(async () => { + bogus.click(); + }); + expect(document.body.textContent).not.toContain( + "Remove values that aren't in the allowed list.", + ); + }); + + it('replaces the invalid-option message with the dependency one when a valid event remains', async () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + prWatchEvents: ['ci_failure', 'bogus_event'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_BOUND, instance }); + + await act(async () => { + ( + Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ) as HTMLButtonElement + ).click(); + }); + expect(document.body.textContent).toContain( + "Remove values that aren't in the allowed list.", + ); + + const bogus = Array.from( + document.querySelectorAll( + '[role="group"][aria-label="PR Watch Events"] input[type="checkbox"]', + ), + ).find((b) => b.closest('label')?.textContent?.includes('bogus_event'))!; + await act(async () => { + bogus.click(); + }); + // The remaining value is allowed, so the invalid-option instruction is + // gone; the still-unsatisfied dependency takes over the slot. + expect(document.body.textContent).not.toContain( + "Remove values that aren't in the allowed list.", + ); + expect(document.body.textContent).toContain( + 'to save the selected watch events.', + ); + }); + + it('does not surface validation before the first submit', async () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + prWatchEvents: ['ci_failure', 'merge_conflict'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_BOUND, instance }); + expect(document.querySelectorAll('[role="alert"]').length).toBe(0); + + // A legitimate gated edit must not reveal validation state the operator + // never submitted; submit stays the only population site. + const box = Array.from( + document.querySelectorAll( + '[role="group"][aria-label="PR Watch Events"] input[type="checkbox"]', + ), + ).find((b) => b.closest('label')?.textContent?.includes('merge_conflict'))!; + await act(async () => { + box.click(); + }); + expect(document.querySelectorAll('[role="alert"]').length).toBe(0); + }); + + it('keeps a shown invalid-option message when delivery is switched off', async () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + deliverToBoundSessions: true, + prWatchEvents: ['bogus_event'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_BOUND, instance }); + const clickSave = async () => { + await act(async () => { + ( + Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ) as HTMLButtonElement + ).click(); + }); + }; + const clickSwitch = async () => { + await act(async () => { + document + .querySelector('button[role="switch"]')! + .click(); + }); + }; + + await clickSave(); + expect(document.body.textContent).toContain( + "Remove values that aren't in the allowed list.", + ); + // The mirror case: a shown instruction survives the switch moving off + // (re-derived, not silently dropped), in both directions. + await clickSwitch(); + expect(document.body.textContent).toContain( + "Remove values that aren't in the allowed list.", + ); + await clickSwitch(); + expect(document.body.textContent).toContain( + "Remove values that aren't in the allowed list.", + ); + }); + + it('renders no bound-session panel for channels without the fields', async () => { + await renderDialog({ descriptor: DINGTALK }); + expect(document.body.textContent).not.toContain('PR-bound sessions'); + }); + + it('renders no bound-session panel for channels without the fields (zh-CN)', async () => { + await renderDialog({ descriptor: DINGTALK, language: 'zh-CN' }); + expect(document.body.textContent).not.toContain('PR 绑定会话'); + }); + + it('scrolls a non-advanced watch-events error into view on save', async () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + prWatchEvents: ['ci_failure'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + // jsdom has no scrollIntoView; stub it so the scroll is observable. + const scrollIntoView = vi.fn(); + const originalScrollIntoView = Element.prototype.scrollIntoView; + Element.prototype.scrollIntoView = scrollIntoView; + try { + await renderDialog({ descriptor: GITHUB_BOUND, instance }); + await act(async () => { + ( + Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ) as HTMLButtonElement + ).click(); + }); + expect(scrollIntoView).toHaveBeenCalledWith({ block: 'center' }); + const target = scrollIntoView.mock.instances[0] as HTMLElement; + expect(target.id.endsWith('-prWatchEvents')).toBe(true); + } finally { + Element.prototype.scrollIntoView = originalScrollIntoView; + } + }); + + it('associates the disabled hint with the checkbox group', async () => { + await renderDialog({ descriptor: GITHUB_BOUND }); + + const group = document.querySelector( + '[role="group"][aria-label="PR Watch Events"]', + )!; + const describedBy = group.getAttribute('aria-describedby'); + expect(describedBy).not.toBeNull(); + const hint = document.getElementById(describedBy!); + expect(hint?.textContent).toContain('above to select watch events'); + expect(hint?.getAttribute('aria-live')).toBe('polite'); + + await act(async () => { + document + .querySelector('button[role="switch"]')! + .click(); + }); + expect(group.getAttribute('aria-describedby')).toBeNull(); + }); + + it('mounts a stored delivery-on instance with its events enabled', async () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + deliverToBoundSessions: true, + prWatchEvents: ['ci_failure'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_BOUND, instance }); + + const boxes = Array.from( + document.querySelectorAll( + '[role="group"][aria-label="PR Watch Events"] input[type="checkbox"]', + ), + ); + expect(boxes.length).toBeGreaterThan(0); + expect(boxes.every((box) => !box.disabled)).toBe(true); + expect( + boxes.find((b) => + b.closest('label')?.textContent?.includes('ci_failure'), + )!.checked, + ).toBe(true); + expect(document.body.textContent).not.toContain( + 'above to select watch events.', + ); + + // The live region must exist (empty) before the hint appears, and the + // same node must survive the transition — a region mounted together + // with its text is exactly what aria-live cannot announce. + const region = Array.from( + document.querySelectorAll('span[aria-live="polite"]'), + ).find((node) => node.id.endsWith('-prWatchEvents-disabled-hint')); + expect(region).toBeDefined(); + expect(region!.textContent).toBe(''); + await act(async () => { + document + .querySelector('button[role="switch"]')! + .click(); + }); + expect(region!.isConnected).toBe(true); + expect(region!.textContent).toContain('above to select watch events'); + }); + it('localizes the instructions copy for zh-CN operators', async () => { await renderDialog({ descriptor: MULTILINE_INSTRUCTIONS, @@ -1275,6 +1809,11 @@ describe('ChannelEditorDialog', () => { toggle!.click(); }); + // The pre-existing hint call sites pass no hintId: the local-gh hint + // must keep rendering without an id or a live region. + expect(document.querySelectorAll('span[aria-live]').length).toBe(0); + expect(document.querySelectorAll('span[id]').length).toBe(0); + const save = Array.from(document.querySelectorAll('button')).find( (button) => button.textContent?.trim() === 'Save', ); diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx index 6535043593a..3546da9403e 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx @@ -134,6 +134,11 @@ const SHARED_SESSION_FIELD_KEYS = new Set([ 'multiSession', 'instructions', ]); +// One panel: watch events only mean something with bound delivery enabled. +const BOUND_SESSION_FIELD_KEYS = new Set([ + 'deliverToBoundSessions', + 'prWatchEvents', +]); const SHARED_FIELD_LABEL_KEYS: Record = { outputMode: 'channels.editor.field.shared.outputMode', @@ -247,6 +252,7 @@ function FieldShell({ label, required, hint, + hintId, description, error, children, @@ -255,6 +261,7 @@ function FieldShell({ label: string; required?: boolean; hint?: string; + hintId?: string; description?: string; error?: string; children: ReactNode; @@ -270,7 +277,17 @@ function FieldShell({ ) : null} - {hint ? {hint} : null} + {hint || hintId ? ( + // A hintId keeps the live region mounted while the hint is empty, + // so screen readers announce the hint when it appears. + + {hint} + + ) : null} {children} {description ? ( @@ -348,7 +365,11 @@ export function ChannelEditorDialog({ (field) => !SHARED_ACCESS_FIELD_KEYS.has(field.key) && !SHARED_SESSION_FIELD_KEYS.has(field.key) && - !ADVANCED_FIELD_KEYS.has(field.key), + !ADVANCED_FIELD_KEYS.has(field.key) && + !BOUND_SESSION_FIELD_KEYS.has(field.key), + ); + const boundSessionFields = descriptor.fields.filter((field) => + BOUND_SESSION_FIELD_KEYS.has(field.key), ); useEffect(() => { @@ -375,10 +396,11 @@ export function ChannelEditorDialog({ }, [workspaceCwd]); useEffect(() => { - if (!advancedOpen || !scrollToErrorKey) return; - // Runs after the forced-open panel content mounts; the id is the same one - // FieldShell renders, so the scroll lands on the erroring control. The - // optional call keeps jsdom (no scrollIntoView) from throwing. + if (!scrollToErrorKey) return; + // Runs after the forced-open panel content mounts (submit sets both in + // one batch); the id is the same one FieldShell renders, so the scroll + // lands on the erroring control. The optional call keeps jsdom (no + // scrollIntoView) from throwing. document .getElementById(`${formId}-${scrollToErrorKey}`) ?.scrollIntoView?.({ block: 'center' }); @@ -451,6 +473,8 @@ export function ChannelEditorDialog({ return t('channels.editor.validation.conflictingRepoAliases'); if (code === 'invalidOption') return t('channels.editor.validation.invalidOption'); + if (code === 'requiresBoundDelivery') + return t('channels.editor.validation.requiresBoundDelivery'); if (code === 'number') return t('channels.editor.validation.number'); if (code === 'outOfRange') { const numberField = field && field.kind === 'number' ? field : undefined; @@ -479,6 +503,37 @@ export function ChannelEditorDialog({ }); }; + // Re-derive only the errors already on screen, so each remedy (switch in + // either direction, untick) retires exactly the instruction it satisfies — + // including invalidOption, whose remedy is unticking the offending value — + // without surfacing an error the operator never submitted. Submit stays the + // only population site; deriving through validateChannelEditorDraft keeps + // the daemon precedence that the parity table pins. + const refreshErrors = (values: ChannelEditorDraft['values']) => + setErrors((current) => { + const shown = Object.keys(current); + if (shown.length === 0) return current; + const validation = validateChannelEditorDraft( + descriptor, + { ...draft, values }, + existingNames, + instance, + ); + const next = { ...current }; + for (const key of shown) { + const code = validation[key]; + if (code) { + next[key] = validationMessage( + descriptor.fields.find((field) => field.key === key), + code, + ); + } else { + delete next[key]; + } + } + return next; + }); + const submit = async (event: FormEvent) => { event.preventDefault(); const validation = validateChannelEditorDraft( @@ -488,15 +543,14 @@ export function ChannelEditorDialog({ instance, ); if (Object.keys(validation).length > 0) { - // An error on a collapsed field would otherwise be invisible: open the - // panel and bring the first erroring advanced field into view. + // An error below the fold or on a collapsed field would otherwise be + // invisible: open the advanced panel when needed and bring the first + // erroring field — advanced or not — into view. const advancedErrorKey = Object.keys(validation).find((key) => ADVANCED_FIELD_KEYS.has(key), ); - if (advancedErrorKey) { - setAdvancedOpen(true); - setScrollToErrorKey(advancedErrorKey); - } + if (advancedErrorKey) setAdvancedOpen(true); + setScrollToErrorKey(advancedErrorKey ?? Object.keys(validation)[0]); setErrors( Object.fromEntries( Object.entries(validation).map(([key, code]) => [ @@ -668,6 +722,14 @@ export function ChannelEditorDialog({ field.key === 'useLocalGh' && isRepositoryOnlyChannelType(descriptor.type); const updateSwitch = (checked: boolean) => { + if (field.key === 'deliverToBoundSessions') { + // Both directions re-derive shown errors: on retires the dependency + // instruction, off keeps a shown one instead of dropping it silently. + refreshErrors({ + ...draft.values, + deliverToBoundSessions: checked, + }); + } if (!isLocalGhSwitch) { update(checked); return; @@ -753,6 +815,11 @@ export function ChannelEditorDialog({ } if (field.kind === 'string-list' && field.options?.length) { const options = field.options; + // Same scope as the validation mirror in channel-editor-state. + const watchField = + field.key === 'prWatchEvents' && descriptor.type === 'github'; + const gated = + watchField && draft.values['deliverToBoundSessions'] !== true; const selected = toList(String(value ?? '')); // The adapter matches these values case-insensitively, so a stored // `COMMENT` has to tick the canonical `comment` box instead of rendering @@ -772,14 +839,18 @@ export function ChannelEditorDialog({ const next = new Set(selectedKeys); if (checked) next.add(target.toLowerCase()); else next.delete(target.toLowerCase()); - update( - [ - ...options - .map((option) => option.value) - .filter((entry) => next.has(entry.toLowerCase())), - ...stored.filter((entry) => next.has(entry.toLowerCase())), - ].join(', '), - ); + const joined = [ + ...options + .map((option) => option.value) + .filter((entry) => next.has(entry.toLowerCase())), + ...stored.filter((entry) => next.has(entry.toLowerCase())), + ].join(', '); + // Unticking is a remedy too: re-derive so a satisfied instruction + // retires without waiting for a second Save. + if (watchField) { + refreshErrors({ ...draft.values, [field.key]: joined }); + } + update(joined); }; return (
{[ ...options, ...stored.map((entry) => ({ value: entry, label: entry })), ].map((option) => { const isSelected = selectedKeys.has(option.value.toLowerCase()); + // A stored selection stays clearable while gated, so turning + // delivery off never strands the operator with events they + // cannot remove. + const optionDisabled = gated && !isSelected; const optionLabel = fieldOptionLabel( field, option.value, @@ -811,11 +893,13 @@ export function ChannelEditorDialog({ key={option.value} className={styles.optionRow} data-selected={isSelected} + data-disabled={optionDisabled} > toggleValue(option.value, event.target.checked) } @@ -1286,6 +1370,20 @@ export function ChannelEditorDialog({ ); })} + {boundSessionFields.length > 0 ? ( +
+
+

+ {t('channels.editor.section.boundSessions')} +

+

+ {t('channels.editor.section.boundSessions.description')} +

+
+ {boundSessionFields.map(renderField)} +
+ ) : null} + {sessionFields.length > 0 ? (

diff --git a/packages/web-shell/client/components/channels/channel-editor-parity.test.ts b/packages/web-shell/client/components/channels/channel-editor-parity.test.ts new file mode 100644 index 00000000000..6eda135f642 --- /dev/null +++ b/packages/web-shell/client/components/channels/channel-editor-parity.test.ts @@ -0,0 +1,103 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { plugin } from '@qwen-code/channel-github'; +import type { + DaemonChannelInstanceSnapshot, + DaemonChannelTypeDescriptor, +} from '@qwen-code/sdk/daemon'; +import { + createChannelEditorDraft, + validateChannelEditorDraft, +} from './channel-editor-state'; + +// Parity between the editor's client mirror and the daemon authority. The +// descriptor is the plugin's own field list, so a lone change on either side +// (dependency dropped, second precondition added, vocabulary moved) fails +// this table instead of leaving both suites green. Test-only import: the +// plugin is a devDependency and never reaches the web-shell bundle. +// management is optional on ChannelPlugin by contract; narrow once so a +// hypothetical missing descriptor fails with a clear message instead of a +// module-load TypeError (and stays TS18048-clean if test files ever join the +// package typecheck). +const management = plugin.management; +if (!management) { + throw new Error('@qwen-code/channel-github declares no management metadata.'); +} + +const descriptor: DaemonChannelTypeDescriptor = { + type: 'github', + displayName: 'GitHub', + manageable: true, + fields: management.fields, +}; + +const CASES = [ + { + name: 'delivery off with a selection', + events: ['ci_failure'], + delivery: false, + code: 'requiresBoundDelivery', + daemon: /requires deliverToBoundSessions/, + }, + { + name: 'delivery on with a selection', + events: ['ci_failure'], + delivery: true, + code: undefined, + daemon: undefined, + }, + { + name: 'delivery off with an empty selection', + events: [], + delivery: false, + code: undefined, + daemon: undefined, + }, + { + name: 'delivery off with an unknown value', + events: ['typo_event'], + delivery: false, + code: 'invalidOption', + daemon: /Unrecognized prWatchEvents/, + }, +] as const; + +describe('bound-delivery rule parity (editor mirror vs daemon authority)', () => { + it.each(CASES)('$name', ({ events, delivery, code, daemon }) => { + const config = { + type: 'github', + useLocalGh: true, + deliverToBoundSessions: delivery, + prWatchEvents: [...events], + }; + const instance: DaemonChannelInstanceSnapshot = { + name: 'parity', + config, + secrets: {}, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(descriptor, instance); + draft.name = 'parity'; + + const clientCode = validateChannelEditorDraft( + descriptor, + draft, + [], + instance, + )['prWatchEvents']; + expect(clientCode).toBe(code); + + const daemonMessage = management.validateConfig?.(config); + if (daemon) { + expect(daemonMessage).toMatch(daemon); + } else { + expect(daemonMessage).toBeUndefined(); + } + }); +}); diff --git a/packages/web-shell/client/components/channels/channel-editor-state.test.ts b/packages/web-shell/client/components/channels/channel-editor-state.test.ts index 53a6b112704..5a68089fc8e 100644 --- a/packages/web-shell/client/components/channels/channel-editor-state.test.ts +++ b/packages/web-shell/client/components/channels/channel-editor-state.test.ts @@ -17,7 +17,10 @@ import { retainedRepositorySettings, validateChannelEditorDraft, } from './channel-editor-state'; -import type { ChannelGroupOverrideDraft } from './channel-editor-state'; +import type { + ChannelEditorDraft, + ChannelGroupOverrideDraft, +} from './channel-editor-state'; const DINGTALK: DaemonChannelTypeDescriptor = { type: 'dingtalk', @@ -972,6 +975,61 @@ const GITHUB: DaemonChannelTypeDescriptor = { ], }; +describe('GitHub watch events require bound delivery', () => { + const BOUND: DaemonChannelTypeDescriptor = { + ...GITHUB, + fields: [ + ...GITHUB.fields, + { + key: 'deliverToBoundSessions', + label: 'Deliver To PR-Bound Sessions', + kind: 'boolean', + }, + { + key: 'prWatchEvents', + label: 'PR Watch Events', + kind: 'string-list', + options: [ + { value: 'ci_failure', label: 'ci_failure' }, + { value: 'merge_conflict', label: 'merge_conflict' }, + ], + }, + ], + }; + + function draftWithEvents(events: string): ChannelEditorDraft { + const draft = createChannelEditorDraft(BOUND); + draft.name = 'my-bot'; + draft.values.useLocalGh = true; + draft.values.prWatchEvents = events; + return draft; + } + + it('refuses a watch selection while bound delivery is off', () => { + expect( + validateChannelEditorDraft(BOUND, draftWithEvents('ci_failure'), []), + ).toEqual({ prWatchEvents: 'requiresBoundDelivery' }); + }); + + it('allows the selection once bound delivery is on', () => { + const draft = draftWithEvents('ci_failure'); + draft.values.deliverToBoundSessions = true; + expect(validateChannelEditorDraft(BOUND, draft, [])).toEqual({}); + }); + + it('leaves an empty selection alone with delivery off', () => { + expect(validateChannelEditorDraft(BOUND, draftWithEvents(''), [])).toEqual( + {}, + ); + }); + + it('reports invalidOption before the delivery dependency', () => { + expect( + validateChannelEditorDraft(BOUND, draftWithEvents('typo_event'), []), + ).toEqual({ prWatchEvents: 'invalidOption' }); + }); +}); + describe('Descriptor-driven privatePolicy', () => { it('defaults enum fields to the first option for new channels', () => { const draft = createChannelEditorDraft(GITHUB); diff --git a/packages/web-shell/client/components/channels/channel-editor-state.ts b/packages/web-shell/client/components/channels/channel-editor-state.ts index 5bb68b2f8c2..405be598f81 100644 --- a/packages/web-shell/client/components/channels/channel-editor-state.ts +++ b/packages/web-shell/client/components/channels/channel-editor-state.ts @@ -60,7 +60,8 @@ export type ChannelEditorValidationCode = | 'invalidGroupEntry' | 'number' | 'outOfRange' - | 'policy'; + | 'policy' + | 'requiresBoundDelivery'; export type ChannelEditorValidationErrors = Record< string, @@ -423,6 +424,16 @@ export function validateChannelEditorDraft( if (!hasToken && draft.values['useLocalGh'] !== true) { errors['token'] = 'credential'; } + // Mirror validateBoundSessionConfig so the dependency is named at the + // field instead of surfacing as a raw daemon rejection after submit. + // invalidOption wins, the same order the runtime validates in. + if ( + !errors['prWatchEvents'] && + toList(String(draft.values['prWatchEvents'] ?? '')).length > 0 && + draft.values['deliverToBoundSessions'] !== true + ) { + errors['prWatchEvents'] = 'requiresBoundDelivery'; + } } if ( !isRepositoryOnlyChannelType(descriptor.type) && diff --git a/packages/web-shell/client/e2e/visuals/screenshots.spec.ts b/packages/web-shell/client/e2e/visuals/screenshots.spec.ts index 67ec42ff7f1..2cac96a4223 100644 --- a/packages/web-shell/client/e2e/visuals/screenshots.spec.ts +++ b/packages/web-shell/client/e2e/visuals/screenshots.spec.ts @@ -1423,6 +1423,22 @@ for (const theme of THEMES) { { value: 'ci_activity', label: 'ci_activity' }, ], }, + { + key: 'deliverToBoundSessions', + label: 'Deliver To PR-Bound Sessions', + kind: 'boolean', + }, + { + key: 'prWatchEvents', + label: 'PR Watch Events', + kind: 'string-list', + options: [ + { value: 'ci_failure', label: 'ci_failure' }, + { value: 'merge_conflict', label: 'merge_conflict' }, + { value: 'head_change', label: 'head_change' }, + { value: 'state_change', label: 'state_change' }, + ], + }, { key: 'privatePolicy', label: 'Private Policy', @@ -1502,6 +1518,29 @@ for (const theme of THEMES) { .scrollIntoViewIfNeeded(); await expect(reasonFilter).toBeInViewport({ ratio: 1 }); await captureScreenshot(page, `github-channel-editor-reasons-${theme}`); + // The bound-session panel gates watch events on delivery. Capture the + // gated state (disabled boxes + hint), then enabled with a selection, + // so the published pictures document the dependency itself. + const watchGroup = page.getByRole('group', { name: 'PR Watch Events' }); + await watchGroup.scrollIntoViewIfNeeded(); + await expect(watchGroup).toBeInViewport({ ratio: 1 }); + await expect(watchGroup.getByRole('checkbox')).toHaveCount(4); + const ciFailure = watchGroup.getByRole('checkbox', { + name: /ci_failure/, + }); + await expect(ciFailure).toBeDisabled(); + await captureScreenshot( + page, + `github-channel-editor-watch-gated-${theme}`, + ); + await page + .getByRole('switch', { name: 'Deliver To PR-Bound Sessions' }) + .click(); + await expect(ciFailure).toBeEnabled(); + await ciFailure.check(); + await watchGroup.scrollIntoViewIfNeeded(); + await expect(watchGroup).toBeInViewport({ ratio: 1 }); + await captureScreenshot(page, `github-channel-editor-watch-on-${theme}`); await page.getByLabel('Instance name').fill('github-bot'); await page.getByRole('button', { name: 'Save' }).click(); await expect( diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index 6d3fcdf63f8..e6f8c62b609 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -4458,6 +4458,9 @@ const EN: Messages = { 'channels.editor.section.credentials': 'Credentials', 'channels.editor.section.connection': 'Connection', 'channels.editor.section.routing': 'Notifications and routing', + 'channels.editor.section.boundSessions': 'PR-bound sessions', + 'channels.editor.section.boundSessions.description': + 'Watch events apply to PRs bound to a live session. Enable delivery first, then choose the transitions to watch.', 'channels.editor.section.session': 'Conversation management', 'channels.editor.section.access': 'Access control', 'channels.editor.section.access.description': @@ -4585,6 +4588,8 @@ const EN: Messages = { 'channels.editor.field.github.prWatchEvents': 'PR Watch Events', 'channels.editor.field.github.prWatchEvents.description': 'Optional. State transitions to watch for bound PRs (requires Deliver To PR-Bound Sessions). Leave empty to disable.', + 'channels.editor.field.github.prWatchEvents.disabledHint': + 'Enable "Deliver To PR-Bound Sessions" above to select watch events.', 'channels.editor.field.github.prWatchEvents.option.ci_failure': 'ci_failure — CI checks turn red on a head commit', 'channels.editor.field.github.prWatchEvents.option.merge_conflict': @@ -4776,6 +4781,8 @@ const EN: Messages = { `This channel stores a group entry that is not an object: ${v?.repositories ?? ''}. The daemon refuses every save while it is there, and the form has no row for it. Fix or remove that entry in settings.json, then reload the channel.`, 'channels.editor.validation.invalidOption': "Remove values that aren't in the allowed list.", + 'channels.editor.validation.requiresBoundDelivery': + 'Enable "Deliver To PR-Bound Sessions" to save the selected watch events.', 'channels.editor.validation.number': 'Enter a valid number.', 'channels.editor.validation.outOfRange': (v) => v?.min !== undefined && v?.max !== undefined @@ -8947,6 +8954,9 @@ const ZH: Messages = { 'channels.editor.section.credentials': '应用凭据', 'channels.editor.section.connection': '连接', 'channels.editor.section.routing': '通知与路由', + 'channels.editor.section.boundSessions': 'PR 绑定会话', + 'channels.editor.section.boundSessions.description': + '监视事件作用于已绑定到活跃会话的 PR。先开启投递，再选择要监视的状态转换。', 'channels.editor.section.session': '会话管理', 'channels.editor.section.access': '访问控制', 'channels.editor.section.access.description': @@ -9071,6 +9081,8 @@ const ZH: Messages = { 'channels.editor.field.github.prWatchEvents': 'PR 状态监视事件', 'channels.editor.field.github.prWatchEvents.description': '可选。为绑定 PR 监视的状态转换（需开启"投递到 PR 绑定会话"）。留空则禁用。', + 'channels.editor.field.github.prWatchEvents.disabledHint': + '开启上方「投递到 PR 绑定会话」后即可选择监视事件。', 'channels.editor.field.github.prWatchEvents.option.ci_failure': 'ci_failure — CI 检查在 head 提交上变红', 'channels.editor.field.github.prWatchEvents.option.merge_conflict': @@ -9252,6 +9264,8 @@ const ZH: Messages = { 'channels.editor.validation.invalidGroupEntry': (v) => `该渠道把以下群组条目存成了非对象：${v?.repositories ?? ''}。只要它还在，daemon 就会拒绝每次保存，而表单里没有对应的行。请在 settings.json 里修正或删除该条目，然后重新加载渠道。`, 'channels.editor.validation.invalidOption': '请移除不在允许列表中的值。', + 'channels.editor.validation.requiresBoundDelivery': + '需开启「投递到 PR 绑定会话」才能保存所选的监视事件。', 'channels.editor.validation.number': '请输入有效数字。', 'channels.editor.validation.outOfRange': (v) => v?.min !== undefined && v?.max !== undefined diff --git a/packages/web-shell/package.json b/packages/web-shell/package.json index 35b590e81f9..9f519435ed2 100644 --- a/packages/web-shell/package.json +++ b/packages/web-shell/package.json @@ -110,6 +110,7 @@ "@pierre/trees": "1.0.0-beta.6", "@playwright/test": "1.61.1", "@qwen-code/acp-bridge": "file:../acp-bridge", + "@qwen-code/channel-github": "file:../channels/github", "@qwen-code/sdk": "file:../sdk-typescript", "@tailwindcss/vite": "^4.3.2", "@types/node": "^22.0.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3a8ad214980..32cabfc6baf 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1715,6 +1715,9 @@ importers: '@qwen-code/acp-bridge': specifier: workspace:* version: link:../acp-bridge + '@qwen-code/channel-github': + specifier: workspace:* + version: link:../channels/github '@qwen-code/sdk': specifier: workspace:* version: link:../sdk-typescript