feat: Blocker Resolution View - Answer Agent Questions#390
Conversation
Add dedicated /blockers page with inline answer forms, real-time polling, and sidebar notification badge for open blocker count.
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds a Blocker Resolution feature: new page and client components for viewing/answering blockers, SWR-backed API client method, sidebar badge for open blockers, relative-time util, tests, and a minor icon mock + types path update. Changes
Sequence DiagramsequenceDiagram
actor User
participant BlockerPage as BlockersPage
participant BlockerResView as BlockerResolutionView
participant BlockerCard as BlockerCard
participant SWR as SWR Cache
participant API as blockersApi
User->>BlockerPage: Navigate to /blockers
BlockerPage->>BlockerResView: Render(workspacePath)
BlockerResView->>SWR: useSWR(/api/v2/blockers?path=...)
SWR->>API: getAll(workspacePath)
API-->>SWR: BlockerListResponse
SWR-->>BlockerResView: data, mutate
BlockerResView->>BlockerResView: derive openBlockers/resolvedBlockers
BlockerResView->>BlockerCard: Render each open blocker
User->>BlockerCard: Enter answer & submit
BlockerCard->>API: answer(workspacePath, blockerId, text)
API-->>BlockerCard: Success
BlockerCard->>BlockerResView: onAnswered()
BlockerResView->>SWR: mutate() to refetch
SWR->>API: getAll(workspacePath)
API-->>SWR: Updated BlockerListResponse
SWR-->>BlockerResView: Updated data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Add Blocker Resolution View to /blockers and enable sidebar Blockers link with live OPEN count to support answering agent questionsIntroduce a blockers dashboard that fetches blockers via 📍Where to StartStart with the top-level view in page.tsx, then review data flow and fetching in BlockerResolutionView.tsx. Macroscope summarized 492f44e. |
PR Review: Blocker Resolution View (#333)This is clean, well-structured frontend work. The component decomposition is logical, accessibility attributes are in place, and the SWR polling approach is appropriate. A few issues to address: Bug:
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/__tests__/components/blockers/ResolvedBlockersSection.test.tsx (1)
91-142: Consider adding a test for blockers without an answer.The component conditionally renders the answer block (
{blocker.answer && ...}), but there's no explicit test verifying behavior whenanswerisnullorundefined. This edge case could be worth covering to ensure the component handles resolved blockers that might not have an associated answer.🧪 Suggested test case
it('renders without answer section when answer is null', async () => { const user = userEvent.setup(); const blocker = makeBlocker({ answer: null }); render(<ResolvedBlockersSection blockers={[blocker]} />); await user.click(screen.getByRole('button')); expect(screen.getByText('Which database should we use?')).toBeInTheDocument(); expect(screen.queryByText('Use PostgreSQL for persistence.')).not.toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/__tests__/components/blockers/ResolvedBlockersSection.test.tsx` around lines 91 - 142, Add a unit test that verifies ResolvedBlockersSection handles blockers with no answer: create a blocker via makeBlocker({ answer: null }), render <ResolvedBlockersSection blockers={[blocker]} />, expand the section (user.click on the role button) and assert the question text is present while the expected answer text is absent (use screen.getByText for the question and screen.queryByText for the answer). Name the test something like 'renders without answer section when answer is null' to mirror other cases and keep assertions consistent with existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/__tests__/components/blockers/ResolvedBlockersSection.test.tsx`:
- Around line 91-142: Add a unit test that verifies ResolvedBlockersSection
handles blockers with no answer: create a blocker via makeBlocker({ answer: null
}), render <ResolvedBlockersSection blockers={[blocker]} />, expand the section
(user.click on the role button) and assert the question text is present while
the expected answer text is absent (use screen.getByText for the question and
screen.queryByText for the answer). Name the test something like 'renders
without answer section when answer is null' to mirror other cases and keep
assertions consistent with existing tests.
- Extract formatRelativeTime to shared lib/format.ts (dedup) - Add "Show form" button when Skip collapses answer form - Fix SWR cache invalidation for sidebar badge after answering - Use 'done' badge variant for all resolved-section blockers
Follow-up ReviewMy previous review contained several incorrect observations that I should correct now that I've read the diff more carefully. Corrections to my previous review:
Apologies for the noise in the previous review. Issues from the previous review that still stand: Fragile badge logic in {label === 'Blockers' && openBlockerCount > 0 && (
<span ...>{openBlockerCount}</span>
)}This ties badge rendering to a hardcoded display string. If the label is ever renamed or internationalized, the badge silently breaks. An optional Missing tests for New issues identified in this pass:
setTimeout(() => onAnswered(), 1500);If the component unmounts (e.g., the page navigates away) before the 1.5 s delay, this fires on an unmounted component. Store the timeout ID and cancel it in a Unnecessary wrapper function on onAnswered={() => revalidateAll()}
const apiErr = err as ApiError;
setError(apiErr.detail || 'Failed to submit answer');If the error is a The core implementation is solid. The two actionable items worth resolving before merge are the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/src/lib/format.ts (1)
5-16: Consider adding defensive handling for invalid date inputs.If
isoDateis malformed or empty,new Date(isoDate)produces an Invalid Date, causinggetTime()to returnNaN. This propagates through the arithmetic and results in"NaNd ago"being displayed.Since this is a shared utility, consider guarding against this edge case:
🛡️ Proposed defensive fix
export function formatRelativeTime(isoDate: string): string { const now = new Date(); const date = new Date(isoDate); + if (isNaN(date.getTime())) { + return ''; + } const diffMs = now.getTime() - date.getTime();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/lib/format.ts` around lines 5 - 16, The formatRelativeTime function can produce "NaNd ago" for invalid isoDate inputs; update formatRelativeTime to defensively handle malformed or empty isoDate by validating the parsed Date (e.g., checking isNaN(date.getTime()) or Date.parse result) before using now/getTime/diffMs, and return a safe fallback (empty string or a clear placeholder like "Unknown date" or "just now") when the input is invalid; ensure the check occurs immediately after creating date in formatRelativeTime and short-circuit the rest of the arithmetic if invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/src/lib/format.ts`:
- Around line 5-16: The formatRelativeTime function can produce "NaNd ago" for
invalid isoDate inputs; update formatRelativeTime to defensively handle
malformed or empty isoDate by validating the parsed Date (e.g., checking
isNaN(date.getTime()) or Date.parse result) before using now/getTime/diffMs, and
return a safe fallback (empty string or a clear placeholder like "Unknown date"
or "just now") when the input is invalid; ensure the check occurs immediately
after creating date in formatRelativeTime and short-circuit the rest of the
arithmetic if invalid.
- Clean up setTimeout on unmount in BlockerCard (memory leak fix) - Pass revalidateAll directly instead of wrapping in arrow function - Add 15 BlockerCard tests (submit flow, error, skip/expand, a11y)
Review: Blocker Resolution View (#390)This is a well-structured implementation that integrates cleanly with the existing UI patterns. The component decomposition, SWR polling strategy, accessibility attributes, and test coverage are all solid. Below are observations that build on the prior CodeRabbit reviews. Carry-forward from prior reviews (not yet addressed)
Additional observations
const badgeVariant = 'done' as const;
// …
<Badge variant={badgeVariant}>{blocker.status}</Badge>
const openBlockerCount = blockerData?.by_status?.OPEN ?? 0;This works correctly given the
The SWR keys embed the raw workspace path string. Workspace paths can contain spaces and special characters. Using the raw path as a key doesn't cause a runtime error (SWR keys are just identifiers), but it's inconsistent with how the fetcher constructs the actual HTTP request via axios params. This is fine as-is, but worth noting in case path-based SWR key collisions become a problem.
The card contains a read-only answer display for non-OPEN blockers: {!isOpen && blocker.answer && (
<div className="rounded-md border bg-muted/50 p-3">…</div>
)}Since No tests for Both are covered indirectly, but:
SummaryThe core functionality is well implemented and follows the project patterns. The two items already raised by CodeRabbit (invalid date guard and null-answer test) are the most actionable fixes. The |
| try { | ||
| await blockersApi.answer(workspacePath, blocker.id, answer.trim()); | ||
| setSubmitted(true); | ||
| timerRef.current = setTimeout(() => onAnswered(), 1500); |
There was a problem hiding this comment.
🟢 Low
blockers/BlockerCard.tsx:42 Consider adding an isMounted ref to guard against state updates and timer scheduling after unmount. The cleanup effect only clears the timer that exists at unmount time, not timers created after the async call resolves post-unmount.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file web-ui/src/components/blockers/BlockerCard.tsx around line 42:
Consider adding an `isMounted` ref to guard against state updates and timer scheduling after unmount. The cleanup effect only clears the timer that exists at unmount time, not timers created after the async call resolves post-unmount.
Evidence trail:
Viewed `web-ui/src/components/blockers/BlockerCard.tsx` lines 22-70 and 45-60 at commit `492f44e` (cleanup effect and async `handleSubmit` with `setTimeout`).
| }, [mutate, globalMutate, workspacePath]); | ||
|
|
||
| const openBlockers = useMemo( | ||
| () => data?.blockers.filter((b) => b.status === 'OPEN') ?? [], |
There was a problem hiding this comment.
🟠 High
blockers/BlockerResolutionView.tsx:32 data?.blockers.filter(...) crashes when data is undefined because optional chaining doesn't extend to .filter. Consider data?.blockers?.filter(...) instead.
| () => data?.blockers.filter((b) => b.status === 'OPEN') ?? [], | |
| () => data?.blockers?.filter((b) => b.status === 'OPEN') ?? [], |
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file web-ui/src/components/blockers/BlockerResolutionView.tsx around line 32:
`data?.blockers.filter(...)` crashes when `data` is undefined because optional chaining doesn't extend to `.filter`. Consider `data?.blockers?.filter(...)` instead.
Evidence trail:
Viewed `web-ui/src/components/blockers/BlockerResolutionView.tsx` lines ~29-38 at commit `492f44e`.
1 participant
Summary
Implements #333: Blocker Resolution View — a dedicated page for responding to agent questions and unblocking stuck tasks.
/blockersroute with inline answer forms and real-time pollingChanges
New Files
web-ui/src/app/blockers/page.tsx— Page route following existing workspace patternweb-ui/src/components/blockers/BlockerResolutionView.tsx— Main view with SWR polling (5s interval)web-ui/src/components/blockers/BlockerCard.tsx— Individual blocker card with inline answer formweb-ui/src/components/blockers/ResolvedBlockersSection.tsx— Collapsible resolved blockersModified Files
web-ui/src/lib/api.ts— AddedblockersApi.getAll()for listing blockers with optional filtersweb-ui/src/components/layout/AppSidebar.tsx— Enabled blockers route + badge count with SWR pollingTest Files
web-ui/__tests__/components/blockers/ResolvedBlockersSection.test.tsx— 14 tests for collapsible sectionAppSidebar.test.tsxto reflect enabled blockers routeAcceptance Criteria
Test Plan
next build— zero TypeScript errors)Closes #333
Summary by CodeRabbit
New Features
Tests