feat(dashboard): add manual trigger & retry run actions to web dashboard#323
Conversation
|
🔍 Reviewing PR... |
aaight
left a comment
There was a problem hiding this comment.
Summary
Clean, well-tested feature addition. The backend mutations, authorization checks, tests, and frontend integration all follow established patterns. One UX gap worth addressing.
Should Fix
- RetryRunButton has no error feedback: When
retryMutationfails, the button simply re-enables with no indication of what went wrong. BothTriggerRunDialog(line 178-183) andDebugAnalysis(lines 73-79, 106-112) displaytriggerMutation.isErrormessages. TheRetryRunButtonshould follow suit — either render an inline error message or show a toast. Without this, users click Retry, it fails silently, and they have no signal to act on.
| title="Retry run" | ||
| > | ||
| <RefreshCw className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
Missing error feedback: unlike TriggerRunDialog and DebugAnalysis which both render triggerMutation.isError messages, this component silently swallows mutation errors. Consider adding an error indicator (tooltip, inline message, or toast) so users know when retry fails. A minimal approach:
{retryMutation.isError && (
<span className="text-xs text-destructive" title={retryMutation.error?.message}>
Failed
</span>
)}Or use the installed sonner toast library for a non-intrusive notification.
There was a problem hiding this comment.
Addressed — added inline error feedback to RetryRunButton. When the mutation fails, a "Failed" text now appears next to the button with the error message as a tooltip (matching the pattern used in TriggerRunDialog and DebugAnalysis).
|
👀 Checking this out... |
|
@aaight address code review concerns |
|
🔍 Reviewing PR... |
zbigniewsobiecki
left a comment
There was a problem hiding this comment.
Code Review: feat(dashboard) — manual trigger & retry run actions
Overall this is a well-structured, clean addition. CI is green, tests are thorough, and the implementation follows established patterns in the codebase. I have a few observations below.
✅ Strengths
- Test coverage is excellent. 14 new unit tests cover every happy path and every error code (
NOT_FOUND,BAD_REQUEST,UNAUTHORIZED). Mocking pattern is consistent with the existingtriggerDebugAnalysissuite — good. e.preventDefault()+e.stopPropagation()inRetryRunButtoncorrectly prevents row-click navigation when the button is inside a<Link>-wrapped row.colSpanfix (8 → 9) is correctly updated to match the new "Actions" column.- Form reset on success in
TriggerRunDialogis handled properly insideonSuccess. _nonesentinel value pattern in the<Select>components is a pragmatic workaround for Radix not accepting empty strings — works fine here.queryClient.invalidateQueriesinonSuccessis correct in both components; the runs list will refresh after a trigger or retry.
🟡 Minor Issues / Suggestions
1. RetryRunButton silently swallows mutation errors (also flagged as an inline comment)
TriggerRunDialog and DebugAnalysis both render error feedback when triggerMutation.isError is true. RetryRunButton has no equivalent — if the retry API call fails (e.g., a 500 or NOT_FOUND), the user sees nothing. Consider either:
- Adding an inline
<span className="text-xs text-destructive">Failed</span>next to the button, or - Using the
sonnertoast library (already installed) for a non-intrusive notification.
2. agentTypes list in TriggerRunDialog is a hardcoded client-side constant
The list (briefing, planning, implementation, review, debug, respond-to-review, respond-to-pr-comment) duplicates what the backend schema knows. If a new agent type is added server-side, this list silently goes stale. This is acceptable for now given the small surface area, but worth a comment:
// Keep in sync with AgentType union in src/triggers/types.ts (or wherever the canonical list lives)
const agentTypes = [ ... ];3. status prop on RetryRunButton is typed as string instead of a union
The component hides itself for status === 'running' but the prop type is string. Using the actual union ('running' | 'completed' | 'failed' | 'cancelled' | ...) would make the behaviour self-documenting and enable exhaustiveness checks. Low priority since Run.status in runs-table.tsx is also string, but worth aligning if that type gets tightened.
4. TriggerRunDialog does not reset state when the dialog is closed without submitting
If a user fills in the form, clicks Cancel (or presses Escape), then reopens the dialog, all previously entered values are still there. This might be intentional (preserving a partially-filled form can be convenient), but it's worth a conscious decision. The onSuccess path already resets; an onOpenChange → close path does not.
5. RetryRunButton invalidates trpc.runs.getById even when rendered in the table context
When used in runs-table.tsx, the getById query for runId is almost certainly not mounted, so the invalidation is harmless but unnecessary. Not a bug — just a minor observation.
✅ Nothing blocking
No security issues, no data races, no broken types caught in CI. LGTM with the suggestions above as non-blocking follow-ups.
|
@aaight address code review concerns |
|
👀 Checking this out... |
|
🔍 Reviewing PR... |
aaight
left a comment
There was a problem hiding this comment.
Solid PR — clean feature addition with good test coverage. A few observations:
What looks good:
- Tests are thorough: 14 new test cases covering happy paths, auth, org-isolation, and edge cases for both
triggerandretrymutations - Fire-and-forget pattern is correct with
.catch()error logging, consistent with the existingtriggerDebugAnalysispattern RetryRunButtonproperly hides forrunningstatus and usese.stopPropagation()to avoid row-click conflicts in the table- Form validation disabling submit when
!projectId || !agentTypeis a nice touch colSpancorrectly updated from 8 → 9 in the empty-state row
Minor suggestions (non-blocking):
-
RetryRunButton— no error feedback: The mutation has noonErrorhandler. If the retry fails, the button just stops spinning with no indication of what happened. Consider adding a toast/alert or at minimum anonErrorcallback similar to howTriggerRunDialogshowstriggerMutation.error.message. -
TriggerRunDialog—_nonesentinel value: The pattern of usingvalue="_none"as a placeholder for the Select and filtering it back to''works but is a little fragile. If Radix ever changes empty-value behavior, this could break. Not a blocker, but worth a// Radix Select requires non-empty valuecomment for future readers. -
trigger-run-dialog.tsx— Cancel button isn't using theButtoncomponent: The "Cancel" button (line ~170) uses a raw<button>with hand-rolled classes while "Trigger Run" uses the<Button>component. Consider using<Button variant="outline">for consistency. -
agentTypeshardcoded list: The agent types array intrigger-run-dialog.tsxis a static list. If a new agent type is added, this list will need a manual update. Consider whether it's worth fetching available types from the API or at least co-locating the list with a shared constant. -
No loading/error state for projects query: If
projectsQueryfails or is loading, the project Select just shows no options. A loading indicator or error message in the dropdown would improve UX.
None of these are blockers — the implementation is clean, well-tested, and follows existing patterns. Ship it! 🚀
|
@aaight address code review concerns and suggestions |
Request AddressedWhat was askedAddress all code review concerns and suggestions from both reviewers. Changes Made
Verification
|
- Add inline error feedback to RetryRunButton (shows "Failed" with error tooltip) - Reset TriggerRunDialog form state when closed without submitting - Replace raw Cancel <button> with <Button variant="outline"> for consistency - Add loading/error states for projects query in TriggerRunDialog select - Add "Keep in sync" comment for hardcoded agentTypes list - Add "Radix Select requires non-empty value" comments for _none sentinel pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
👀 Checking this out... |
|
Addressed all code review concerns and suggestions from both reviewers. Here's what changed: 1. RetryRunButton — error feedback (flagged by both reviewers) 2. TriggerRunDialog — form reset on close (@zbigniewsobiecki #4) 3. Cancel button consistency (@aaight #3) 4. Projects loading/error state (@aaight #5) 5. 6. All checks pass: lint ✅ typecheck ✅ tests (1518/1518) ✅ |
|
🔍 Reviewing PR... |
zbigniewsobiecki
left a comment
There was a problem hiding this comment.
Review: feat(dashboard): add manual trigger & retry run actions to web dashboard
This is a well-structured and complete implementation. The PR adds exactly what it says with clean code, proper error handling, and solid test coverage.
✅ What's done well
retry-run-button.tsx
- Correct use of
useMutationwithonSuccessinvalidation of bothruns.listandruns.getByIdqueries. e.stopPropagation()in the click handler is a nice touch to prevent accidentally navigating to the run detail when the button is used inline in the table.- Inline error feedback (
retryMutation.isError→"Failed"span with tooltip) matches the pattern used inTriggerRunDialogandDebugAnalysis— consistent UX. status === 'running'guard returnsnullas documented in the prop comment.
trigger-run-dialog.tsx
- The
_nonesentinel approach for RadixSelectis clearly documented and correctly handles the round-trip (setProjectId(v === '_none' ? '' : v)). resetFormis memoized withuseCallbackand wired tohandleOpenChange, so state doesn't leak between dialog opens.- Loading/error states for the projects query are surfaced inline in the dropdown — good defensive UX.
- Submit button correctly disables while pending and when required fields (
projectId,agentType) are empty.
runs-table.tsx
colSpanupdated from 8 → 9 to match the new "Actions" column. ✓
src/api/routers/runs.ts
trigger: org ownership check via DB query before invoking the runner — correct authorization order.retry: theprojectIdnull guard is placed after the org check (which short-circuits on a missing/wrong-org project), then rechecks for the null case — clean guard ordering consistent withtriggerDebugAnalysis.- Both mutations are true fire-and-forget with
.catch()error logging — consistent with existingtriggerDebugAnalysispattern.
Tests (runs.test.ts)
- 14 new test cases covering both happy paths and all error branches (UNAUTHORIZED, NOT_FOUND × multiple, BAD_REQUEST, model override passthrough).
- Mocks reset in
beforeEachwithmockReturnValue(Promise.resolve())— prevents test bleed. expect.objectContaining(...)used for partial argument assertions where appropriate.- The
retryBAD_REQUEST test (projectId: null) correctly omitsmockDbWheresetup, since the router short-circuits before reaching that check.
Minor observations (non-blocking)
-
agentTypesarray intrigger-run-dialog.tsxincludesdebug— ThetriggerDebugAnalysisendpoint blocks debug-on-debug with aBAD_REQUEST; thetriggermutation doesn't have the same guard. If manually triggering adebugagent is intentional, this is fine — just worth confirmingtriggerManualRunhandles thedebugtype gracefully downstream. -
retryorg check ordering —if (run.projectId) { /* DB org check */ }runs beforeif (!run.projectId) { throw BAD_REQUEST }. A run withprojectId: nullowned by another org will reach theBAD_REQUESTbranch without an org check, which leaks its existence. The behaviour is safe (all callers areprotectedProcedure), and it matches thetriggerDebugAnalysispattern — just something to be aware of. -
triggerinput —agentTypeisz.string()— The frontend constrains this to the known list, but a direct API call could pass an arbitrary string. Addingz.enum([...agentTypes])(or a shared import fromsrc/types) would make the contract explicit and catch bad inputs at the tRPC layer.
Summary
All CI checks pass. 14 new test cases alongside the existing 30. Code is clean, patterns are consistent across new and existing routers, and the PR description accurately reflects what's implemented.
|
🔍 Reviewing PR... |
aaight
left a comment
There was a problem hiding this comment.
Summary
Clean implementation. The UI components are well-structured, tests are thorough, and the backend router properly validates org access and handles errors. One maintainability issue worth noting.
Should Fix
- web/src/components/runs/trigger-run-dialog.tsx:18-26 — The hardcoded
agentTypesarray is missingrespond-to-ciandrespond-to-planning-commentthat exist in the canonical list atsrc/agents/prompts/index.ts:13-23. The comment says "Keep in sync with AgentType in src/types/index.ts" butsrc/types/index.tsdoesn't actually define this list — the closest canonical source is in prompts/index.ts. Since the backendtriggermutation accepts anyz.string()foragentType(no enum validation), the dialog will silently exclude valid agent types. Consider importing or sharing this list to avoid manual sync, or at minimum fix the missing entries and update the comment to reference the actual source file.
| 'respond-to-review', | ||
| 'respond-to-pr-comment', | ||
| ]; | ||
|
|
There was a problem hiding this comment.
This list is missing respond-to-ci and respond-to-planning-comment compared to the canonical list in src/agents/prompts/index.ts. The comment references src/types/index.ts but that file doesn't define an agent type list — the actual source of truth is src/agents/prompts/index.ts:13-23.
Since the server accepts any string for agentType, users can't trigger these agent types from the UI. Consider importing from a shared constant to avoid this drift.
|
👀 Checking this out... |
|
🔍 Reviewing PR... |
zbigniewsobiecki
left a comment
There was a problem hiding this comment.
Code Review
Overall this is a solid feature addition — the component structure is clean, error handling is consistent, the mutation patterns follow the existing codebase conventions, and the 14 new unit tests provide good coverage of both happy paths and error cases. CI is fully green. A few notes below.
✅ What's well done
- Error feedback in
RetryRunButton— TheisError/Failedindicator with tooltip is present and consistent with the pattern used inTriggerRunDialogandDebugAnalysis. e.preventDefault()+e.stopPropagation()in the retry button'sonClickis correct: prevents accidental row navigation in the table._nonesentinel for Radix Select unselected state is well-documented with comments.- Form reset on dialog close via
handleOpenChange+resetFormis clean. - DB-level org check before triggering (both
triggerandretry) is correctly applied and mirrored in the tests. - Test coverage: all major error branches (NOT_FOUND, BAD_REQUEST, UNAUTHORIZED, org mismatch) are covered for both mutations.
🔴 Open issue: agentTypes list is out of sync
web/src/components/runs/trigger-run-dialog.tsx line 18–25:
// Keep in sync with AgentType in src/types/index.ts
const agentTypes = [
'briefing', 'planning', 'implementation', 'review', 'debug',
'respond-to-review', 'respond-to-pr-comment',
];Two agent types are missing compared to the canonical list in src/agents/prompts/index.ts (lines 13–23):
respond-to-cirespond-to-planning-comment
The comment also references the wrong source file — src/types/index.ts does not define an agent type list. The source of truth is src/agents/prompts/index.ts.
Users cannot trigger these two agent types from the UI in the current state. Suggested fix: add the missing types and update the comment.
🟡 Minor: status prop could be a typed union
In RetryRunButton:
interface RetryRunButtonProps {
runId: string;
/** Hide button when status is 'running' */
status: string;
}Since the component only branches on 'running', a string type is acceptable, but a union type (e.g., 'running' | 'completed' | 'failed' | 'pending') would make the intent clearer and catch potential typos at the call site. Low priority since the Run interface in runs-table.tsx already uses status: string.
🟡 Minor: trigger tests don't cover loadConfig failure path
The trigger describe block has a test for findProjectById returning undefined (→ NOT_FOUND), but no test for loadConfig throwing or returning undefined. The retry tests have the same gap. Likely low-risk given the router probably propagates the error as-is, but explicit coverage would be consistent with the triggerDebugAnalysis suite style.
Summary
The blocking item is the missing respond-to-ci and respond-to-planning-comment agent types in the trigger dialog. Everything else looks good and can be addressed as follow-ups.
Summary
runs.triggerandruns.retrytRPC mutations (14 new test cases)Changes
web/src/components/runs/trigger-run-dialog.tsx(new) — Dialog form wired totrpcClient.runs.trigger.mutate()web/src/components/runs/retry-run-button.tsx(new) — Button withRefreshCwicon wired totrpcClient.runs.retry.mutate()web/src/routes/index.tsx— "Trigger Run" button + dialog in runs list page headerweb/src/routes/runs/$runId.tsx—RetryRunButtonrendered next toRunStatusBadgein the headerweb/src/components/runs/runs-table.tsx— "Actions" column withRetryRunButtonper rowtests/unit/api/routers/runs.test.ts— Tests fortrigger(valid trigger, NOT_FOUND, org check, config not found, UNAUTHORIZED) andretry(valid retry, model override, NOT_FOUND, org check, BAD_REQUEST for no projectId, UNAUTHORIZED)Test plan
npm test)npm run typecheck)npm run lint)Card
https://trello.com/c/Up8v6d1O/21-add-manual-trigger-retry-run-actions-to-web-dashboard
🤖 Generated with Claude Code