diff --git a/client/src/components/cos/ReviewerPicker.jsx b/client/src/components/cos/ReviewerPicker.jsx index 118f10a6d1..3c05b6d964 100644 --- a/client/src/components/cos/ReviewerPicker.jsx +++ b/client/src/components/cos/ReviewerPicker.jsx @@ -58,11 +58,34 @@ const CUSTOM_MODEL_OPTION = '[custom]'; * - **Max Iterations** → the numeric `~max=` round cap (blank = slashdo's * built-in default, `0` = loop until clean). * - * Controlled: emits the full next shape via onChange so the parent can store + * Controlled: emits the next shape via onChange so the parent can store * `reviewers` / `usernames` / `optionalReviewers` / `reviewerModels` / * `reviewerEfforts` / `reviewerMaxRounds` / `reviewStopMode` / `reviewerApplies` * however it persists them. * + * `defaults` is the resolved fallback the parent seeded the props from (the + * install-wide Code Review Defaults, as token-keyed maps for the pins). When + * provided, `emit()` OMITS any key whose value is deep-equal to + * `defaults[key]` — a field the user never touched stays absent from the + * payload, which is exactly what `resolveReviewerConfig` reads as "inherit". + * Without it the picker would freeze the defaults-of-that-moment into a + * permanent task override on first touch (#6208). Omit the prop where a full + * emit is correct — the surface that edits the defaults themselves has no + * fallback to inherit from. + * + * Absent keeps meaning inherit and an explicitly-empty value keeps meaning + * "clear": the comparison is against the resolved default, never against + * emptiness, so `{}` / `[]` equal only a matching default and are otherwise + * emitted as a real override that clears it. (One key is exempt: the server + * drops an empty `reviewers` list before persisting, so `reviewers: []` + * resolves to the default chain either way — pre-existing server behavior.) + * + * Known trade-off: the diff is against the CURRENT default, so an override + * that happens to equal it (e.g. set before the default changed to match) is + * indistinguishable from "never touched" and reverts to inherit the next time + * any other field is edited. The effective reviewers are unchanged at that + * moment — the pin only stops shadowing future default changes. + * * `modelOptions` is the resolved model-picker data, shaped like * `useReviewerModelOptions()`'s return: `{ optionsByReviewer, defaultModels, * freeText, unavailable, providerDisabled, loaded }`. Callers keep owning their @@ -101,6 +124,7 @@ export default function ReviewerPicker({ installed = null, stopMode = DEFAULT_REVIEW_STOP_MODE, reviewerApplies = false, + defaults = null, onChange, disabled = false, showRunFlags = true @@ -221,17 +245,68 @@ export default function ReviewerPicker({ ? addable : addable.filter(opt => !hiddenAddable.includes(opt)); - const emit = (next) => onChange?.({ - reviewers: selected, - usernames: selectedUsernames, - optionalReviewers: optionalTokens, - reviewerMaxRounds: maxRoundsMap, - reviewerModels: modelsMap, - reviewerEfforts: effortsMap, - stopMode, - reviewerApplies, - ...next - }); + // Case-insensitive equality for the token lists (reviewer slugs are already + // lowercased; GitHub usernames are case-insensitive). Order matters ONLY for + // `reviewers` — the chain runs in click order — so the username lists compare + // as sorted sets; otherwise a same-membership reorder would over-emit. + const listsEqual = (a, b, ordered) => { + if (!Array.isArray(a) || !Array.isArray(b)) return a === b; + if (a.length !== b.length) return false; + const left = a.map((value) => String(value).toLowerCase()); + const right = b.map((value) => String(value).toLowerCase()); + if (!ordered) { + left.sort(); + right.sort(); + } + return left.every((value, index) => value === right[index]); + }; + // Case-insensitive-key equality for the token-keyed pin maps. Values compare + // strictly: `0` (loop until clean) must never equal absent, and `{}` equals + // only a matching default so an explicit clear is still emitted. + const mapsEqual = (a, b) => { + const entries = (map) => { + if (!map || typeof map !== 'object' || Array.isArray(map)) return map; + return Object.entries(map) + .map(([key, value]) => [key.toLowerCase(), value]) + .sort(([x], [y]) => (x < y ? -1 : x > y ? 1 : 0)); + }; + const left = entries(a); + const right = entries(b); + if (!Array.isArray(left) || !Array.isArray(right)) return left === right; + if (left.length !== right.length) return false; + return left.every(([key, value], index) => right[index][0] === key && right[index][1] === value); + }; + const equalsBaseline = (key, value, baseline) => { + if (key === 'reviewers') return listsEqual(value, baseline, true); + if (key === 'usernames' || key === 'optionalReviewers') return listsEqual(value, baseline, false); + if (key === 'reviewerMaxRounds' || key === 'reviewerModels' || key === 'reviewerEfforts') return mapsEqual(value, baseline); + return value === baseline; + }; + + const emit = (next) => { + const full = { + reviewers: selected, + usernames: selectedUsernames, + optionalReviewers: optionalTokens, + reviewerMaxRounds: maxRoundsMap, + reviewerModels: modelsMap, + reviewerEfforts: effortsMap, + stopMode, + reviewerApplies, + ...next + }; + // No baseline (the surface editing the defaults themselves): full snapshot, + // exactly as before. + if (!defaults || typeof defaults !== 'object') { + onChange?.(full); + return; + } + const partial = {}; + for (const key of Object.keys(full)) { + if (!equalsBaseline(key, full[key], defaults[key])) partial[key] = full[key]; + } + onChange?.(partial); + }; const toggleOptional = (token) => emit({ optionalReviewers: isOptional(token) ? withoutToken(token) : [...optionalTokens, token] diff --git a/client/src/components/cos/ReviewerPicker.test.jsx b/client/src/components/cos/ReviewerPicker.test.jsx index 427144c5be..e24a18f5dc 100644 --- a/client/src/components/cos/ReviewerPicker.test.jsx +++ b/client/src/components/cos/ReviewerPicker.test.jsx @@ -668,4 +668,129 @@ describe('ReviewerPicker', () => { expect(details).toHaveAttribute('open'); }); }); + + describe('defaults-aware emit (#6208)', () => { + const DEFAULTS = { + reviewers: ['copilot'], + usernames: [], + optionalReviewers: [], + reviewerMaxRounds: {}, + reviewerModels: {}, + reviewerEfforts: {}, + stopMode: 'all', + reviewerApplies: false, + }; + + it('emits the full snapshot when no defaults are provided', async () => { + const onChange = vi.fn(); + const user = userEvent.setup(); + render(); + await user.click(screen.getByLabelText('Remove Codex')); + expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ + reviewers: ['antigravity'], + usernames: [], + optionalReviewers: [], + reviewerMaxRounds: {}, + reviewerModels: {}, + reviewerEfforts: {}, + })); + }); + + it('omits every key that still equals the defaults when only the stop-mode changes', async () => { + const onChange = vi.fn(); + const user = userEvent.setup(); + render( + + ); + await user.selectOptions(screen.getByLabelText('Stop mode:'), 'on-clean'); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith({ stopMode: 'on-clean' }); + }); + + it('emits only reviewers when one is removed from a seeded list', async () => { + const onChange = vi.fn(); + const user = userEvent.setup(); + render( + + ); + await user.click(screen.getByLabelText('Remove Codex')); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith({ reviewers: ['antigravity'] }); + }); + + it('emits an explicitly-emptied map that clears a default pin (absent ≠ empty)', async () => { + const onChange = vi.fn(); + const user = userEvent.setup(); + render( + + ); + await user.click(screen.getByLabelText('Make Codex blocking')); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith({ optionalReviewers: [] }); + }); + + it('omits a pin map that already matches the defaults when an unrelated control changes', async () => { + const onChange = vi.fn(); + const user = userEvent.setup(); + render( + + ); + await user.click(screen.getByLabelText('Make Codex non-blocking')); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith({ optionalReviewers: ['codex'] }); + }); + + it('compares membership lists order-insensitively (reviewers stay order-sensitive)', async () => { + const onChange = vi.fn(); + const user = userEvent.setup(); + render( + + ); + // Reordering the reviewers IS a change (run order); the same-membership + // optional set in another order is not — only reviewers is emitted. + await user.click(screen.getByLabelText('Move Antigravity earlier')); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith({ reviewers: ['antigravity', 'codex'] }); + }); + + it('compares pin-map keys case-insensitively', async () => { + const onChange = vi.fn(); + const user = userEvent.setup(); + render( + + ); + await user.click(screen.getByLabelText('Make Codex non-blocking')); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith({ optionalReviewers: ['codex'] }); + }); + }); }); diff --git a/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx b/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx index a81269b5d7..40ef816729 100644 --- a/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx +++ b/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx @@ -675,13 +675,38 @@ export default function GlobalConfigControls({ taskType, config, onUpdate, onTri reviewerApplies={config.taskMetadata?.reviewerApplies !== undefined ? (config.taskMetadata?.reviewerApplies === true || config.taskMetadata?.reviewerApplies === 'true') : reviewDefaults.reviewerApplies} + // The same fallback the props above were seeded from. The picker + // omits whatever still equals it, so touching one control no + // longer freezes the rest into a permanent task override (#6208). + defaults={{ + reviewers: reviewDefaults.reviewers, + usernames: reviewDefaults.usernames, + optionalReviewers: reviewDefaults.optionalReviewers, + reviewerMaxRounds: reviewDefaults.reviewerMaxRounds, + reviewerModels: seededPins.models, + reviewerEfforts: seededPins.efforts, + stopMode: reviewDefaults.stopMode, + reviewerApplies: reviewDefaults.reviewerApplies, + }} disabled={updating} - onChange={({ reviewers, usernames, optionalReviewers, reviewerMaxRounds, reviewerModels, reviewerEfforts, stopMode, reviewerApplies }) => { + onChange={(patch) => { + // The picker emits only what differs from `defaults` above, so + // rebuild the reviewer slice from scratch: strip every override + // key (a key that reverted to the default must be DELETED, not + // left pinning its old value), then re-apply what arrived. // Drop the legacy single `reviewer` key so storage converges on `reviewers`. const { reviewer: _reviewer, ...rest } = config.taskMetadata || {}; - onUpdate(taskType, { - taskMetadata: { ...rest, reviewers, usernames, optionalReviewers, reviewerMaxRounds, reviewerModels, reviewerEfforts, reviewStopMode: stopMode, reviewerApplies } - }); + for (const key of REVIEW_CONFIG_KEYS) delete rest[key]; + const taskMetadata = { ...rest }; + if (patch.reviewers !== undefined) taskMetadata.reviewers = patch.reviewers; + if (patch.usernames !== undefined) taskMetadata.usernames = patch.usernames; + if (patch.optionalReviewers !== undefined) taskMetadata.optionalReviewers = patch.optionalReviewers; + if (patch.reviewerMaxRounds !== undefined) taskMetadata.reviewerMaxRounds = patch.reviewerMaxRounds; + if (patch.reviewerModels !== undefined) taskMetadata.reviewerModels = patch.reviewerModels; + if (patch.reviewerEfforts !== undefined) taskMetadata.reviewerEfforts = patch.reviewerEfforts; + if (patch.stopMode !== undefined) taskMetadata.reviewStopMode = patch.stopMode; + if (patch.reviewerApplies !== undefined) taskMetadata.reviewerApplies = patch.reviewerApplies; + onUpdate(taskType, { taskMetadata }); }} /> diff --git a/client/src/components/cos/tabs/schedule/GlobalConfigControlsReviewers.test.jsx b/client/src/components/cos/tabs/schedule/GlobalConfigControlsReviewers.test.jsx new file mode 100644 index 0000000000..ae837fcd9c --- /dev/null +++ b/client/src/components/cos/tabs/schedule/GlobalConfigControlsReviewers.test.jsx @@ -0,0 +1,109 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +// Same hook stubs as GlobalConfigControls.test.jsx, but ReviewerPicker is REAL +// here — these tests prove the picker + controls emit only what changed (#6208). +vi.mock('../../../../hooks/useCodeReviewDefaults', () => ({ + useCodeReviewDefaults: () => ({ + reviewers: ['codex', 'antigravity'], + usernames: [], + optionalReviewers: [], + reviewerMaxRounds: {}, + stopMode: 'all', + reviewerApplies: false, + }), +})); +vi.mock('../../../../hooks/useReviewerModelOptions', () => ({ + default: () => ({ optionsByReviewer: {}, freeText: {}, unavailable: {}, loaded: true }), +})); + +import GlobalConfigControls from './GlobalConfigControls'; + +const BASE_CONFIG = { + type: 'cron', + cronExpression: '0 7 * * *', + enabled: true, + providerId: null, + model: null, + effort: null, + prompt: 'do the thing', + status: {}, +}; + +function renderControls({ taskMetadata, onUpdate = vi.fn(async () => {}), taskType = 'claim-work' } = {}) { + render( + {}} + providers={[]} + apps={[]} + updating={false} + setUpdating={() => {}} + allTaskTypes={['claim-work']} + /> + ); + return onUpdate; +} + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); +}); + +describe('GlobalConfigControls — reviewer override emits only what changed (#6208)', () => { + it('changing only the stop-mode on a task with no reviewer override persists reviewStopMode and nothing else', async () => { + const onUpdate = renderControls({ + taskMetadata: { useWorktree: false, openPR: false, claimFlow: true }, + }); + const user = userEvent.setup(); + await user.selectOptions(screen.getByLabelText('Stop mode:'), 'on-clean'); + expect(onUpdate).toHaveBeenCalledTimes(1); + expect(onUpdate).toHaveBeenCalledWith('claim-work', { + taskMetadata: { useWorktree: false, openPR: false, claimFlow: true, reviewStopMode: 'on-clean' }, + }); + }); + + it('removing one reviewer from a seeded list persists reviewers and nothing else', async () => { + const onUpdate = renderControls({ + taskMetadata: { useWorktree: false, openPR: false, claimFlow: true }, + }); + const user = userEvent.setup(); + await user.click(screen.getByLabelText('Remove Codex')); + expect(onUpdate).toHaveBeenCalledTimes(1); + expect(onUpdate).toHaveBeenCalledWith('claim-work', { + taskMetadata: { useWorktree: false, openPR: false, claimFlow: true, reviewers: ['antigravity'] }, + }); + }); + + it('reverting a pin to the default deletes the key instead of persisting the snapshot', async () => { + // Task carries a stale full-snapshot override; removing the extra reviewer + // back to the seeded list must drop the reviewers key, not rewrite it. + const onUpdate = renderControls({ + taskMetadata: { + useWorktree: false, + openPR: false, + claimFlow: true, + reviewers: ['codex', 'antigravity', 'copilot'], + }, + }); + const user = userEvent.setup(); + await user.click(screen.getByLabelText('Remove Copilot')); + expect(onUpdate).toHaveBeenCalledTimes(1); + expect(onUpdate).toHaveBeenCalledWith('claim-work', { + taskMetadata: { useWorktree: false, openPR: false, claimFlow: true }, + }); + }); + + it('the reset button still clears a stop-mode-only override', () => { + const onUpdate = renderControls({ + taskMetadata: { useWorktree: false, openPR: false, claimFlow: true, reviewStopMode: 'on-clean' }, + }); + fireEvent.click(screen.getByRole('button', { name: 'Use system Code Review Defaults' })); + expect(onUpdate).toHaveBeenCalledWith('claim-work', { + taskMetadata: { useWorktree: false, openPR: false, claimFlow: true }, + }); + }); +});