feat(web): wire the triage frontend (Review + Maintenance) to the real backend - #17
Conversation
Covers the new issue-summary vertical slice (migration, wiring, My Work join) and the backfill/Sync fallout from moving PR summaries to plain prose (structured-format skip check breaks, needs redefining; Sync gets a second progress bar for issues). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
10-task TDD plan covering the issue_summaries vertical slice, the PR prompt rewrite, and the backfill/Sync fallout (skip-check redefinition, issue-summary loop, shared AI-call budget, two-bar progress UI). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…confidence proposals
…dentity confirm guard
…oot-time badge loads
…data Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…codec guard Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
canopy | 5b84317 | Commit Preview URL Branch Preview URL |
Jul 04 2026, 08:16 PM |
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThis PR wires the triage frontend to backend-backed reads and writes, removes the mock triage module, adds shared mapping/render helpers, and updates Maintenance identity and assignment flows. It also adds planning/spec documentation for a separate worker issue-summarization feature. ChangesFrontend triage wire-up
Worker summarizer planning docs
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
web/src/api.ts (1)
178-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten
event_typeto a literal union.The comment documents
event_typeas'pr_merged' | 'pr_closed' | 'issue', but the field is typed as plainstring. SinceidentityFromTaskintriage-map.tsbranches on this value (s.event_type === "issue" ? "ISSUE" : "PR"), a typo or backend drift wouldn't be caught by the compiler.♻️ Suggested tightening
export interface IdentitySample { semantic_key: string; - event_type: string; // 'pr_merged' | 'pr_closed' | 'issue' + event_type: "pr_merged" | "pr_closed" | "issue"; ref_number: number; title: string | null; // null when the event's raw snapshot is malformed occurred_at: string | null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/api.ts` around lines 178 - 183, The `event_type` field in the `semantic_key` shape is too loose and should be narrowed from `string` to the documented literal union so TypeScript can catch invalid values earlier. Update the type definition near `semantic_key` to use the exact `'pr_merged' | 'pr_closed' | 'issue'` union, and ensure any dependent logic such as `identityFromTask` in `triage-map.ts` still compiles against the tightened `event_type` contract.web/src/review.ts (1)
167-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRendered preview silently drops collapsed context — no visual gap indicator.
Unlike
unifiedDiff/splitDiffRows, which render a "N unchanged lines" marker for collapsed runs,renderedPreviewfiltersellipsisrows out entirely. Since the entries here already have unchanged context trimmed to a ±3-line window, this can make the rendered prose look like disconnected fragments with no cue that content was omitted.Consider inserting a subtle divider (e.g., a horizontal rule) in place of dropped
ellipsisrows for readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/review.ts` around lines 167 - 183, The renderedPreview() output is dropping ellipsis rows entirely, so collapsed unchanged context has no visible cue and the preview can look like disconnected fragments. Update renderedPreview() to handle DiffEntry.t === "ellipsis" with a subtle separator or divider instead of filtering it out, similar to how unifiedDiff and splitDiffRows preserve collapsed context. Keep the change localized to renderedPreview and preserve the existing styling for h, add, and del entries.web/src/diff.ts (1)
9-53: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider memoizing/lazily computing diffs — recomputed per item, per render.
lineDiffis O(n·m) time and space (full DP table). Per the downstream context (triage-map.ts'sproposalReviewItem/diffEntries), this runs for every proposal on every call toreviewItemsFromReads, whichreviewPropsinvokes on every render of the review screen — not just for the currently selected/detail item. For larger doc bodies or many pending proposals, this recomputation on every re-render (filter toggle, selection change, etc.) is wasted work, since only one item's diff is actually displayed at a time.Consider caching diff results keyed by
(oldText, newText)at the call site, or computing the diff lazily only for the selected item.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/diff.ts` around lines 9 - 53, The diff generation is being recomputed too often: `lineDiff`/`collapsedLineDiff` are expensive O(n·m) operations and are currently triggered for every proposal on each `reviewProps` render via `reviewItemsFromReads`/`proposalReviewItem`/`diffEntries`. Move the work to a lazy path or add memoization at the call site so the diff is only computed when needed, ideally keyed by the `(oldText, newText)` pair or only for the selected item. Keep the existing `lineDiff` and `collapsedLineDiff` logic intact, but avoid invoking them for items that are not being displayed.web/src/main.ts (1)
201-264: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLoaders have no staleness guard against out-of-order responses.
Each
loadX()/loadXIfNeeded()pair always kicks off a new fetch and unconditionally overwritesstate.Xwhen the promise settles, with no sequence/request-id check. This is called directly (not just viaIfNeeded) from several write-completion handlers — e.g.loadProposals()/loadDraftAdrs()in the verdict handler (Lines 416-417),loadNeedsTriage()/loadProposals()/loadDraftAdrs()/loadFeed()inmaintFile(Lines 516-519), and all four fired concurrently at boot (Lines 643-646). If two triggers for the same slice overlap (e.g. two quick verdicts on different proposals, each refetchingloadProposals()), the earlier request's response can resolve after the later one and silently overwrite the fresher list/error state with stale data. It's self-correcting on the next fetch, but a cheap sequence guard removes the window entirely.🔧 Proposed fix: per-slice sequence guard
+let proposalsSeq = 0; function loadProposals(): void { + const seq = ++proposalsSeq; state.proposals = { status: "loading", data: state.proposals.data }; rerender(); listStagedProposals() - .then((rows) => { state.proposals = { status: "ok", data: rows }; rerender(); }) + .then((rows) => { + if (seq !== proposalsSeq) return; // superseded by a newer request + state.proposals = { status: "ok", data: rows }; + rerender(); + }) .catch((e) => { if (e instanceof Unauthorized) { state.view = "auth"; state.authStep = "login"; rerender(); return; } + if (seq !== proposalsSeq) return; state.proposals = { status: "error", data: [], error: e instanceof Error ? e.message : String(e) }; rerender(); }); }Same pattern for
loadDraftAdrs,loadNeedsTriage,loadIdentityTasks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/main.ts` around lines 201 - 264, The async loaders can overwrite newer state with stale responses because `loadProposals`, `loadDraftAdrs`, `loadNeedsTriage`, and `loadIdentityTasks` have no per-request staleness check. Add a per-slice sequence/request-id guard inside each `loadX()` so only the latest in-flight call may commit to `state.X` after `listStagedProposals`, `listAdrs`, `listNeedsTriage`, or `listIdentityTasks` resolves or rejects. Keep the `loadXIfNeeded()` helpers as-is, but make the promise handlers in these loader functions ignore outdated responses before calling `rerender()`.docs/superpowers/plans/2026-07-04-canopy-frontend-wireup.md (1)
514-527: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSnippet predates the later "tightened" codec — treat as historical, not authoritative.
The
decodeReviewIdshown here validates the version viaNumber.isInteger(Number(...)), which accepts empty/malformed segments (e.g.,"doc:slug@"→Number("")→0). The PR's commit history indicates a follow-up "tightening the codec guard" fix (confirmed by the regex-based/^\d+$/check in the actualweb/src/triage-map.ts). Since this is a static implementation-plan document rather than living documentation, this is very low priority, but worth a one-line callout so future readers don't copy the stale snippet instead of the shipped implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-04-canopy-frontend-wireup.md` around lines 514 - 527, The decodeReviewId snippet is stale and still shows the older Number.isInteger-based version parsing, which can mislead readers into copying the wrong guard. Update the documentation around decodeReviewId to add a brief note that this is historical/non-authoritative and that the shipped implementation uses the tightened codec guard in web/src/triage-map.ts, so future readers know to follow the regex-based validation instead of this example.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-07-04-worker-summarizer-prompts-plan.md`:
- Line 1777: The markdown in the plan doc has bare quoted code fences that
trigger MD040. Update the affected fenced blocks near the quoted prompt examples
to use a language tag such as text on both fences, and make the same change for
the additional quoted fence mentioned in the note. Use the surrounding prompt
sections in the document to locate and label the fences consistently.
- Around line 442-455: `storeIssueSummary` already returns a resolved row
object, so the `resolves.not.toThrow()` matcher is being applied to the wrong
thing in the fallback test. Update the test around `storeIssueSummary(env.DB,
throwingStub, ...)` to await the promise directly, then keep the existing
assertions on the returned row (`model` and `summary`) to verify the fallback
behavior.
In `@web/src/render.ts`:
- Around line 1149-1161: The screen-level gating in reviewScreen and
maintenanceScreen is too broad because it returns a full-page loading/error
notice when either slice is pending or errored, hiding data that already loaded
in the sibling slice. Update these functions so they render the main view from
the successful slice(s) and surface loading/error state more locally, using the
existing reviewView, maintenanceView, reviewProps, maintenanceProps,
slicePending, and the slice status checks rather than short-circuiting the whole
screen.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-07-04-canopy-frontend-wireup.md`:
- Around line 514-527: The decodeReviewId snippet is stale and still shows the
older Number.isInteger-based version parsing, which can mislead readers into
copying the wrong guard. Update the documentation around decodeReviewId to add a
brief note that this is historical/non-authoritative and that the shipped
implementation uses the tightened codec guard in web/src/triage-map.ts, so
future readers know to follow the regex-based validation instead of this
example.
In `@web/src/api.ts`:
- Around line 178-183: The `event_type` field in the `semantic_key` shape is too
loose and should be narrowed from `string` to the documented literal union so
TypeScript can catch invalid values earlier. Update the type definition near
`semantic_key` to use the exact `'pr_merged' | 'pr_closed' | 'issue'` union, and
ensure any dependent logic such as `identityFromTask` in `triage-map.ts` still
compiles against the tightened `event_type` contract.
In `@web/src/diff.ts`:
- Around line 9-53: The diff generation is being recomputed too often:
`lineDiff`/`collapsedLineDiff` are expensive O(n·m) operations and are currently
triggered for every proposal on each `reviewProps` render via
`reviewItemsFromReads`/`proposalReviewItem`/`diffEntries`. Move the work to a
lazy path or add memoization at the call site so the diff is only computed when
needed, ideally keyed by the `(oldText, newText)` pair or only for the selected
item. Keep the existing `lineDiff` and `collapsedLineDiff` logic intact, but
avoid invoking them for items that are not being displayed.
In `@web/src/main.ts`:
- Around line 201-264: The async loaders can overwrite newer state with stale
responses because `loadProposals`, `loadDraftAdrs`, `loadNeedsTriage`, and
`loadIdentityTasks` have no per-request staleness check. Add a per-slice
sequence/request-id guard inside each `loadX()` so only the latest in-flight
call may commit to `state.X` after `listStagedProposals`, `listAdrs`,
`listNeedsTriage`, or `listIdentityTasks` resolves or rejects. Keep the
`loadXIfNeeded()` helpers as-is, but make the promise handlers in these loader
functions ignore outdated responses before calling `rerender()`.
In `@web/src/review.ts`:
- Around line 167-183: The renderedPreview() output is dropping ellipsis rows
entirely, so collapsed unchanged context has no visible cue and the preview can
look like disconnected fragments. Update renderedPreview() to handle DiffEntry.t
=== "ellipsis" with a subtle separator or divider instead of filtering it out,
similar to how unifiedDiff and splitDiffRows preserve collapsed context. Keep
the change localized to renderedPreview and preserve the existing styling for h,
add, and del entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 145680c3-6a88-4262-bd77-a17281c5669c
📒 Files selected for processing (16)
docs/superpowers/plans/2026-07-04-canopy-frontend-wireup.mddocs/superpowers/plans/2026-07-04-worker-summarizer-prompts-plan.mddocs/superpowers/specs/2026-07-04-worker-summarizer-prompts-design.mdtest/render.review.test.tstest/triage-map.test.tstsconfig.web.jsontsconfig.worker.jsonweb/src/api.tsweb/src/diff.tsweb/src/main.tsweb/src/maintenance.tsweb/src/render.tsweb/src/review.tsweb/src/triage-map.tsweb/src/triage-mock.tsweb/src/ui.ts
💤 Files with no reviewable changes (1)
- web/src/triage-mock.ts
| it("falls back to excerptSummary when the summarizer throws, and never throws", async () => { | ||
| const throwingStub: Summarizer = { | ||
| model: "stub", | ||
| summarize: async () => { | ||
| throw new Error("boom"); | ||
| }, | ||
| }; | ||
| await expect( | ||
| storeIssueSummary(env.DB, throwingStub, { issue_number: 3, title: "Third issue", body: "" }) | ||
| ).resolves.not.toThrow(); | ||
| const row = await storeIssueSummary(env.DB, throwingStub, { issue_number: 3, title: "Third issue", body: "" }); | ||
| expect(row.model).toBe("excerpt"); | ||
| expect(row.summary).toBe("Third issue"); // empty body → title | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target snippet and nearby context
sed -n '430,470p' docs/superpowers/plans/2026-07-04-worker-summarizer-prompts-plan.md
# Find the helper and any related tests/usages
rg -n "storeIssueSummary|resolves\.not\.toThrow|toThrow\(" docs/superpowers/plans/2026-07-04-worker-summarizer-prompts-plan.mdRepository: SaplingLearn/canopy
Length of output: 5517
🌐 Web query:
Vitest docs resolves toThrow matcher promise-returning function not.toThrow async assertion
💡 Result:
In Vitest, the toThrow matcher is designed to catch synchronous errors when provided with a function, or to be used in conjunction with the.rejects modifier to handle asynchronous promise rejections [1][2][3]. To test if an asynchronous function or promise throws (rejects) an error, you must use the.rejects modifier [1][3]. You should await the assertion to ensure the test correctly waits for the promise to resolve or reject [1][4]. Example for asserting an async rejection: await expect(myAsyncFunction).rejects.toThrow('error message'); If you want to assert that an asynchronous function does not throw (i.e., it resolves successfully), you can use.resolves [5] or simply await the function call without using.rejects, as a successful resolution of a promise will not trigger an error in the test unless it is caught or rejected [6]. Specifically for.not.toThrow, when combined with.rejects, it asserts that the promise does not reject with an error: await expect(myAsyncFunction).resolves.not.toThrow; Attempting to use toThrow directly on an async function without.rejects will typically fail to catch the error because the promise will not be awaited or unwrapped in the context of the matcher, leading to an unhandled promise rejection rather than a successful assertion [1][7][4]. Always ensure that you are passing either a synchronous function (for direct toThrow) or a promise (for.rejects.toThrow) to avoid common pitfalls [1][7].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/expect.md
- 2: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/docs/api/expect.md
- 3: https://github.com/vitest-dev/vitest/blob/v4.1.7/docs/api/expect.md
- 4:
throwfrom anasyncmethod, exitsvitest watchvitest-dev/vitest#1165 - 5: https://vitest.dev/api/expect.html
- 6: https://vitest.dev/guide/learn/async
- 7: Rejects does not behave as expected - poor uncaught error handling vitest-dev/vitest#4163
Drop the resolves.not.toThrow() assertion
storeIssueSummary(...) resolves to a row object, so this matcher is applied to the wrong value and won’t express the intended check. Await the promise directly and keep the row assertions below.
🔧 Suggested fix
- await expect(
- storeIssueSummary(env.DB, throwingStub, { issue_number: 3, title: "Third issue", body: "" })
- ).resolves.not.toThrow();
const row = await storeIssueSummary(env.DB, throwingStub, { issue_number: 3, title: "Third issue", body: "" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("falls back to excerptSummary when the summarizer throws, and never throws", async () => { | |
| const throwingStub: Summarizer = { | |
| model: "stub", | |
| summarize: async () => { | |
| throw new Error("boom"); | |
| }, | |
| }; | |
| await expect( | |
| storeIssueSummary(env.DB, throwingStub, { issue_number: 3, title: "Third issue", body: "" }) | |
| ).resolves.not.toThrow(); | |
| const row = await storeIssueSummary(env.DB, throwingStub, { issue_number: 3, title: "Third issue", body: "" }); | |
| expect(row.model).toBe("excerpt"); | |
| expect(row.summary).toBe("Third issue"); // empty body → title | |
| }); | |
| it("falls back to excerptSummary when the summarizer throws, and never throws", async () => { | |
| const throwingStub: Summarizer = { | |
| model: "stub", | |
| summarize: async () => { | |
| throw new Error("boom"); | |
| }, | |
| }; | |
| const row = await storeIssueSummary(env.DB, throwingStub, { issue_number: 3, title: "Third issue", body: "" }); | |
| expect(row.model).toBe("excerpt"); | |
| expect(row.summary).toBe("Third issue"); // empty body → title | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-04-worker-summarizer-prompts-plan.md` around
lines 442 - 455, `storeIssueSummary` already returns a resolved row object, so
the `resolves.not.toThrow()` matcher is being applied to the wrong thing in the
fallback test. Update the test around `storeIssueSummary(env.DB, throwingStub,
...)` to await the promise directly, then keep the existing assertions on the
returned row (`model` and `summary`) to verify the fallback behavior.
|
|
||
| In the "Roadmap & My Work" section, find this paragraph: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the quoted fences.
Those bare fences will keep tripping MD040. Add a language tag like text to both blocks.
📝 Suggested fix
-```
+```text-```
+```textAlso applies to: 1790-1790
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1777-1777: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-04-worker-summarizer-prompts-plan.md` at line
1777, The markdown in the plan doc has bare quoted code fences that trigger
MD040. Update the affected fenced blocks near the quoted prompt examples to use
a language tag such as text on both fences, and make the same change for the
additional quoted fence mentioned in the note. Use the surrounding prompt
sections in the document to locate and label the fences consistently.
Source: Linters/SAST tools
…ards, rendered-diff divider - IdentitySample.event_type tightened to EventRow['event_type'] on both the worker read and the web mirror, so a backend drift breaks the compile - the four triage slice loaders take a per-slice sequence guard so an overlapping refetch can't commit a stale response over fresher data - renderedPreview marks collapsed unchanged runs with a dashed divider instead of silently dropping them Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
reviewScreen/maintenanceScreen showed a blanket loading/error notice if either underlying slice was pending or errored, hiding data already loaded from the sibling slice. Now the full-page notice only appears when neither slice has anything to show; a partial failure surfaces as an inline degraded hint above the still-rendered view. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Wires the componentized mock triage frontend to the finished backend reads and writes, per the wire-contract audit, and deletes the mock module.
GET /proposals+GET /adrs?status=draftthrough a new mapping layer (web/src/triage-map.ts); diffs are computed client-side from the two raw bodies (collapsedLineDiff, withellipsisrows in all three viewer modes); stale-base warnings derive frombase_versionvscurrent_version;low_confidenceproposals get a FLAGGED marker; Promote/Reject/Ratify post the real verdict routes and refetch the affected list.GET /needs-triagewith title/snippet derived fromraw(JSON or free-form), the verbatim gate reason in the detail, and an assign panel rebuilt from@shared/vocabularyper type (doc → section + optional space, feed → multi-select tags, adr/milestone → no target). Free-form items surface the gate's "discard it instead" error via the flash.listIdentityTasks/mapIdentityapi helpers; samples render real event kinds (PR/ISSUE, no fabricated counts); mapping requires an explicit pick plus a two-step confirm that states the concrete effect. Code path kept localized for the planned acknowledge/dismiss reshape.Loadableslices with loading/error states, boot-time loads so the sidebar badges are correct on every screen, refetch-after-verdict everywhere (never local decrement), andweb/src/triage-mock.tsis gone.Also: escaped the toast sink (
flashnow carries server-derived strings), made errored triage loads retryable, and hardened the review-id codec — from the final whole-branch review.Test plan
npm test— 362/362 across 50 files (new:test/triage-map.test.tsmapping-layer suite; extendedtest/render.review.test.tsfor ellipsis rows, FLAGGED, assign panel, confirm guard)npm run typecheck— clean (web-importing tests now type-checked undertsconfig.web.json)npm run build:web— Vite build succeeds🤖 Generated with Claude Code
Summary by CodeRabbit