refactor(ui): shared ConfirmDialog, Panel, SettingRow, KeyValue, tone and error-rate primitives - #1265
Merged
Conversation
…a, ListFooter, TruncatedId/Text primitives Badge gains pill shape, xs size, mono and muted/meta variants; Table gains sm/xs density, a bare variant and a sticky header; Alert gets a compact size; Item gets card/flush variants and an lg size. Shared tone, error-rate and HTTP-status maps live in @maple/ui/lib. RelativeTime, SheetDetailHeader, PageHero, DataTable, SearchToolbar and the link tab strip move to apps/web common.
ConfirmDialog, SettingRow, Panel, KeyValueList, Meter/SegmentedBar, Delta, ListFooter, SkeletonList, TruncatedId/Text, RelativeTime, SheetDetailHeader, Badge pill/xs/mono, Table density, Alert sm, StatRail tiles and the shared tone, error-rate and HTTP-status maps replace their hand-rolled copies across web, local-ui and the ui package. Error rates now share one threshold pair (5% crit, 1% warn). Deletes MetaChip, Pill, FilterTabs, KpiCard, StatCard and the local delta, countdown and id-shortening helpers.
|
Note A newer push replaced |
|
Important Review skippedToo many files! This PR contains 311 files, which is 161 over the limit of 150. To get a review, reduce the PR to 150 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configuration
📒 Files selected for processing (311)
You can disable this status message by setting the
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
…lure Exit-returning mutations resolve on failure, so closing on any resolution dismissed the dialog after a failed API key revoke. A handler can now resolve to false to keep it open for a retry; revoke does so on failure.
|
Note A newer push replaced |
The effect lint rejects a prop named for structure, so `shape="pill"` becomes `pill`. ConfirmDialog now pads its children itself, which the dialog-padding sweep requires for AlertDialog bodies; callers drop their px-6.
Maple review🔴 Confidence 2/5 · risky as written 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.
Findings🟠 Warning · F1 ·
|
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
| confirmLabel="Delete account" | ||
| pending={isDeleting} | ||
| confirmDisabled={!confirmMatches} | ||
| onConfirm={() => void handleDelete()} |
There was a problem hiding this comment.
Warning
onConfirm={() => void handleDelete()} never closes the delete dialog
F1 · Warning · correctness
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}`
🤖 Prompt to fix with an AI agent
In `apps/web/src/components/account/profile-section.tsx:295`: `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.
Suggested fix: `onConfirm={handleDelete}`
Verify the problem exists at that location before changing it, and keep the fix to those lines.
| : httpInfo.statusCode >= 300 | ||
| ? "text-severity-warn" | ||
| : "text-severity-info" | ||
| httpStatusTone(httpInfo.statusCode) === "neutral" |
There was a problem hiding this comment.
Warning
3xx spans lose their redirect colour in the span tooltip
F2 · Warning · correctness
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"`.
🤖 Prompt to fix with an AI agent
In `packages/ui/src/components/traces/span-tooltip.tsx:102`: 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.
Suggested fix: Keep the 3xx case explicit, e.g. `tone !== "neutral" ? TONE_TEXT[tone] : code >= 300 ? "text-chart-p50" : "text-severity-info"`.
Verify the problem exists at that location before changing it, and keep the fix to those lines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A fourth pass at unifying hand-rolled UI onto shared primitives. It adds the missing primitives to
@maple/ui, then moves about 270 call sites acrossapps/web,apps/local-uiandpackages/uionto them, deleting the local copies.New in
@maple/uiConfirmDialog: the confirm-before-acting AlertDialog. IfonConfirmreturns a promise, the dialog shows the pending spinner and closes when it resolves. Also hasconfirmDisabledandsecondaryAction.Panel/PanelHeader/PanelBody: flat bordered section frame.SectionCardandChartCardnow build on it.SettingRow: label and description on the left, control on the right. The control is labelled through Base UI Field.KeyValueList/KeyValue: split, grid and stacked layouts, withmono,copyValueandwrap.Meter,SegmentedBar,Delta,ListFooter/LoadMoreButton/LoadingMoreRow,SkeletonList,TruncatedId,TruncatedText,HttpStatusCode,ErrorRateValue.tone.ts(one severity palette),error-rate.ts(one threshold pair),ids.ts(display lengths for trace, span, SHA and session ids),formatCountdown,httpStatusTone.shape="pill",size="xs",mono,muted/metavariants.size="sm"|"xs",variant="bare",scroll={false},TableHeader sticky.size="sm".card/flushvariants and anlgsize.size="xs".tone="live".New in
apps/web/src/components/commonRelativeTime(absolute time on hover, in the viewer's timezone setting) andSheetDetailHeader.PageHero,DataTableand the underline link tabs.SearchToolbar, so it no longer shares a name with the tabsListToolbar.Deleted local copies:
MetaChip,Pill,FilterTabs,KpiCard,StatCard, about 7 local delta components, and duplicateformatErrorRate/formatRate/formatCountdown/truncateId/shortId/truncateCommitShahelpers.Why
The same patterns existed in many slightly different versions:
toLocaleString()tooltips that ignored the timezone setting.Behaviour changes for review
<0.01%(the shared formatter) instead of<1%.severity-*palette.PageLayout.Titletypography, so the anomaly and investigation heroes shrink from 3xl/4xl to 2xl.StatRailtiles. One integration card's whole-tile link became an "Open" link in the tile's action slot.SettingRows now have accessible names (they had none).Deliberately left alone:
Verification
tsc --noEmitis clean forpackages/ui,apps/webandapps/local-ui.packages/uilib tests (208 tests)./labinfra, agent-session, errors and verdict pages rendered with no React errors (only network errors from running without the API).The commits are cherry-picked onto
mainfrom the previous sweep branch (#1256 was squash-merged). The touched files are byte-identical to the branch where the checks above ran.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.