feat: redesign TanStack Devtools workbench - #492
Conversation
📝 WalkthroughWalkthroughThis PR adds a branded TanStack Devtools Workbench. It introduces semantic themes and fonts, replaces the tab layout with plugin workspaces, adds accessibility behavior, updates PiP handling, and expands tests and documentation. ChangesTanStack Devtools Workbench redesign
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Devtools
participant WorkbenchHeader
participant PluginsStrip
participant PluginWorkspace
participant Plugin
Devtools->>WorkbenchHeader: render destinations and actions
WorkbenchHeader->>Devtools: select Plugins or Marketplace
Devtools->>PluginsStrip: render closed-plugin entries
PluginsStrip->>PluginWorkspace: activate or drag plugin
PluginWorkspace->>Plugin: mount and preserve pane
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
View your CI Pipeline Execution ↗ for commit a75aa79
☁️ Nx Cloud last updated this comment at |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:sherif,t... |
❌ Failed | 36m 35s | View ↗ |
nx run-many --target=test:e2e --parallel=1 --pr... |
❌ Failed | 13m 17s | View ↗ |
nx run-many --targets=build --exclude=examples/... |
✅ Succeeded | 39s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-08-04 14:20:45 UTC
More templates
@tanstack/angular-devtools
@tanstack/devtools
@tanstack/devtools-a11y
@tanstack/devtools-bundler-core
@tanstack/devtools-client
@tanstack/devtools-rspack
@tanstack/devtools-ui
@tanstack/devtools-utils
@tanstack/devtools-vite
@tanstack/devtools-event-bus
@tanstack/devtools-event-client
@tanstack/preact-devtools
@tanstack/react-devtools
@tanstack/solid-devtools
@tanstack/svelte-devtools
@tanstack/vue-devtools
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/devtools-ui/src/components/tree.tsx (1)
407-453: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the copy button’s unnecessary
createTheme()dependency.
CopyButtoncallscreateTheme()only to passtheme()toCopiedCopier, soJsonTreewithcopyablenow requiresThemeContextProvidereven though the value is not used. RemovecreateTheme()and the unusedthemeprop unless this provider becomes part of the publicJsonTreecontract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-ui/src/components/tree.tsx` around lines 407 - 453, Remove the unnecessary `createTheme()` call and `theme` variable declaration at the start of the button component, since the theme is only passed to `CopiedCopier` and this dependency is not required for the component's public contract. Update the `CopiedCopier` component invocation within the Match block to remove the `theme={theme()}` prop so the component no longer depends on ThemeContextProvider.
🟡 Minor comments (14)
docs/superpowers/specs/2026-07-31-tanstack-devtools-branding-design.md-250-250 (1)
250-250: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEscape the pipe so the table row keeps its third cell.
The literal pipe inside
light | darkstarts a fourth cell. The "Rendered result and lifecycle" content is dropped when the table renders. markdownlint reports this as MD056.📝 Proposed fix
-| Theme change | Existing `theme` value changes | Active plugin `render` and custom name callbacks receive the new `light | dark` value | +| Theme change | Existing `theme` value changes | Active plugin `render` and custom name callbacks receive the new `light \| dark` value |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-07-31-tanstack-devtools-branding-design.md` at line 250, Escape the literal pipe in the “Theme change” table row so “light | dark” remains within the third cell and the existing “Rendered result and lifecycle” content is preserved. Update only that row’s Markdown formatting.Source: Linters/SAST tools
docs/superpowers/specs/2026-07-31-tanstack-devtools-branding-design.md-210-219 (1)
210-219: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the strip geometry sections to the shipped fixed-height strip.
The specification describes a 32px idle strip that expands to 48px on hover,
:focus-within, and Marketplace-open, with a 400ms leave grace period. The tests added in this PR assert different behavior:
packages/devtools/tests/workbench.test.tsxline 203 asserts a 44px strip height, and lines 205-210 assert that hover and focus events change neither the height nor the timer count.packages/devtools/tests/workbench.test.tsxline 871 assertsgrid-template-rows: 36px 44px minmax(0, 1fr).packages/devtools/tests/workbench.test.tsxlines 658-674 assert that the Marketplace control renders in the header destinations and not inside the strip, which contradicts line 246.Update line 210 (grid row), line 214 (32/48px expansion), line 217 (32→48px growth and grace period), line 242 (forced 48px on Marketplace), line 246 (Marketplace inside the strip), and the acceptance criteria on lines 372 and 376 so the specification matches the implemented fixed-height strip.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-07-31-tanstack-devtools-branding-design.md` around lines 210 - 219, Update the strip geometry and interaction behavior in the specification to match the implemented fixed-height strip rather than the dynamic expansion behavior currently described. Replace the grid-template-rows value on line 210 from the dynamic minmax expression to reflect the fixed 44px strip height, update line 214 to remove the description of 32px idle and 48px expansion states with hover and focus triggers, remove the 32→48px growth and 400ms grace period logic from line 217, correct line 242 to remove the forced 48px expansion on Marketplace-open, update line 246 to remove Marketplace as a strip interior element, and update the acceptance criteria on lines 372 and 376 to reflect a static strip configuration that does not change height based on interaction state.packages/devtools/tests/workbench-secondary-tabs.test.tsx-155-160 (1)
155-160: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winProve the bar background is opaque before measuring contrast.
contrastRatiodrops the alpha channel on line 33. IfgetComputedStyle(bar).backgroundColorresolves torgba(0, 0, 0, 0), the helper treats the background as opaque black and reports a high ratio. The 4.5 threshold then passes without measuring the real pairing. Line 151 already applies this check to the selected tab.💚 Proposed fix
+ expect(getComputedStyle(bar).backgroundColor).not.toBe('rgba(0, 0, 0, 0)') expect( contrastRatio( getComputedStyle(two).color, getComputedStyle(bar).backgroundColor, ), ).toBeGreaterThanOrEqual(4.5)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/tests/workbench-secondary-tabs.test.tsx` around lines 155 - 160, The contrast ratio test for the bar element does not validate that the background color is fully opaque before measuring contrast. Add an assertion before the contrastRatio call (similar to the check on line 151 for the selected tab) to verify that getComputedStyle(bar).backgroundColor has an alpha channel of 1, ensuring the background is opaque before the contrast ratio calculation. This prevents the helper from incorrectly treating transparent colors as opaque when computing the ratio.packages/devtools/tests/semantic-color-usage.test.ts-89-92 (1)
89-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the Marketplace section marker exists before slicing.
If
// Plugin Marketplace Stylesis renamed or removed,indexOfreturns-1.slice(-1)then yields the last character of the file and thenot.toMatch(/\bt\(/)assertion passes trivially. The guard stops protecting the Marketplace styles at the moment it is most likely to regress.💚 Proposed fix
- const marketplaceSource = cssSource.slice( - cssSource.indexOf('// Plugin Marketplace Styles'), - ) + const marketplaceStart = cssSource.indexOf('// Plugin Marketplace Styles') + expect(marketplaceStart).toBeGreaterThanOrEqual(0) + const marketplaceSource = cssSource.slice(marketplaceStart) expect(marketplaceSource).not.toMatch(/\bt\(/)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/tests/semantic-color-usage.test.ts` around lines 89 - 92, Validate that the `// Plugin Marketplace Styles` marker exists before slicing `cssSource` in the Marketplace assertion. Store its index, assert it is non-negative, then slice from that validated index so the `not.toMatch(/\bt\(/)` check cannot pass trivially when the marker is missing.packages/devtools/src/styles/use-styles.ts-1785-1791 (1)
1785-1791: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the inverted banner hover on a solid white hover state.
semantic.color.state.hoveris#1111110fin light mode over a#ffffffbackground and#ffffff14in dark mode over a#1f1f1fbackground; this is workspace hover style, not a solid elevated hover surface for this inverted button. Use a light elevated hover token here socolors.blue[600]maintains contrast at the banner edge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/styles/use-styles.ts` around lines 1785 - 1791, Update the inverted banner hover rule near the existing transform and box-shadow to use the light elevated hover token instead of semantic.color.state.hover, ensuring the hover background remains solid white and preserves colors.blue[600] contrast.packages/devtools/src/tabs/settings-tab.tsx-89-106 (1)
89-106: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch the configured URL parameter exactly.
Line 103 describes
urlFlagas a URL parameter name.DevToolschecks it withwindow.location.search.includes(...), sodebugalso matches?not-debug=1, and an empty value matches every URL. ParseURLSearchParamswithhas()and reject an empty parameter name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/tabs/settings-tab.tsx` around lines 89 - 106, Update the DevTools URL-flag validation and matching flow to reject an empty urlFlag and use URLSearchParams.has(urlFlag) instead of window.location.search.includes(...). Preserve the configured parameter-name behavior so “debug” matches only URLs containing the exact debug parameter, not similarly named parameters.packages/devtools/src/devtools.tsx-85-92 (1)
85-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove focus out of the drawer when it closes.
createDisableTabbing()only setstabIndex="-1"on the closed container. The Escape handler and header close button calltoggleOpen(), which changes state without returning focus to the trigger. If focus remains inside the drawer after collapse, move it to the trigger before hiding the drawer and add coverage for the Escape and header close-button paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/devtools.tsx` around lines 85 - 92, In the toggleOpen function, when the drawer is closing (newState is false), restore focus to the trigger element before the drawer becomes hidden. This ensures that keyboard focus returns to the trigger button when the drawer closes via any path, including the Escape handler and header close button, so focus doesn't remain trapped inside the drawer while it's hidden.packages/devtools/src/components/tab-content.tsx-25-31 (1)
25-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the marketplace test selector unique.
When
showMarketplaceis true, Line 26 andpackages/devtools/src/tabs/plugin-marketplace.tsxLine 327 create nested elements with the samedata-testid. AgetByTestId('plugin-marketplace')query then matches two elements and fails. Remove the wrapper test ID or give it a distinct value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/components/tab-content.tsx` around lines 25 - 31, Update the wrapper around PluginMarketplace in the tab-content component to remove its duplicate data-testid="plugin-marketplace" or replace it with a distinct selector, preserving the existing plugin-marketplace test ID in the PluginMarketplace implementation.packages/devtools-ui/src/components/checkbox.tsx-26-28 (1)
26-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSynchronize
data-tsd-selectedwith the effective checkbox state.The native
checkedbinding usesprops.checked ?? isChecked(), but the marker reads onlyisChecked(). If a controlled parent changescheckedwithout an input event, the marker remains stale.
packages/devtools-ui/src/components/checkbox.tsx#L26-L28: derivedata-tsd-selectedfromprops.checked ?? isChecked().packages/devtools-ui/tests/index.test.ts#L124-L127: update a controlledcheckedprop after mount and assert that the marker changes with the native checkbox.Proposed fix
- data-tsd-selected={isChecked() ? 'true' : undefined} + data-tsd-selected={ + (props.checked ?? isChecked()) ? 'true' : undefined + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-ui/src/components/checkbox.tsx` around lines 26 - 28, The data-tsd-selected marker in packages/devtools-ui/src/components/checkbox.tsx (lines 26-28) is derived only from isChecked(), but the native checked binding uses props.checked ?? isChecked(). When a controlled parent changes the checked prop without triggering an input event, the marker becomes stale. Update data-tsd-selected to use the same fallback logic as the native checkbox binding (props.checked ?? isChecked()) so the marker stays synchronized with the effective checkbox state. Additionally, in packages/devtools-ui/tests/index.test.ts (lines 124-127), add or update a test case that sets a controlled checked prop after the component mounts and verifies that the data-tsd-selected marker updates along with the native checkbox element.packages/devtools-ui/tests/tree.tsx-70-73 (1)
70-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the original
navigator.clipboarddescriptor.This test replaces
navigator.clipboarddirectly, andvi.restoreAllMocks()only restores tracked spies, not manualObject.definePropertyassignments. Capture the original descriptor before overriding and restore it inafterEachor afinallyblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-ui/tests/tree.tsx` around lines 70 - 73, Update the test setup around the manual navigator.clipboard override to capture its original property descriptor before Object.defineProperty replaces it, then restore that descriptor in afterEach or a finally block. Keep vi.restoreAllMocks() for tracked mocks, but ensure each test restores the original navigator.clipboard state.packages/devtools-ui/src/styles/semantic-theme.ts-1-2 (1)
1-2: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the Inter font as a variable font or expand the import list for non-inlined assets.
packages/devtools-ui/src/styles/semantic-theme.ts:26-32usesfont-weight: 100 900, butpackages/devtools-ui/src/assets/fonts/Inter-latin.woff2is WOFF2 without anfvaraxis, so it is a static instance. That can cause browser synthesis outside the declared weight range or different typography from the variable design source. The Vite version supports?url&no-inline, so the asset query itself does not need change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-ui/src/styles/semantic-theme.ts` around lines 1 - 2, Update the Inter font asset used by semantic-theme.ts so it is a true variable font supporting the declared 100–900 weight range, or expand the imports and font-face declarations to cover each required static Inter weight. Keep the existing ?url&no-inline query unchanged and ensure the font-weight declarations match the imported assets.packages/devtools-a11y/src/core/styles/styles.ts-257-265 (1)
257-265: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove
cursor: pointerfrom the issue card.
A11yIssueCardnow renders an<article>root and moves selection into a dedicatedissueSelectButton. The card root is no longer interactive, but it still shows a pointer cursor across its whole area, including the tag row and the aside. That signals a click target that does not exist.🛠 Proposed fix
issueCard: css` padding: ${space[3]}; margin-bottom: ${space[2]}; border: 1px solid ${color.border.decorative}; border-radius: ${radius.group}; - cursor: pointer; background: ${color.surface.elevated}; box-shadow: ${shadow.xs}; `,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-a11y/src/core/styles/styles.ts` around lines 257 - 265, Remove the cursor: pointer declaration from the issueCard style in the styles configuration, leaving the dedicated issueSelectButton responsible for interactive cursor behavior.packages/devtools-a11y/src/core/utils/ui.utils.ts-92-98 (1)
92-98: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winRebuild the highlight stylesheet only when the theme changes.
injectStylesnow removes the existing style element and builds a new one on every call.highlightElementcallsinjectStylesonce per call, andIssueList.handleIssueClickcallshighlightElementonce per node of the selected issue. An issue with many nodes therefore removes and re-creates the same stylesheet many times in one click. During each gap the highlighted elements lose their outline, and the browser recomputes styles for the whole document.Track the injected theme and skip the rebuild when it is unchanged.
🛠 Proposed fix
+let injectedTheme: TanStackDevtoolsTheme | null = null + function injectStyles(theme: TanStackDevtoolsTheme): void { - document.getElementById(HIGHLIGHT_STYLE_ID)?.remove() + const existing = document.getElementById(HIGHLIGHT_STYLE_ID) + if (existing && injectedTheme === theme) return + existing?.remove() + injectedTheme = theme🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-a11y/src/core/utils/ui.utils.ts` around lines 92 - 98, Update injectStyles to track the theme used for the currently injected highlight stylesheet and return early when the incoming theme is unchanged. Only remove the existing style element and rebuild the stylesheet when the theme differs, preserving highlightElement behavior for repeated calls across multiple nodes.packages/devtools-a11y/src/core/components/IssueCard.tsx-35-60 (1)
35-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe select button contains flow content, which is invalid inside
<button>.The HTML content model for
<button>allows phrasing content only. Lines 57 and 59 place a<p>and a<div>inside the button. Browsers recover from this, but validators report it, and this is an accessibility plugin whose own markup should pass an audit. Nested block elements inside a button also produce inconsistent layout across browsers.Replace the two elements with
<span>and setdisplay: blockinissueSelectButtondescendants.🛠 Proposed fix
- <p class={styles().issueMessage}>{props.issue.message}</p> + <span class={styles().issueMessage}>{props.issue.message}</span> - <div class={styles().selector}>{selector()}</div> + <span class={styles().selector}>{selector()}</span>In
packages/devtools-a11y/src/core/styles/styles.ts, adddisplay: block;toissueMessageandselectorso the spans keep the current stacked layout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-a11y/src/core/components/IssueCard.tsx` around lines 35 - 60, The button element contains block-level elements (p and div) which violates HTML button content rules and causes accessibility/validation issues. In IssueCard.tsx, replace the p element wrapping issueMessage and the div element wrapping selector with span elements. Then in the styles file (packages/devtools-a11y/src/core/styles/styles.ts), add display: block styling to both the issueMessage and selector style definitions to preserve the current stacked layout while using inline elements.
🧹 Nitpick comments (20)
packages/devtools/tests/index.test.ts (2)
116-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
elementFromPointstub after the test.
Object.definePropertyis not a Vitest mock, sovi.restoreAllMocks()inafterEachleaves this stub installed. Later tests in this file keep receiving the detachedtargetbutton fromdocument.elementFromPoint. Usevi.spyOnso the existingafterEachrestores it.♻️ Proposed change
- Object.defineProperty(document, 'elementFromPoint', { - configurable: true, - value: vi.fn().mockReturnValue(target), - }) + vi.spyOn(document, 'elementFromPoint').mockReturnValue(target)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/tests/index.test.ts` around lines 116 - 119, Update the document.elementFromPoint setup in the affected test to use vi.spyOn instead of Object.defineProperty, preserving the target return value so the existing vi.restoreAllMocks cleanup restores the original implementation.
113-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStub only
navigator.clipboardinstead of replacingnavigator.Object spread copies own enumerable properties. In jsdom,
navigatorexposesuserAgent,platform,language, andclipboardthrough prototype accessors, so the spread produces an almost empty object. Code under test that reads any othernavigatorproperty then receivesundefined. Hotkey and platform checks are common consumers ofnavigator.platformandnavigator.userAgent.♻️ Proposed change
- vi.stubGlobal('navigator', { ...navigator, clipboard: { writeText } }) + vi.spyOn(navigator, 'clipboard', 'get').mockReturnValue({ + writeText, + } as unknown as Clipboard)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/tests/index.test.ts` at line 113, The current stub in the vi.stubGlobal call uses object spread on navigator which copies only own enumerable properties, causing prototype accessor properties like userAgent and platform to be lost. Instead of spreading the entire navigator object, stub only the clipboard property on the existing navigator global to preserve all other navigator properties and their accessor behaviors.packages/devtools/tests/workbench-secondary-tabs.test.tsx (1)
22-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract
resolvedCssColorinto a shared test helper.
packages/devtools/tests/seo-workbench.test.tsxlines 17-23 define the identical helper. Move it to one module underpackages/devtools/tests/and import it in both files.contrastRatiois a second candidate if further tests need it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/tests/workbench-secondary-tabs.test.tsx` around lines 22 - 28, Extract the resolvedCssColor function from workbench-secondary-tabs.test.tsx into a new shared test helper module under packages/devtools/tests/. Remove the local resolvedCssColor definition from both workbench-secondary-tabs.test.tsx and seo-workbench.test.tsx, then import the shared function in both files. This eliminates the duplicate helper definitions while preserving the existing implementation.packages/devtools/src/styles/use-styles.ts (6)
494-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not key layout on a
data-testidvalue.The grid row template depends on
[data-testid='plugins-strip']. A test-hook rename then silently breaks the workbench layout, and the coupling is invisible fromplugins-strip.tsx.Add a dedicated presentation attribute to the strip and select on it. Do not reuse
data-workbench-secondary-tabs, becauseSeoTabalso rendersWorkbenchSecondaryTabsinside the content row and:has()matches at any depth.♻️ Proposed change
- &:has([data-testid='plugins-strip']) { + &:has([data-tsd-plugins-strip]) { grid-template-rows: ${WORKBENCH_HEADER_HEIGHT}px ${PLUGINS_STRIP_HEIGHT}px minmax(0, 1fr); }Then set the attribute on the strip container in
packages/devtools/src/components/plugins-strip.tsx, for example by adding apluginsStripflag toWorkbenchSecondaryTabsthat rendersdata-tsd-plugins-strip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/styles/use-styles.ts` around lines 494 - 498, Replace the [data-testid='plugins-strip'] layout selector in the styles with a dedicated presentation attribute. Add a pluginsStrip flag to WorkbenchSecondaryTabs and set the resulting data-tsd-plugins-strip attribute on the PluginsStrip container, then select that attribute so nested SeoTab WorkbenchSecondaryTabs instances cannot match.
1653-1660: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd reduced-motion handling to the remaining animated rules.
pluginMarketplace(Line 1232) andpluginMarketplaceGrid(Line 1441) suppress their animation underprefers-reduced-motion: reduce.pluginMarketplaceEmptyrunsfadeInwith no such guard.pluginMarketplaceCardStatus(Lines 1628 and 1633) has the same gap. These elements do not carry thetsd-motion-safeclass, so the global rule inensureWorkbenchGeometryStylesdoes not reach them.♻️ Proposed change
pluginMarketplaceEmpty: css` padding: 3rem 2rem; text-align: center; background: ${semantic.color.surface.elevated}; border: 2px dashed ${semantic.color.border.control}; border-radius: 0.75rem; animation: ${fadeIn} 0.3s ease; + `@media` (prefers-reduced-motion: reduce) { + animation: none; + } `,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/styles/use-styles.ts` around lines 1653 - 1660, Update the animated style rules pluginMarketplaceEmpty and pluginMarketplaceCardStatus to disable or suppress their fadeIn animations under prefers-reduced-motion: reduce, matching the existing handling in pluginMarketplace and pluginMarketplaceGrid. Keep the normal animation behavior unchanged when reduced motion is not requested.
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo breakpoints hide the wordmark.
This global rule hides
.tsd-workbench-wordmarkbelow 360px. TheworkbenchWordmarkclass already hides the same element below 430px (Lines 619-621), so this rule never changes the outcome. Remove it, or keep one source of truth for the wordmark breakpoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/styles/use-styles.ts` around lines 28 - 30, Remove the redundant global `.tsd-workbench-wordmark` rule from the `@media (max-width: 360px)` block, keeping the existing `workbenchWordmark` breakpoint as the single source of truth for hiding the wordmark.
713-716: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the resize handle focus indicator distinct from hover.
The handle is now focusable and drives keyboard resizing.
:focus-visible::afterrenders the same 4px bar as:hover::after. A keyboard user cannot tell focus from a pointer hover. Use a stronger treatment for focus, for example thesemantic.color.border.focustoken and a larger bar.♻️ Proposed change
&:hover::after { background-color: ${semantic.color.border.control}; } + &:focus-visible::after { + top: 8px; + height: 8px; + background-color: ${semantic.color.border.focus}; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/styles/use-styles.ts` around lines 713 - 716, The resize handle's `:focus-visible::after` pseudo-element currently applies the same styling as `:hover::after`, using `semantic.color.border.control` for the background, making keyboard focus indistinguishable from mouse hover. Separate the `:focus-visible::after` rule from the `:hover::after` rule and update it to use `semantic.color.border.focus` for the background color and increase the size or height of the bar to create a visually distinct focus indicator for keyboard users.
1594-1601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLiteral
whiteforegrounds on semantic solid fills. Both rules paint text with a hard-codedwhiteover a theme-dependentsolidFill, so the contrast outcome is not guaranteed in light and dark themes.pluginMarketplaceFeatureBannerTextat Line 1766 already shows the intended pattern withsemantic.color.status.info.onFill.
packages/devtools/src/styles/use-styles.ts#L1594-L1601: replacecolor: whitewithsemantic.color.status.success.onFillonpluginMarketplaceNewBanner.packages/devtools/src/styles/use-styles.ts#L1750-L1753: replacecolor: whitewithsemantic.color.status.info.onFillonpluginMarketplaceFeatureBannerTitle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/styles/use-styles.ts` around lines 1594 - 1601, The pluginMarketplaceNewBanner rule at packages/devtools/src/styles/use-styles.ts#L1594-L1601 and the pluginMarketplaceFeatureBannerTitle rule at packages/devtools/src/styles/use-styles.ts#L1750-L1753 both use hard-coded white text color over theme-dependent semantic solid fills, which does not guarantee sufficient contrast in light and dark themes. Replace the color: white declaration in pluginMarketplaceNewBanner with semantic.color.status.success.onFill and replace the color: white declaration in pluginMarketplaceFeatureBannerTitle with semantic.color.status.info.onFill to follow the established pattern already demonstrated in the pluginMarketplaceFeatureBannerText rule.
125-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe ramp casts declare steps that do not exist.
SemanticRamprequires all ten steps.blue,green, andredare built from partial step lists and then cast withas SemanticRamp. For exampleredhas no300or900. A latercolors.red[300]compiles, resolves toundefined, and emitscolor: undefinedinto the generated CSS. The current call sites happen to use only present steps, so nothing is broken today.Populate every step in the source arrays so the cast matches reality.
♻️ Example for the `red` ramp
red: Object.fromEntries( - [100, 400, 500, 600, 700].map((step) => [ + [50, 100, 200, 300, 400, 500, 600, 700, 800, 900].map((step) => [ step, - step === 100 + step <= 100 ? semantic.color.status.error.subtleFill : semantic.color.status.error.text, ]), ) as SemanticRamp,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/styles/use-styles.ts` around lines 125 - 154, The blue, green, red, and purple color ramp definitions are missing required steps to satisfy the SemanticRamp type contract, causing later access to undefined properties. Add all ten required steps (100, 200, 300, 400, 500, 600, 700, 800, 900) to each ramp's step array in the Object.fromEntries calls and the purple object literal. For blue, green, and red, continue applying the existing conditional logic to all steps so that step 100 uses subtleFill, higher steps use text or border as defined, and all intermediate steps are represented.packages/devtools/src/tabs/seo-tab/index.tsx (1)
30-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one capitalization style for the two tab labels.
Line 30 uses sentence case ("Social previews"). Line 37 uses title case ("SERP Preview"). Pick one, for example "SERP preview".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/tabs/seo-tab/index.tsx` around lines 30 - 37, Use consistent sentence-case capitalization for the tab labels: update the SERP Preview label in the WorkbenchSecondaryTab with activeView() === 'serp-preview' to “SERP preview,” matching “Social previews.”packages/devtools/src/components/workbench-header.tsx (1)
53-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate accessible names around the logo.
Three elements now expose the same name. The
headerhasaria-label="TanStack Devtools", the wrapperspanhas the samearia-label, and theimghasalt="TanStack Devtools".aria-labelon a plainspanhas no role to attach to, so support is inconsistent. Theimgname also repeats the visible wordmark on Line 60.Mark the logo as decorative and keep the name on the
headeronly.♻️ Proposed change
- <span aria-label="TanStack Devtools" class={styles().workbenchLogo}> - <img src={TanStackLogo} alt="TanStack Devtools" /> + <span class={styles().workbenchLogo}> + <img src={TanStackLogo} alt="" /> </span>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/components/workbench-header.tsx` around lines 53 - 61, Remove the aria-label="TanStack Devtools" from the span wrapper with class workbenchLogo since it is not a semantic element and creates redundant accessible names. Change the img alt attribute from "TanStack Devtools" to an empty string to mark it as decorative, since the visible wordmark text is already provided by the strong element on line 60 with the same label. Keep the aria-label on the header element as the single accessible name source for the entire component.packages/devtools-a11y/src/core/utils/ui.utils.ts (1)
288-293: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo maps now define severity labels.
getSeverityStylealready returns alabelfor each severity, andseverity-theme.tsowns those strings. This code uses a separateSEVERITY_LABELSmap. The two maps can drift, and the tooltip text would then disagree with the panel badge text. Consider reading the label fromgetSeverityStylehere, or derivingSEVERITY_LABELSfrom theseveritydefinition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-a11y/src/core/utils/ui.utils.ts` around lines 288 - 293, Update the tooltip label construction in the surrounding single- and multiple-issue branches to reuse the label provided by getSeverityStyle, or derive SEVERITY_LABELS from the shared severity definition in severity-theme.ts. Remove the independent label source so tooltip text stays consistent with panel badge text.packages/devtools-ui/tests/contrast.test.ts (1)
69-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the syntax keys by name instead of by position.
Object.values(theme.color.syntax).slice(0, 6)depends on the key insertion order insemanticThemes. If someone reorders thesyntaxkeys, this loop silently checksselectionFillandselectionTextagainst a surface color and skips a real syntax color. Name the keys to make the intent explicit and order-independent.♻️ Proposed change
- for (const color of Object.values(theme.color.syntax).slice(0, 6)) - expect(ratio(color, syntaxBackground)).toBeGreaterThanOrEqual(4.5) + for (const key of [ + 'keyword', + 'string', + 'number', + 'comment', + 'property', + 'punctuation', + ] as const) + expect( + ratio(theme.color.syntax[key], syntaxBackground), + ).toBeGreaterThanOrEqual(4.5)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-ui/tests/contrast.test.ts` around lines 69 - 70, Update the contrast test loop around theme.color.syntax to select the intended syntax colors by their explicit key names rather than relying on Object.values(...).slice(0, 6). Keep the existing ratio check against syntaxBackground and ensure selectionFill and selectionText are not included.packages/devtools-ui/tests/fonts.test.ts (2)
46-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueByte-level license pinning can fail on line-ending normalization.
The test pins both a sha256 digest and an exact byte size for each license file. If a contributor checks out the repository with
core.autocrlf=true, the files gain CRLF line endings and both assertions fail with an opaque message. Thesizeassertion also adds nothing beyond the digest.Consider dropping the
sizeassertions and hashing the content with newlines normalized. The existingtoContainassertions already cover copyright and license provenance.♻️ Proposed change
-async function sha256(path: string): Promise<string> { - return createHash('sha256') - .update(await readFile(path)) - .digest('hex') -} +async function sha256(path: string): Promise<string> { + const text = (await readFile(path, 'utf8')).replace(/\r\n/g, '\n') + return createHash('sha256').update(text, 'utf8').digest('hex') +}Recompute both expected digests after this change, and remove the two
toMatchObject({ size: ... })assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-ui/tests/fonts.test.ts` around lines 46 - 57, Update the license verification in fonts.test.ts by removing the size assertions for bricolageLicensePath and interLicensePath, and normalize line endings before hashing each file so checkout-specific CRLF changes do not affect the digest. Recompute and replace both expected SHA-256 values using the normalized content while preserving the existing provenance checks.
11-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth new test files resolve repository paths from
process.cwd(). The working directory depends on how the test runner is invoked. If Vitest runs from the workspace root instead of a package root,readdirandreaddirSyncthrowENOENT. Resolve the paths from the module location withnew URL(..., import.meta.url)andfileURLToPath.
packages/devtools-ui/tests/fonts.test.ts#L11-L12: replacejoin(process.cwd(), 'src', 'assets', 'fonts')with a path derived fromimport.meta.url.packages/devtools-a11y/tests/theme.test.ts#L17-L18: derivepackageRootandcoreRootfromimport.meta.urlso the source scan and therelative()paths in the violation messages stay stable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-ui/tests/fonts.test.ts` around lines 11 - 12, Both tests must resolve repository paths independently of the test runner’s working directory. In packages/devtools-ui/tests/fonts.test.ts lines 11-12, update the fontDirectory setup near expectedFontNames to derive the path from import.meta.url using fileURLToPath and new URL; in packages/devtools-a11y/tests/theme.test.ts lines 17-18, derive packageRoot and coreRoot the same way so source scanning and relative() violation paths remain stable.packages/devtools-a11y/tests/theme.test.ts (1)
124-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis assertion does not discriminate the selected styling.
document.head.textContentholds every class goober has generated so far in the process. The colors#ffffffand#111111appear in many unrelated rules, and the sheet accumulates across bothdescribe.eachiterations. The assertion therefore passes even if the selected-state styles are removed.Assert on the resolved class name of the selected element instead, so the check binds to the styling under test.
♻️ Proposed change
- expect(document.head.textContent).toContain( - resolveSemanticTheme(theme).color.state.selectionText, - ) + const selectedClass = createStyles()().issueCardSelected + expect(host.querySelector('[data-tsd-surface]')?.className).toContain( + selectedClass, + )
createStylesrequires a theme context, so read it inside the rendered tree, or export the selected class name from the style factory for assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-a11y/tests/theme.test.ts` around lines 124 - 126, Replace the broad document.head.textContent assertion in the selected-styling test with an assertion against the selected element’s resolved class name. Read the theme context inside the rendered tree when calling createStyles, or expose the selected class name from the style factory, and verify that class is applied to the selected element rather than matching unrelated accumulated stylesheet colors.packages/devtools-a11y/src/core/styles/styles.ts (2)
227-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the severity style once per style function.
getSeverityStyleruns two or three times inside a single template. Each call allocates a new object through the spread inseverity-theme.ts. Call it once and reuse the result.♻️ Proposed change for `severityLabel`
- severityLabel: (impact: SeverityThreshold) => css` - font-size: ${type.labelSm.size}; - line-height: ${type.labelSm.lineHeight}; - color: ${getSeverityStyle(impact, themeName).colors.text}; - background: ${getSeverityStyle(impact, themeName).colors.subtleFill}; - border: 1px solid ${getSeverityStyle(impact, themeName).colors.border}; - padding: 2px ${space[1]}; - border-radius: ${radius.control}; - `, + severityLabel: (impact: SeverityThreshold) => { + const severity = getSeverityStyle(impact, themeName).colors + return css` + font-size: ${type.labelSm.size}; + line-height: ${type.labelSm.lineHeight}; + color: ${severity.text}; + background: ${severity.subtleFill}; + border: 1px solid ${severity.border}; + padding: 2px ${space[1]}; + border-radius: ${radius.control}; + ` + },Apply the same pattern to
summaryButtonActive,summaryCount,sectionTitle, anddot.Also applies to: 249-256, 311-326
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-a11y/src/core/styles/styles.ts` around lines 227 - 237, Update the style functions summaryButtonActive, summaryCount, severityLabel, sectionTitle, and dot to call getSeverityStyle once per invocation, store the returned style, and reuse it for all color properties within the template.
100-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused style keys from
styles.ts.
primaryButton,primaryButtonDisabled,button,toggleOverlay,toggleOverlayOn,smallLinkButton,disableRule,doneButton,select,issueMain, andsearchhave nostyles().<key>consumers inpackages/devtools-a11y. Remove the unused definitions while keeping keys that components still apply.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-a11y/src/core/styles/styles.ts` around lines 100 - 123, In the styles definition, remove the unused keys primaryButton, primaryButtonDisabled, button, toggleOverlay, toggleOverlayOn, smallLinkButton, disableRule, doneButton, select, issueMain, and search. Preserve all style keys that still have styles().<key> consumers in the devtools-a11y components.packages/devtools-ui/src/styles/use-styles.ts (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid buttons lose hover and pressed color feedback.
solidHoverusesstatus.borderandsolidActiveusesstatus.solidFill. In the light theme,border,text, andsolidFillare the same value for every status role. For example,successuses#1d4226for all three. In the dark theme,borderandsolidFillare also equal forsuccess,warning,error, andinfo.The result is that a solid button keeps the same background on hover and on press. Only the box-shadow changes. That is weak feedback for pointer and keyboard users.
Consider composing the hover and pressed layers from
semantic.color.state.hoverandsemantic.color.state.pressed, which already encode translucent overlays.♻️ Proposed change to restore state feedback
solid: css` background: ${solidBg}; color: ${solidText}; border-color: ${solidBorder}; &:hover { - background: ${solidHover}; - border-color: ${solidHover}; + background: linear-gradient( + ${semantic.color.state.hover}, + ${semantic.color.state.hover} + ), + ${solidHover}; + border-color: ${solidHover}; box-shadow: ${semantic.shadow.xs}; } &:active { - background: ${solidActive}; - border-color: ${solidActive}; + background: linear-gradient( + ${semantic.color.state.pressed}, + ${semantic.color.state.pressed} + ), + ${solidActive}; + border-color: ${solidActive}; box-shadow: ${semantic.shadow.sm}; } `,Also applies to: 60-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-ui/src/styles/use-styles.ts` around lines 25 - 29, The solidHover and solidActive variable assignments use status color properties that are identical to solidFill in light theme and dark theme, preventing visual feedback on hover and pressed states. Replace the solidHover assignment with a reference to semantic.color.state.hover and the solidActive assignment with semantic.color.state.pressed, which provide translucent overlay states. Apply the same pattern fixes to the analogous hover and active state assignments in the outlineHover, outlineActive, ghostHover, and ghostActive variable definitions to ensure consistent state feedback across all button variants.packages/devtools-a11y/tests/index.test.ts (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a development-mode case for
a11yDevtoolsPlugin.The mock tuple order matches
createReactPlugin, but the current test only covers the production no-op import path. Add a companion case that stubsNODE_ENVtodevelopmentand asserts the activeA11yDevtoolsPanel/a11yDevtoolsPluginbranch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools-a11y/tests/index.test.ts` around lines 3 - 9, Add a development-mode test case in the existing test suite that stubs NODE_ENV to development, imports or invokes the active a11yDevtoolsPlugin branch, and asserts it uses the mocked A11yDevtoolsPanel/createReactPlugin path rather than the production no-op path. Preserve the existing production coverage and restore the environment or module state between cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a5ebdd3-1aaa-4775-914b-dac45d834f11
⛔ Files ignored due to path filters (4)
packages/devtools-ui/src/assets/fonts/BricolageGrotesque-Bold.ttfis excluded by!**/*.ttfpackages/devtools-ui/src/assets/fonts/Inter-latin.woff2is excluded by!**/*.woff2packages/devtools/src/components/tanstack-logo.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (87)
.changeset/tanstack-devtools-branding.mddocs/architecture.mddocs/overview.mddocs/superpowers/specs/2026-07-31-tanstack-devtools-branding-design.mdexamples/react/basic/README.mdexamples/react/basic/package.jsonexamples/react/basic/scripts/check-a11y-fixture.mjsexamples/react/basic/src/a11y-audit-fixture.tsxexamples/react/basic/src/example.cssexamples/react/basic/src/index.tsxexamples/react/basic/src/setup.tsxexamples/react/basic/src/vite-env.d.tspackages/devtools-a11y/src/core/components/IssueCard.tsxpackages/devtools-a11y/src/core/components/IssueList.tsxpackages/devtools-a11y/src/core/components/Settings.tsxpackages/devtools-a11y/src/core/components/Shell.tsxpackages/devtools-a11y/src/core/contexts/allyContext.tsxpackages/devtools-a11y/src/core/styles/severity-theme.tspackages/devtools-a11y/src/core/styles/styles.tspackages/devtools-a11y/src/core/utils/ui.utils.tspackages/devtools-a11y/tests/index.test.tspackages/devtools-a11y/tests/theme.test.tspackages/devtools-a11y/vite.config.tspackages/devtools-ui/package.jsonpackages/devtools-ui/scripts/check-font-assets.mjspackages/devtools-ui/src/assets/fonts/OFL-Bricolage-Grotesque.txtpackages/devtools-ui/src/assets/fonts/OFL-Inter.txtpackages/devtools-ui/src/components/button.tsxpackages/devtools-ui/src/components/checkbox.tsxpackages/devtools-ui/src/components/header.tsxpackages/devtools-ui/src/components/icons.tsxpackages/devtools-ui/src/components/input.tsxpackages/devtools-ui/src/components/logo.tsxpackages/devtools-ui/src/components/main-panel.tsxpackages/devtools-ui/src/components/section.tsxpackages/devtools-ui/src/components/select.tsxpackages/devtools-ui/src/components/tag.tsxpackages/devtools-ui/src/components/theme.tsxpackages/devtools-ui/src/components/tree.tsxpackages/devtools-ui/src/internal.tspackages/devtools-ui/src/styles/semantic-theme.tspackages/devtools-ui/src/styles/use-styles.tspackages/devtools-ui/tests/contrast.test.tspackages/devtools-ui/tests/fonts.test.tspackages/devtools-ui/tests/index.test.tspackages/devtools-ui/tests/semantic-theme.test.tspackages/devtools-ui/tests/tree.tsxpackages/devtools-ui/tsconfig.jsonpackages/devtools-ui/vite.config.tspackages/devtools/src/components/content-panel.tsxpackages/devtools/src/components/main-panel.tsxpackages/devtools/src/components/plugins-strip.tsxpackages/devtools/src/components/source-inspector.tsxpackages/devtools/src/components/tab-content.tsxpackages/devtools/src/components/tabs.test.tsxpackages/devtools/src/components/tabs.tsxpackages/devtools/src/components/trigger.tsxpackages/devtools/src/components/workbench-header.tsxpackages/devtools/src/components/workbench-secondary-tabs.tsxpackages/devtools/src/context/devtools-context.test.tspackages/devtools/src/context/devtools-context.tsxpackages/devtools/src/context/draw-context.tsxpackages/devtools/src/context/pip-context.tsxpackages/devtools/src/context/use-devtools-context.tspackages/devtools/src/devtools.tsxpackages/devtools/src/styles/tokens.tspackages/devtools/src/styles/use-styles.tspackages/devtools/src/tabs/index.tsxpackages/devtools/src/tabs/marketplace/marketplace-header.tsxpackages/devtools/src/tabs/marketplace/plugin-card.tsxpackages/devtools/src/tabs/marketplace/plugin-section.tsxpackages/devtools/src/tabs/marketplace/settings-panel.tsxpackages/devtools/src/tabs/marketplace/tag-filters.tsxpackages/devtools/src/tabs/plugin-marketplace.tsxpackages/devtools/src/tabs/plugins-tab.tsxpackages/devtools/src/tabs/seo-tab/index.tsxpackages/devtools/src/tabs/seo-tab/serp-preview.tsxpackages/devtools/src/tabs/seo-tab/social-previews.tsxpackages/devtools/src/tabs/settings-tab.test.tsxpackages/devtools/src/tabs/settings-tab.tsxpackages/devtools/src/utils/constants.tspackages/devtools/src/utils/storage.tspackages/devtools/tests/index.test.tspackages/devtools/tests/semantic-color-usage.test.tspackages/devtools/tests/seo-workbench.test.tsxpackages/devtools/tests/workbench-secondary-tabs.test.tsxpackages/devtools/tests/workbench.test.tsx
💤 Files with no reviewable changes (5)
- packages/devtools/src/components/tabs.test.tsx
- packages/devtools/src/components/tabs.tsx
- packages/devtools/src/tabs/index.tsx
- packages/devtools/src/styles/tokens.ts
- packages/devtools/src/context/draw-context.tsx
| let nextHeight: number | undefined | ||
| if (grows) nextHeight = clampedHeight() + step | ||
| if (shrinks) nextHeight = clampedHeight() - step | ||
| if (event.key === 'Home') nextHeight = PANEL_CLOSE_THRESHOLD | ||
| if (event.key === 'End') nextHeight = maxHeight() | ||
| if (nextHeight === undefined) return | ||
| event.preventDefault() | ||
| props.handleHeightChange?.(Math.min(maxHeight(), nextHeight)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clamp the keyboard resize to PANEL_CLOSE_THRESHOLD.
Line 45 clamps only the upper bound. When clampedHeight() is already at PANEL_CLOSE_THRESHOLD, ArrowDown (bottom panel) reports 20 px. updateHeight in packages/devtools/src/devtools.tsx then sets isOpen to false, so the panel collapses and the separator is unmounted. A keyboard user cannot restore the panel from that control.
This also contradicts aria-valuemin={PANEL_CLOSE_THRESHOLD} on Line 66 and the Home behavior on Line 41, which both declare 70 as the floor.
🐛 Proposed fix to clamp both bounds
if (nextHeight === undefined) return
event.preventDefault()
- props.handleHeightChange?.(Math.min(maxHeight(), nextHeight))
+ props.handleHeightChange?.(
+ Math.min(maxHeight(), Math.max(PANEL_CLOSE_THRESHOLD, nextHeight)),
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let nextHeight: number | undefined | |
| if (grows) nextHeight = clampedHeight() + step | |
| if (shrinks) nextHeight = clampedHeight() - step | |
| if (event.key === 'Home') nextHeight = PANEL_CLOSE_THRESHOLD | |
| if (event.key === 'End') nextHeight = maxHeight() | |
| if (nextHeight === undefined) return | |
| event.preventDefault() | |
| props.handleHeightChange?.(Math.min(maxHeight(), nextHeight)) | |
| let nextHeight: number | undefined | |
| if (grows) nextHeight = clampedHeight() + step | |
| if (shrinks) nextHeight = clampedHeight() - step | |
| if (event.key === 'Home') nextHeight = PANEL_CLOSE_THRESHOLD | |
| if (event.key === 'End') nextHeight = maxHeight() | |
| if (nextHeight === undefined) return | |
| event.preventDefault() | |
| props.handleHeightChange?.( | |
| Math.min(maxHeight(), Math.max(PANEL_CLOSE_THRESHOLD, nextHeight)), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/components/content-panel.tsx` around lines 38 - 45,
Update the keyboard resize handler around nextHeight and
props.handleHeightChange so the reported height is clamped to both
PANEL_CLOSE_THRESHOLD and maxHeight(). Preserve the existing key-specific height
calculations and ensure ArrowDown cannot produce a value below the declared
minimum.
| <div | ||
| id={TANSTACK_DEVTOOLS} | ||
| data-testid="tsd-main-panel" | ||
| data-open={props.isOpen() ? 'true' : 'false'} | ||
| data-testid="tanstack-devtools-panel" | ||
| data-open={String(props.isOpen())} | ||
| data-collapsed={String(isAttachedCollapsed())} | ||
| data-tsd-surface |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use an instance-specific panel target.
Line 86 assigns every MainPanel the same tanstack_devtools ID. createDisableTabbing resolves that ID with document.getElementById, so multiple DevTools owners update tab stops only in the first panel. A later closed panel can remain keyboard reachable, and the document contains duplicate IDs. Pass the panel element to createDisableTabbing or generate a per-instance ID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/components/main-panel.tsx` around lines 85 - 90, Update
MainPanel and createDisableTabbing so tab handling targets the current panel
element rather than the shared TANSTACK_DEVTOOLS ID. Pass each panel’s element
directly to createDisableTabbing (or generate and consistently use a
per-instance ID), ensuring multiple MainPanel instances have no duplicate IDs
and closed panels are correctly removed from keyboard navigation.
| it('reactively replaces plugins through the existing onSetPlugins callback', async () => { | ||
| let replace!: (plugins: Array<TanStackDevtoolsPlugin>) => void | ||
| let ids: Array<string> = [] | ||
| const dispose = createRoot((disposeRoot) => { | ||
| createComponent(DevtoolsProvider, { | ||
| plugins: [{ name: 'Old', render: () => {} }], | ||
| onSetPlugins: (setter) => { | ||
| replace = setter | ||
| }, | ||
| get children() { | ||
| const context = useContext(DevtoolsContext)! | ||
| createEffect(() => { | ||
| ids = context.store.plugins?.map((entry) => entry.id!) ?? [] | ||
| }) | ||
| return null | ||
| }, | ||
| }) | ||
| return disposeRoot | ||
| }) | ||
| await Promise.resolve() | ||
| expect(ids).toEqual(['old-0']) | ||
| replace([{ name: 'New', render: () => {} }]) | ||
| expect(ids).toEqual(['new-0']) | ||
| dispose() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reconcile active plugin IDs during replacement.
Line 393 replaces store.plugins with new-0. DevtoolsProvider.updatePlugins only updates store.plugins, so the initial one-plugin state retains activePlugins: ['old-0']. PluginsTab then creates a pane for old-0 and finds no plugin to render. Reconcile active IDs with the replacement list and apply the normal default-selection behavior when no valid active ID remains. Extend this test to assert the active state and rendered replacement plugin.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/context/devtools-context.test.ts` around lines 372 -
395, Update DevtoolsProvider.updatePlugins to reconcile activePlugins against
the replacement plugin IDs, removing stale IDs and applying the existing
default-selection behavior when none remain valid. Extend the reactive
replacement test around onSetPlugins to assert the active plugin state and
verify that the replacement plugin is rendered instead of the removed plugin.
| const updateHeight = (nextHeight: number) => { | ||
| setHeight(nextHeight) | ||
| setIsOpen(nextHeight >= PANEL_CLOSE_THRESHOLD) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve a usable height after a drag-close.
Line 81 stores the raw drag height, and Line 82 closes the drawer below PANEL_CLOSE_THRESHOLD. After a user drags to 50px, toggleOpen reopens the drawer without restoring its height. The drawer then opens at 50px. Preserve the last valid expanded height or clamp the stored height before closing. Apply the same maximum bound used by keyboard resizing.
Also applies to: 117-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/devtools.tsx` around lines 80 - 83, Update updateHeight
and the keyboard-resizing logic to clamp stored heights using the same maximum
bound, while preserving the last valid expanded height when a drag closes below
PANEL_CLOSE_THRESHOLD. Ensure toggleOpen reopens the drawer at a usable expanded
height rather than the raw drag height.
| <div | ||
| class={styles().pluginMarketplaceSettingsPanel} | ||
| data-tsd-surface | ||
| role="dialog" | ||
| aria-label="Marketplace settings" | ||
| > | ||
| <div class={styles().pluginMarketplaceSettingsPanelHeader}> | ||
| <h3 class={styles().pluginMarketplaceSettingsPanelTitle}> | ||
| Marketplace Settings | ||
| </h3> | ||
| <button | ||
| type="button" | ||
| aria-label="Close marketplace settings" | ||
| data-tsd-control | ||
| class={styles().pluginMarketplaceSettingsPanelClose} | ||
| onClick={props.onClose} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Move focus into the settings dialog.
When the settings button opens this dialog, no code moves focus to the panel. SettingsPanel is inserted before MarketplaceHeader, so focus stays on the trigger and forward Tab moves past the close button and checkbox. Focus the close button or first control on open. Restore focus to the trigger on close.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/tabs/marketplace/settings-panel.tsx` around lines 18 -
33, Update SettingsPanel to move focus to its close button or first control when
the dialog opens, ensuring keyboard navigation begins inside the panel. On
close, restore focus to the settings trigger that opened it, using the existing
open/close lifecycle and refs or focusable element symbols rather than leaving
focus on the removed trigger.
| export const getStorageItem = (key: string) => { | ||
| return localStorage.getItem(key) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle unavailable local storage.
Line 2 can throw when storage is blocked or the document has an opaque origin. Devtools context initialization reads settings and state through this helper, so the exception prevents Devtools from mounting. Return null on storage failure and add a test for a throwing getItem.
Proposed fix
export const getStorageItem = (key: string) => {
- return localStorage.getItem(key)
+ try {
+ return localStorage.getItem(key)
+ } catch {
+ return null
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getStorageItem = (key: string) => { | |
| return localStorage.getItem(key) | |
| } | |
| export const getStorageItem = (key: string) => { | |
| try { | |
| return localStorage.getItem(key) | |
| } catch { | |
| return null | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/utils/storage.ts` around lines 1 - 3, Update
getStorageItem to catch failures from localStorage.getItem and return null when
storage is unavailable or access throws, while preserving normal retrieved
values. Add a test covering a throwing getItem call and asserting the helper
returns null.
…sions Separate chrome from canvas: the header and the secondary strips paint the brand surface and close with a translucent ink hairline, while destination content and plugin panes paint the workspace surface. `border.decorative` is the cream brand colour, so any rule drawn on the chrome band was invisible. Align everything to one gutter. `WORKBENCH_GUTTER` (16px, 12px below 430px) is now the single inline gutter for the header, the strips and each destination's content, which previously started at 0, 8, 12 and 32px depending on the tab. Trade competing accents for the semantic theme: - The Marketplace partnership banner was a saturated block; it is brand paper with a charcoal rule. Tag filters lost their outlined container, section headings stopped being cards, and emoji labels are plain text. - SEO social cards were each outlined in their network's brand colour. The colour survives as a small dot; the card border is neutral. The tab also ran on the legacy grey ramp and is on the semantic theme now. - Featured and active plugin cards keep the neutral outline and let their badge carry the state; "Active" was wearing the info colour. The palm emblem is inline SVG instead of a raster filtered with `brightness(2.5)` to fake dark mode, so it stays sharp and takes its colour from the theme. Plugin destinations get a real empty state. Fold the subheader, not the panel. A pull tab on the strip's bottom edge animates the strip's height to zero and drops to the header's edge; the panel height, the header and the destination content are untouched, and the tab is only rendered where a strip exists. Folded, the strip is inert. Interaction fixes: - The resize handle had grown to 24px at `top: -10px`, covering the top 14px of the 36px header, so a press aimed at a header button started a resize. It is a thin bar on the panel edge again. - The Marketplace settings drawer was `position: fixed` and covered the host page instead of the Workbench. The marketplace is a shell with an inner scroll region and the drawer is absolute inside it. - Scroll gestures chained on to the host page. The outermost scroller in each destination contains them; deliberately not their descendants, since a plugin nests empty `overflow: auto` wrappers that must chain up to the pane. - Plugin mounts are their own positioning context. Plugins position chrome absolutely from a statically positioned root, so it resolved against the whole Workbench and painted over our header. - The floating trigger and drawer toggle replaced their brand fill with a translucent state colour on hover, making them vanish over the page. - The "New" ribbon overlapped the card icon; it is an inline pill. - HotkeyConfig never rendered its `title`, so each shortcut was headed by its own description, and its modifier chips were styled as success. Interactive controls animate over 0.3s, with one reduced-motion guard scoped to the core-owned `data-tsd-control` / `data-tsd-surface` markers. Also removes 41 dead style blocks, which took nearly all remaining legacy-grey colour usage with them, and a duplicated `plugin-marketplace` test id.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/devtools/src/tabs/marketplace/plugin-card.tsx (1)
118-141: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMount the status live region before the status changes.
role="status"is placed on theShowfallback. That element enters the DOM at the same moment its text appears. Screen readers usually announce a live region only when the content of an already-present region changes, so install progress, success, and failure messages can go unannounced. Render an always-present live region and change only its content.♻️ Proposed refactor
+ <div class={styles().pluginMarketplaceCardStatusLive} role="status"> + <Show when={card.status !== 'idle'}> + <Show when={card.status === 'installing'}>Installing…</Show> + <Show when={card.status === 'success'}>Installed</Show> + <Show when={card.status === 'error'}> + {card.error || 'Failed to install'} + </Show> + </Show> + </div> <Show when={card.status === 'idle'} fallback={ - <div class={styles().pluginMarketplaceCardStatus} role="status"> + <div class={styles().pluginMarketplaceCardStatus} aria-hidden="true">Add
pluginMarketplaceCardStatusLiveas a visually hidden style inpackages/devtools/src/styles/use-styles.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/tabs/marketplace/plugin-card.tsx` around lines 118 - 141, Update the status rendering around the card status Show so an always-present, visually hidden live region uses role="status", while only its text/content changes as card.status transitions through installing, success, and error; keep the visible status UI behavior unchanged. Add the pluginMarketplaceCardStatusLive style in use-styles.ts and apply it to this persistent live region.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/devtools/src/context/use-devtools-context.ts`:
- Around line 106-112: Move the collapsed signal out of module scope and make it
instance-owned for each createCollapsed() or DevTools mount, so isCollapsed,
toggleCollapsed, and setCollapsed operate only on that instance. Preserve the
existing collapsed API while ensuring later mounts do not reset earlier roots,
and add a test mounting two DevTools roots that verifies toggling one does not
affect the other.
In `@packages/devtools/src/styles/use-styles.ts`:
- Around line 544-567: Remove the stale first JSDoc block immediately before
workbenchCollapseToggle. Keep the second JSDoc block, which accurately documents
the toggle’s top positioning against the subheader or header when collapsed.
- Around line 390-395: Update the selector in the grid styling near the
plugins-strip rule to anchor on the shared secondary-tab element or ensure the
SEO strip renderer uses data-testid="plugins-strip". Verify the SEO secondary
strip receives the auto-sized row while its height animates, without changing
the existing header and content rows.
In `@packages/devtools/tests/workbench.test.tsx`:
- Around line 355-357: Update the folded-state assertion in the relevant
workbench test to expect the plugins strip element to remain mounted rather than
be null. Verify its folded accessibility state through the existing inert and
aria-hidden behavior while preserving the expectation that the panel stays open,
retains its height, and keeps the plugin running.
---
Nitpick comments:
In `@packages/devtools/src/tabs/marketplace/plugin-card.tsx`:
- Around line 118-141: Update the status rendering around the card status Show
so an always-present, visually hidden live region uses role="status", while only
its text/content changes as card.status transitions through installing, success,
and error; keep the visible status UI behavior unchanged. Add the
pluginMarketplaceCardStatusLive style in use-styles.ts and apply it to this
persistent live region.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9821b521-eaef-4dfb-886c-994e4348c051
📒 Files selected for processing (29)
.changeset/tanstack-devtools-branding.mddocs/architecture.mddocs/overview.mdpackages/devtools-ui/src/styles/use-styles.tspackages/devtools/src/components/main-panel.tsxpackages/devtools/src/components/tab-content.tsxpackages/devtools/src/components/tanstack-emblem.tsxpackages/devtools/src/components/trigger.tsxpackages/devtools/src/components/workbench-header.tsxpackages/devtools/src/components/workbench-secondary-tabs.tsxpackages/devtools/src/context/use-devtools-context.tspackages/devtools/src/devtools.tsxpackages/devtools/src/styles/use-styles.tspackages/devtools/src/tabs/hotkey-config.tsxpackages/devtools/src/tabs/marketplace/card-utils.test.tspackages/devtools/src/tabs/marketplace/card-utils.tspackages/devtools/src/tabs/marketplace/marketplace-header.tsxpackages/devtools/src/tabs/marketplace/plugin-card.tsxpackages/devtools/src/tabs/marketplace/plugin-utils.test.tspackages/devtools/src/tabs/marketplace/plugin-utils.tspackages/devtools/src/tabs/plugin-marketplace.tsxpackages/devtools/src/tabs/plugins-tab.tsxpackages/devtools/src/tabs/seo-tab/index.tsxpackages/devtools/src/tabs/seo-tab/social-previews.tsxpackages/devtools/src/utils/constants.tspackages/devtools/tests/index.test.tspackages/devtools/tests/semantic-color-usage.test.tspackages/devtools/tests/workbench-secondary-tabs.test.tsxpackages/devtools/tests/workbench.test.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/devtools/src/tabs/seo-tab/index.tsx
- .changeset/tanstack-devtools-branding.md
- docs/overview.md
- packages/devtools/src/tabs/seo-tab/social-previews.tsx
- packages/devtools/tests/semantic-color-usage.test.ts
- packages/devtools/src/components/tab-content.tsx
- packages/devtools/tests/index.test.ts
- packages/devtools/src/tabs/marketplace/marketplace-header.tsx
- docs/architecture.md
- packages/devtools/src/devtools.tsx
- packages/devtools/src/tabs/plugins-tab.tsx
- packages/devtools/src/components/workbench-header.tsx
- packages/devtools/src/utils/constants.ts
- packages/devtools/tests/workbench-secondary-tabs.test.tsx
| const [collapsed, setCollapsed] = createSignal(false) | ||
|
|
||
| export const createCollapsed = () => ({ | ||
| isCollapsed: collapsed, | ||
| toggleCollapsed: () => setCollapsed((previous) => !previous), | ||
| setCollapsed, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Scope collapsed state to each DevTools mount.
collapsed is module-level, so every createCollapsed() caller reads and updates the same signal. If two DevTools roots mount in one document, toggling one root folds the secondary tabs in both roots. A later mount also resets the shared state to false.
Store this signal in instance-owned context, or pass it from DevTools. Add a two-root isolation test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/context/use-devtools-context.ts` around lines 106 -
112, Move the collapsed signal out of module scope and make it instance-owned
for each createCollapsed() or DevTools mount, so isCollapsed, toggleCollapsed,
and setCollapsed operate only on that instance. Preserve the existing collapsed
API while ensuring later mounts do not reset earlier roots, and add a test
mounting two DevTools roots that verifies toggling one does not affect the
other.
| /** | ||
| * A pull tab protruding from the bottom edge of the lowest chrome band — | ||
| * below the secondary strip when one is on screen, below the header when | ||
| * not. It is positioned against the panel rather than nested inside the | ||
| * strip, because the strip scrolls horizontally and would clip it. | ||
| * | ||
| * Collapsed is the exception: the panel is then only as tall as the header | ||
| * and sits flush against the viewport edge, so a downward tab would be off | ||
| * screen. There it flips to the panel's outer edge instead. | ||
| */ | ||
| /** | ||
| * A pull tab protruding from the bottom edge of the subheader, dropping back | ||
| * to the header's bottom edge once the subheader is folded away — so it | ||
| * always hangs off whatever chrome band is lowest, always inside the panel. | ||
| * | ||
| * It is positioned against the panel rather than nested inside the strip, | ||
| * because the strip scrolls horizontally and would clip it. Both bands are | ||
| * border-box, so their hairlines already sit inside these heights. | ||
| */ | ||
| workbenchCollapseToggle: (isCollapsed: boolean) => css` | ||
| position: absolute; | ||
| top: ${isCollapsed | ||
| ? WORKBENCH_HEADER_HEIGHT | ||
| : WORKBENCH_HEADER_HEIGHT + PLUGINS_STRIP_HEIGHT}px; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale duplicate JSDoc block on workbenchCollapseToggle.
Two JSDoc blocks document the same member. The first block (Lines 544-553) states that the toggle flips to the panel's outer edge when collapsed. The implementation does not do that: top moves from WORKBENCH_HEADER_HEIGHT + PLUGINS_STRIP_HEIGHT to WORKBENCH_HEADER_HEIGHT, which is the header's bottom edge. The second block (Lines 554-562) describes the actual behavior. Keep the second block only.
♻️ Proposed fix
- /**
- * A pull tab protruding from the bottom edge of the lowest chrome band —
- * below the secondary strip when one is on screen, below the header when
- * not. It is positioned against the panel rather than nested inside the
- * strip, because the strip scrolls horizontally and would clip it.
- *
- * Collapsed is the exception: the panel is then only as tall as the header
- * and sits flush against the viewport edge, so a downward tab would be off
- * screen. There it flips to the panel's outer edge instead.
- */
/**
* A pull tab protruding from the bottom edge of the subheader, dropping back
* to the header's bottom edge once the subheader is folded away — so it
* always hangs off whatever chrome band is lowest, always inside the panel.
*
* It is positioned against the panel rather than nested inside the strip,
* because the strip scrolls horizontally and would clip it. Both bands are
* border-box, so their hairlines already sit inside these heights.
*/As per coding guidelines "JSDoc should read like documentation as it gets converted to markdown docs for the website".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * A pull tab protruding from the bottom edge of the lowest chrome band — | |
| * below the secondary strip when one is on screen, below the header when | |
| * not. It is positioned against the panel rather than nested inside the | |
| * strip, because the strip scrolls horizontally and would clip it. | |
| * | |
| * Collapsed is the exception: the panel is then only as tall as the header | |
| * and sits flush against the viewport edge, so a downward tab would be off | |
| * screen. There it flips to the panel's outer edge instead. | |
| */ | |
| /** | |
| * A pull tab protruding from the bottom edge of the subheader, dropping back | |
| * to the header's bottom edge once the subheader is folded away — so it | |
| * always hangs off whatever chrome band is lowest, always inside the panel. | |
| * | |
| * It is positioned against the panel rather than nested inside the strip, | |
| * because the strip scrolls horizontally and would clip it. Both bands are | |
| * border-box, so their hairlines already sit inside these heights. | |
| */ | |
| workbenchCollapseToggle: (isCollapsed: boolean) => css` | |
| position: absolute; | |
| top: ${isCollapsed | |
| ? WORKBENCH_HEADER_HEIGHT | |
| : WORKBENCH_HEADER_HEIGHT + PLUGINS_STRIP_HEIGHT}px; | |
| /** | |
| * A pull tab protruding from the bottom edge of the subheader, dropping back | |
| * to the header's bottom edge once the subheader is folded away — so it | |
| * always hangs off whatever chrome band is lowest, always inside the panel. | |
| * | |
| * It is positioned against the panel rather than nested inside the strip, | |
| * because the strip scrolls horizontally and would clip it. Both bands are | |
| * border-box, so their hairlines already sit inside these heights. | |
| */ | |
| workbenchCollapseToggle: (isCollapsed: boolean) => css` | |
| position: absolute; | |
| top: ${isCollapsed | |
| ? WORKBENCH_HEADER_HEIGHT | |
| : WORKBENCH_HEADER_HEIGHT + PLUGINS_STRIP_HEIGHT}px; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/styles/use-styles.ts` around lines 544 - 567, Remove
the stale first JSDoc block immediately before workbenchCollapseToggle. Keep the
second JSDoc block, which accurately documents the toggle’s top positioning
against the subheader or header when collapsed.
Source: Coding guidelines
The `Test` job died with a JavaScript heap OOM and the `E2e` job failed on all eight apps. Two separate causes. The head watcher looped. `createHeadChanges` observes attributes and character data across the whole `<head>` subtree, and goober rewrites a `<style>` tag there on every `css()` call — it even re-stamps the tag's `nonce` attribute each time. So an SEO analysis re-rendered, the re-render emitted CSS, the CSS mutated `<head>`, and the analysis ran again. The loop is synchronous, so no test timeout could break it and the worker ran to the 4 GB heap limit. Stylesheets carry no SEO metadata, so they are filtered out. jsdom multiplied goober's stylesheets. goober finds its single `<style id="_goober">` through `window._goober`, the global a browser creates for any element with an `id`. jsdom does not do that for `<style>`, so goober appended a new sheet on every `css()` call — about 2500 per Workbench mount, never removed. Test 1 took 0.9s and test 20 took 21s. The test setup now gives goober the global a browser would have: 2504 sheets per mount become 8, and the package's 236 tests run in 38s instead of running out of memory. That let `workbench.test.tsx` finish for the first time, which exposed nine assertions still describing the pre-polish design — the header's trailing gutter, the strip's 8px gutter, the 24px resize handle, a fixed 44px grid row, the strip unmounting when folded, and the SEO label foregrounds. Each now matches the shipped Workbench. The redesign also dropped the test hooks `@tanstack/devtools-e2e` locates the panel and header controls with, which is why every e2e app failed on `openViaTrigger()`. The header carries them again and the tab assertions read `data-tsd-selected` instead of the `active` class the old tabs used.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/devtools/src/hooks/use-head-changes.ts`:
- Around line 45-49: Update the mutation observer callback in useHeadChanges to
skip child-list records whose mutation target is a STYLE element before
processing addedNodes and removedNodes. Preserve the existing per-node
isStyleNode checks for elements inserted directly into HEAD, and add a
regression test covering style.textContent replacement without emitting a head
removal.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cf6fd8e-68ce-41ff-9657-94a89fefe38e
📒 Files selected for processing (8)
.changeset/tanstack-devtools-branding.mde2e/apps/react-vite/tests/tabs-and-plugin.spec.tse2e/helpers/src/selectors.tspackages/devtools/src/components/workbench-header.tsxpackages/devtools/src/hooks/use-head-changes.tspackages/devtools/tests/seo-workbench.test.tsxpackages/devtools/tests/test-setup.tspackages/devtools/tests/workbench.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/devtools/src/components/workbench-header.tsx
- .changeset/tanstack-devtools-branding.md
- packages/devtools/tests/seo-workbench.test.tsx
| const isStyleNode = (node: Node): boolean => { | ||
| const element = | ||
| node.nodeType === 3 /* Node.TEXT_NODE */ ? node.parentNode : node | ||
| return (element as Element | null)?.nodeName === 'STYLE' | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'packages/devtools/src/hooks/use-head-changes\.ts|packages/devtools/src/tabs/seo-tab/(serp-preview|social-previews)\.tsx' || true
echo
echo "use-head-changes outline:"
ast-grep outline packages/devtools/src/hooks/use-head-changes.ts --view expanded || true
echo
echo "Relevant use-head-changes lines:"
cat -n packages/devtools/src/hooks/use-head-changes.ts | sed -n '1,120p'
echo
echo "SEO preview relevant usages:"
for f in packages/devtools/src/tabs/seo-tab/serp-preview.tsx packages/devtools/src/tabs/seo-tab/social-previews.tsx; do
echo "### $f"
wc -l "$f"
cat -n "$f" | sed -n '180,230p'
done
echo
echo "Search head changes consumers and goober/style text usage:"
rg -n "useHeadChanges|onChange\\({ kind: (added|removed)|textContent|style\\." packages/devtools/src -SRepository: TanStack/devtools
Length of output: 8787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant consumers:"
rg -n "createHeadChanges|setSerp\\(getSerpFromHead|reports\\(\\)" packages/devtools/src -S
echo
echo "Style/text mutations in packages/devtools/src:"
rg -n "textContent|style\\.nonce|css\\(|useHeadChanges|createHeadChanges|onChange\\(\\{ kind:" packages/devtools/src -S
echo
echo "Search for removedNodes behavior tests or docs if present:"
rg -n "removedNodes|childList|MutationObserver|goober|nonce" packages -SRepository: TanStack/devtools
Length of output: 6289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Node behavior probe:"
node - <<'JS'
class MockElement {
constructor(tagName) {
this.nodeName = tagName.toUpperCase()
this.parentNode = null
this.childNodes = []
}
appendChild(child) {
child.parentNode = this
this.childNodes.push(child)
}
}
class MockText {
constructor(text) {
this.nodeType = 3
this.parentNode = null
this.nodeValue = text
}
}
const style = new MockElement('style')
style.appendChild(new MockText('body{}'))
const removed = style.childNodes[0]
style.childNodes.splice(0, removed.childNodes.length)
removed.parentNode = null
const isStyleNode = (node) => {
const element = node.nodeType === 3 ? node.parentNode : node
return element?.nodeName === 'STYLE'
}
console.log('removed parentNode:', removed.parentNode)
console.log('isStyleNode(removed):', isStyleNode(removed))
console.log('isStyleNode(style):', isStyleNode(style))
JSRepository: TanStack/devtools
Length of output: 863
🌐 Web query:
MDN MutationRecord removedNodes removed element child text node before callback detached parentNode
💡 Result:
The MutationRecord property removedNodes is a NodeList containing all nodes removed from the target of a childList mutation [1][2]. This collection includes both element nodes and text nodes if they were removed from the direct children of the observed target node [3][4]. Key technical points regarding this behavior include: 1. Scope of removedNodes: This property contains only the direct children that were removed from the target node [1]. If a parent node is removed, the MutationRecord for that removal will contain the parent node in its removedNodes list, but it will not list the descendants of that parent [5]. Consequently, if you need to track the removal of a specific deep descendant, you must check whether the removed nodes or their ancestors match the target you are monitoring [5]. 2. Detached state: Once a node is removed, it becomes detached from the DOM tree [6]. Its parentNode property will be null [7]. Because MutationObserver callbacks are asynchronous and batched, the state of the DOM at the time the callback executes may differ from the state at the moment the removal actually occurred [8][5]. 3. NodeList characteristics: The NodeList returned by removedNodes is static, not live [6]. This means that the collection does not update automatically if the DOM changes further after the mutation record was generated [6]. 4. Distinguishing mutations: When performing operations like setting innerHTML or textContent, the browser may remove existing text nodes and replace them with new ones [9][10]. This registers as a childList mutation (with the old text node appearing in removedNodes) rather than a characterData mutation [9][10]. Many developers filter these records by checking if the removed node is an element node (nodeType === 1) to ignore implicit text node removals if they are only interested in structural changes [9][10]. 5. Accessing parent context: Because the removed node's parentNode is null, you cannot retrieve the original parent directly from the removed node instance [7]. You must rely on the MutationRecord.target property, which identifies the node whose children were mutated, to understand the context of the removal [7][8].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/MutationRecord/removedNodes
- 2: https://developer.mozilla.org/en-US/docs/Web/API/MutationRecord
- 3: https://addyosmani.com/blog/mutation-observers/
- 4: https://davidwalsh.name/mutationobserver-api
- 5: https://stackoverflow.com/questions/44935865/detect-when-a-node-is-deleted-or-removed-from-the-dom-because-a-parent-was
- 6: https://stackoverflow.com/questions/78285902/are-the-addednodes-and-removednodes-properties-of-mutationrecord-live-or-static
- 7: https://stackoverflow.com/questions/35056643/how-can-i-get-the-old-parent-of-a-mutationobservers-removednode-before-it-was
- 8: https://stackoverflow.com/questions/49263285/mutationobservers-dom-context-is-lost-because-it-fires-too-late
- 9: https://quirksmode.org/blog/archives/2017/11/mutation_observ.html
- 10: https://www.quirksmode.org/blog/archives/2017/11/mutation_observ.html
Filter child-list records by their mutation target.
When a <style> element replaces its text node via textContent, the observer callback receives that old text node in removedNodes, but its parentNode is already detached. isStyleNode then returns false, so line 70 emits a head removal. Skip childList records whose m.target is a style element before iterating addedNodes and removedNodes. Keep the per-node checks for style elements inserted directly into <head>, and add a regression test for replacing style.textContent.
Proposed fix
if (m.type === 'childList') {
+ if (isStyleNode(m.target)) continue
m.addedNodes.forEach((node) => {
if (!isStyleNode(node)) onChange({ kind: 'added', node }, m)
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/hooks/use-head-changes.ts` around lines 45 - 49, Update
the mutation observer callback in useHeadChanges to skip child-list records
whose mutation target is a STYLE element before processing addedNodes and
removedNodes. Preserve the existing per-node isStyleNode checks for elements
inserted directly into HEAD, and add a regression test covering
style.textContent replacement without emitting a head removal.
…resize (#493) * feat(devtools): add the plugin workspace layout tree The workspace layout becomes a tree of splits and tab groups so plugins can be arranged in rows, columns and stacks instead of one equal-width flex row. This commit is the maths only: no UI is wired up and no dependency is added yet. Everything in `layout-tree.ts` is pure and imports nothing. That is deliberate. jsdom has no layout engine, so `getBoundingClientRect` returns zeros, and rect maths verified through the DOM would only be verifying its own mocks. Keeping it here makes it exhaustively testable — 67 cases in 58ms — and keeps the layout logic out of the components. Every returned tree upholds the same invariants: a group has at least one tab, a split has at least two children, sizes match the child count and sum to 1, the active index names a real tab, and a plugin id appears at most once. `prune` restores them bottom up after any edit, so closing a tab can collapse an emptied group, unwrap a single-child split, and flatten a same-direction nested split without the callers knowing. `repairLayout` cannot throw. A malformed layout is a data problem, the same as the unknown plugin ids that are already pruned on load, and it must not stop the panel from opening; storage *access* errors still propagate. It prunes unknown and duplicated ids, renormalises sizes, clamps the active index, and falls back to salvaging whatever plugin ids it can find from an unrecognisable shape so a bad write costs the arrangement but not the open plugins. The hostile-input test caught a real stack overflow on a self-referencing object, so reads are depth capped and the salvage walk tracks visited objects. Design and the decisions behind it, including the measured bundle cost of each neodrag primitive, are in docs/superpowers/specs/2026-08-07-plugin-layout-tree-design.md. * feat(devtools): make the layout tree the stored truth for open plugins `state.activePlugins` is replaced by `state.layout`. The tree is now the only record of which plugins are open, and `activePlugins` is a memo that flattens it, so the two cannot disagree. Rendering is unchanged: the flattened order feeds the same flex row, so this commit moves the state without moving any pixels. Hydration migrates and repairs. State written before the tree reopens as a single group in the stored order, an existing tree wins over the superseded key, and everything goes through `repairLayout`, which prunes unknown plugin ids exactly as the old `activePlugins` filter did. The result is written back once so the migration does not repeat. Storage *access* errors still propagate. Two things the existing tests caught, both worth recording. `flattenTabs` builds a fresh array each call, so a bare memo made every unrelated store write look like a change and re-ran each plugin's `render` — the activation order test failed with a duplicated entry. The memo now compares contents. `plugin.destroy` cannot yet hang off the pane's own `onCleanup`, which is where the design puts it. The panes live inside the destination-switched subtree, so navigating to Marketplace unmounts them and would destroy every open plugin — "moves among Marketplace and core destinations without plugin destruction" failed immediately. Teardown stays on the close path until the panes live in a container that outlives the navigation, which is the next commit. `MAX_ACTIVE_PLUGINS` stays at 3 for now. Raising it to 9 only makes sense once the workspace can split and scroll, otherwise nine panes share one flex row. * feat(devtools): render the workspace from the layout tree, up to 9 panes The plugin panes move out of the destination-switched subtree into a workspace that is mounted once and hidden rather than unmounted. Each pane is a direct child of that workspace for its whole life and is placed with offsets computed from the tree, so no drag, split or resize ever re-parents it. That is what stops an iframe plugin reloading and a canvas plugin losing its context — the React basic example registers a plugin whose whole body is an iframe. Because the workspace outlives navigation, `plugin.destroy` finally moves to the pane's own `onCleanup`: exactly once, however the pane was closed, and before the node is detached so the plugin can still tidy up. Removing the call from the close path at the same time was necessary, not tidying — with both in place every close destroyed twice, which the lifecycle test caught. `MAX_ACTIVE_PLUGINS` goes from 3 to 9. Panes can now split and stack, so the cap limits how many are open rather than how many fit across. Splitters, tab bars with per-tab close controls, drop-zone highlighting and full keyboard operation all arrive with it. Each gutter is a real focusable `role="separator"` driven by the same arrow/Home/End pattern as the whole-panel resizer, and a tab can be picked up with Enter, moved with the arrows and dropped with Enter, so nothing needs a pointer. A drop that has no room to split becomes a stacked tab instead of being refused. Three things worth recording. `appendPane` exists because `splitAt` was wrong for opening from the strip: it halves the last pane, so three plugins came out 1/2, 1/4, 1/4. Panes opened side by side should match, and a test now pins the thirds. The move hint's id was `${PLUGIN_CONTAINER_ID}-move-hint`, which matches the `[id^="plugin-container-"]` selector the tests use and counted as a phantom pane. PLUGIN_CONTAINER_ID is a public export and the shared prefix of every pane id; nothing else may borrow it. The tab bar is not a `role="tablist"`. Its arrow keys move a pane rather than walking the tabs, so claiming the role would promise a keyboard contract this does not implement. Selection is `aria-pressed`, the close control is a sibling button rather than nested inside the tab, targets are 24px, and the state of a move is narrated through a live region because `aria-grabbed` is deprecated. * feat(devtools): drag panes from the strip, hold to drag, and raise the cap to 18 Builds on the workspace with the interactions that make it usable, and fixes what turned up once it was driven by hand rather than by tests. Dragging. A press only becomes a drag after being **held** for 500ms. A movement threshold was tried first and was wrong: any distance small enough to feel responsive is also small enough that ordinary click jitter crosses it, so clicking a stacked tab resolved a drop target from the pointer sitting over the tab bar and split the pane straight back out. Holding is unambiguous — a click selects, a press picks up. Dropping on a tab bar now always means "put it in this group" rather than splitting its top edge, so the two gestures never compete for the same few pixels. The tab being carried follows the cursor and every surface shows the grabbing cursor while it does. The preview is portalled to the body because `MainPanel` sets a transform, which makes it a containing block, so a `position: fixed` child resolved against the panel and was clipped by the workspace's overflow. Plugins strip. Entries can be held and dragged into the workspace to place a pane where you want it instead of appending it, including onto an empty workspace, where it takes the whole area. The strip now lists only the plugins that are *not* open, so each plugin has exactly one control: its strip entry while closed, its pane tab once open. It folds itself away when everything is open and returns when a plugin closes. `MAX_ACTIVE_PLUGINS` goes 9 -> 18. The tests were already pinned to the constant rather than a literal, so this was a one-line change. Three fixes worth naming. The workspace measured itself once at mount. A hidden element measures zero, every rect derived from a zero box is zero, and hit-testing then silently found nothing — so a drag did nothing at all rather than looking broken. It re-measures when the panel opens or the destination returns, ignores zero measurements, and measures again at the start of every drag. The strip's click-suppression flag was sticky. A drag that ends away from the entry produces no `click` at all, so the flag survived and swallowed the *next* genuine click, which is why opening a plugin started taking several attempts. It resets on each press. The strip-to-workspace handoff moved from module-level state onto the context. This package ships several bundles, so two components can hold different copies of the same module and never see each other's writes. Two e2e cases are `test.fixme` rather than deleted: both drags that start from a strip entry work with real pointer input, verified by hand in two apps, but do not trigger under Playwright's synthetic mouse. Each carries a comment saying what is covered elsewhere and what is left unproven. * fix(devtools): unbreak CI — knip, duplicated Vue plugins, unstable gutters Three separate causes behind the red `Test` and `E2e` jobs. **knip.** `layout-tree.ts` exported five things nothing outside it uses — `isGroup`, `isSplit`, `findGroupById`, `nodeAtPath` and the `Size` type. They are module-private now. The two findings that remain locally (`check-font-assets.mjs`, a `svelte` config hint) reproduce on the untouched base branch and pass on CI. **Vue rendered every plugin twice.** The adapter's `render` and title callbacks appended to `pluginsToRender` / `titlesToRender` without dropping the previous entry for that mount element. `render` is called again whenever the theme or the panel's open state changes, so this was always wrong — it only became visible now that the core keeps one mount node per plugin for its lifetime instead of building a fresh one. Previously each call landed in a new node and the duplicate was never in the document. Both callbacks now replace by id. React was already correct: it keys by element id. **Gutters were rebuilt on every re-measure.** `splitterHandles` returns fresh objects, so a keyed `For` destroyed and recreated every splitter whenever the geometry changed. That threw keyboard focus away mid-resize and left stale element references behind — the cause of both the flaky keyboard-resize test and `boundingBox()` returning null in the drag test. Switched to `Index`, which keeps the elements and updates their values, and the handle is read through its accessor at gesture time so a re-measured gutter still moves the right sizes. The e2e specs also wait for the geometry to settle rather than the tab bars alone, and drive the keyboard through `locator.press` so focus and keypress are one step. react-vite: 30 passed, 2 skipped, no flakes over repeated runs. vue: passing. 323 unit tests green. * fix(devtools): make a tab's close button reliable, and drop the design spec Closing a tab worked only sometimes. The close button sits inside the sortable row's element, so the drag layer saw its pointerdown, decided a sort might be starting, and swallowed the `click` that would have followed — which press landed and which did not came down to a pixel of pointer movement. The close control now stops the pointer events at itself and closes on pointerup, with the `click` handler kept for keyboard activation, which fires no pointer events at all. Closing twice is harmless: the second call finds no such tab and returns the tree unchanged. Verified by closing three panes in a row, each press landing. Also removes `docs/superpowers/specs/2026-08-07-plugin-layout-tree-design.md`. It is a planning artifact, not source, and does not belong in the history. The two earlier commits mention it by path; the reasoning that matters is in the code comments, `docs/plugin-workspace.md` and the changeset. * fix(devtools): take the close button out of the drag surface, stop restyling the host page **Closing a tab.** `SortableRow` is only a data attribute — the drag engine listens globally and walks *up* from whatever the pointer hit looking for that key. Anything inside the row is therefore a drag surface no matter what its own handlers do, so the `stopPropagation` in the previous commit could not have worked. The close button is now a sibling of the sortable row, positioned over the tab's right end, so a press on it never reaches a sortable key. Verified in the browser: pressing the X, moving 6px and holding for 700ms produces no drag preview and still closes the pane, and three consecutive closes all land. **The host page's cursor is not ours to change.** The grabbing cursor was applied to `document.documentElement` with a descendant selector, which forced `cursor: grabbing !important` onto every element of the page under inspection for the length of a drag. It is scoped to the devtools panel now. Measured during a drag: the host `<html>`, `<body>` and `<h1>` all stay at `auto` while the panel shows `grabbing`, and the e2e test asserts both halves of that. Everything else the devtools inject was already contained: an audit of every rule in every injected stylesheet found zero selectors that could match an element the host page owns — each one is scoped to a goober hash, `.tsd-*`, `[data-tsd*]`, `[data-plugin*]` or `#tanstack_devtools`. SSR verified across all three server runtimes: react-start, react-nitro and react-cloudflare e2e all pass, including the server-to-client event bridge. Nothing added here touches a browser global at module scope, and `layout-tree.ts` has no DOM access at all. * chore(devtools): pin @neodrag/solid to 3.0.0-next.11 The new release does not change `splitpane` — that file is byte-identical to `next.10`, which this branch already used. What is new is its documentation page. It does change `sortable`, `drop` and `resize`, and this uses `createSortable`, so the bump is worth taking for whatever landed there. Size goes 59.56 kB -> 61.23 kB, still inside the 65 kB limit. 323 unit tests and 30 react-vite e2e pass on it with no flakes. Still pinned exactly rather than floated on `@next`: v3 is unreleased and its published exports have already drifted from its docs more than once.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
packages/devtools/src/utils/layout-tree.ts (1)
51-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the depth-ceiling comment.
The comment states "Nine panes cannot nest deeper than nine", but
MAX_ACTIVE_PLUGINSis now 18 inpackages/devtools/src/utils/constants.ts. The constant value of 32 still holds. Restate the reasoning against the current pane cap so the ceiling stays justified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/utils/layout-tree.ts` around lines 51 - 56, Update the comment above MAX_STORED_DEPTH to reference the current MAX_ACTIVE_PLUGINS cap of 18 instead of nine, while preserving the explanation that values beyond the ceiling may indicate corrupt or self-referencing data and must not be recursively traversed. Keep MAX_STORED_DEPTH at 32.e2e/apps/react-vite/tests/plugin-workspace.spec.ts (1)
259-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo
test.fixmetests leave the strip-to-workspace pointer handoff unverified.The reasons are documented clearly, which is good. The gap covers a headline feature of this PR: dragging a plugin out of the strip to choose its placement. Playwright's synthetic mouse usually needs the hold to be produced with explicit timed steps rather than
mouse.down()followed immediately bymouse.move(); dispatchingpointermoveevents during the hold window and adding awaitForTimeout(DRAG_HOLD_MS + margin)betweendownand the first move often unblocks this pattern.Do you want me to open a tracking issue for the two skipped tests, or draft a revised
pressAndHoldhelper ine2e/helpers/src/page-objects/devtools.ts?Also applies to: 410-435
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/apps/react-vite/tests/plugin-workspace.spec.ts` around lines 259 - 298, Replace the strip-to-workspace test.fixme cases around the dragging scenario with working tests by updating the pressAndHold helper in DevtoolsPage to perform an explicit timed hold, including pointermove events and a wait beyond DRAG_HOLD_MS before the first drag movement. Preserve the existing drop assertions and apply the same helper behavior to both strip-to-workspace tests.packages/devtools/src/components/plugins-strip.tsx (1)
17-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare
DRAG_HOLD_MSfrom one module.
plugin-workspace.tsxdefines the sameDRAG_HOLD_MS = 500constant at line 70, ande2e/helpers/src/selectors.tsexportsDRAG_HOLD_MS = 650. Three separate definitions of one timing contract will drift. Move the value intopackages/devtools/src/utils/constants.ts, import it in both components, and derive the e2e margin from it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/components/plugins-strip.tsx` around lines 17 - 21, Move the shared DRAG_HOLD_MS timing constant into packages/devtools/src/utils/constants.ts, then import and reuse it in plugins-strip.tsx and plugin-workspace.tsx instead of defining local copies. Update e2e/helpers/src/selectors.ts to derive its exported timing value from the shared constant while preserving the existing 150ms margin.e2e/helpers/src/page-objects/devtools.ts (2)
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
directionandzoneas unions instead ofstring.
dragTabToZonevalidateszoneat runtime and throws on an unknown value.movePaneWithKeyboarddoes not validatedirectionat all: a wrong key name presses an unrecognised key, and the test then fails on a later assertion with no indication of the cause. Union types catch both mistakes at compile time and remove the need for the runtime check.♻️ Proposed types
+type Zone = 'left' | 'right' | 'top' | 'bottom' | 'center' +type ArrowKey = 'ArrowLeft' | 'ArrowRight' | 'ArrowUp' | 'ArrowDown'- async movePaneWithKeyboard(pluginId: string, direction: string) { + async movePaneWithKeyboard(pluginId: string, direction: ArrowKey) {- async dragTabToZone(pluginId: string, targetPluginId: string, zone: string) { + async dragTabToZone( + pluginId: string, + targetPluginId: string, + zone: Zone, + ) {Also applies to: 98-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/helpers/src/page-objects/devtools.ts` at line 90, Change the direction parameter of movePaneWithKeyboard and the zone parameter of dragTabToZone from string to unions of their supported literal values. Preserve the existing behavior while allowing TypeScript to reject unsupported directions and zones at compile time, and remove any now-unnecessary runtime validation if applicable.
79-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport the storage key rather than repeating the literal.
storedLayouthardcodes'tanstack_devtools_state'. The product defines the same value asTANSTACK_DEVTOOLS_STATEinpackages/devtools/src/utils/storage.ts. If the key changes, this helper returnsundefinedinstead of failing, and the tests that read the layout assert againstundefined. Import the exported constant if the e2e helpers can depend on the package; otherwise add a comment naming the source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/helpers/src/page-objects/devtools.ts` around lines 79 - 84, Update storedLayout to use the exported TANSTACK_DEVTOOLS_STATE constant from the devtools storage utility instead of the hardcoded localStorage key, preserving the existing null and layout parsing behavior; only add a source-of-truth comment if importing the package is not feasible.packages/devtools/src/utils/get-default-active-plugins.test.ts (3)
167-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove this test: it duplicates the case at lines 72-103.
Both cases pass five
defaultOpen: trueplugins and assert that all five are returned. The title claims the test stops at the limit, but five is below the cap of 18, so no limiting occurs. The inline comment at lines 192-193 states this. The cap is covered at lines 204-212. Delete this case, or change its inputs so it actually crosses the cap.Also applies to: 192-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/utils/get-default-active-plugins.test.ts` at line 167, Remove the duplicate test case titled “should stop at MAX_ACTIVE_PLUGINS limit when more plugins have defaultOpen: true” from get-default-active-plugins tests, including its associated inline comment and assertions; the actual cap behavior is already covered elsewhere.
72-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo cap-named tests no longer reach the cap. Raising
MAX_ACTIVE_PLUGINSfrom 3 to 18 left both tests supplying fivedefaultOpenplugins. Neither crosses the limit, so both now assert the same thing asshould activate plugins with defaultOpen: true, and their titles claim coverage that the new test at lines 204-212 actually provides.
packages/devtools/src/utils/get-default-active-plugins.test.ts#L72-L103: rename the case to describe activation below the cap, becauseplugins.slice(0, MAX_ACTIVE_PLUGINS)returns all five inputs and thetoBeLessThanOrEqualassertion is trivially true.packages/devtools/src/utils/get-default-active-plugins.test.ts#L167-L202: delete this case as a duplicate of the one above, or raise its plugin count pastMAX_ACTIVE_PLUGINSso it exercises the limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/utils/get-default-active-plugins.test.ts` around lines 72 - 103, Update packages/devtools/src/utils/get-default-active-plugins.test.ts at lines 72-103 by renaming the test to describe activation below the MAX_ACTIVE_PLUGINS cap. At lines 167-202, either remove the duplicate cap-named test or increase its defaultOpen plugin fixture count beyond MAX_ACTIVE_PLUGINS so it verifies truncation; ensure the cap behavior is covered by an input that exceeds the constant.
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename this test: it no longer exercises the limit.
The body supplies five plugins and
MAX_ACTIVE_PLUGINSis 18.plugins.slice(0, MAX_ACTIVE_PLUGINS)therefore returns all five, so the assertion is satisfied for any implementation that returns everydefaultOpenplugin. ThetoBeLessThanOrEqualcheck is also trivially true. The real cap coverage is the new test at lines 204-212. Rename this case to describe what it verifies.📝 Proposed rename
- it('should limit defaultOpen plugins to MAX_ACTIVE_PLUGINS', () => { + it('should activate every defaultOpen plugin when the count is under MAX_ACTIVE_PLUGINS', () => {Also applies to: 97-102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/devtools/src/utils/get-default-active-plugins.test.ts` at line 72, Rename the test case around the defaultOpen plugin selection to describe its actual behavior of returning the default-open plugins, rather than claiming it verifies MAX_ACTIVE_PLUGINS enforcement; leave the separate cap-coverage test unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/apps/react-vite/tests/plugin-workspace.spec.ts`:
- Around line 121-131: Rename the test case title around dragTabToZone to state
that dropping on the bottom edge splits into a column, matching the existing
'bottom' zone and layout.dir === 'col' assertion; leave the test behavior
unchanged.
In `@e2e/helpers/src/page-objects/devtools.ts`:
- Around line 119-124: Update both drag helpers, including dragTabToZone and the
corresponding wait near the other mouse.down call, to wrap the drag-preview wait
in try/finally and call page.mouse.up() when the wait fails, ensuring the button
is released before the timeout propagates.
In `@e2e/helpers/src/selectors.ts`:
- Around line 3-8: Update the product-side drag threshold constants in
plugins-strip.tsx and plugin-workspace.tsx to 650, keep
selectors.ts:DRAG_HOLD_MS synchronized, and rewrite its comment to describe the
workspace drag-start threshold used by the wait timeout.
In `@packages/devtools/package.json`:
- Line 70: Update the `@neodrag/solid` dependency in the published devtools
package from the prerelease 3.0.0-next.11 to the current stable 2.3.1, unless
the package requires the next major line; if retaining the prerelease, document
the specific incompatibility that necessitates it.
In `@packages/devtools/src/components/plugin-workspace.tsx`:
- Around line 776-782: Update the aria-valuenow calculation on SplitterHandle to
normalize the handle position relative to the parent split's origin before
dividing by handle().extent, using the horizontal origin for row direction and
vertical origin for column direction. Preserve the existing rounding and 0–100
scale.
- Around line 296-304: Update the createEffect render flow around pluginRefs()
so registering or unregistering one pane does not rerender every mounted plugin.
Keep the ref lookup untracked or otherwise guard each plugin with its last
render inputs, while still triggering renders for activePlugins(), theme(), and
props.isOpen changes and rendering newly mounted panes once.
In `@packages/devtools/src/context/use-devtools-context.ts`:
- Around line 49-54: Route layout updates through replaceLayout rather than
directly spreading state.layout. In
packages/devtools/src/context/use-devtools-context.ts:49-54, update setLayout to
call the context’s replaceLayout implementation; in
packages/devtools/src/context/devtools-context.tsx:258-272, expose or consume
replaceLayout in the plugin context so createPlugins() uses the reconciled tree
write and split panes can collapse when emptied.
In `@packages/devtools/src/utils/layout-tree.test.ts`:
- Around line 220-224: Split the combined test around moveTab into separate
cases: keep the unknown-group assertion verifying the tree remains unchanged,
and add an unknown-tab case explicitly asserting that tab "zz" is inserted into
group "g0". Update the test descriptions to match these distinct behaviors.
---
Nitpick comments:
In `@e2e/apps/react-vite/tests/plugin-workspace.spec.ts`:
- Around line 259-298: Replace the strip-to-workspace test.fixme cases around
the dragging scenario with working tests by updating the pressAndHold helper in
DevtoolsPage to perform an explicit timed hold, including pointermove events and
a wait beyond DRAG_HOLD_MS before the first drag movement. Preserve the existing
drop assertions and apply the same helper behavior to both strip-to-workspace
tests.
In `@e2e/helpers/src/page-objects/devtools.ts`:
- Line 90: Change the direction parameter of movePaneWithKeyboard and the zone
parameter of dragTabToZone from string to unions of their supported literal
values. Preserve the existing behavior while allowing TypeScript to reject
unsupported directions and zones at compile time, and remove any now-unnecessary
runtime validation if applicable.
- Around line 79-84: Update storedLayout to use the exported
TANSTACK_DEVTOOLS_STATE constant from the devtools storage utility instead of
the hardcoded localStorage key, preserving the existing null and layout parsing
behavior; only add a source-of-truth comment if importing the package is not
feasible.
In `@packages/devtools/src/components/plugins-strip.tsx`:
- Around line 17-21: Move the shared DRAG_HOLD_MS timing constant into
packages/devtools/src/utils/constants.ts, then import and reuse it in
plugins-strip.tsx and plugin-workspace.tsx instead of defining local copies.
Update e2e/helpers/src/selectors.ts to derive its exported timing value from the
shared constant while preserving the existing 150ms margin.
In `@packages/devtools/src/utils/get-default-active-plugins.test.ts`:
- Line 167: Remove the duplicate test case titled “should stop at
MAX_ACTIVE_PLUGINS limit when more plugins have defaultOpen: true” from
get-default-active-plugins tests, including its associated inline comment and
assertions; the actual cap behavior is already covered elsewhere.
- Around line 72-103: Update
packages/devtools/src/utils/get-default-active-plugins.test.ts at lines 72-103
by renaming the test to describe activation below the MAX_ACTIVE_PLUGINS cap. At
lines 167-202, either remove the duplicate cap-named test or increase its
defaultOpen plugin fixture count beyond MAX_ACTIVE_PLUGINS so it verifies
truncation; ensure the cap behavior is covered by an input that exceeds the
constant.
- Line 72: Rename the test case around the defaultOpen plugin selection to
describe its actual behavior of returning the default-open plugins, rather than
claiming it verifies MAX_ACTIVE_PLUGINS enforcement; leave the separate
cap-coverage test unchanged.
In `@packages/devtools/src/utils/layout-tree.ts`:
- Around line 51-56: Update the comment above MAX_STORED_DEPTH to reference the
current MAX_ACTIVE_PLUGINS cap of 18 instead of nine, while preserving the
explanation that values beyond the ceiling may indicate corrupt or
self-referencing data and must not be recursively traversed. Keep
MAX_STORED_DEPTH at 32.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 192f5146-fb11-40ba-9c2a-34fb30440bde
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.changeset/plugin-workspace-layout-tree.mddocs/config.jsondocs/plugin-workspace.mde2e/apps/react-vite/tests/plugin-workspace.spec.tse2e/helpers/src/index.tse2e/helpers/src/page-objects/devtools.tse2e/helpers/src/selectors.tspackage.jsonpackages/devtools/package.jsonpackages/devtools/src/components/plugin-workspace.tsxpackages/devtools/src/components/plugins-strip.tsxpackages/devtools/src/components/tab-content.tsxpackages/devtools/src/components/workbench-secondary-tabs.tsxpackages/devtools/src/context/devtools-context.test.tspackages/devtools/src/context/devtools-context.tsxpackages/devtools/src/context/devtools-store.tspackages/devtools/src/context/use-devtools-context.tspackages/devtools/src/styles/use-styles.tspackages/devtools/src/tabs/index.tsxpackages/devtools/src/tabs/plugins-tab.tsxpackages/devtools/src/utils/constants.tspackages/devtools/src/utils/get-default-active-plugins.test.tspackages/devtools/src/utils/layout-tree.test.tspackages/devtools/src/utils/layout-tree.tspackages/devtools/tests/seo-workbench.test.tsxpackages/devtools/tests/workbench.test.tsxpackages/vue-devtools/src/vue-devtools.vue
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/devtools/src/components/workbench-secondary-tabs.tsx
- packages/devtools/tests/seo-workbench.test.tsx
- packages/devtools/src/tabs/index.tsx
- packages/devtools/src/components/tab-content.tsx
- packages/devtools/tests/workbench.test.tsx
- packages/devtools/src/styles/use-styles.ts
| test('dropping a tab on a bottom edge splits into a row', async ({ page }) => { | ||
| const dt = new DevtoolsPage(page) | ||
| await openTwoPanes(dt) | ||
|
|
||
| await dt.dragTabToZone(PROBE, DEMO, 'bottom') | ||
| const layout = (await dt.storedLayout()) as { dir?: string; kind?: string } | ||
| // Either a column split was created, or the pane was too short to split and it | ||
| // stacked instead. Both are correct; a silent no-op is not. | ||
| const stacked = (await dt.groupTabBars().count()) === 1 | ||
| expect(stacked || layout.dir === 'col').toBe(true) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The test title contradicts the assertion.
The title says "splits into a row", but the drop zone is bottom and the assertion checks layout.dir === 'col'. A bottom drop creates a column split. Rename the test to "dropping a tab on a bottom edge splits into a column".
💚 Proposed title fix
-test('dropping a tab on a bottom edge splits into a row', async ({ page }) => {
+test('dropping a tab on a bottom edge splits into a column', async ({
+ page,
+}) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('dropping a tab on a bottom edge splits into a row', async ({ page }) => { | |
| const dt = new DevtoolsPage(page) | |
| await openTwoPanes(dt) | |
| await dt.dragTabToZone(PROBE, DEMO, 'bottom') | |
| const layout = (await dt.storedLayout()) as { dir?: string; kind?: string } | |
| // Either a column split was created, or the pane was too short to split and it | |
| // stacked instead. Both are correct; a silent no-op is not. | |
| const stacked = (await dt.groupTabBars().count()) === 1 | |
| expect(stacked || layout.dir === 'col').toBe(true) | |
| }) | |
| test('dropping a tab on a bottom edge splits into a column', async ({ | |
| page, | |
| }) => { | |
| const dt = new DevtoolsPage(page) | |
| await openTwoPanes(dt) | |
| await dt.dragTabToZone(PROBE, DEMO, 'bottom') | |
| const layout = (await dt.storedLayout()) as { dir?: string; kind?: string } | |
| // Either a column split was created, or the pane was too short to split and it | |
| // stacked instead. Both are correct; a silent no-op is not. | |
| const stacked = (await dt.groupTabBars().count()) === 1 | |
| expect(stacked || layout.dir === 'col').toBe(true) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/apps/react-vite/tests/plugin-workspace.spec.ts` around lines 121 - 131,
Rename the test case title around dragTabToZone to state that dropping on the
bottom edge splits into a column, matching the existing 'bottom' zone and
layout.dir === 'col' assertion; leave the test behavior unchanged.
| await this.page.mouse.down() | ||
| // Wait for the drag to have actually begun, not a fixed duration — see | ||
| // `pressAndHold`. | ||
| await this.page | ||
| .getByTestId(SELECTORS.dragPreview) | ||
| .waitFor({ state: 'visible', timeout: DRAG_HOLD_MS * 8 }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Release the mouse button if the drag preview never appears.
Both helpers call mouse.down() and then wait for the drag preview. If the wait times out, the exception propagates while the button is still pressed. Playwright keeps mouse state for the whole page context, so every later test in the same context starts with a held button and fails for an unrelated reason. Wrap the wait in try/finally and release the button on failure.
🛡️ Proposed fix
async pressAndHold(locator: Locator) {
const box = await locator.boundingBox()
if (box === null) throw new Error('cannot hold an element with no box')
await this.page.mouse.move(box.x + box.width / 2, box.y + box.height / 2)
await this.page.mouse.down()
- await this.page
- .getByTestId(SELECTORS.dragPreview)
- .waitFor({ state: 'visible', timeout: DRAG_HOLD_MS * 8 })
+ try {
+ await this.page
+ .getByTestId(SELECTORS.dragPreview)
+ .waitFor({ state: 'visible', timeout: DRAG_HOLD_MS * 8 })
+ } catch (error) {
+ await this.page.mouse.up()
+ throw error
+ }
}Apply the same pattern to the wait in dragTabToZone at lines 122-124.
Also applies to: 143-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/helpers/src/page-objects/devtools.ts` around lines 119 - 124, Update both
drag helpers, including dragTabToZone and the corresponding wait near the other
mouse.down call, to wrap the drag-preview wait in try/finally and call
page.mouse.up() when the wait fails, ensuring the button is released before the
timeout propagates.
| /** | ||
| * How long a tab or strip entry must be held before a drag begins. Mirrors | ||
| * `DRAG_HOLD_MS` in the workspace; a little longer here so a slow machine does not | ||
| * release the button early. | ||
| */ | ||
| export const DRAG_HOLD_MS = 650 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the e2e mirror against the product-side DRAG_HOLD_MS definition.
rg -nP --type=ts --type=tsx -g '!e2e/**' '\bDRAG_HOLD_MS\b' -C 3 || \
rg -nP -g '*.ts' -g '*.tsx' -g '!e2e/**' '\bDRAG_HOLD_MS\b' -C 3Repository: TanStack/devtools
Length of output: 187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidates =="
fd -a 'selectors\.ts$|devtools\.ts$|packages|apps|src' . | sed 's#^\./##' | head -200
echo
echo "== DRAG_HOLD_MS occurrences, excluding e2e =="
rg -n -C 4 '\bDRAG_HOLD_MS\b' -g '!e2e/**' .
echo
echo "== relevant e2e occurrences =="
rg -n -C 4 '\bDRAG_HOLD_MS\b' e2e || true
echo
echo "== selector file =="
fd -a 'selectors\.ts$' e2e/helpers/src | while read -r f; do
echo "--- $f"
cat -n "$f"
done
echo
echo "== page-objects devtools outline =="
ast-grep outline e2e/helpers/src/page-objects/devtools.ts --view expanded || true
echo
echo "== devtools relevant slices =="
wc -l e2e/helpers/src/page-objects/devtools.ts
sed -n '1,320p' e2e/helpers/src/page-objects/devtools.ts | cat -nRepository: TanStack/devtools
Length of output: 19822
Correct the mirrored drag-hold constant.
The e2e value is 650 while packages/devtools/src/components/plugins-strip.tsx and packages/devtools/src/components/plugin-workspace.tsx both use 500. Update the product-side threshold and bring e2e/helpers/src/selectors.ts:DRAG_HOLD_MS back in sync, then rewrite the comment to describe it as the workspace's drag start threshold used by the wait timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/helpers/src/selectors.ts` around lines 3 - 8, Update the product-side
drag threshold constants in plugins-strip.tsx and plugin-workspace.tsx to 650,
keep selectors.ts:DRAG_HOLD_MS synchronized, and rewrite its comment to describe
the workspace drag-start threshold used by the wait timeout.
| "build": "tsup" | ||
| }, | ||
| "dependencies": { | ||
| "@neodrag/solid": "3.0.0-next.11", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check published versions and dist-tags for `@neodrag/solid`.
curl -s https://registry.npmjs.org/@neodrag/solid | jq '{latest: .["dist-tags"], versions: (.versions | keys)}'Repository: TanStack/devtools
Length of output: 800
Prefer the stable Neodrag Solid release for the published devtools package.
3.0.0-next.11 is still a prerelease and not behind any stable dist-tag; 2.3.1 is the current latest stable. Use the stable release when migration to the next line is not required, or document the exact prerelease-pinning reason if a stable version is incompatible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/package.json` at line 70, Update the `@neodrag/solid`
dependency in the published devtools package from the prerelease 3.0.0-next.11
to the current stable 2.3.1, unless the package requires the next major line; if
retaining the prerelease, document the specific incompatibility that
necessitates it.
| createEffect(() => { | ||
| for (const pluginId of activePlugins()) { | ||
| const plugin = pluginById(pluginId) | ||
| const ref = pluginRefs().get(pluginId) | ||
| if (plugin && ref) { | ||
| plugin.render(ref, { theme: theme(), devtoolsOpen: props.isOpen }) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The render effect re-runs for every open plugin when any pane ref registers.
The effect reads pluginRefs(). setPluginRefs creates a new Map each time a pane mounts or unmounts, so the signal changes identity. Opening or closing one plugin therefore calls render again on every already-mounted plugin with the same mount node. A plugin that appends nodes in render instead of replacing them will duplicate its content.
Track the last render inputs per plugin, or read the ref map untracked and drive the effect only from activePlugins(), theme(), and props.isOpen.
🐛 Sketch of a guarded render
+ const rendered = new Map<string, string>()
createEffect(() => {
+ const key = `${theme()}:${props.isOpen}`
for (const pluginId of activePlugins()) {
const plugin = pluginById(pluginId)
- const ref = pluginRefs().get(pluginId)
- if (plugin && ref) {
+ const ref = untrack(() => pluginRefs().get(pluginId))
+ if (plugin && ref && rendered.get(pluginId) !== key) {
+ rendered.set(pluginId, key)
plugin.render(ref, { theme: theme(), devtoolsOpen: props.isOpen })
}
}
})A newly mounted pane still needs one render, so keep a separate signal or effect keyed only by the pane ids.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/components/plugin-workspace.tsx` around lines 296 -
304, Update the createEffect render flow around pluginRefs() so registering or
unregistering one pane does not rerender every mounted plugin. Keep the ref
lookup untracked or otherwise guard each plugin with its last render inputs,
while still triggering renders for activePlugins(), theme(), and props.isOpen
changes and rendering newly mounted panes once.
| aria-valuenow={Math.round( | ||
| ((handle().dir === 'row' | ||
| ? handle().rect.left | ||
| : handle().rect.top) / | ||
| Math.max(handle().extent, 1)) * | ||
| 100, | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
aria-valuenow is wrong for a nested split.
handle().rect.left and handle().rect.top are absolute positions in workspace coordinates. handle().extent is only the parent split's own usable extent. For a split that does not start at the workspace origin, the ratio exceeds the declared aria-valuemax of 100. Subtract the parent split's origin before dividing, or expose that origin on SplitterHandle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/components/plugin-workspace.tsx` around lines 776 -
782, Update the aria-valuenow calculation on SplitterHandle to normalize the
handle position relative to the parent split's origin before dividing by
handle().extent, using the horizontal origin for row direction and vertical
origin for column direction. Preserve the existing rounding and 0–100 scale.
| const setLayout = (next: LayoutNode | null) => { | ||
| setStore((previous) => ({ | ||
| ...previous, | ||
| state: { ...previous.state, layout: next }, | ||
| })) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate all callers of replaceLayout and all layout write paths.
set -euo pipefail
echo '--- replaceLayout references ---'
rg -nP -C4 '\breplaceLayout\b' -g '*.ts' -g '*.tsx' || echo 'no matches'
echo '--- setLayout definitions and call sites ---'
rg -nP -C4 '\bsetLayout\b' -g '*.ts' -g '*.tsx' || echo 'no matches'
echo '--- direct store writes touching layout ---'
rg -nP -C4 "setStore\(\s*'state'|state:\s*\{[^}]*layout" -g '*.ts' -g '*.tsx' || echo 'no matches'Repository: TanStack/devtools
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tracked relevant files ---'
git ls-files | rg 'packages/devtools/src/(context/(use-devtools-context|devtools-context)|components/plugin-workspace)\.(ts|tsx)$' || true
echo '--- outline use-devtools-context.ts ---'
ast-grep outline packages/devtools/src/context/use-devtools-context.ts --view expanded || true
echo '--- outline devtools-context.tsx (focused symbols) ---'
ast-grep outline packages/devtools/src/context/devtools-context.tsx --match createDevtoolsContext --view expanded || true
echo '--- use-devtools-context.ts lines 1-140 ---'
cat -n packages/devtools/src/context/use-devtools-context.ts | sed -n '1,140p'
echo '--- devtools-context.tsx lines 220-290 ---'
cat -n packages/devtools/src/context/devtools-context.tsx | sed -n '220,290p'
echo '--- setLayout all references ---'
rg -n -C3 '\bsetLayout\b' packages/devtools/src || true
echo '--- plugin-workspace relevant sections ---'
rg -n -C2 'setLayout|setTabs|moveTab|activateTab|closeTab|resize|commitDrop' packages/devtools/src/components/plugin-workspace.tsx packages/devtools/src/components || trueRepository: TanStack/devtools
Length of output: 36289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- devtools-context exports and imports ---'
rg -n -C2 'createDevtoolsContext|DevtoolsContext|setStore|replaceLayout' packages/devtools/src/context packages/devtools/src -g '*.ts' -g '*.tsx' || true
echo '--- plugin-workspace layout mutation call sites focused ---'
python3 - <<'PY'
from pathlib import Path
p = Path('packages/devtools/src/components/plugin-workspace.tsx')
text = p.read_text()
for needle in ['commitDrop', 'startSplitterDrag', 'startTabDrag', 'onReorder', 'onTransfer', 'onSelect', 'onClose']:
idx = text.find(needle)
print(f'--- {needle} at {idx} ---')
if idx >= 0:
start = max(text.rfind('\n\n', 0, idx - 1), 0)
end = text.find('\n\n', idx + len(needle))
print(text[start:end][:1200])
PY
echo '--- layout-tree closeTab implementation ---'
cat -n packages/devtools/src/utils/layout-tree.ts | sed -n '1,220p'
echo '--- package manifests for solid-js presence/version ---'
for f in package.json packages/devtools/package.json; do
if [ -f "$f" ]; then
echo "### $f"
jq -r '.dependencies["solid-js"]? // .devDependencies["solid-js"]? // "solid-js not listed"' "$f"
fi
doneRepository: TanStack/devtools
Length of output: 35365
Route plugin layout writes through replaceLayout.
setLayout still mutates state.layout by partial store spread, and PluginWorkspace routes tab reorder, transfer, select, close, drop, split, and resize writes through it. replaceLayout has the documented reconcile tree write, but createPlugins() does not consume it. Move replaceLayout into the plugin context and use it from setLayout so closing the last panes in a split can actually collapse.
📍 Affects 2 files
packages/devtools/src/context/use-devtools-context.ts#L49-L54(this comment)packages/devtools/src/context/devtools-context.tsx#L258-L272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/context/use-devtools-context.ts` around lines 49 - 54,
Route layout updates through replaceLayout rather than directly spreading
state.layout. In packages/devtools/src/context/use-devtools-context.ts:49-54,
update setLayout to call the context’s replaceLayout implementation; in
packages/devtools/src/context/devtools-context.tsx:258-272, expose or consume
replaceLayout in the plugin context so createPlugins() uses the reconciled tree
write and split panes can collapse when emptied.
| it('is a no-op for an unknown group or tab', () => { | ||
| const tree = group('g0', ['a']) | ||
| expect(moveTab(tree, 'a', 'gX', 0)).toEqual(tree) | ||
| expect(moveTab(tree, 'zz', 'g0', 0)).toEqual(group('g0', ['zz', 'a'], 0)) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect moveTab to confirm whether an absent tab id is inserted or ignored.
fd -t f 'layout-tree.ts' packages | xargs -r ast-grep run --lang typescript \
--pattern 'export const moveTab = $$$'
fd -t f 'layout-tree.ts' packages | xargs -r rg -n -A 30 'export const moveTab'Repository: TanStack/devtools
Length of output: 4098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg 'packages/devtools/src/utils/layout-tree\.(ts|test\.ts)$|createPlugins|activePlugins' || true
echo
echo "== layout-tree relevant sections =="
sed -n '1,90p' packages/devtools/src/utils/layout-tree.ts
sed -n '220,295p' packages/devtools/src/utils/layout-tree.ts
echo
echo "== test section =="
sed -n '200,235p' packages/devtools/src/utils/layout-tree.test.ts
echo
echo "== createPlugins references =="
rg -n "createPlugins|activePlugins|openPlugins|moveTab" packages/devtools/src -g '*.ts' -g '*.tsx'Repository: TanStack/devtools
Length of output: 13878
Split the no-op cases for unknown move_tab targets.
moveTab(tree, 'zz', 'g0', 0) inserts zz into g0, but the test title says an unknown tab must be a no-op. Split this into two cases: unknown group returns unchanged; unknown tab inserts zz. This keeps the expected behavior explicit and avoids passing a stale tab id through setLayout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/utils/layout-tree.test.ts` around lines 220 - 224,
Split the combined test around moveTab into separate cases: keep the
unknown-group assertion verifying the tree remains unchanged, and add an
unknown-tab case explicitly asserting that tab "zz" is inserted into group "g0".
Update the test descriptions to match these distinct behaviors.
2 participants
Summary
Test plan
git diff --check.Summary by CodeRabbit