refactor(app): replace right-panel status todo dots with widget marker#861
Conversation
Right-panel Status tab rendered todo progress with coloured 6/8px dots (bg-icon-success-base, bg-icon-info-base, bg-border-weak). DESIGN.md L201 forbids dots as state signals; the only sanctioned dot in PawWork is the sidebar session-row unread orange dot. This commit aligns the Status tab todo rendering with the canonical todo widget already used in session-todo-dock.tsx and todowrite.tsx: - completed → `<Icon name="circle-check">` 16x16 - pending / cancelled → `<Icon name="circle">` 16x16 - in_progress → 13x13 ring with brand-primary top border and pw-spin - completed and cancelled rows uniformly get line-through + text-fg-weak (previously only cancelled was struck; cancelled was text-fg-weaker) - gap-2.5 → gap-2 to match the dock spacing Net: same Status tab, same todo data, marker style swapped to the visual already shipping elsewhere in the app. Refs #602 (Area B right-panel rewrite, slice 3) Out of scope for this PR: session-status-connections.tsx (servers / MCP / LSP / plugins) keeps its dots for now; whether that section belongs in the right panel at all is a separate product question (alert channel needed if it moves) and will be tracked in a follow-up issue.
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 15 minutes and 55 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ 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.
Suggested priority: P2 (includes user-path files (packages/app/src/components/session/session-status-summary.test.ts, packages/app/src/components/session/session-status-summary.tsx)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
|
Follow-up filed: #862 tracks the Connections section position decision (kept out of this PR's scope per Risk Notes). |
There was a problem hiding this comment.
Code Review
This pull request updates the session-status-summary component to replace status dots with canonical icons and a spinner, aligning the UI with design specifications. It also introduces a new test file to verify these UI contracts. Feedback includes a recommendation to use import.meta.url for file resolution in tests to ensure stability and a suggestion to add break-words to the todo content to handle long text within the sidebar.
|
CI red but unrelated to this PR's code:
Code checks that did run: |
Perf delta summaryComparator: pass
|
- Add packages/app/e2e/snap/status-summary-todos.snap.ts: opens the right panel after seeding todos in all four states (completed / in_progress / pending / cancelled) and screenshots the panel. Gives Slice 3 a durable visual baseline in addition to the todo-dock-restored target that already covers the marker shape. - Address gemini-code-assist[bot] review on PR #861: - session-status-summary.test.ts: resolve source via new URL("…", import.meta.url) instead of __dirname + path.join, matching the repository test convention. - session-status-summary.tsx: add break-words to the todo content container so long todo strings wrap inside the narrow right panel, matching session-todo-dock.tsx text styling. Refs #602, #861
Applies fixes flagged by the crosscheck round on PR #861: - snap target: pass animations: "disabled" to panel.screenshot() so the in_progress 13×13 pw-spin ring captures the same orientation every run. Previously the screenshot froze whatever frame Playwright happened to land on, making the baseline non-deterministic across rebuilds (Codex P2). - snap target: poll until at least four todo rows are visible before capturing, so a partial render does not silently produce a misleading baseline (Claude P3). - unit test: assert literal 13px width/height on the spin ring so a size drift surfaces as a test failure instead of a passing comment (Codex P3).
Fresh-eye crosscheck micro-fixes on PR #861: - snap: strict equality on todo row count (toBe(4) instead of toBeGreaterThanOrEqual(4)) so a leaked prior turn or duplicate render fails instead of silently extending the baseline (Claude P3). - summary: drop unused data-slot="status-summary-todo-running" — no test or selector references it (YAGNI, Claude P3).
Crosscheck recordThree rounds of multi-model code review (Claude Opus reviewer + Codex high-reasoning) were run on this PR. Summary:
Final status: zero P0, zero P1, zero P2 across both reviewers. Pass criteria met. Deferred (not blocking)
Per-finding rejection rationale
Local verification of the final state |
Three places rendered the same 13×13 ring + circle/circle-check marker: packages/ui/src/components/message-part/tools/todowrite.tsx (37 lines) packages/app/src/pages/session/composer/session-todo-dock.tsx (33 lines) packages/app/src/components/session/session-status-summary.tsx (37 lines, added by Slice 3) Both rounds of crosscheck on PR #861 and a follow-up review flagged this as a drift risk. Extract the marker once into packages/ui so any future DESIGN.md change (ring size, palette, icon swap) propagates with a single edit. - New: packages/ui/src/components/todo-status-marker.tsx - status prop typed as TodoStatus | (string & {}) so the SDK's bare string status field (gen/types.gen.ts) flows through without casts. - Optional marginTop prop: dock + status-summary pass "1px" to align with body text baseline; todowrite leaves it unset (different row baseline). - New: todo-status-marker.test.ts — source-text contract covering the status → icon mapping, 13×13 ring dimensions, --animate-pw-spin token, and conditional marginTop application. - session-status-summary.test.ts: marker assertions moved out; only row behaviour (no leftover dot classes, strikethrough rule, delegation to the shared component) remains here. Visual: byte-for-byte identical to the prior inline JSX at each callsite. The new marker is a 1:1 lift of the existing pattern, not a rewrite, so the existing snap baselines (status-summary-todos, todo-dock-restored) both pass unchanged. Refs #602, #861
Both Show branches now share a single 16x16 inline-flex wrapper that owns the optional marginTop nudge. Previously the in_progress branch put the nudge on its outer span, while the fallback Icon routed it through splitProps onto the inner <svg> where the wrapper's align-items: center erased it — so marginTop=1px only nudged in_progress rows, not the pending/completed/cancelled ones. The shared wrapper makes the prop behave symmetrically regardless of state. Also drops the redundant 'color: var(--icon-base)' on the fallback Icon; icon.css already applies the same colour to [data-component=icon]. Surfaced by crosscheck reviewer feedback on #861.
Spell out that marginTop is read once at render time and that callsites pass a static literal — guards against a future reader assuming the inline-style object will re-spread reactively when the prop changes. Touch only the JSDoc comment; behaviour unchanged.
|
Reopening to retrigger pr-triage with corrected enhancement label (labeler sync-labels was removing the manually-added task label per .github/labeler.yml glob rules). |
Reviewer P3 反馈处理:新测试未接入 CI已验证 reviewer 的判断有效且范围更大:
结果: 为什么不在本 PR 修修 且这是项目级 CI 覆盖洞,影响范围 33 个其他测试,不应让 Slice 3(UI 重构)PR 兼带这次清扫。 已开 follow-up#869 — 含完整证据 + 三个修复方向(推荐方向 B:在 |
Summary
Right-panel Status tab rendered todo progress with coloured 6/8px dots.
DESIGN.md L201forbids dots as state signals; the only sanctioned dot in PawWork is the sidebar session-row unread orange dot.This PR swaps the dot for the canonical todo widget marker already used in
session-todo-dock.tsxandtodowrite.tsx:completed→<Icon name="circle-check">16×16pending/cancelled→<Icon name="circle">16×16in_progress→ 13×13 ring with brand-primary top border +--animate-pw-spinline-through+text-fg-weak. Previously only cancelled was struck, and cancelled wastext-fg-weaker; aligning to the dock convention.gap-2.5→gap-2to match the dock row spacing.Net effect: same Status tab, same todo data, marker style now matches the same todos rendered elsewhere in the app.
Why
Slice 3 of the Area B right-panel audit (
docs/research/2026-05-23-area-b-right-panel-audit.md, local-only) flagged status dots as a P0 DESIGN.md drift. Slice 1+2 (#854) was the first cut; this is the second.The audit originally bundled
session-status-connections.tsx(servers / MCP / LSP / plugins) into the same slice. During implementation we narrowed scope: whether that section belongs in the right panel at all is a separate product question (it carries the only live infrastructure-health signal, and moving it requires deciding on an alert channel first). A follow-up issue will track that question.Related Issue
Refs #602 (Area B right-panel rewrite, slice 3).
Human Review Status
Pending
Review Focus
TodoMarker: the icon-name mapping and the in_progress 13×13 ring are a direct port of the same JSX block insession-todo-dock.tsxlines 227-259. Confirm parity if you have a strong opinion on either visual.completedrows now also getline-through(previously onlycancelledwas struck). This is the dock convention; flag if the Status tab should diverge.--fg-weakerto--fg-weak(one tier darker, dock convention). Tiny perceptual shift.session-status-connections.tsxis intentionally untouched; its dots remain. Follow-up issue tracks the position question.Risk Notes
How To Verify
A dedicated
right-panel-status-todossnap target was considered. Deferred for cost/benefit: ~80 lines of e2e setup (open session → seed todos → toggle right panel → screenshot) to verify markers that are byte-for-byte the same as a surface already in baseline. Happy to add it if a reviewer wants the durable check.Screenshots or Recordings
Local design-decision page (untracked, local checkout only):
docs/design/scratch/right-pane-status-icons-compare.htmlshows current dots vs new markers in light and dark themes for both Status Summary and Status Connections sections (Connections side preserved for reference; not touched in this PR).Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.