refactor(web): sweep AGENTS.md TypeScript + Mantine rule violations (#1762)#1771
Conversation
…1762) Codebase-wide audit sweep against the AGENTS.md TypeScript and Mantine/React conventions. The codebase was already in strong shape (post-#1690/#1756), so the actionable set is small and cohesive: Styling - ServerListScreen: fold `align-items: start` into the existing `styles` prop and drop the `.grid-align-start` CSS class (+ its App.css rule). Structure — extract repeated inline elements to named `.withProps()` constants - NetworkEntry: `DimmedNote` (xs/dimmed) and `SectionLabel` (sm/500) replace 9 repeated inline `<Text>`s. - ServerImportConfigModal: `GroupHeading` replaces 3 repeated `<Text fw={600}>`s. - ViewHeader: `HeaderStackCell` replaces the twice-repeated animated Box wrapper (mirrors the file's existing `RightConnectedGroup` className-in-withProps convention). - ResourceControls: `TightRow` replaces two `<Group gap="xs" wrap="nowrap">`. Structure — extract inline logic to named functions - ToolsScreen: `handleSelectTool` replaces the ~20-line inline `onSelectTool`. - ServerConfigModal: `setTextField`/`clearTextField` factories replace 6 repeated text-field onChange/clear closures. TypeScript - App.tsx `messagesToLogEntries`: replace an `as unknown as` double cast with a single narrowing cast on just the `params` value. - createRemoteLogger: the `pino/browser.js` `@ts-expect-error` is genuinely unavoidable (TS resolves the real untyped JS file, so an ambient `declare module` can't type it) — kept, with a comment explaining why. Audit found zero `any` types, zero config-level error suppressions, and every remaining non-test `as unknown as` already justified in-comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
@claude review |
|
Claude finished @cliffhall's task in 2m 51s —— View job Code review — PR #1771
VerdictLooks good — no blocking issues. This is a genuinely behavior-preserving sweep. I traced each change against the AGENTS.md rule it claims to satisfy and against the CSS/type context, and every extraction is faithful. Comments below are nits and one thing worth a conscious sign-off. What I verified
Notes1.
2. type TextField = Exclude<keyof FormState, "transport">;Self-maintaining: adding a string field to 3. AGENTS.md's preference order puts theme variants above CSS classes, and a 4. Pre-existing (not introduced here): 5. Nothing found on
· branch |
Per review of PR #1771: replace the hand-written `TextField` union in ServerConfigModal with `Exclude<keyof FormState, "transport">`, so it stays in sync with FormState automatically — a new string field is picked up, and a non-string field surfaces as a type error at the setter factory instead of silently drifting from the list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Thanks for the thorough trace-through. Responses to each note: 1. 2. 3. 4. Pre-existing 5. |
|
@claude review |
|
Claude finished @cliffhall's task in 3m 20s —— View job Code review — PR #1771 (re-review)
VerdictApprove-equivalent — one comment-accuracy nit, nothing blocking. The follow-up commit Finding — the
|
Per re-review of PR #1771: the `Exclude<keyof FormState, "transport">` comment overclaimed — a computed union key widens to an index signature, so a future non-string field would NOT error at the setter factory. Replace with a mapped type that selects keys by value (`PlainStringKeys<FormState>`), which actually makes passing a non-string key a compile error at the call site and drops the hand-written "transport" exclusion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Good catch — you're right that the computed union key widens to an index signature, so the "type error at the factory" claim didn't hold. Applied your option (b) in 6a7b85d: |
|
@claude review |
|
Claude finished @cliffhall's task in 2m 54s —— View job Code review — PR #1771 (re-review after
|
…1762) Per re-review of PR #1771: `.header-stack-cell` was flat-only CSS (grid-area, place-self) with no pseudo-selector/keyframe companion — the same class of finding this sweep fixed when it removed `.grid-align-start`. Fold it into the `HeaderStackCell` withProps constant's `styles.root` and delete the App.css rule, keeping the keyframe-driven `header-anim` class. The reduced-motion block targets only `.header-anim[data-anim]`, so it's unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Nice consistency catch — you're right that Also confirmed your |
|
@claude review |
|
Claude finished @cliffhall's task in 3m 23s —— View job Code review — PR #1771 (re-review after
|
| Location | Element |
|---|---|
NetworkEntry.tsx:518 |
<Text span ff="monospace"> |
NetworkEntry.tsx:569 |
<Text size="xs" ff="monospace" c="red"> |
ServerConfigModal.tsx:425 |
<Text c="red" size="sm" role="alert"> |
Each is a single occurrence within its file, so leaving them is consistent with the criterion actually used — NetworkEntry.tsx:569 sitting directly under the newly-extracted SectionLabel c="red" just makes the boundary visible. There's also cross-file repetition the within-file criterion structurally can't catch (<Group justify="space-between" mb="sm"> ×5 in TaskListPanel / LogStreamPanel / ProtocolListPanel / NetworkStreamPanel / ConsoleScreen; <Text c="red" size="sm" role="alert"> ×2 in ServerConfigModal / ServerRemoveConfirmModal).
Suggestion: rather than widening this PR, record the interpretation — either note on #1762 that "repeated within a file" is the operative bar, or tighten the AGENTS.md wording so the rule and the practice agree. As written, the two diverge by ~30 instances.
Re-verified clean
PlainStringKeys<FormState>(ServerConfigModal.tsx:51-60) —FormState's six non-transportfields are all plainstringandtransportis the literal union"stdio" | "sse" | "streamable-http", sostring extends T[K]excludes it andTextFieldresolves to exactly the previous set — no call-site churn. Matches last round'stsc --strictcheck (both@ts-expect-errors consumed), so the comment's enforcement claim now holds.setTextField/clearTextField—ChangeEvent<HTMLInputElement | HTMLTextAreaElement>is contravariantly assignable to bothTextInput's andTextarea's narroweronChangeslots;clearTextField's thunk correctly ignoresClearButton'sMouseEvent. Handler identity churn per render is unchanged (inline arrows before, factory calls now).App.tsxcast —"method" in m.messagenarrows to the request/notification arm, both of which exposeparams;LogEntryData.paramsis declaredLoggingMessageNotification["params"], so the single cast now targets the consumer's own type instead of routing the whole object throughunknown. Real tightening..grid-align-start→GRID_SURFACE_STYLES—classNameandstyles.rootboth land onSimpleGrid's root;stylesemits inline, which outranks the deleted class, so no specificity regression.alignItems: "start"type-checks against csstype'sSelfPositioneven underas const.Box→GrouponHeaderStackCell— withjustify-self: centerfromplace-self, both a block div and a flex row shrink-to-fit;gap/wrap/align-itemsare inert at one child, andpreventGrowOverflowonly engages undergrow, which isn't set. Tests locate the cell viaclosest("[data-anim]"), element- and class-agnostic. Combined with your screenshot, settled.NetworkEntry/ResourceControls/ServerImportConfigModal— prop-for-prop identical;SectionLabel c="red"overrides andDimmedNote aria-live="polite"extends correctly (Mantine'swithPropsspreads call-site props last). No dead imports.ToolsScreen— theformValues→nextFormValuesrename is required (the old name would shadow theformValuesdestructured fromui), and/* v8 ignore next */still sits immediately aboveif (tool), so the ignored branch is unchanged.createRemoteLogger— agreed on TS7016; the expanded comment explains why no better option exists, which is what the AGENTS.md escape-hatch policy asks for.
Nothing found on
any usage, config-level lint suppressions, dead CSS, changed prop semantics, coverage-ignore drift, or new inline style={{…}} props.
Caveat: this checkout has no node_modules, so I did not run the suite — taking your npm run ci result at face value. All CSS/type reasoning above was checked against the files in the branch.
--- · branch v2/fix-1762-agents-md-sweep
|
Thanks — glad the animation-path check confirmed the fold is safe (the On the observation: agreed it's a criterion question, not a PR defect, and I've taken your suggestion to record rather than widen. Posted an interpretation note on #1762 documenting that the operative bar for That leaves this PR with no outstanding code changes — all four review rounds' actionable findings are applied. |
…ps() (#1762) Applies the AGENTS.md subcomponent rule strictly across the entire web client: every inline Mantine element carrying two or more static (literal) props is now a named per-file `.withProps()` constant, with dynamic props passed at the call site. This closes the ~170-instance gap between the rule's wording and prior practice (the earlier sweep only extracted repeated-within-a-file patterns). Scope: ~77 files across components/{elements,groups,screens,views} + App.tsx. Also tightens the AGENTS.md rule wording (in this PR, not a follow-up) to state explicitly that it applies in all cases including single-use elements, and that "static props" means literal-valued props (dynamic props are passed at the call site, not counted). Box handling per the rule: ResizeHandle and ScreenStage Boxes converted to named Flex constants; the App.tsx reauth-banner Box converted to a named Paper constant; the ViewHeader `display:grid` container and the AppRenderer iframe Box left inline with justifying comments (no Mantine primitive fits). Two constructs can't be expressed via `.withProps()` and stay inline with a justifying comment (same class of exemption as Box): `Accordion` (a compound, `multiple`-discriminated generic whose `.withProps()` loses its JSX call signature), and `data-*` attributes (not part of a component's typed props, so passed at the call site). Verified: `npm run ci` green end to end — 3472 unit tests, the per-file >=90 coverage gate (98.58% overall), all smokes incl. the headless-browser boot, and 456 Storybook play-tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
Full strict sweep applied (dba00b5)Per maintainer direction, the AGENTS.md subcomponent rule is now applied strictly across the entire web client — every inline Mantine element with 2+ static (literal) props is a named per-file Also tightened the AGENTS.md wording in this PR (not deferred): the rule now states explicitly that it applies in all cases including single-use elements, and defines "static props" as literal-valued (dynamic props are passed at the call site, not counted). Box handling
Two constructs that can't use
|
|
@claude review |
|
Claude finished @cliffhall's task in 6m 2s —— View job Code review — PR #1771 (re-review after
|
| File | Doc now attached to | Doc obviously describes |
|---|---|---|
ExpandToggle.tsx:18-29 |
ExpandActionIcon |
"…aria-expanded exposes the disclosure state and ariaLabel can distinguish sibling toggles" |
PinToggle.tsx:10-19 |
PinActionIcon |
"…the aria-label stays 'Pin'/'Unpin'" |
MonitoringToggle.tsx:14-27 |
MonitoringActionIcon |
"…the caller only mounts it when the sidebar is available" |
ReplayButton.tsx:9-19 |
ReplayActionIcon |
"Icon form of the 'Replay' action, used in the compact (column) ProtocolEntry layout" |
Hovering the exported component in an editor now shows nothing; hovering the styling constant shows the component's docs. The sweep got this right everywhere else (McpErrorBadge, FilterToggleButton, EmbeddableScrollArea, SubscriptionStreamBadge, … all place the constant above the doc block) — these four are the outliers. Moving the constant above the /** fixes all four. Fix this →
4. Minor: Box → Paper adds a border-radius to the full-bleed re-auth bar — clients/web/src/App.tsx:398-405
Paper's root carries border-radius: var(--paper-radius), defaulting to the theme's defaultRadius: "md" (src/theme/theme.ts:172) — the previous Box had none. On a pos="sticky" full-width bar that means rounded bottom corners under the box-shadow. Small, but a visible delta rather than a refactor.
The shadow is safe: .reauth-banner-bar { box-shadow: var(--mantine-shadow-sm) } (App.css:578-581) and Paper's box-shadow: var(--paper-shadow) are both single-class specificity, and App.css is imported after @mantine/core/styles.css (main.tsx:5-7), so the class wins. bg was already pinned to --mantine-color-body, which is Paper's default anyway. Adding radius: 0 to the constant restores the old geometry.
5. Nits
ServerConfigModal.tsx:68-75—IdTextInputandRequiredTextInputare byte-identical (required: true, rightSectionPointerEvents: "auto"). The ID field's only distinguishing props (label,description,placeholder,data-autofocus) are all passed at the call site, so one constant covers both.ServerConfigModal.tsx:394-409— the "Working directory"TextInputstill carries inlineplaceholder="(inherit)"+rightSectionPointerEvents="auto"(two static props) while all five of its siblings in the same file were extracted.ClearableTextInput— the exact single-prop constant this needs — already exists inServerSettingsFormandClientSettingsForm.
Re-verified clean
- AGENTS.md wording — the new rule text ("static prop … a single-use element with two or more static props must still be extracted") matches what the commit actually did, and the static/dynamic split it defines is the one applied throughout (
value/on*/children/keyconsistently at the call site). Tightening the contract in the same PR rather than deferring is the right call. - Component identity — all 179 constants at module scope; no per-render component types, so no remount/focus-loss class of bug.
- Prop fidelity, spot-checked across all 77 files —
ControlsClustergenuinely equals the<Group gap="sm" wrap="nowrap">it replaced inNetworkEntry's wide layout (:87-96);SectionLabel c="red",DimmedNote aria-live="polite",SessionBadge color="green",ScopeBadge color={…}all override/extend correctly (call-site props spread last). No call site passes an explicitundefinedfor a prop the constant bakes, which is the one waywithPropscan silently drop a default. classNamehandling — onlyresize-handleandheader-animare baked, neither collides, andHeaderStackCell'sstyles.root(gridArea/placeSelf) stays disjoint from the keyframes'opacity/transform, so the crossfade is unaffected (as established last round).- Documented exemptions —
Accordion(compound +multiple-discriminated generic) anddata-*attributes (not in the typed props object, so excess-property-checked out of awithPropsliteral) are both genuine TS limits, correctly left inline with comments.ViewHeader'sdisplay="grid"BoxandAppRenderer'scomponent="iframe"Boxare likewise correctly exempted — no Mantine primitive is a grid, and the iframe is a native element. - Logic extractions —
ToolsScreen.handleSelectToolkeeps/* v8 ignore next */immediately aboveif (tool); theformValues→nextFormValuesrename is still required to avoid shadowingui.formValues. PlainStringKeys<FormState>/App.tsxcast /.grid-align-start/createRemoteLogger— unchanged since the last round and still correct.
Nothing found on
any usage, config-level lint suppressions, dead CSS or dead imports, coverage-ignore drift, new inline style={{…}} props, or raw hex/rgba() literals.
Caveat: this checkout has no node_modules, so I did not run the suite or a browser — finding 1 is reasoned from the CSS box model plus the files in the branch, and is exactly the class a headless first-render smoke can't see. Please confirm it visually before merge.
--- · branch v2/fix-1762-agents-md-sweep
Follow-up to dba00b5, per PR #1771 review: - ScreenStage: the Box→Flex conversion made the child screen a row-flex item (content-sized) instead of a block child (full-width) — a real layout regression (ServerListScreen's container-typed grid has zero intrinsic width, so it collapsed to ~header width). Add `direction: "column"` so the width moves to the cross axis where `align-items: stretch` fills it. Verified visually: the Servers screen renders full-width again. - ResourceControls: remove the garbled comment left by the reverted Accordion constant (the transitionDuration rationale already lives at the call site); add a coherent Accordion-exemption note there. - ExpandToggle/PinToggle/MonitoringToggle/ReplayButton: move the new .withProps() constant above each exported component's JSDoc so the doc documents the component again, not the constant. - App.tsx ReAuthBannerBar: add `radius: 0` — Paper's default `md` radius would round the full-bleed sticky bar's corners; the Box it replaced had none. - ServerConfigModal: collapse the byte-identical IdTextInput into RequiredTextInput; extract the still-inline "Working directory" field via a ClearableTextInput constant. Verified: `npm run ci` green end to end (3472 unit tests, >=90 coverage gate, smokes incl. headless-browser boot, 456 Storybook play-tests) + a manual wide-viewport browser check of the Servers screen. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
All five findings addressed in 5e175b3. Thank you for the box-model analysis on finding 1 — that was a real regression a first-render smoke can't see. 1. ScreenStage layout regression — Fixed and visually verified. Added 2. Garbled ResourceControls comment — Removed the spliced orphan block; the 3. Four orphaned JSDoc blocks — Moved the 4. Paper border-radius on the reauth bar — Added 5. ServerConfigModal nits — Collapsed the now-byte-identical
|
|
@claude review |
|
Claude finished @cliffhall's task in 5m 48s —— View job Code review — PR #1771 (re-review after
|
| Location | Element | Static props |
|---|---|---|
clients/web/src/components/elements/EmbeddableScrollArea/EmbeddableScrollArea.tsx:59 |
<Stack> |
flex={1} mih={0} gap={0} |
clients/web/src/components/groups/ToolResultPanel/ToolCallErrorPanel.tsx:73 |
<Title> |
order={3} size="h4" |
clients/web/src/components/groups/ToolResultPanel/ToolCallErrorPanel.tsx:77 |
<Alert> |
color="red" variant="light" |
clients/web/src/components/groups/SortableServerCard/SortableServerCard.tsx:33 |
<ActionIcon> |
variant="subtle" color="gray" size="md" className |
clients/web/src/components/elements/ContentViewer/CsvTable.tsx:52 |
<Table> |
striped highlightOnHover withTableBorder withColumnBorders |
clients/web/src/components/elements/ContentViewer/BinaryNotice.tsx:18 |
<Code> |
block p={36} |
The EmbeddableScrollArea one is the notable one — this PR edited that file, extracting StreamScrollArea three lines below while leaving the wrapper <Stack> inline. The other five are in files the sweep didn't open, and two of them are near-misses by adjacency: ToolCallErrorPanel.tsx sits in the same folder as the swept ToolResultPanel.tsx, and its <Title order={3} size="h4"> is the identical pattern the sweep extracted as PanelTitle (ServerListScreen) and ToolsTitle (ToolControls); SortableServerCard.tsx sits beside the swept ServerCard.tsx.
Separately, there's a boundary worth stating out loud rather than leaving to the next auditor: ~20 <Checkbox label="…" description="…"> / <Select label="…" description="…" allowDeselect={false}> in ServerSettingsForm/ClientSettingsForm are literal-valued but are content, not "styling/layout/behavior" — so excluding them is consistent with the AGENTS.md text as written. The sweep did bake literal label/placeholder elsewhere (ExcludedDivider's label="Excluded (SEP-2243)", SearchInput's placeholder="Search tools..."), so the line is real but implicit. Either is defensible; it just isn't written down.
2. The Accordion-exemption comment still dangles in three other files
ResourceControls got this fixed; the same note is stranded in the module-scope constant block of three more files, nowhere near the <Accordion> it explains:
clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.tsx:60-62— sits betweenErrorAlertandClearableTextInput, ~115 lines above the<Accordion>at:176, and immediately above an unrelated/** Human-readable one-liner for an RFC 7591 DCR rejection. */.clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx:140-142— betweenisModernLogLevelValue()andClearableTextInput; the<Accordion>is at:392.clients/web/src/components/groups/ImportServerJsonPanel/ImportServerJsonPanel.tsx:97-99— betweenChooseFileButtonandFileContentsTextarea; the<Accordion>is at:159.
Reading any of these in place, the note appears to explain the constant below it. Moving each to a JSX comment above its <Accordion> — exactly what ResourceControls.tsx:237-239 now does — makes all four consistent. Fix this →
3. AGENTS.md codifies the rule but not the two exemptions the sweep discovered
The wording change is good and matches the practice. But the sweep found two constructs that can't satisfy it — compound multiple-discriminated generics (Accordion) and data-* attributes (not in the typed props object, so excess-property-checked out of a withProps literal) — and documented them only in ~6 scattered code comments. AGENTS.md's subcomponent section lists the Box exemption explicitly, so a reader takes that list as exhaustive and the next audit re-derives these from scratch (as this one did). One sentence next to the Box bullet would close it.
Re-verified clean
- Component identity — all 184
.withProps()constants are declared at module scope (zero indented declarations), so no per-render component types and no remount/focus-loss class of bug. classNamebakes — only three in added code (reauth-banner-bar,resize-handle,header-anim); none collides with a call-siteclassName, andHeaderStackCell'sstyles.root(gridArea/placeSelf) stays disjoint from the keyframes'opacity/transform, so the crossfade is unaffected.- Prop fidelity — spot-checked
NetworkEntry,ToolControls,ClientSettingsForm,ExperimentalFeaturesPanel,ServerCard,ServerListScreen,EmbeddableScrollArea: all prop-for-prop.SentinelTooltip label={…},SessionBadge color="green"|"yellow",ControlsClusterreuse in bothNetworkEntrylayouts,ClearableTextInputacross three forms — every override lands correctly (call-site props spread last).EmbeddableScrollArea's hoistedviewportProps: { tabIndex: 0 }is a small bonus: stable object identity instead of a fresh literal per render. - Box exemptions —
ViewHeader'sdisplay="grid"container (no Mantine primitive is a grid; converting would break the shared-cell overlay) andAppRenderer'scomponent="iframe"are both genuine and now commented.HtmlFrame.tsx:28/PdfFrame.tsx:33are the same iframe pattern, uncommented, but equally exempt. PlainStringKeys<FormState>/App.tsxsingle cast /.grid-align-startremoval /createRemoteLogger@ts-expect-error/ToolsScreen.handleSelectTool— unchanged since prior rounds and still correct.
Nothing found on
any usage, config-level lint suppressions, dead constants or imports, coverage-ignore drift, new inline style={{…}} props, or raw hex/rgba() literals.
Caveat: this checkout has no node_modules, so I did not run the suite or a browser — taking your npm run ci result and wide-viewport check at face value. The layout, CSS, and scope analysis above was run against the files in the branch.
--- · branch v2/fix-1762-agents-md-sweep
) Round-6 review follow-up: - Extract the 6 inline elements the strict sweep missed (all 2+ styling/layout props): EmbeddableScrollArea's fill-column Stack, ToolCallErrorPanel's Title + Alert, SortableServerCard's drag-handle ActionIcon, CsvTable's Table, and BinaryNotice's Code. (Their files were secondary members of a component folder, so the per-folder audit didn't open them.) - Move the Accordion-exemption comment to the call site in ClientSettingsForm, ServerSettingsForm, and ImportServerJsonPanel (matching ResourceControls), so it no longer dangles above an unrelated constant. - Add the iframe-Box exemption comment to HtmlFrame/PdfFrame for consistency with AppRenderer. - AGENTS.md: codify the two exemptions the sweep discovered (Accordion, data-*) and the iframe/grid Box exemptions next to the Box rule; and state the static-prop boundary explicitly — per-instance content/accessibility literals (label/description/placeholder/title/aria-label) do NOT by themselves trigger extraction (only styling/layout/behavior props do). This matches the practice and resolves the "not written down" gap the review flagged. Verified: `npm run ci` green (3472 unit tests, >=90 coverage gate, smokes incl. headless-browser boot, 456 Storybook play-tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
All three residual items addressed in 431dae6. 1. Six missed stragglers — extracted all six ( 2. Three more dangling Accordion comments — moved the exemption note to a JSX comment directly above the 3. AGENTS.md exemptions + boundary — codified in this PR (not deferred):
|
|
@claude review |
|
@claude review |
|
Claude finished @cliffhall's task in 6m 17s —— View job Code review — PR #1771 (re-review after
|
Round-8 review follow-up: `.reauth-banner-bar` was flat-only CSS (z-index + box-shadow) — the same finding closed for `.header-stack-cell`, on a constant this PR created. Now that the element is a `Paper` (not a Box), `box-shadow` is a first-class prop: `shadow: "sm"` emits `var(--mantine-shadow-sm)`, identical to the class. `z-index` has no Mantine style prop, so it goes through `styles.root`. Drop the className, delete the App.css block, and fix the two stale comments that still referenced "the Box". Verified: `npm run ci` green (3472 unit tests, >=90 coverage gate, smokes incl. headless-browser boot, 456 Storybook play-tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Fixed in 9356f9e — good catch, this is the same flat-CSS-class finding as
|
|
@claude review |
|
Claude finished @cliffhall's task in 8m 25s —— View job Code review — PR #1771 (re-review after
|
…CSS (#1762) Round-9 review follow-up (both pre-existing, same cleanup classes this PR already applied): - ToolCallErrorPanel: move ERROR_TITLES above the component's JSDoc so the doc documents the exported component again, not the lookup table (same defect class as round 5's four element components). - App.css: delete the dead `.inspector-spin` class and its `@keyframes inspector-spin` — zero references repo-wide (all file types), same removal as `.grid-align-start` / `.header-stack-cell` / `.reauth-banner-bar`. Verified: `npm run ci` green (3472 unit tests, >=90 coverage gate, smokes incl. headless-browser boot, 456 Storybook play-tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Both round-9 items addressed in 7abc9f5 (both pre-existing, but the same cleanup classes this PR already applied, so worth closing out):
Separately, while manually testing this PR I hit a modern-tasks server that connected but showed no header/tabs. I verified it's not caused by this styling sweep ( |
|
@claude review |
|
Claude finished @cliffhall's task in 6m 20s —— View job Code review — PR #1771 (re-review after
|
| Hits | Status |
|---|---|
4 × Box — AppRenderer:527, HtmlFrame:30, PdfFrame:35 (component="iframe"), ViewHeader:347 (display="grid") |
Documented exemption, all commented |
4 × Accordion — ClientSettingsForm:175, ImportServerJsonPanel:158, ResourceControls:240, ServerSettingsForm:391 |
Documented exemption, each note now at its call site |
1 × Transition — ScreenStage:56 |
Documented exemption, commented |
2 × MantineProvider — src/test/renderWithMantine.tsx |
Test helper; context provider with no Styles API root — out of scope on both counts |
Nothing else in the tree clears the two-static-prop bar. My round-8 list is fully closed.
No a11y label is shared by dedup — a hazard specific to this kind of sweep. Folding N inline elements into one constant can silently give two different controls the same accessible name. I extracted every constant that bakes an aria-label/label/title/placeholder/role literal and cross-checked its call-site count: zero such constant is rendered more than once. Separately, all 20 baked "aria-label" / "aria-orientation" literals are accounted for against the base — the ones my multiset diff showed as "lost" (ReplayButton's "Replay", ResizeHandle's "vertical", AppsScreen's "Close", LogControls' two selects, MonitoringControls, OutputValidationModal, SamplingRequestPanel, ExperimentalFeaturesPanel, ImportServerJsonPanel, UrlElicitationErrorModal) are all present as quoted keys inside their constants, not dropped.
Every baked variant= resolves. A variant="…" folded into a withProps literal that doesn't exist in the theme would render unstyled and silently. I collected every variant value used anywhere in src and diffed against the names recognized in src/theme/*.ts plus Mantine's built-ins: zero unresolved. All 27 custom variants (disclosure, preview, screen, reauth, listItem, …) have a theme home.
No baked styles object is clobbered. Because withProps spreads {...fixed} {...props}, a call-site styles replaces the baked one rather than merging. Three constants bake styles (ReAuthBannerBar's zIndex, HeaderStackCell's gridArea/placeSelf, ServerGrid's GRID_SURFACE_STYLES) — none of their call sites passes styles. Conversely EmbeddableScrollArea's StreamScrollArea and LogLevelBadge's FilledBadge receive possibly-undefined styles/fw at the call site and are safe precisely because neither bakes those keys.
Static-prop multiset, base vs. head, all 86 changed files. Every apparent drop is dedup into a shared constant, and I traced each non-obvious one: NetworkEntry (fw={500} 8→4, size="xs" 11→8, c="dimmed" 6→3, gap="sm" 6→5, w={280} 2→1 — all DimmedNote/SectionLabel/ControlsCluster/SentinelTooltip); ClientSettingsForm (variant="light" 4→3 = ErrorAlert + SessionBadge×2 + SignOutButton, so 4 renders preserved); ServerSettingsForm (suffix=" ms" 3→1); ServerConfigModal (the id: "" / command: "" inline resets became clearTextField(…) calls); AppsScreen (flex={1} 5→4, h="100%" 3→2 into ContentCard, used twice). The GAINED boolean entries (striped, autoContrast, withArrow, readOnly, autosize, centered) are all JSX shorthand → : true in an object literal, verified against base.
NetworkEntry read line by line (the largest diff at +119/−77). Every extraction is prop-for-prop: ControlsCluster genuinely equals the wide layout's <Group gap="sm" wrap="nowrap">; SentinelTooltip label={…} and SectionLabel c="red" are additions, not overrides; UrlScrollArea preserves the viewportProps: { tabIndex: 0 } WCAG 2.1.1 affordance and its comment. The one thing worth checking was the JSX whitespace reflow in the cancellation alert (…sending a{" "}<MonoSpan>notifications/cancelled</MonoSpan> frame (SEP-2575).) — the rendered sentence is character-identical after JSX's newline collapsing.
Everything else — zero function-scope withProps declarations (so no per-render component types), zero dead constants, zero chained X.withProps().withProps(), zero src/theme/ files touched, zero elements/ file importing from src/theme/, one inline style={{…}} in the tree (the sanctioned dnd-kit transform at SortableServerCard.tsx:61), zero raw hex/rgba() in .tsx. ScreenStage's direction: "column" is still correct and its comment records why it's load-bearing; style={styles} from Transition carries only opacity/transform/transitionDuration, disjoint from the baked position/top/left/right, so the merge can't clobber the anchoring.
Nit (ignorable) — HeadersTableEl names the disambiguator, not the thing
clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx:222
const HeadersTableEl = Table.withProps({ striped: true, withColumnBorders: true, fz: "xs" });The El suffix exists only to avoid colliding with the HeadersTable function 100 lines below — it carries no meaning, where every other constant in the file is named for its role (ValueCellRow, WideHeaderCluster, SentinelTooltip, MonoSpan). HeadersGrid would read as intentionally as its neighbors. Purely cosmetic; nothing depends on it.
Nothing found on
any usage, config-level lint suppressions, dead constants/imports/CSS, orphaned JSDoc, coverage-ignore drift, changed prop semantics, className collisions, unresolved theme variants, shared accessible names, per-render component types, or new inline styles.
Caveat: this checkout has no node_modules, so I ran neither the suite, a browser, nor tsc — taking your npm run ci result and the earlier Transition.withProps TS2339 check at face value. Every scan, CSS-cascade, and Mantine withProps merge-semantics claim above was derived from the files in the branch. Two of my scanners produced false positives I ran down and discarded rather than reporting: a <SortSelect> "override" that was actually size={16} on the nested <Icon> inside rightSection, and a batch of "unused type imports" caused by my import-stripping regex over-consuming (web's noUnusedLocals gates these anyway).
· branch v2/fix-1762-agents-md-sweep
Round-10 review nit: the `El` suffix only disambiguated from the `HeadersTable` function below and carried no meaning; `HeadersGrid` names the constant for its role, matching its neighbours (ValueCellRow, SentinelTooltip, MonoSpan). Verified: `npm run ci` green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Applied the naming nit in 369b3ef: That was the only item in the round-10 review, and it was explicitly optional — every prior round's findings are applied and the fresh scans (a11y-label dedup, baked-variant resolution, styles-clobbering, full static-prop multiset diff) all came back clean. |
|
@claude review |
|
Claude finished @cliffhall's task in 3m 59s —— View job Code review — PR #1771 (re-review after
|
| Hits | Status |
|---|---|
4 × Box — AppRenderer, HtmlFrame, PdfFrame (component="iframe"), ViewHeader (display="grid") |
Documented exemption, all commented |
4 × Accordion — ClientSettingsForm, ImportServerJsonPanel, ResourceControls, ServerSettingsForm |
Documented exemption, note at each call site |
1 × Transition — ScreenStage.tsx:56 |
Documented exemption, commented |
MdChevronRight (react-icons), ContentViewer (first-party, no .withProps) |
Non-Mantine → out of scope per the new wording |
2 × MantineProvider — src/test/renderWithMantine.tsx |
Test helper, no Styles API root |
1 × LoggingLevel |
False positive in my scanner (a TS union type, not JSX) |
Nothing else in the tree clears the two-static-prop bar. Every item on my round-8 straggler list stays closed.
Name collisions: zero. A withProps constant sharing a name with an imported identifier in the same file is the class of bug the HeadersTableEl suffix existed to dodge, so I checked it globally: across all 86 changed files, no constant name collides with an import in its own file, and no constant name shadows an existing components/elements/* component (which would be the confusing kind of near-duplicate). Only one withProps constant in the tree is exported (ClearButton, pre-existing and intentional), so the sweep widened no public surface.
ref forwarding through withProps: safe, and already load-bearing. Three constants receive a ref at their call site — EntryContainer (NetworkEntry:514), RendererContainer (AppsScreen:653), and DragHandle (SortableServerCard:44, new this PR). The first two already existed on origin/v2/main with the same ref= pattern, so the forwarding path is proven by the existing tree rather than assumed; DragHandle is the same shape. That matters because setActivatorNodeRef needs the real DOM node for dnd-kit's activator to work at all.
Baked variant values all resolve. A variant folded into a withProps literal that no theme recognizes renders silently unstyled. Of the variants baked in this diff, the four custom ones (filterToggle, listItem, reauth, warning) each have a definition under src/theme/; the rest (default, filled, light, outline, subtle) are Mantine built-ins. Zero unresolved.
Deleted CSS classes leave no references. grid-align-start, header-stack-cell, reauth-banner-bar, inspector-spin — zero hits across clients/, core/, docs/, specification/, including tests and stories.
No test asserts on a changed DOM shape. The two tagName/nodeName assertions in the suite (PendingClientRequestModal → TEXTAREA, AppRenderer → IFRAME) target elements this sweep didn't convert, so the Box → Flex/Paper swaps can't have silently satisfied or broken them.
ScreenStage / LogEntry read line by line. StageLayer doesn't bake bottom, so the call-site bottom={fill ? 0 : undefined} can't clobber a default; style={styles} carries only opacity/transform, disjoint from the baked position/top/left/right. The direction: "column" comment records exactly why it's load-bearing, which is the note a future refactor needs. LogRow is prop-for-prop the <Group gap="sm" wrap="nowrap"> it replaced.
AGENTS.md — read the final wording against the tree: the three-exemption enumeration, the factory-based scoping, the static-vs-content prop split, and the role carve-out all match what the code actually does.
Optional — "non-Mantine" is illustrated only with third-party examples
AGENTS.md (subcomponent rules)
a non-Mantine element (a
react-iconsglyph, another library's component) is out of scope entirely.
Both examples are third-party, but the live case the rule has to cover is first-party: <ContentViewer copyable wrap={false}> (clients/web/src/components/groups/ServerCard/ServerCard.tsx:283-287) is a plain export function in this repo with no .withProps static. It's correctly out of scope — it isn't a Mantine factory component — but a reader could take the parenthetical as meaning "third-party is out of scope" and then re-derive first-party dumb components as offenders on the next audit, which is the exact re-derivation cost the AGENTS.md tightening in this PR was meant to eliminate. Adding "or a first-party component that isn't a Mantine factory" to that clause closes it in one phrase. MdChevronRight (AppListItem.tsx:62) is the third-party half and needs nothing.
Genuinely cosmetic — the rule already yields the right answer today. Fix this →
Nothing found on
any usage, config-level lint suppressions, dead constants/imports/CSS, orphaned JSDoc, coverage-ignore drift, changed prop semantics, className collisions, unresolved theme variants, per-render component types (zero function-scope withProps declarations), or new inline style={{…}} props beyond the sanctioned dnd-kit transform.
Caveat: this checkout has no node_modules, so I ran neither the suite, a browser, nor tsc — taking your npm run ci result and the earlier Transition.withProps TS2339 check at face value. Every scan above was written fresh this round and run against the files in the branch; my scanner's one false positive (a TS union type parsed as a JSX tag in LogEntry.tsx) is disclosed in the table rather than reported as a finding.
--- · branch v2/fix-1762-agents-md-sweep
…b components (#1762) Round-11 review nit: the out-of-scope clause illustrated "non-Mantine" only with third-party examples, but the live case is first-party — a dumb `export function` like `ContentViewer` has no `.withProps` static either. Name both so a future audit doesn't re-derive first-party dumb components as offenders. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Applied the optional doc clarification in $(git rev-parse --short HEAD): the AGENTS.md out-of-scope clause now names both halves — a third-party element AND a first-party component that isn't a Mantine factory (a dumb That closes the last item across 11 review rounds. Every finding — the ScreenStage layout regression, the mapped-type enforcement, the four Box/Accordion/Transition/data-* exemptions, ~170 extractions, the AGENTS.md tightening, and each round's cleanups — is applied, with |
|
@claude review |
|
Claude finished @cliffhall's task in 4m 12s —— View job Code review — PR #1771 (re-review after
|
| Hits | Status |
|---|---|
4 × Accordion (ClientSettingsForm:175, ImportServerJsonPanel:158, ResourceControls:240, ServerSettingsForm:391) |
Documented exemption, note at each call site |
4 × Box (AppRenderer:527, HtmlFrame:30, PdfFrame:35 — component="iframe"; ViewHeader:347 — display="grid") |
Documented exemption, all commented |
1 × Transition (ScreenStage:56) |
Documented exemption, commented |
ContentViewer, MdChevronRight |
Non-Mantine → out of scope per the clause 9a767b8b just clarified |
2 × MantineProvider (src/test/renderWithMantine.tsx) |
Test helper; context provider with no Styles API root |
No baked prop is overridden at any call site — 0 hits. This is the one way withProps silently drops a value (Mantine spreads {...fixed} {...props}, so a call-site prop — even undefined — replaces the baked default). I extracted the baked key set for every withProps constant in the tree and machine-checked it against every JSX call site of that constant. Clean, including the three styles-baking constants (ReAuthBannerBar → zIndex, HeaderStackCell → gridArea/placeSelf, ServerGrid → GRID_SURFACE_STYLES), none of whose call sites passes styles (which would replace, not merge).
No duplicate or dead constants introduced. I detected same-base + same-normalized-props constant pairs per file: 8 pairs exist (ProtocolEntry's DurationText/SubscriptionLabel, AppsScreen's HeaderRow/PanelHeaderRow and MessageItemStack/AppLogList, plus pairs in ToolDetailPanel/ResourcePreviewPanel/TaskCard) — and I checked each against origin/v2/main: all pre-existing, and all name distinct roles, so they're deliberate. Zero unused constants (the one flagged, ClearButton, is exported and used across three forms).
Dead CSS: none. Every App.css class resolves — the 8 my scan flagged as unreferenced-from-TS are Mantine internal selector names (.mantine-Select-dropdown, .mantine-SegmentedControl-indicator, …) used as nested selectors, which is exactly what AGENTS.md says App.css is for. All 4 remaining @keyframes are consumed (the one "unused" hit was the word animations inside the file's header comment). grid-align-start, header-stack-cell, reauth-banner-bar, inspector-spin have zero references repo-wide.
Doc attribution: 15 */-followed-by-declaration sites, all correct. Checked each in the touched files — App.tsx:619 StepUpSource, ServerImportConfigModal's SOURCE_OPTIONS/OUTCOME_META, ServerConfigModal's ID_PATTERN, AppsScreen's MAX_APP_CHANNEL_ENTRIES/AppLogEntry, CodeHighlight's eight — every doc genuinely describes the declaration below it. Round 5's four element components and round 9's ToolCallErrorPanel stay fixed.
Read line by line this round: OutputValidationModal, UrlElicitationErrorModal, StepUpAuthModal (all prop-for-prop; Modal.Title still off the original import, so aria-labelledby stays wired), MethodBadge, LogLevelBadge (conditional fw safe because FilledBadge doesn't bake it), ResourceLink, MonitoringControls, ContentViewer/CodeBlock, EmbeddableScrollArea, ResizeHandle, ScreenStage, ToolsScreen.handleSelectTool (/* v8 ignore next */ still immediately above if (tool)). No test asserts DOM structure across a converted element — ViewHeader's nine closest("[data-anim]") lookups are element- and class-agnostic.
Optional follow-up — ScreenStage is the only element component with no story, and it's where the one real regression lived
clients/web/src/components/elements/ScreenStage/ScreenStage.tsx
Not a defect in this PR, and not something I'd hold the merge for. But it's a pointed gap: the Box → Flex swap here produced the single genuine behavior regression across 12 rounds (row-flex content-sizing every screen), and nothing in the suite could see it. ScreenStage.test.tsx has three assertions, all presence-only under happy-dom (which does no layout); the headless-browser smoke boots the app but asserts the "Add Servers" control renders, not its width. And of the 5 components missing a *.stories.tsx, ScreenStage is the only one under elements/ — so it's also the one gap against the README's "every screen and element component has a stories file."
A story with a play function asserting the child's getBoundingClientRect().width matches the stage's (for both the default and fill variants) runs in real Chromium via test:storybook and would have caught round 6's finding automatically. Cheap, and it guards the direction: "column" comment that is currently the only thing protecting that behavior. Fix this →
Recorded, not findings
AppModalMd(StepUpAuthModal.tsx:29) names its size rather than its role — same shape as theHeadersTableElnit closed in369b3efd.ScopeList/ScopeListPlainlikewise distinguish by "which one" rather than by purpose. Purely cosmetic; nothing depends on it, and I'm not suggesting another round for it.- The 8 identical-prop constant pairs listed above are pre-existing and role-distinct — noting so a future audit doesn't dedupe them as redundancy.
Nothing found on
any usage, config-level lint suppressions, dead constants/imports/CSS, orphaned JSDoc, coverage-ignore drift, changed prop semantics, className collisions, unresolved theme variants, per-render component types (zero function-scope withProps declarations), or new inline style={{…}} props beyond the sanctioned dnd-kit transform.
Caveat: this checkout has no node_modules, so I ran neither the suite, a browser, nor tsc — taking your npm run ci result and the earlier Transition.withProps TS2339 check at face value. Every scan above was written fresh this round and run against the files in the branch.
--- · branch v2/fix-1762-agents-md-sweep
) Round-12 review follow-up: ScreenStage was the only element component with no Storybook story, and it's where this PR's one real regression lived (the `Box → Flex` conversion content-sizing the child screen). Storybook play functions run in real Chromium, so they can assert layout that happy-dom can't. Adds ScreenStage.stories.tsx: - FillsHostWidth — the child fills the stage's width (the direct regression guard; a row flex would content-size it). Verified it FAILS when `direction: "column"` is removed and passes with it. - FillVariant — the `fill` prop stretches the stage *layer* (not the plain child) to the host height. - Inactive — an inactive stage is unmounted (Transition keepMounted={false}). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Handled the optional round-12 follow-up in f165d2c — added The key story, FillsHostWidth, is a real regression guard: it renders a stage child with negligible intrinsic width (a single
This closes every item across all 12 review rounds, including the optional follow-up. |
|
@claude review |
|
Claude finished @cliffhall's task in 3m 53s —— View job Code review — PR #1771 (re-review after
|
…ral (#1762) Round-13 review nit: `HOST_WIDTH - 4` coupled the assertion to the host Box actually rendering at 600px (which `w={}` sets as `width`, not `min-width`, so a shrinkable ancestor could narrow it). Compare the child width to its parent stage layer's actual width with `toBeCloseTo` instead — asserts the fill invariant directly, matches the repo's relative-comparison convention (InspectorView/ResourcesScreen stories), and is a sharper discriminator (a row flex leaves the layer full-width while collapsing the child). Re-verified it still FAILS when `direction: "column"` is removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Applied the round-13 robustness nit in $(git rev-parse --short HEAD): both play functions now assert the child width relative to its stage layer's actual width ( |
|
@claude review |
|
Claude finished @cliffhall's task in 1m 37s —— View job Code review — PR #1771 (re-review after
|
Closes #1762
Summary
Codebase-wide audit sweep against the AGENTS.md TypeScript and Mantine/React conventions, driven by three parallel per-cluster audits (TypeScript rules, Mantine styling, Mantine structure). The codebase was already in strong shape (post-#1690/#1756), so the actionable set is small and cohesive — this is one reviewable PR rather than a batch.
Audit outcome (verified inventory)
anytypes; zero config-level suppressions (no-unused-varsstays aterrorwith the sanctioned_-prefix pattern); all 12 non-testas unknown ascasts already carry justification comments. Two tightenings applied (below).SortableServerCardis the sanctioned dnd-kit per-frame transform), no stray hex/rgba()in props or theme overrides (theme.ts hex are the Mantine palette tuples; issue-number#1234comments are not colors), no layout divs. One minor CSS-class finding fixed.Box.withPropsmisuse (0), domain logic in theme files (0), and elements importing fromsrc/theme/(0) are all clean. Repeated-inline-element and inline-logic findings fixed (below).Changes
Styling
ServerListScreen: foldalign-items: startinto the existingstylesprop; drop the.grid-align-startclass + itsApp.cssrule.Structure — extract repeated inline elements to named
.withProps()constantsNetworkEntry:DimmedNote(xs/dimmed) +SectionLabel(sm/500) replace 9 repeated inline<Text>s.ServerImportConfigModal:GroupHeadingreplaces 3 repeated<Text fw={600} size="sm">s.ViewHeader:HeaderStackCellreplaces the twice-repeated animated wrapper (mirrors the file's existingRightConnectedGroupclassName-in-withPropsconvention).ResourceControls:TightRowreplaces two<Group gap="xs" wrap="nowrap">.Structure — extract inline logic to named functions
ToolsScreen:handleSelectToolreplaces the ~20-line inlineonSelectToolhandler.ServerConfigModal:setTextField/clearTextFieldfactories replace 6 repeated text-field onChange/clear closures.TypeScript
App.tsxmessagesToLogEntries: replace anas unknown asdouble cast with a single narrowing cast on just theparamsvalue.createRemoteLogger: thepino/browser.js@ts-expect-erroris genuinely unavoidable — TS resolves the real untyped JS file, so an ambientdeclare modulecan't type it (I tried; it fails with TS7016). Kept, with a comment now explaining why no better option exists.Testing
npm run cipasses end to end:validate→ the per-file ≥90 coverage gate → smokes (incl. the headless-browser boot smoke) → all 456 Storybook play-function tests. All 3472 unit tests pass; every touched file clears the ≥90 coverage threshold.🤖 Generated with Claude Code