fix(errors): report SuspenseWrapper errors to Bugsnag + SecretsSettings adopts SuspenseWrapper - #2623
Conversation
🎩 PreviewA preview build has been created at: |
|
I have updated the registry skill to encourage |
morgan-wowk
left a comment
morgan-wowk
left a comment
There was a problem hiding this comment.
🤖 Agent review (perf / behaviour / breaking / UX / security). One app-wide behaviour change worth confirming before merge (inline). Also: the PR description says "1 file, +3/-3", but the diff is 4 files / +65/-15 — the SuspenseWrapper change below isn't mentioned in the summary.
Actions **B17** in the gardening queue (#2626): blesses `Icon` as the canonical way to render an icon, so the `consistency` pillar is allowed to migrate the stragglers instead of only proposing. Two files, docs only. **No `src` changes** — the migration itself is a later, reviewable PR. ## Why it needed a human The empirical bar was already met — **222** files import `Icon`, **30** still import `lucide-react` directly (88.1%, over `supermajorityRatio: 0.75`), and **10 of the 30 do both**, which reads as drift rather than intent. But `pillars/consistency.md` forbids migrating on measurement alone: without a registry entry that would be the bot inventing a convention and enforcing it repo-wide. Hence the entry. ## Non-Lucide icons are exceptions, not stragglers `Icon`'s `name` is `keyof typeof icons` from `lucide-react`, so it **structurally cannot** render anything else. Recorded as KEEP so no future run "fixes" them: - **`react-icons` brand/language logos** — `FaPython` ×2, `SiGnubash`, `SiRuby`, `TbBrandJavascript`, `FaGoogleDrive` across 3 files. Lucide dropped brand logos; there is no equivalent to migrate to. - **`src/components/ui/**`** — 9 files. shadcn primitives (`dialog`, `select`, `sheet`, `command`, `calendar`, `checkbox`, `breadcrumb`, `date-picker`) import their own glyphs and are CLI-regenerable, so a hand edit is overwritten by the next `shadcn add`. `icon.tsx` *is* the primitive. - **Raw `<svg>` that isn't an icon** — `<defs>`/`<marker>` arrowheads in `FlowCanvas/Edges/*`, the `Spinner` primitive's own markup, and the bespoke resize grip in `FloatingWindow.tsx`. ## The swap is not size-neutral, so the entry is not blanket `apply: yes` Worth reading before approving, because it contradicts the "mechanical swap" framing in #2623: `iconVariants` emits `!w-4 !h-4`. Leading-`!` **still compiles to `!important`** in Tailwind v4.3.3 (verified by compiling `!w-4` with the project's own Tailwind: `.\!w-4 { width: …; !important }`), and `twMerge@3.6.0` does **not** dedupe `!w-4` against a later `size-5` — it emits both, so `!important` wins. A blind `<ChevronRight className="size-5" />` → `<Icon name="ChevronRight" className="size-5" />` renders **16px instead of 20px**. Across the 30 files there are **59** Lucide JSX usages: | Bucket | Count | Disposition | | --- | --- | --- | | Size class on the scale (`size-4`, `w-5 h-5`, …) | 37 | `apply: yes` — size moves to the `size` prop | | **No** size class | 9 | `apply: yes` **as `size="xl"`** — Lucide's own default is `width: 24` (`lucide-react.js:39`), so `size="md"` would shrink them | | Off-scale (`w-2 h-2` ×6, `h-8 w-8`, `w-12 h-12`) | 8 | `apply: flag` — no variant matches and `className` can't beat `!important` | | Computed `className={cn(…)}` | 5 | `apply: flag` — size not statically readable | So ~46 of 59 usages are provably size-identical; the other 13 need a human. The off-scale ones are mostly `StatusIndicator.tsx` at `w-2 h-2` (8px, below `xs`) — if you'd rather they migrate too, the `Icon` scale needs a step, which is a primitive change and deliberately not in this PR. ## Also fixes a claim I got wrong #2623 said there was "no `canon-registry.md` entry **and** no convention-skill rule naming `Icon` as canonical." The registry entry was indeed missing; the convention rule was not — `ui-primitives#icons` already said "Use `Icon` from `@/components/ui/icon`". That line was one sentence with no `size` guidance, which is how the `!important` interaction went unnoticed, so this PR fills it in: the size scale, the warning that a size `className` loses to the variant, and the `react-icons` escape hatch. ## Checks `prettier --check` passes on both files. No `src`, test, or config changes, so `lint`/`typecheck`/`test` are unaffected — I did not run the full `validate:test` gate because its `fix` step rewrites the working tree, and there is no code here for it to validate.
camielvs
changed the title
|
Both good catches — thanks. The Bugsnag rollout is intended, and you're right that it deserved its own billing rather than The secret-echo concern doesn't apply here. The error messages on that path interpolate a One gap I found and flagged rather than hid: the new reporting has no test coverage. Happy to add one |
morgan-wowk
left a comment
morgan-wowk
left a comment
There was a problem hiding this comment.
🤖 Agent-assisted re-review (posted by Morgan). Approving. My HIGH was 'confirm intended + volume-sized' on turning onError → Bugsnag reporting for ~48 SuspenseWrapper consumers app-wide — you've confirmed it's a deliberate net positive, and the PR title/body now describe the real scope (Bugsnag reporting + SecretsSettings adoption, 4 files) rather than the old 1-file summary, so the surprise is gone. Deferring to your call on volume. One residual, non-blocking: on the secrets fallback path error.message is now both rendered and shipped to Bugsnag — worth a glance someday that backend errors there can't echo secret-adjacent content, but nothing to hold this PR on.
Merge activity
|
7790d72 to
6b6a174
Compare
…atterns#suspense The only bare inline <Suspense> left in non-test source. 46 files use the canonical SuspenseWrapper. Fallback is preserved exactly; the wrapper adds the QueryErrorResetBoundary + ErrorBoundary pair the bare form dropped. canon-registry: async-boundary (apply: yes)
Swapping Suspense for SuspenseWrapper put a boundary in front of the router's errorComponent, and SuspenseWrapper had no onError — so render errors in this subtree stopped reaching Bugsnag and degraded to a 16px MonitorX icon button where the settings panel used to be. - add onError to SuspenseWrapper, closing the reporting gap for all of its call sites rather than just this one - extract reportError() into the bugsnag service and reuse it from ErrorPage, which had the same notify-with-pathname logic inline - pass a page-scale errorFallback from SecretsSettings: the error text in an InfoBox plus a Try Again that resets the boundary Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6b6a174 to
0ea5cfa
Compare
2 participants
Two changes. The second is the one to review.
1.
SecretsSettingsadoptsSuspenseWrapperIt held the last bare inline
<Suspense>in non-test source; 46 files already use the canonicalwrapper. Same skeleton, so the loading state is unchanged. It gets a custom error fallback — an
InfoBoxwith the message and a Try Again button — because an icon-only retry isn't enough for awhole settings panel.
2.
SuspenseWrappernow reports caught errors to BugsnagNew
reportError()helper inbugsnag.ts, wired intoSuspenseWrapperviaonError.ErrorPageisrefactored onto the same helper with no behaviour change.
This is app-wide: 54 boundary sites across 47 files start reporting errors that previously went
nowhere. That's intended, but it's a real increase in Bugsnag volume and the main thing to sign off on.
Sizing: one event per boundary failure (react-query retries 3× before throwing, so it's not one per
attempt), and existing Bugsnag grouping and suppression already apply since they run for manual
notifies too. Events are tagged
boundary: "SuspenseWrapper"with a component stack, so the newvolume is filterable.
What to check
but it is a visible change)
pnpm run validate:test✅ 192 files / 1,972 passedNotes
PreviewTaskNodeCard.tsx:32(renders nothing on error by design — migratingwouldn't preserve behaviour), the app-shell boundary at
index.tsx:32, and router-ownedpendingComponent/errorComponent.secretsStorage.tsinterpolate aReadableStreaminsteadof the response text, so they read
[object ReadableStream]. Harmless but useless — worth a separateone-line fix.
Icon, 39 still import fromlucide-react— 85% supermajority, but no registry entry blesses it yet, so it's a proposal only.This PR touches none of them.