fix(devtools): isolate panel state per QueryDevTools instance#10799
Open
AliMahmoudDev wants to merge 2 commits into
Open
fix(devtools): isolate panel state per QueryDevTools instance#10799AliMahmoudDev wants to merge 2 commits into
AliMahmoudDev wants to merge 2 commits into
Conversation
When multiple QueryDevTools instances exist on a page (e.g. micro-frontends with separate QueryClients), shared module-level signals caused state leakage between instances - selecting a query in one devtools would affect the other. Fixes TanStack#9681 Changes: - Create DevtoolsPanelContext to scope panel state per instance - Move selectedQueryHash, selectedMutationId, panelWidth, and offline signals from module-level to context-based (per-instance) - Add DevtoolsPanelProvider wrapper in DevtoolsComponent and DevtoolsPanelComponent - Each QueryDevtools mount now gets its own isolated state
Contributor
📝 WalkthroughWalkthroughMultiple DevTools instances now maintain isolated panel state through a scoped Solid context. A new ChangesDevTools Instance Isolation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/query-devtools/src/Devtools.tsx (1)
1800-1811:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
QueryStatusreferences undefined variables after context migration.The
showLabelmemo referencesselectedQueryHash()andpanelWidth()directly, but these module-level signals were moved into the context. This component doesn't calluseDevtoolsPanelState(), so these will beundefinedat runtime, causing a crash.🐛 Proposed fix
const QueryStatus: Component<QueryStatusProps> = (props) => { const theme = useTheme() const css = useQueryDevtoolsContext().shadowDOMTarget ? goober.css.bind({ target: useQueryDevtoolsContext().shadowDOMTarget }) : goober.css const styles = createMemo(() => { return theme() === 'dark' ? darkStyles(css) : lightStyles(css) }) + const { selectedQueryHash, panelWidth } = useDevtoolsPanelState() const { colors, alpha } = tokens🤖 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/query-devtools/src/Devtools.tsx` around lines 1800 - 1811, The showLabel createMemo uses module-level signals selectedQueryHash() and panelWidth() which were moved into context; call useDevtoolsPanelState() at the top of the component to get the context-provided selectedQueryHash and panelWidth (or destructure { selectedQueryHash, panelWidth } from useDevtoolsPanelState()) and replace direct references so showLabel uses the context signals instead of undefined globals; ensure you import useDevtoolsPanelState if not already.
🤖 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/query-devtools/src/Devtools.tsx`:
- Around line 108-117: The DevtoolsPanelState interface is using Signal<T> for
properties but the provider exposes Accessor and Setter separately; update the
interface to use Accessor<T> for the read side (replace Signal<string | null> /
Signal<number | null> / Signal<number> / Signal<boolean> with Accessor<string |
null> / Accessor<number | null> / Accessor<number> / Accessor<boolean>
respectively) for selectedQueryHash, selectedMutationId, panelWidth, and
offline, and ensure Accessor and Setter are imported from "solid-js" alongside
Setter in the file.
---
Outside diff comments:
In `@packages/query-devtools/src/Devtools.tsx`:
- Around line 1800-1811: The showLabel createMemo uses module-level signals
selectedQueryHash() and panelWidth() which were moved into context; call
useDevtoolsPanelState() at the top of the component to get the context-provided
selectedQueryHash and panelWidth (or destructure { selectedQueryHash, panelWidth
} from useDevtoolsPanelState()) and replace direct references so showLabel uses
the context signals instead of undefined globals; ensure you import
useDevtoolsPanelState if not already.
🪄 Autofix (Beta)
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
Run ID: fa26897c-6124-4b60-8cb8-ff938e462862
📒 Files selected for processing (3)
packages/query-devtools/src/Devtools.tsxpackages/query-devtools/src/DevtoolsComponent.tsxpackages/query-devtools/src/DevtoolsPanelComponent.tsx
- Fix DevtoolsPanelState interface: use Accessor<T> instead of Signal<T> for read-side properties (SolidJS signals are [Accessor, Setter] tuples) - Add useDevtoolsPanelState() call to QueryStatus component which was referencing the old module-level selectedQueryHash and panelWidth signals that were removed in the isolation fix
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/query-devtools/src/Devtools.tsx (2)
2634-2693:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftScope the cache subscription registries per Devtools instance.
queryCacheMapandmutationCacheMapare still module-level. With two Devtools mounted against differentQueryClients, a notification from client A walks every registered callback and recomputes client B's setters against A's cache; then unmounting eitherContentViewclears the entire registry for the other instance. That still breaks instance isolation even after the panel-state context work.A per-instance registry in
DevtoolsPanelProvider/ContentView, or aWeakMapkeyed byQueryCache/MutationCache, would avoid cross-instance updates and cleanup collisions.Also applies to: 2696-2748
🤖 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/query-devtools/src/Devtools.tsx` around lines 2634 - 2693, The module-level registries queryCacheMap (and similarly mutationCacheMap) cause cross-instance notifications and clobbered cleanup; make these per-Devtools-instance or convert them into WeakMaps keyed by the actual QueryCache/MutationCache so entries are isolated by client. Update setupQueryCacheSubscription and createSubscribeToQueryCacheBatcher to use the per-instance map or queryCache-keyed WeakMap (use the createMemo() queryCache() as the key), store/remove entries scoped to that key, and ensure onCleanup only clears or deletes the entries for that specific QueryCache/MutationCache rather than clearing the whole module map; ensure unsubscribe still unregisters only the callbacks tied to that cache.
121-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the
onlineManagersync intoDevtoolsPanelProvider.
packages/query-devtools/src/DevtoolsPanelComponent.tsxnow mountsContentViewunderDevtoolsPanelProviderwithout ever renderingDevtools. In that tree,offline()stays at the provider default because the onlyonlineManager().subscribe(...)lives inDevtools, so the offline toggle label and pressed state never update in panel-only mode.Suggested fix
export const DevtoolsPanelProvider: Component<{ children: JSX.Element }> = (props) => { + const onlineManager = createMemo(() => useQueryDevtoolsContext().onlineManager) const [selectedQueryHash, setSelectedQueryHash] = createSignal<string | null>( null, ) const [selectedMutationId, setSelectedMutationId] = createSignal<number | null>( null, ) const [panelWidth, setPanelWidth] = createSignal(0) - const [offline, setOffline] = createSignal(false) + const [offline, setOffline] = createSignal(!onlineManager().isOnline()) + + onMount(() => { + setOffline(!onlineManager().isOnline()) + const unsubscribe = onlineManager().subscribe((online) => { + setOffline(!online) + }) + + onCleanup(() => { + unsubscribe() + }) + }) const value = createMemo( () => ({ selectedQueryHash,Also applies to: 175-186
🤖 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/query-devtools/src/Devtools.tsx` around lines 121 - 149, The offline state used by the panel stays at its default because the onlineManager subscription that updates offline() is only registered in Devtools; move the onlineManager().subscribe(...) logic into DevtoolsPanelProvider so the provider's setOffline is updated even when Devtools isn't mounted. Specifically, add a subscription in DevtoolsPanelProvider that calls setOffline(!isOnline) (and cleans up the unsubscribe) using the same onlineManager().subscribe handler currently in Devtools (or DevtoolsPanelComponent), so the offline signal provided by DevtoolsPanelProvider reflects actual network status for consumers like ContentView.
🤖 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.
Outside diff comments:
In `@packages/query-devtools/src/Devtools.tsx`:
- Around line 2634-2693: The module-level registries queryCacheMap (and
similarly mutationCacheMap) cause cross-instance notifications and clobbered
cleanup; make these per-Devtools-instance or convert them into WeakMaps keyed by
the actual QueryCache/MutationCache so entries are isolated by client. Update
setupQueryCacheSubscription and createSubscribeToQueryCacheBatcher to use the
per-instance map or queryCache-keyed WeakMap (use the createMemo() queryCache()
as the key), store/remove entries scoped to that key, and ensure onCleanup only
clears or deletes the entries for that specific QueryCache/MutationCache rather
than clearing the whole module map; ensure unsubscribe still unregisters only
the callbacks tied to that cache.
- Around line 121-149: The offline state used by the panel stays at its default
because the onlineManager subscription that updates offline() is only registered
in Devtools; move the onlineManager().subscribe(...) logic into
DevtoolsPanelProvider so the provider's setOffline is updated even when Devtools
isn't mounted. Specifically, add a subscription in DevtoolsPanelProvider that
calls setOffline(!isOnline) (and cleans up the unsubscribe) using the same
onlineManager().subscribe handler currently in Devtools (or
DevtoolsPanelComponent), so the offline signal provided by DevtoolsPanelProvider
reflects actual network status for consumers like ContentView.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd735554-1b06-4d55-8119-efdb2c1101c0
📒 Files selected for processing (1)
packages/query-devtools/src/Devtools.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #9681
When multiple
QueryDevToolsinstances exist on a page (e.g. micro-frontends each with their ownQueryClient), shared module-level signals caused state leakage between instances:Changes
DevtoolsPanelContextto scope panel state per instanceselectedQueryHash,selectedMutationId,panelWidth, andofflinesignals from module-level to context-based (per-instance)DevtoolsPanelProviderwrapper inDevtoolsComponentandDevtoolsPanelComponentQueryDevtoolsmount now gets its own isolated stateHow it works
Each
<DevtoolsPanelProvider>creates its own set of SolidJS signals for:selectedQueryHash/setSelectedQueryHashselectedMutationId/setSelectedMutationIdpanelWidth/setPanelWidthoffline/setOfflineComponents consume these via
useDevtoolsPanelState()hook which reads from the nearestDevtoolsPanelContext.Provider.Testing
Summary by CodeRabbit