fix(devtools): Moved DevTool Instance specific settings to a context to have proper isolation #10006
Conversation
🦋 Changeset detectedLatest commit: 47aa1fb The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughIntroduces a new DevtoolsStateContext and useDevtoolsState hook to centralize UI state (selectedQueryHash, selectedMutationId, panelWidth, offline) and typed cache maps (queryCacheMap, mutationCacheMap); Devtools components are updated to provide and consume that context instead of local signals. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Provider as DevtoolsPanelComponent (Provider)
participant State as DevtoolsStateContext
participant Child as Devtools Child Components
participant Cache as Query/Mutation Cache
Provider->>State: create signals & maps\n(selectedQueryHash, selectedMutationId,\npanelWidth, offline, queryCacheMap, mutationCacheMap)
Provider->>Child: render children under Provider
Child->>State: call useDevtoolsState() (read accessors/setters)
Child->>Cache: subscribe(callbackSelector)
Cache-->>Child: notify(event)
alt shouldUpdate(event) is true
Cache->>State: find map entry -> { setter, shouldUpdate }
State->>Child: invoke setter(eventPayload)
Child->>State: update UI state via setter
else shouldUpdate(event) is false
Cache->>Child: no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 47aa1fb
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.changeset/true-loops-report.md (1)
5-5: Consider rephrasing the changeset to describe the fix.The current description states the problem rather than the solution. Consider something like: "Fix isolation issues when multiple Devtool instances share the same state" to better communicate what was changed.
🔎 Suggested improvement
-Multiple Devtool instances sharing same state causing isolation issues +Fix isolation issues when multiple Devtool instances share the same statepackages/query-devtools/src/contexts/DevtoolsStateContext.ts (1)
9-16: Consider narrowing theSetter<any>type for better type safety.Using
Setter<any>loses type information. While this is acceptable for the current implementation since the maps store setters for heterogeneous value types, you might consider using a generic or more specific type in the future if type safety becomes important.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/true-loops-report.mdpackages/query-devtools/src/Devtools.tsxpackages/query-devtools/src/DevtoolsPanelComponent.tsxpackages/query-devtools/src/contexts/DevtoolsStateContext.tspackages/query-devtools/src/contexts/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T03:18:18.303Z
Learnt from: oscartbeaumont
Repo: TanStack/query PR: 9564
File: packages/solid-query-devtools/src/production.tsx:2-3
Timestamp: 2025-08-19T03:18:18.303Z
Learning: In the solid-query-devtools package, the codebase uses a pattern of type-only default imports combined with typeof for component type annotations (e.g., `import type SolidQueryDevtoolsComp from './devtools'` followed by `typeof SolidQueryDevtoolsComp`). This pattern is consistently used across index.tsx and production.tsx files, and the maintainers prefer consistency over changing this approach.
Applied to files:
packages/query-devtools/src/contexts/index.tspackages/query-devtools/src/contexts/DevtoolsStateContext.tspackages/query-devtools/src/Devtools.tsxpackages/query-devtools/src/DevtoolsPanelComponent.tsx
🧬 Code graph analysis (1)
packages/query-devtools/src/DevtoolsPanelComponent.tsx (4)
packages/query-devtools/src/Devtools.tsx (1)
DevtoolsComponentType(110-112)packages/query-devtools/src/contexts/DevtoolsStateContext.ts (3)
QueryCacheMap(18-21)MutationCacheMap(23-26)DevtoolsStateContext(41-41)packages/query-devtools/src/contexts/PiPContext.tsx (1)
PiPProvider(35-192)packages/query-devtools/src/contexts/ThemeContext.ts (1)
ThemeContext(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (12)
packages/query-devtools/src/contexts/index.ts (1)
4-4: LGTM!The new re-export follows the existing pattern and properly exposes the
DevtoolsStateContextmodule.packages/query-devtools/src/DevtoolsPanelComponent.tsx (2)
20-29: LGTM!The component correctly creates its own isolated state signals and cache maps, which are then provided through the
DevtoolsStateContext.Provider. This ensures proper isolation when multipleDevtoolsPanelComponentinstances are rendered.
43-73: LGTM!The
DevtoolsStateContext.Provideris correctly positioned in the component hierarchy and provides all required state properties. Thedisabledprop onPiPProvideris appropriate for this panel-only view.packages/query-devtools/src/contexts/DevtoolsStateContext.ts (2)
43-51: LGTM!The
useDevtoolsStatehook correctly throws a descriptive error when used outside of theDevtoolsStateContext.Provider, following the standard pattern for required context hooks.
18-26: No action needed - this is not a concern in Solid.js.Callbacks passed to
createSubscribeToQueryCacheBatcherandcreateSubscribeToMutationCacheBatchermaintain stable references because they are defined at the component body level. In Solid.js, component setup code executes once during mount (unlike React where it runs on every render), so inline arrow functions retain the same reference throughout the component's lifetime. Reference equality for Map key lookups works correctly in this context.Likely an incorrect or invalid review comment.
packages/query-devtools/src/Devtools.tsx (7)
115-124: LGTM!The
Devtoolscomponent correctly creates isolated state signals and cache maps at the component level, which are then provided to descendants via context. This ensures eachDevtoolsinstance has its own isolated state.
206-219: LGTM!The
DevtoolsStateContext.Providercorrectly wraps all content that needs access to the shared state, including the PiP portal content and the main panel.
719-726: LGTM!The
ContentViewcomponent correctly uses theuseDevtoolsStatehook to access state from the context, destructuring only the properties it needs. This pattern is consistently applied across all components that require devtools state.
2625-2647: LGTM!The
setupQueryCacheSubscriptionfunction correctly obtains thequeryCacheMapfrom the context, ensuring each DevTools instance has isolated cache subscriptions. The cleanup logic properly clears the map and unsubscribes.
2681-2702: LGTM!The
setupMutationCacheSubscriptionfunction follows the same pattern as the query cache subscription, correctly using the context-providedmutationCacheMapfor proper isolation.
2649-2679: LGTM!The
createSubscribeToQueryCacheBatcherfunction correctly uses the context-provided map and properly cleans up entries viaonCleanupwhen the component unmounts.
2704-2732: LGTM!The
createSubscribeToMutationCacheBatcherfunction follows the same correct pattern for context usage and cleanup.
gk9848970
force-pushed
the
fix_isolation_of_devtools
branch
from
904d076 to
b2c6fbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/query-devtools/src/contexts/DevtoolsStateContext.ts (1)
43-45: Consider adding a runtime check for better error messaging.The non-null assertion (
!) is TypeScript-only and doesn't provide a helpful error if the hook is used outside theDevtoolsStateContext.Provider. If a developer accidentally calls this hook outside the provider, they'll get a cryptic "Cannot destructure property of undefined" error.🔎 Suggested improvement for better DX
export function useDevtoolsState() { - return useContext(DevtoolsStateContext)! + const context = useContext(DevtoolsStateContext) + if (!context) { + throw new Error( + 'useDevtoolsState must be used within a DevtoolsStateContext.Provider', + ) + } + return context }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/query-devtools/src/Devtools.tsxpackages/query-devtools/src/DevtoolsPanelComponent.tsxpackages/query-devtools/src/contexts/DevtoolsStateContext.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/query-devtools/src/DevtoolsPanelComponent.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T03:18:18.303Z
Learnt from: oscartbeaumont
Repo: TanStack/query PR: 9564
File: packages/solid-query-devtools/src/production.tsx:2-3
Timestamp: 2025-08-19T03:18:18.303Z
Learning: In the solid-query-devtools package, the codebase uses a pattern of type-only default imports combined with typeof for component type annotations (e.g., `import type SolidQueryDevtoolsComp from './devtools'` followed by `typeof SolidQueryDevtoolsComp`). This pattern is consistently used across index.tsx and production.tsx files, and the maintainers prefer consistency over changing this approach.
Applied to files:
packages/query-devtools/src/Devtools.tsxpackages/query-devtools/src/contexts/DevtoolsStateContext.ts
🧬 Code graph analysis (2)
packages/query-devtools/src/Devtools.tsx (2)
packages/query-devtools/src/contexts/DevtoolsStateContext.ts (4)
QueryCacheMap(18-21)MutationCacheMap(23-26)DevtoolsStateContext(41-41)useDevtoolsState(43-45)packages/query-devtools/src/contexts/QueryDevtoolsContext.ts (1)
useQueryDevtoolsContext(45-47)
packages/query-devtools/src/contexts/DevtoolsStateContext.ts (1)
packages/svelte-query/src/types.ts (1)
Accessor(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (18)
packages/query-devtools/src/contexts/DevtoolsStateContext.ts (2)
9-26: Well-structured type definitions for cache subscription maps.The types properly capture the setter and optional filtering behavior needed for the cache subscription batching pattern. The use of
Setter<any>is acceptable here given the generic callback signatures.
28-39: Clean interface design for context state.The
DevtoolsStateinterface correctly exposes Solid.js reactive primitives (Accessor/Setter pairs) and cache maps, enabling proper isolation between multiple DevTools instances.packages/query-devtools/src/Devtools.tsx (16)
54-60: Clean import organization.The context imports are properly consolidated with the existing context imports from
./contexts.
115-124: Correct implementation of per-instance state isolation.Each
Devtoolscomponent instance creates its own signals and cache maps, ensuring proper isolation between multiple DevTools instances as intended by this PR.
206-219: Provider correctly wraps component tree with isolated state.The
DevtoolsStateContext.Provideris positioned at the root of the Devtools component tree, exposing the per-instance state to all child components.
310-310: Correct context usage.The
PiPPanelproperly consumes the context for panel width state management.
437-437: Correct context usage for panel state management.
719-726: ContentView correctly accesses context state.The component appropriately destructures the state values it needs for handling selection, offline status, and responsive layout.
1421-1421: QueryRow correctly uses context for selection state.
1552-1553: MutationRow correctly uses context for selection state.
1800-1801: QueryStatus component correctly uses context for responsive behavior.
1918-1919: QueryDetails correctly uses context for selection state.
2447-2448: MutationDetails correctly uses context for selection state.
2625-2647: Query cache subscription correctly uses context-provided map.The subscription setup function properly accesses the instance-specific
queryCacheMapfrom context, ensuring isolation between DevTools instances.
2681-2702: Mutation cache subscription correctly uses context-provided map.The subscription setup function properly accesses the instance-specific
mutationCacheMapfrom context.
2649-2679: Query cache batcher correctly uses context and proper entry structure.The function correctly accesses
queryCacheMapfrom context and sets entries with the expected{ setter, shouldUpdate }structure.
2704-2732: Mutation cache batcher correctly uses context and proper entry structure.The function correctly accesses
mutationCacheMapfrom context and sets entries with the{ setter }structure.
2688-2693: Correct setter invocation pattern matching the new map value structure.The iteration correctly accesses
value.setterfrom the map entries, aligning with theMutationCacheMapValuetype.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/query-devtools/src/contexts/DevtoolsStateContext.ts (1)
9-16: Consider stronger typing for setters.The
anytype inSetter<any>reduces type safety. If the actual types of cached values are known or can be constrained (e.g., using generics), consider making these types more specific to catch potential type errors at compile time.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-devtools/src/contexts/DevtoolsStateContext.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T03:18:18.303Z
Learnt from: oscartbeaumont
Repo: TanStack/query PR: 9564
File: packages/solid-query-devtools/src/production.tsx:2-3
Timestamp: 2025-08-19T03:18:18.303Z
Learning: In the solid-query-devtools package, the codebase uses a pattern of type-only default imports combined with typeof for component type annotations (e.g., `import type SolidQueryDevtoolsComp from './devtools'` followed by `typeof SolidQueryDevtoolsComp`). This pattern is consistently used across index.tsx and production.tsx files, and the maintainers prefer consistency over changing this approach.
Applied to files:
packages/query-devtools/src/contexts/DevtoolsStateContext.ts
🧬 Code graph analysis (1)
packages/query-devtools/src/contexts/DevtoolsStateContext.ts (1)
packages/svelte-query/src/types.ts (1)
Accessor(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (3)
packages/query-devtools/src/contexts/DevtoolsStateContext.ts (3)
1-7: LGTM!The imports are appropriate for a Solid.js context implementation managing DevTools state.
28-41: Good state interface design with Accessor/Setter pairs.The DevtoolsState interface properly captures all instance-specific state needed for isolation. However, note that
createContext<DevtoolsState>()is called without a default value, making the context undefined until a Provider supplies it. This is acceptable if the hook always runs within a Provider (see comment on lines 43-45).
18-26: No action required—function-as-key pattern is intentionally correct here.The Map types using functions as keys are part of a subscription registry pattern that is properly implemented. Each
createSubscribeToQueryCacheBatcherandcreateSubscribeToMutationCacheBatchercall captures its callback function in a closure, then consistently uses that same function reference forset()(line 2669), iteration (line 2631), anddelete()(line 2674). Since the callback reference is stable throughout the subscription's lifetime and cleanup handlers remove entries before unmounting, there is no risk of reference mismatch. This design is sound and intentional.
| export function useDevtoolsState() { | ||
| return useContext(DevtoolsStateContext)! | ||
| } |
There was a problem hiding this comment.
Add runtime guard for missing context provider.
The non-null assertion (!) assumes the context is always provided, but calling this hook outside a DevtoolsStateContext.Provider will cause a runtime error when the undefined value is accessed.
🔎 Proposed fix with runtime guard
export function useDevtoolsState() {
- return useContext(DevtoolsStateContext)!
+ const context = useContext(DevtoolsStateContext)
+ if (!context) {
+ throw new Error('useDevtoolsState must be used within a DevtoolsStateContext.Provider')
+ }
+ return context
}This provides a clear error message during development rather than cryptic undefined access errors at runtime.
📝 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 function useDevtoolsState() { | |
| return useContext(DevtoolsStateContext)! | |
| } | |
| export function useDevtoolsState() { | |
| const context = useContext(DevtoolsStateContext) | |
| if (!context) { | |
| throw new Error('useDevtoolsState must be used within a DevtoolsStateContext.Provider') | |
| } | |
| return context | |
| } |
🤖 Prompt for AI Agents
In packages/query-devtools/src/contexts/DevtoolsStateContext.ts around lines 43
to 45, the hook returns the context with a non-null assertion which will throw a
confusing error if used outside a Provider; add a runtime guard that checks if
the context value is undefined and throw a clear, descriptive error (e.g.
"useDevtoolsState must be used within a DevtoolsStateContext.Provider") so
callers get an explicit message during development; implement the check before
returning the value and keep types intact.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10006 +/- ##
===========================================
- Coverage 45.82% 20.65% -25.18%
===========================================
Files 200 43 -157
Lines 8525 2489 -6036
Branches 1977 646 -1331
===========================================
- Hits 3907 514 -3393
+ Misses 4158 1727 -2431
+ Partials 460 248 -212
🚀 New features to boost your workflow:
|
2 participants
🎯 Changes
Fixes: 9681
Motivation - I was looking to have my first meaningful contribution, As last time I only did some docs link change.
Issue - We were having many issues of isolation like:
Before this PR:
Screen.Recording.2025-12-31.at.1.06.01.AM.mov
Fix - I saw how we handle query client being separate for each devtool, Using context. So I put all the things related to one devtools instance in it's own context
After this PR:
Screen.Recording.2025-12-31.at.1.12.27.AM.mov
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.