Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client Code
participant Init as init()
participant Key as reactorKey()
participant Cache as Global Client Cache
participant Reactor as Reactor ctor
Caller->>Init: call init(config)
Init->>Init: compute cardinalityInference (schema ? (flag ?? true) : false)
Init->>Key: reactorKey(configStrict with cardinalityInference)
Key->>Cache: lookup client by key
alt cache hit
Cache-->>Init: return existing client
else cache miss
Init->>Reactor: new Reactor(configStrict with cardinalityInference)
Reactor-->>Init: constructed Reactor
Init->>Cache: store client by key
end
Init-->>Caller: return client
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
client/packages/core/src/index.ts (1)
980-985:⚠️ Potential issue | 🔴 CriticalLine 984 overrides user config and re-breaks inference behavior.
cardinalityInference: configStrict.schema ? true : falseignores explicitcardinalityInference: falsewhen schema exists. This also risks cache-key/config mismatch downstream (e.g.,shutdown()key deletion uses_reactor.config).Proposed fix
const reactor = new Reactor<RoomsOf<Schema>>( { ...defaultConfig, ...configStrict, - cardinalityInference: configStrict.schema ? true : false, + cardinalityInference: configStrict.cardinalityInference, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/core/src/index.ts` around lines 980 - 985, The current initialization of Reactor forcibly sets cardinalityInference based only on presence of configStrict.schema, which overwrites an explicit user-provided false and can cause config/cache-key mismatches (see Reactor, defaultConfig, configStrict and shutdown() referencing _reactor.config); change the merge logic so that an explicit configStrict.cardinalityInference (even false) is preserved and only default to true when cardinalityInference is undefined and a schema exists—i.e., respect configStrict.cardinalityInference if present, otherwise set it to true only when configStrict.schema is truthy (fall back to defaultConfig.cardinalityInference when neither is set).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@client/packages/core/src/index.ts`:
- Around line 980-985: The current initialization of Reactor forcibly sets
cardinalityInference based only on presence of configStrict.schema, which
overwrites an explicit user-provided false and can cause config/cache-key
mismatches (see Reactor, defaultConfig, configStrict and shutdown() referencing
_reactor.config); change the merge logic so that an explicit
configStrict.cardinalityInference (even false) is preserved and only default to
true when cardinalityInference is undefined and a schema exists—i.e., respect
configStrict.cardinalityInference if present, otherwise set it to true only when
configStrict.schema is truthy (fall back to defaultConfig.cardinalityInference
when neither is set).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4b1a0cdf-aa60-4c43-820e-0e381d8cf366
📒 Files selected for processing (1)
client/packages/core/src/index.ts
|
View Vercel preview at instant-www-js-fix-qi-inference-jsv.vercel.app. |
| config.useDateObjects | ||
| config.useDateObjects + | ||
| '_' + | ||
| String(!!config.cardinalityInference) |
There was a problem hiding this comment.
One thing I realize:
Would this mean, that if a user goes between say "Explorer" and "Inspector" a few times, they will end up with 2 websocket connections?
There was a problem hiding this comment.
Yeah, they'll get a new connection as soon as they view the query inspector page.
2 participants
The cardinality inference in the query inspector is not working. We create a separate db with the schema in the query inspector component. But then we just end up using the existing db because it has the same reactor cache key.
This updates the reactor cache key so that it changes when inference is disabled/enabled.