Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changelog/NEXT.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,9 @@
- A pin is re-checked against your live settings each run rather than trusted blindly: if you later turn off the Codex or Grok toggle in Settings, the affected commission quietly falls back to a backend that works instead of failing every nightly fire. The form says so too — a pin whose backend is currently disabled is flagged in place rather than silently dropped, so you can see why your choice stopped applying.
- The pin genuinely overrides the planner rather than politely deferring to it: a model you named for Local rendering wins over whatever model the planner wrote, and a backend the planner tried to name is discarded in favour of yours. A Grok-pinned video also gets its clip length translated to a length Grok actually delivers, rounding up — so a 10-second commission renders a 10-second clip instead of silently falling to Grok's 6-second minimum.
- Commissions federate, so the pin syncs across your machines; the commission sync version was raised to keep an older peer from silently stripping the field and pushing the un-pinned version back.

## Code Review reviewer table

- **[issue-3133] Every reviewer chain in PortOS now reads as one per-reviewer table — Provider, Model, Optional, Max Iterations — and you can pick a reviewer's model from all four places that show a reviewer chain, not just the settings panel.** Code Review Defaults used to render reviewers as a chip row with a stack of per-backend model dropdowns floating below it; that model-picking logic lived in the panel, so the Add Task form, a task type's Schedule config, and the Agent Operations run drawer — all of which reuse the same picker — had no way to say which model a reviewer should run. Each reviewer is now a row carrying all four of its controls, so "run codex on this tier, cap ollama at one round, and don't let it block the merge" is one readable line instead of three controls in three places. On a phone the row stacks into labelled fields rather than scrolling sideways.
- A local backend (LM Studio / Ollama) offers the models you actually have installed; Codex and Claude accept a typed id as well as a pick, because an Ollama-backed `claude` or a Bedrock-form model id is whatever your machine provides and can't be enumerated. A pinned model that's since been uninstalled is still shown as pinned rather than rendering blank and reading as unset, and a backend that isn't running says so instead of just looking empty.
- The model pin now belongs to the task, not only to the install: a per-task or per-task-type choice overrides your Code Review Defaults the same way its `~opt` and round-cap siblings already do, and an explicitly empty choice means "each reviewer's own default" rather than silently inheriting. Clearing the field removes the pin instead of storing an empty model id. Your saved Code Review Defaults are read and written unchanged, so nothing needs migrating.
65 changes: 46 additions & 19 deletions client/src/components/apps/SlashDoRunDrawer.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { useState, useCallback } from 'react';
import { useState, useCallback, useMemo } from 'react';
import { Loader2, Terminal, Wand2 } from 'lucide-react';
import Drawer from '../Drawer';
import ProviderModelSelector from '../ProviderModelSelector';
import ReviewerPicker from '../cos/ReviewerPicker';
import EffortSelect from '../cos/EffortSelect';
import useProviderModels from '../../hooks/useProviderModels';
import useReviewerModelOptions from '../../hooks/useReviewerModelOptions';
import { reviewerModelsFromDefaults } from '../../lib/reviewerModels';
import { CodeReviewDefaultsProvider, useCodeReviewDefaults } from '../../hooks/useCodeReviewDefaults';
import { isProcessProvider } from '../../utils/providers';
import WorkItemPicker from './WorkItemPicker';
Expand All @@ -31,6 +33,9 @@ const SELECT_CLASS = 'w-full px-3 py-2 bg-port-bg border border-port-border roun
*/
function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, onQueued }) {
const codeReviewDefaults = useCodeReviewDefaults();
// Resolved model lists for the reviewer table's Model column (the picker never
// fetches — see its `modelOptions` prop).
const reviewerModelOptions = useReviewerModelOptions();
const {
providers, selectedProviderId, selectedModel, availableModels, selectedProvider,
setSelectedProviderId, setSelectedModel
Expand All @@ -41,7 +46,13 @@ function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, o
// Seeded from the install's Code Review Defaults for display. `reviewDirty`
// gates whether they're SENT — see the component doc.
const [review, setReview] = useState(null);
const reviewValue = review ?? codeReviewDefaults;
// The defaults carry per-reviewer models as `<reviewer>Model` scalars; the picker
// takes the token-keyed map, so fold them in for the seeded (untouched) display.
const seededReview = useMemo(
() => ({ ...codeReviewDefaults, reviewerModels: reviewerModelsFromDefaults(codeReviewDefaults) }),
[codeReviewDefaults]
);
const reviewValue = review ?? seededReview;

const [work, setWork] = useState({ mode: 'auto', target: '', issueAuthorFilter: '' });
const [submitting, setSubmitting] = useState(false);
Expand All @@ -67,11 +78,15 @@ function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, o
model: selectedModel || undefined,
effort: effort || undefined,
simplify,
...(review ? {
// `isNext &&` states the route's contract rather than leaning on the picker
// being unrendered: only the `next` branch of POST /tasks/slashdo reads these,
// so sending them for another command would be a silent no-op.
...(isNext && review ? {
reviewers: review.reviewers,
usernames: review.usernames,
optionalReviewers: review.optionalReviewers,
reviewerMaxRounds: review.reviewerMaxRounds
reviewerMaxRounds: review.reviewerMaxRounds,
reviewerModels: review.reviewerModels
} : {})
}, { silent: true }).catch((err) => {
setSubmitError(err.message || 'Failed to queue the task');
Expand Down Expand Up @@ -130,21 +145,33 @@ function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, o
Simplify before committing
</span>
</label>
<ReviewerPicker
reviewers={reviewValue.reviewers}
usernames={reviewValue.usernames}
optionalReviewers={reviewValue.optionalReviewers}
reviewerMaxRounds={reviewValue.reviewerMaxRounds}
// The claim flows substitute a reviewer CSV into their prompt and have
// no slashdo flag string, so stop-mode / reviewer-applies can't be honored.
showRunFlags={false}
onChange={({ reviewers, usernames, optionalReviewers, reviewerMaxRounds }) =>
setReview({ reviewers, usernames, optionalReviewers, reviewerMaxRounds })}
/>
<p className="text-xs text-gray-500">
The claim flow opens and merges its own PR, so these reviewers gate that merge (slashdo <code>--review-with</code>).
{!review && ' Leave them untouched to use this app’s configured reviewers.'}
</p>
{/* Reviewer choices are `/do:next`-only: they're substituted into the
claim prompt's reviewer CSV, and `POST /tasks/slashdo` reads them only
on that branch. Every other `/do:*` body owns its own review/PR
sequence, so rendering the picker there would be four knobs wired to
nothing — the user would pick a reviewer and a model and the run would
silently discard both. */}
{isNext && (
<>
<ReviewerPicker
reviewers={reviewValue.reviewers}
usernames={reviewValue.usernames}
optionalReviewers={reviewValue.optionalReviewers}
reviewerMaxRounds={reviewValue.reviewerMaxRounds}
reviewerModels={reviewValue.reviewerModels}
modelOptions={reviewerModelOptions}
// The claim flows substitute a reviewer CSV into their prompt and have
// no slashdo flag string, so stop-mode / reviewer-applies can't be honored.
showRunFlags={false}
onChange={({ reviewers, usernames, optionalReviewers, reviewerMaxRounds, reviewerModels }) =>
setReview({ reviewers, usernames, optionalReviewers, reviewerMaxRounds, reviewerModels })}
/>
<p className="text-xs text-gray-500">
The claim flow opens and merges its own PR, so these reviewers gate that merge (slashdo <code>--review-with</code>).
{!review && ' Leave them untouched to use this app’s configured reviewers.'}
</p>
</>
)}
</section>

{submitError && <p className="text-xs text-port-error">{submitError}</p>}
Expand Down
28 changes: 28 additions & 0 deletions client/src/components/apps/SlashDoRunDrawer.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import SlashDoRunDrawer from './SlashDoRunDrawer';
const api = vi.hoisted(() => ({
getCodeReviewDefaults: vi.fn(),
getProviders: vi.fn(),
// Backs the reviewer table's Model column (useReviewerModelOptions).
getLocalLlmStatus: vi.fn(),
getAppWorkItems: vi.fn(),
createSlashdoTask: vi.fn()
}));
Expand All @@ -30,6 +32,7 @@ describe('SlashDoRunDrawer', () => {
vi.clearAllMocks();
api.getCodeReviewDefaults.mockResolvedValue({ reviewers: ['copilot'], usernames: [], optionalReviewers: [] });
api.getProviders.mockResolvedValue({ providers: [] });
api.getLocalLlmStatus.mockResolvedValue({ ollama: { models: [] }, lmstudio: { models: [] } });
api.getAppWorkItems.mockResolvedValue({
tracker: 'github',
issueAuthorFilter: 'self',
Expand Down Expand Up @@ -83,6 +86,31 @@ describe('SlashDoRunDrawer', () => {
expect(screen.queryByText(/Reviewer applies fixes/)).not.toBeInTheDocument();
});

it('hides the reviewer picker entirely for a command that cannot honor it', async () => {
// Only the `next` branch of POST /tasks/slashdo reads the reviewer fields —
// every other `/do:*` body owns its own review/PR sequence. Rendering the
// picker there would let the user pick a reviewer and a model and have the run
// silently discard both.
renderDrawer({ command: 'better', label: '/do:better' });

await waitFor(() => expect(screen.getByText('Task settings')).toBeInTheDocument());
expect(screen.queryByText('Reviewers (in order):')).not.toBeInTheDocument();
expect(screen.queryByText('GitHub reviewers (gate merge):')).not.toBeInTheDocument();
});

it('never sends reviewer fields for a non-next command', async () => {
renderDrawer({ command: 'better', label: '/do:better' });

await waitFor(() => expect(screen.getByText('Task settings')).toBeInTheDocument());
await userEvent.click(screen.getByRole('button', { name: /Queue/ }));

await waitFor(() => expect(api.createSlashdoTask).toHaveBeenCalled());
const [, , body] = api.createSlashdoTask.mock.calls[0];
expect(body.reviewers).toBeUndefined();
expect(body.reviewerModels).toBeUndefined();
expect(body.reviewerMaxRounds).toBeUndefined();
});

it('lists the tracker items once "pick a specific" is chosen and sends the pinned ref', async () => {
const onQueued = vi.fn();
renderDrawer({ onQueued });
Expand Down
Loading