Repository navigation
refactor(ui): shared ConfirmDialog, Panel, SettingRow, KeyValue, tone and error-rate primitives - #1265
Confidence 2/5 · 2 issues to address
🔴 Confidence 2/5 · risky as written
Only the packages/ui primitives and a handful of call-site groups were read; the bulk of the 259 migrated apps/web files went unreviewed.
quality 76/100 · 2 warnings · 2 notes · tests partial · risk medium
Warning
This review ended early; what follows is what it established.
Adds shared UI primitives (ConfirmDialog, Panel, SettingRow, KeyValueList, Meter/SegmentedBar, Delta, TruncatedId/Text, tone and error-rate helpers) and migrates ~250 call sites onto them. The primitives are additive and safe, but a few migrations change user-visible behavior.
ConfirmDialogkeeps itself open when the confirm handler returnsfalseor rejectstone.tsanderror-rate.tsunify severity and error-rate colour thresholds across tables and maps- Migrations drop local helpers:
pill.tsx,meta-chip.tsx,infra/severity-tokensmaps, per-file id/relative-time formatting Tablegainssize/variant="bare"/stickyheader andBadgegainspill/mono/muted/meta
Findings
🟠 Warning · F1 · onConfirm={() => void handleDelete()} never closes the delete dialog
correctness · apps/web/src/components/account/profile-section.tsx:295
ConfirmDialog only auto-closes when the confirm handler returns a promise (confirm-dialog.tsx:67-78), so returning undefined leaves open true. handleDelete resets isDeleting only in its catch and never calls setDeleteOpen(false), so after a successful deleteAccount() the dialog stays mounted with the Cancel button disabled and no way out except the redirect. Pass the promise (onConfirm={handleDelete}) or call setDeleteOpen(false) on success.
`onConfirm={handleDelete}`
🟠 Warning · F2 · 3xx spans lose their redirect colour in the span tooltip
correctness · packages/ui/src/components/traces/span-tooltip.tsx:102
httpStatusTone returns neutral for every code below 400 (http.ts:201), so a 302 falls into the same branch as 2xx and renders text-severity-info. The sibling migrations of this helper (traces-table.tsx:115, trace-anatomy-strip.tsx:25) both keep a code >= 300 branch, so redirects stay distinguishable here only if that branch is restored.
Keep the 3xx case explicit, e.g. `tone !== "neutral" ? TONE_TEXT[tone] : code >= 300 ? "text-chart-p50" : "text-severity-info"`.
🔵 Note · F3 · Error-rate thresholds become inclusive on the service map
correctness · packages/ui/src/components/service-map/service-map-node.tsx:61
errorRateLevel tests rate >= 0.05 and rate >= 0.01 while the local helpers this replaces tested > 0.05 and > 0.01, so exactly 5% now draws the critical dot/border and exactly 1% the warning one. If the new inclusive boundary is intended, it is worth stating it in error-rate.ts; otherwise use > in the replacement helpers.
Either accept the inclusive boundary as the contract, or keep the strict comparison in the node's helper.
🔵 Note · F4 · "vs previous" subline renders on tiles with no delta
correctness · apps/web/src/components/code-review/code-review-analytics.tsx:109
When previous.pullRequests / reviews / avgMergeSeconds is null or 0, relativeChange returns null and the tile renders no Delta, but subline={versus} still claims a comparison against the previous window (also at 116 and 124). The removed Headline only showed that label when a delta existed, so the tile now asserts a comparison it has no baseline for.
Pass `subline` only when the matching `changeOf(...)` returns a ratio, or move the label into `Delta`'s `suffix`.
What was checked
ConfirmDialogpromise contract at confirm-dialog.tsx:67-78 matches the fix in dba4c12 and the earlier review commentformatCountdown,shortId,widthPercentclamp/round arithmetic checked at zero, negative and non-finite input- New Badge/Table/Toggle/Alert variants are additive (new keys only), so existing call sites keep their classes
Files not reviewed (189)
The review ended before it read these diffs, so nothing above vouches for them.
apps/local-ui/src/views/metrics-list-view.tsxapps/local-ui/src/views/service-detail-view.tsxapps/local-ui/src/views/trace-detail-view.tsxapps/web/src/components/agent-sessions/agent-sessions-list.tsxapps/web/src/components/agent-sessions/session-detail/payload-view.tsxapps/web/src/components/agent-sessions/session-detail/pill.tsxapps/web/src/components/agent-sessions/session-detail/session-header.tsxapps/web/src/components/agent-sessions/session-detail/session-overview.tsxapps/web/src/components/agent-sessions/session-detail/session-transcript.tsxapps/web/src/components/agent-sessions/session-detail/session-waterfall.tsxapps/web/src/components/agent-sessions/session-detail/span-expansion.tsxapps/web/src/components/agent-sessions/session-detail/tool-io.tsxapps/web/src/components/ai-elements/inline/inline-error.tsxapps/web/src/components/ai-elements/inline/inline-log.tsxapps/web/src/components/ai-elements/inline/inline-service.tsxapps/web/src/components/ai-elements/inline/inline-trace.tsxapps/web/src/components/ai-elements/markdown-table.tsxapps/web/src/components/ai-elements/renderers/components/data-table.tsxapps/web/src/components/ai-elements/renderers/components/error-list.tsxapps/web/src/components/ai-elements/renderers/components/metrics-list.tsxapps/web/src/components/ai-elements/renderers/components/span-tree.tsxapps/web/src/components/ai-elements/renderers/components/trace-list.tsxapps/web/src/components/alerts/alert-severity-badge.tsxapps/web/src/components/alerts/destination-card.tsxapps/web/src/components/alerts/destination-dialog.tsxapps/web/src/components/alerts/notifications-section.tsxapps/web/src/components/analytics/ai/ai-sections.tsxapps/web/src/components/analytics/ai/analytics-ai-tab.tsxapps/web/src/components/analytics/analytics-breakdown-panel.tsxapps/web/src/components/analytics/analytics-metric-strip.tsx- and 159 more
9be30bd · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.
Annotations
Check warning on line 295 in apps/web/src/components/account/profile-section.tsx
maple-review-bot / Maple / review
correctness: `onConfirm={() => void handleDelete()}` never closes the delete dialog
`ConfirmDialog` only auto-closes when the confirm handler returns a promise (confirm-dialog.tsx:67-78), so returning `undefined` leaves `open` true. `handleDelete` resets `isDeleting` only in its `catch` and never calls `setDeleteOpen(false)`, so after a successful `deleteAccount()` the dialog stays mounted with the Cancel button disabled and no way out except the redirect. Pass the promise (`onConfirm={handleDelete}`) or call `setDeleteOpen(false)` on success.
Check warning on line 102 in packages/ui/src/components/traces/span-tooltip.tsx
maple-review-bot / Maple / review
correctness: 3xx spans lose their redirect colour in the span tooltip
`httpStatusTone` returns `neutral` for every code below 400 (http.ts:201), so a 302 falls into the same branch as 2xx and renders `text-severity-info`. The sibling migrations of this helper (traces-table.tsx:115, trace-anatomy-strip.tsx:25) both keep a `code >= 300` branch, so redirects stay distinguishable here only if that branch is restored.
Check notice on line 61 in packages/ui/src/components/service-map/service-map-node.tsx
maple-review-bot / Maple / review
correctness: Error-rate thresholds become inclusive on the service map
`errorRateLevel` tests `rate >= 0.05` and `rate >= 0.01` while the local helpers this replaces tested `> 0.05` and `> 0.01`, so exactly 5% now draws the critical dot/border and exactly 1% the warning one. If the new inclusive boundary is intended, it is worth stating it in `error-rate.ts`; otherwise use `>` in the replacement helpers.
Check notice on line 109 in apps/web/src/components/code-review/code-review-analytics.tsx
maple-review-bot / Maple / review
correctness: "vs previous" subline renders on tiles with no delta
When `previous.pullRequests` / `reviews` / `avgMergeSeconds` is null or 0, `relativeChange` returns null and the tile renders no `Delta`, but `subline={versus}` still claims a comparison against the previous window (also at 116 and 124). The removed `Headline` only showed that label when a delta existed, so the tile now asserts a comparison it has no baseline for.