diff --git a/client/src/components/cos/TaskAddForm.jsx b/client/src/components/cos/TaskAddForm.jsx index 4e16be46be..fa7b293a69 100644 --- a/client/src/components/cos/TaskAddForm.jsx +++ b/client/src/components/cos/TaskAddForm.jsx @@ -24,6 +24,26 @@ import { safeReadJsonStorage, safeReadStorage, safeRemoveStorage, safeWriteJsonS const TASK_DESCRIPTION_DRAFT_KEY = 'portos-cos-task-description-draft'; const INVALID_DRAFT = Symbol('invalid task description draft'); +// ReviewerPicker's onChange patch keys, mapped to their addCosTask payload +// names — only `stopMode` differs (→ `reviewStopMode`). Only keys present in +// `reviewOverrides` (i.e. actually touched by the user) go on the wire; see +// the submit payload below and #6219. +const REVIEW_PICKER_TO_PAYLOAD_KEY = { + reviewers: 'reviewers', + usernames: 'usernames', + optionalReviewers: 'optionalReviewers', + reviewerMaxRounds: 'reviewerMaxRounds', + reviewerModels: 'reviewerModels', + reviewerEfforts: 'reviewerEfforts', + stopMode: 'reviewStopMode', + reviewerApplies: 'reviewerApplies', +}; +const reviewOverridePayload = (reviewOverrides) => Object.fromEntries( + Object.entries(REVIEW_PICKER_TO_PAYLOAD_KEY) + .filter(([pickerKey]) => reviewOverrides[pickerKey] !== undefined) + .map(([pickerKey, payloadKey]) => [payloadKey, reviewOverrides[pickerKey]]) +); + const readTaskDescriptionDraft = (defaultApp) => { const raw = safeReadStorage(TASK_DESCRIPTION_DRAFT_KEY); if (raw === null) return { description: '', app: defaultApp }; @@ -65,14 +85,22 @@ export default function TaskAddForm({ providers, providersLoaded = true, apps, o // so the server keeps its own default. Only a slashdo-backed template sets it. const [worktreeChangesExpected, setWorktreeChangesExpected] = useState(undefined); const [prCompletion, setPrCompletion] = useState(DEFAULT_PR_COMPLETION); - const [reviewers, setReviewers] = useState(DEFAULT_REVIEWERS); - const [reviewUsernames, setReviewUsernames] = useState([]); - const [optionalReviewers, setOptionalReviewers] = useState([]); - const [reviewerMaxRounds, setReviewerMaxRounds] = useState({}); - const [reviewerModels, setReviewerModels] = useState({}); - const [reviewerEfforts, setReviewerEfforts] = useState({}); - const [reviewStopMode, setReviewStopMode] = useState(DEFAULT_REVIEW_STOP_MODE); - const [reviewerApplies, setReviewerApplies] = useState(false); + const [reviewDefaults, setReviewDefaults] = useState({ + reviewers: DEFAULT_REVIEWERS, + usernames: [], + optionalReviewers: [], + reviewerMaxRounds: {}, + reviewerModels: {}, + reviewerEfforts: {}, + stopMode: DEFAULT_REVIEW_STOP_MODE, + reviewerApplies: false, + }); + // Only the reviewer fields the user actually touched on THIS form, keyed like + // ReviewerPicker's onChange patch. Everything else inherits reviewDefaults, and + // only these (mapped to their task-metadata names) are ever submitted — so an + // untouched field keeps following future Code Review Defaults changes instead + // of freezing today's values into the new task permanently (#6219). + const [reviewOverrides, setReviewOverrides] = useState({}); const [reviewerCliInstalled, setReviewerCliInstalled] = useState({}); // Which federated instance runs this task (#4520). '' = any instance, the // opportunistic default. Hidden entirely on a single-instance install. @@ -142,16 +170,18 @@ export default function TaskAddForm({ providers, providersLoaded = true, apps, o api.getCodeReviewDefaults({ silent: true }) .then((d) => { if (cancelled || !d) return; - if (Array.isArray(d.reviewers) && d.reviewers.length) setReviewers(d.reviewers); - if (Array.isArray(d.usernames)) setReviewUsernames(d.usernames); - if (Array.isArray(d.optionalReviewers)) setOptionalReviewers(d.optionalReviewers); - if (d.reviewerMaxRounds && typeof d.reviewerMaxRounds === 'object' && !Array.isArray(d.reviewerMaxRounds)) setReviewerMaxRounds(d.reviewerMaxRounds); - // The defaults persist per-reviewer models as scalars; the picker takes the - // token-keyed map (see client/src/lib/reviewerModels.js). - setReviewerModels(reviewerModelsFromDefaults(d)); - setReviewerEfforts(reviewerEffortsFromDefaults(d)); - if (d.stopMode) setReviewStopMode(d.stopMode); - if (d.reviewerApplies === true) setReviewerApplies(true); + setReviewDefaults({ + reviewers: Array.isArray(d.reviewers) && d.reviewers.length ? d.reviewers : DEFAULT_REVIEWERS, + usernames: Array.isArray(d.usernames) ? d.usernames : [], + optionalReviewers: Array.isArray(d.optionalReviewers) ? d.optionalReviewers : [], + reviewerMaxRounds: d.reviewerMaxRounds && typeof d.reviewerMaxRounds === 'object' && !Array.isArray(d.reviewerMaxRounds) ? d.reviewerMaxRounds : {}, + // The defaults persist per-reviewer models as scalars; the picker takes the + // token-keyed map (see client/src/lib/reviewerModels.js). + reviewerModels: reviewerModelsFromDefaults(d), + reviewerEfforts: reviewerEffortsFromDefaults(d), + stopMode: d.stopMode || DEFAULT_REVIEW_STOP_MODE, + reviewerApplies: d.reviewerApplies === true, + }); if (d.installed && typeof d.installed === 'object' && !Array.isArray(d.installed)) setReviewerCliInstalled(d.installed); }) .catch(() => {}); @@ -563,17 +593,11 @@ export default function TaskAddForm({ providers, providersLoaded = true, apps, o : worktreeChangesExpected !== undefined ? { worktreeChangesExpected } : {}), prCompletion: !planOnly && useWorktree && openPR ? prCompletion : undefined, // One gate for every per-reviewer field: they only apply when this task - // opens a PR that PortOS reviews before merging. - ...(!planOnly && openPR && prCompletion === 'review-then-merge' ? { - reviewers, - usernames: reviewUsernames, - optionalReviewers, - reviewerMaxRounds, - reviewerModels, - reviewerEfforts, - reviewStopMode, - reviewerApplies, - } : {}), + // opens a PR that PortOS reviews before merging. Only fields the user + // actually touched (reviewOverrides) go on the wire — an untouched field + // stays absent so the task keeps inheriting future Code Review Defaults + // changes instead of freezing today's values in on create (#6219). + ...(!planOnly && openPR && prCompletion === 'review-then-merge' ? reviewOverridePayload(reviewOverrides) : {}), screenshots: screenshots.length > 0 ? screenshots.map(s => s.path) : undefined, attachments: attachments.length > 0 ? attachments.map(a => ({ filename: a.filename, @@ -904,25 +928,27 @@ export default function TaskAddForm({ providers, providersLoaded = true, apps, o {openPR && prCompletion === 'review-then-merge' && (
{ - setReviewers(r); - setReviewUsernames(u); - setOptionalReviewers(o); - setReviewerMaxRounds(m); - setReviewerModels(rm); - setReviewerEfforts(re); - setReviewStopMode(stopMode); - setReviewerApplies(ra); + stopMode={reviewOverrides.stopMode ?? reviewDefaults.stopMode} + reviewerApplies={reviewOverrides.reviewerApplies ?? 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 every field into a permanent + // override (#6219, mirroring #6208's GlobalConfigControls fix). + defaults={reviewDefaults} + onChange={(patch) => { + // The picker emits only what differs from `defaults`, so + // the patch IS the complete override set — replace outright + // rather than merge, or a key reverted back to the default + // would keep pinning its stale value. + setReviewOverrides(patch); }} />
diff --git a/client/src/components/cos/TaskAddForm.test.jsx b/client/src/components/cos/TaskAddForm.test.jsx index 856439f863..6e503aecf5 100644 --- a/client/src/components/cos/TaskAddForm.test.jsx +++ b/client/src/components/cos/TaskAddForm.test.jsx @@ -149,6 +149,67 @@ describe('TaskAddForm responsive layout', () => { expect(api.addCosTask.mock.calls[0][0].app).toBe('current-app'); }); + // #6219: ReviewerPicker gets no `defaults` prop wired, the submit payload + // unconditionally spread all eight reviewer fields whenever review-then-merge + // was active — even when the picker was never touched — freezing today's + // Code Review Defaults into the new task's metadata permanently. + describe('reviewer defaults inheritance (#6219)', () => { + const REVIEWER_PAYLOAD_KEYS = [ + 'reviewers', 'usernames', 'optionalReviewers', 'reviewerMaxRounds', + 'reviewerModels', 'reviewerEfforts', 'reviewStopMode', 'reviewerApplies', + ]; + const app = { + id: 'example-app', + name: 'Example App', + repoPath: 'example.com/repo', + defaultOpenPR: true, + defaultPrCompletion: 'review-then-merge', + }; + + it('leaves every reviewer field absent when the picker is never touched', async () => { + const user = userEvent.setup(); + api.getCodeReviewDefaults.mockResolvedValue({ + reviewers: ['copilot', 'claude'], usernames: [], optionalReviewers: [], + reviewerMaxRounds: {}, stopMode: 'all', reviewerApplies: false, + }); + api.addCosTask.mockResolvedValue({ success: true }); + render(); + + await waitFor(() => expect(screen.getByText('Reviewers (in order):')).toBeInTheDocument()); + await user.type(screen.getByPlaceholderText('Task description *'), 'Fix the bug'); + await user.click(screen.getByRole('button', { name: /^Add$/ })); + + await waitFor(() => expect(api.addCosTask).toHaveBeenCalled()); + const payload = api.addCosTask.mock.calls.at(-1)[0]; + for (const key of REVIEWER_PAYLOAD_KEYS) expect(payload).not.toHaveProperty(key); + }); + + it('sends only the field the user actually changed', async () => { + const user = userEvent.setup(); + api.getCodeReviewDefaults.mockResolvedValue({ + reviewers: ['copilot', 'claude'], usernames: [], optionalReviewers: [], + reviewerMaxRounds: {}, stopMode: 'all', reviewerApplies: false, + }); + api.addCosTask.mockResolvedValue({ success: true }); + render(); + + await waitFor(() => expect(screen.getByText('Reviewers (in order):')).toBeInTheDocument()); + const stopModeSelect = await screen.findByLabelText('Stop mode:'); + await user.selectOptions(stopModeSelect, 'on-clean'); + + await user.type(screen.getByPlaceholderText('Task description *'), 'Fix the bug'); + await user.click(screen.getByRole('button', { name: /^Add$/ })); + + await waitFor(() => expect(api.addCosTask).toHaveBeenCalled()); + const payload = api.addCosTask.mock.calls.at(-1)[0]; + expect(payload.reviewStopMode).toBe('on-clean'); + for (const key of REVIEWER_PAYLOAD_KEYS) { + if (key === 'reviewStopMode') continue; + expect(payload).not.toHaveProperty(key); + } + }); + }); + it('restores a plain-text draft from the previous storage format', async () => { localStorage.setItem('portos-cos-task-description-draft', 'Legacy task draft'); render();