garden(consistency): weekly groundskeeping — 2026-W33 - #2623
Open
camielvs wants to merge 2 commits into
Open
Conversation
…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)
🎩 PreviewA preview build has been created at: |
Collaborator
Author
|
I have updated the registry skill to encourage |
camielvs
marked this pull request as ready for review
August 13, 2026 21:21
camielvs
commented
Aug 13, 2026
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>
morgan-wowk
reviewed
Aug 17, 2026
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.
| <ErrorBoundary onReset={reset} fallbackRender={errorFallback}> | ||
| <ErrorBoundary | ||
| onReset={reset} | ||
| onError={(error, info) => |
There was a problem hiding this comment.
🤖 This adds onError → reportError to the shared error boundary, which turns on Bugsnag reporting for all ~48 consumers of SuspenseWrapper/withSuspenseWrapper. Previously these boundaries reported nothing to Bugsnag. Concrete impact: a potentially large jump in Bugsnag event volume/cost and a changed error-grouping surface across the whole app — well beyond the described SecretsSettings consistency migration. Please confirm it's intended and volume-sized. Minor related note: on the secrets path, error.message is now both rendered (SecretsSettings fallback) and transmitted to Bugsnag; worth a glance if backend errors there can ever echo secret-related content.
camielvs
added a commit
that referenced
this pull request
Aug 17, 2026
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.
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
2 participants
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 this is
src/routes/Settings/sections/SecretsSettings.tsxheld the last bare inline<Suspense>innon-test source. 46 files use the canonical
SuspenseWrapper.SecretsSettings.tsx:17—<Suspense fallback={<SecretsSettingsSkeleton />}>→<SuspenseWrapper fallback={<SecretsSettingsSkeleton />}>.react-patterns#suspense,canon-registry: async-boundary(apply: yes)The fallback is passed through unchanged, so the loading state renders identically. What changes is
that the subtree now gets the
QueryErrorResetBoundary+ErrorBoundarypairSuspenseWrapperbundles— the registry lists this additive error boundary as the intended effect of the migration, not a
side effect.
+3 / −3)Suspense,SuspenseWrapper,ErrorBoundary,lazy()async-boundary)0.85(config)pnpm run validate:test✅ — 191 test files / 1,966 testsReviewer checklist
(
requiresBehaviorReview: truefor this category)The engine requires every PR to survive the
reviewskill first, but that skill isdisable-model-invocationand only a human can run it.<Outlet />previously bubbled tothe app-shell
ErrorBoundary(src/index.tsx:32) and took out the page. It is now caught insidethe Secrets settings panel and renders the shared retry button. That is the point of the
migration, but it is a visible change — confirm you want a local retry affordance here.
SecretsSettingsSkeleton)Deliberately not migrated
PreviewTaskNodeCard.tsx:32—<ErrorBoundary resetKeys={[componentText]} fallbackRender={() => null}>with no
Suspense. It intentionally renders nothing on error;SuspenseWrapperrenders a retrybutton instead. Not the deviation shape, and migrating would not be behavior-preserving. KEEP.
src/index.tsx:32— the app-shell boundary. Outermost by design. KEEP.pendingComponent/errorComponent— the registry's stated exception. KEEP.SuspenseWrapper/withSuspenseWrapperfallback in thetree is either a co-located
*Skeletonor the sharedSpinner. Nothing to flag.Proposed for the registry (decision queue — not applied)
Icon usage.
222files import the canonicalIconfrom@/components/ui/icon;39still importdirectly from
lucide-react— an 85.1% supermajority, abovesupermajorityRatio: 0.75, with farmore than
minConceptSites: 4. That clears the empirical bar but there is nocanon-registry.mdentry and no convention-skill rule naming
Iconas canonical, so per the pillar spec it isapply: proposeonly. Blessing it with a registry entry would let a future run migrate those 39 filesautomatically. This PR does not touch any of them.