feat(editor): query workbench with live results and TanStack Query#66
feat(editor): query workbench with live results and TanStack Query#66
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds full query-node support: UI detection and iconography, an inline QueryResults component with server-backed evaluation and edit workbench, new query helpers and tests, seeded DB init for evaluations, cache invalidation wiring, and a new workbench dependency. Changes
Sequence DiagramsequenceDiagram
participant NodeBlock as NodeBlock (Client)
participant QueryResults as QueryResults (Component)
participant Server as evaluateQueryServerFn (Server)
participant DB as NodeFacade/DB (Data)
NodeBlock->>NodeBlock: detect isQuery via QUERY_SYSTEM_ID\nextract queryDefinition & visibleFields
NodeBlock->>QueryResults: render with nodeId, definition & depth
QueryResults->>QueryResults: compute stable key via safeStringify
QueryResults->>Server: POST evaluateQueryServerFn(definition)
Server->>DB: initDatabaseSeeded() -> init & seed\nevaluate query via node facade
DB-->>Server: return matching nodes + totalCount
Server-->>QueryResults: return results (id, content, supertags, totalCount)
QueryResults->>QueryResults: render QueryResultRow list\nshow loading/empty/error/truncation states
QueryResultRow->>NodeBlock: navigate to node on click
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…idation - Replace useState/useEffect with useQuery for query result caching - Add inline QueryLinter summary + "Configure" button on query nodes - Embed QueryBuilder from @nxus/workbench for inline query editing - Add updateQueryDefinitionServerFn to persist definition changes - Add queryClient.invalidateQueries after all outline mutations - Extract query helpers (isQueryNode, extractQueryDefinition, etc.) - Add 24 unit tests for query helper functions - Add ensure-seeded.server.ts for full database seeding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Replace z.any() with QueryDefinitionSchema for evaluateQueryServerFn and updateQueryDefinitionServerFn input validation - Change QueryResultRow from div to button for keyboard accessibility - Use span instead of div inside button to avoid invalid nesting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| const handleDefinitionChange = useCallback( | ||
| (newDef: QueryDefinition) => { | ||
| updateQueryDefinitionServerFn({ | ||
| data: { nodeId, definition: newDef }, | ||
| }) | ||
| .then(() => { | ||
| queryClient.invalidateQueries({ queryKey: outlineQueryKeys.all }) | ||
| }) | ||
| .catch((err) => { | ||
| console.error('[query-workbench] Failed to save definition:', err) | ||
| }) | ||
| }, | ||
| [nodeId, queryClient], | ||
| ) |
There was a problem hiding this comment.
🔴 Query definition change never updates local state, making inline editing non-functional
handleDefinitionChange in query-results.tsx:54-67 calls updateQueryDefinitionServerFn to persist the new definition to the DB and then invalidates TanStack Query caches. However, it never updates the node's fields in the local Zustand store. The definition prop passed to QueryResults originates from extractQueryDefinition(node) in the parent NodeBlock (node-block.tsx:215), which reads from the Zustand store — so the prop remains stale. Because definitionKey at line 37 is derived from this stale definition, the useQuery cache key doesn't change, and the invalidation merely re-fetches the old query (the queryFn closure at line 46 also captures the old definition). The net effect is that editing a query via the QueryBuilder appears to do nothing until the page is fully reloaded.
Prompt for agents
In apps/nxus-editor/src/components/outline/query-results.tsx, the handleDefinitionChange callback (lines 54-67) needs to also update the node's query_definition field in the local Zustand store so that the definition prop flowing from the parent NodeBlock updates, which in turn changes the TanStack Query cache key and triggers a re-fetch with the new definition.
One approach: import useOutlineStore and call something like useOutlineStore.getState().updateNodeField(nodeId, 'field:query_definition', newDef) inside handleDefinitionChange before or after the server call. If no such store action exists, create one in the outline store that updates the field value for a given node. Alternatively, use optimistic updates with TanStack Query's mutation API so the local query data updates immediately.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/nxus-editor/src/components/outline/query-results.tsx (1)
37-47:⚠️ Potential issue | 🟠 MajorNormalize
definitionbefore gating evaluation and the workbench.Line 38 still treats
'{}'as “no definition”, and the laterisQueryDefinition(definition)checks reject partial objects. That blocks default/partial query definitions from both evaluating and opening the inline linter/builder even though the server endpoint already normalizes them withQueryDefinitionSchema.As per coding guidelines: "Use Zod schemas with parse() for validating external data instead of assuming validity" and "Validate external inputs using Zod schemas before processing"
Also applies to: 78-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/query-results.tsx` around lines 37 - 47, Normalize and validate the incoming definition using the QueryDefinitionSchema before any gating or evaluation: call QueryDefinitionSchema.parse (or safe parse) on definition to produce parsedDefinition, use parsedDefinition (not the raw safeStringify(definition) or '{}' checks) for hasDefinition and as the payload to evaluateQueryServerFn and outlineQueryKeys.evaluation, and replace subsequent isQueryDefinition(definition) checks with checks against the parsedDefinition so partial/default definitions that the server accepts are allowed through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/nxus-editor/src/components/outline/query-results.tsx`:
- Around line 191-200: The button lacks a reliable accessible name when
node.content is empty (it currently relies on title and a zero‑width space);
update the button in the component rendering (the <button> using props like
node.content, depth, onClick) to provide an explicit accessible name by adding
an aria-label such as aria-label={`Go to: ${node.content || 'Untitled result'}`}
(or another clear fallback like "Untitled result") so assistive tech gets a
stable label even when node.content is empty or there are no supertags.
- Around line 54-65: The auto-save handler handleDefinitionChange is calling
updateQueryDefinitionServerFn directly from QueryBuilder onChange (with
showSave={false}), causing concurrent saves to finish out-of-order and overwrite
newer edits; wrap the save logic in a debounce (e.g. 300–800ms) or implement a
simple queued/latest-only save: keep a local incremental requestId/token for
each save, cancel/replace pending debounce timers, call
updateQueryDefinitionServerFn only for the debounced/latest change, and when
each promise resolves check the requestId to ignore stale responses before
calling queryClient.invalidateQueries({ queryKey: outlineQueryKeys.all }). Apply
the same debounce/latest-only pattern to the other save handler referenced in
the diff (the similar save at 108-114).
---
Duplicate comments:
In `@apps/nxus-editor/src/components/outline/query-results.tsx`:
- Around line 37-47: Normalize and validate the incoming definition using the
QueryDefinitionSchema before any gating or evaluation: call
QueryDefinitionSchema.parse (or safe parse) on definition to produce
parsedDefinition, use parsedDefinition (not the raw safeStringify(definition) or
'{}' checks) for hasDefinition and as the payload to evaluateQueryServerFn and
outlineQueryKeys.evaluation, and replace subsequent
isQueryDefinition(definition) checks with checks against the parsedDefinition so
partial/default definitions that the server accepts are allowed through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8f0aeba-3cf4-4a2e-9340-d64409858e81
📒 Files selected for processing (2)
apps/nxus-editor/src/components/outline/query-results.tsxapps/nxus-editor/src/services/outline.server.ts
| const handleDefinitionChange = useCallback( | ||
| (newDef: QueryDefinition) => { | ||
| updateQueryDefinitionServerFn({ | ||
| data: { nodeId, definition: newDef }, | ||
| }) | ||
| .then(() => { | ||
| queryClient.invalidateQueries({ queryKey: outlineQueryKeys.all }) | ||
| }) | ||
| .catch((err) => { | ||
| console.error('[query-workbench] Failed to save definition:', err) | ||
| }) | ||
| }, |
There was a problem hiding this comment.
Serialize the auto-saves from QueryBuilder.
onChange is wired straight to updateQueryDefinitionServerFn, and showSave={false} means every edit can trigger a full-definition write. Two quick edits can finish out of order, so an older request can overwrite the newer definition and then invalidate the outline back to stale data. Debounce or queue these saves so only the latest edit persists.
Also applies to: 108-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/components/outline/query-results.tsx` around lines 54 -
65, The auto-save handler handleDefinitionChange is calling
updateQueryDefinitionServerFn directly from QueryBuilder onChange (with
showSave={false}), causing concurrent saves to finish out-of-order and overwrite
newer edits; wrap the save logic in a debounce (e.g. 300–800ms) or implement a
simple queued/latest-only save: keep a local incremental requestId/token for
each save, cancel/replace pending debounce timers, call
updateQueryDefinitionServerFn only for the debounced/latest change, and when
each promise resolves check the requestId to ignore stale responses before
calling queryClient.invalidateQueries({ queryKey: outlineQueryKeys.all }). Apply
the same debounce/latest-only pattern to the other save handler referenced in
the diff (the similar save at 108-114).
| <button | ||
| type="button" | ||
| className={cn( | ||
| 'flex w-full items-start rounded-sm cursor-pointer text-left', | ||
| 'hover:bg-foreground/[0.03] transition-colors duration-75', | ||
| )} | ||
| style={{ paddingLeft: `${(depth + 1) * 24}px` }} | ||
| onClick={onClick} | ||
| title={`Go to: ${node.content || 'Untitled'}`} | ||
| > |
There was a problem hiding this comment.
Add an accessible name for untitled result buttons.
When node.content is empty and there are no supertags, this button's visible text is just a zero-width space. title is not a dependable accessible name, so assistive tech can end up with an unlabeled button here.
♿ Small fix
<button
type="button"
+ aria-label={`Go to: ${node.content || 'Untitled'}`}
className={cn(Also applies to: 228-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/components/outline/query-results.tsx` around lines 191 -
200, The button lacks a reliable accessible name when node.content is empty (it
currently relies on title and a zero‑width space); update the button in the
component rendering (the <button> using props like node.content, depth, onClick)
to provide an explicit accessible name by adding an aria-label such as
aria-label={`Go to: ${node.content || 'Untitled result'}`} (or another clear
fallback like "Untitled result") so assistive tech gets a stable label even when
node.content is empty or there are no supertags.
popemkt
changed the title
…bench - Configure button renders as a badge (same h-6 + text-[11px] as supertag badges) next to the node content text, only when query is expanded - QueryLinter now lives inside the QueryBuilder workbench panel (showLinter) - workbenchOpen state owned by NodeBlock, passed down to QueryResults Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add afterContent slot to NodeContent so the Configure badge renders right after the editable text, before the supertag badges: ● My Query [Configure] #Query Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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)
apps/nxus-editor/src/components/outline/node-block.tsx (1)
206-220:⚠️ Potential issue | 🔴 CriticalCritical:
useStatecalled after early return violates Rules of Hooks.The
useState(false)at line 212 is called after the conditionalreturn nullat line 206. React requires hooks to be called unconditionally and in the same order on every render. Whennodeisundefined, the component returns early anduseStateis never called, but on the next render whennodeexists, it will be called—causing hook order mismatch and runtime bugs.Move
workbenchOpenstate declaration before the early return, alongside other hooks.🐛 Proposed fix
const { updateNodeContent, createNodeAfter, deleteNode, indentNode, outdentNode, moveNodeUp, moveNodeDown, } = useOutlineSync() + const [workbenchOpen, setWorkbenchOpen] = useState(false) + const handleBulletClick = useCallback(Then remove line 212:
if (!node) return null const isActive = activeNodeId === nodeId const isSelected = selectedNodeId === nodeId const hasChildren = node.children.length > 0 const primaryTagColor = node.supertags[0]?.color ?? null - const [workbenchOpen, setWorkbenchOpen] = useState(false) const isSupertag = node.supertags.some((t) => t.systemId === SUPERTAG_DEFINITION_SYSTEM_ID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/node-block.tsx` around lines 206 - 220, The useState hook for workbenchOpen is declared after an early return when node is falsy, violating React Hooks rules; move the useState call (workbenchOpen, setWorkbenchOpen via useState(false)) to before the "if (!node) return null" check so hooks are invoked unconditionally and in stable order, then remove the post-return declaration; ensure references to workbenchOpen/setWorkbenchOpen elsewhere in the component (e.g., in node-block.tsx) still point to the moved state.
🧹 Nitpick comments (1)
apps/nxus-editor/src/components/outline/query-results.tsx (1)
45-50: Consider validatingdefinitionbefore casting toQueryDefinition.The
definitionprop is typed asunknown, but it's cast toQueryDefinitionat line 47 without client-side validation. While the server validates via Zod, the client currently trusts the input blindly. IfisQueryDefinition(definition)already performs validation (based on usage at line 75), consider using that check here or adding a guard.Potential safeguard
const { data, isLoading, error, } = useQuery({ queryKey: outlineQueryKeys.evaluation(definitionKey), - queryFn: () => evaluateQueryServerFn({ data: { definition: definition as QueryDefinition } }), - enabled: hasDefinition, + queryFn: () => evaluateQueryServerFn({ data: { definition: definition as QueryDefinition } }), + enabled: hasDefinition && isQueryDefinition(definition), staleTime: 30_000, })As per coding guidelines: "Use Zod schemas with parse() for validating external data instead of assuming validity".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/query-results.tsx` around lines 45 - 50, The code casts `definition` to `QueryDefinition` when calling evaluateQueryServerFn inside useQuery; instead validate `definition` first (use the existing isQueryDefinition(definition) guard or the Zod schema's parse()) and only call evaluateQueryServerFn with a properly typed value. Concretely, compute a typedDefinition (e.g., if (isQueryDefinition(definition)) typedDefinition = definition; else try schema.parse(definition) or set enabled=false), update the useQuery `enabled` to depend on that check, and pass typedDefinition to evaluateQueryServerFn (reference: useQuery, outlineQueryKeys.evaluation, evaluateQueryServerFn, isQueryDefinition).
🤖 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 `@apps/nxus-editor/src/components/outline/node-block.tsx`:
- Around line 206-220: The useState hook for workbenchOpen is declared after an
early return when node is falsy, violating React Hooks rules; move the useState
call (workbenchOpen, setWorkbenchOpen via useState(false)) to before the "if
(!node) return null" check so hooks are invoked unconditionally and in stable
order, then remove the post-return declaration; ensure references to
workbenchOpen/setWorkbenchOpen elsewhere in the component (e.g., in
node-block.tsx) still point to the moved state.
---
Nitpick comments:
In `@apps/nxus-editor/src/components/outline/query-results.tsx`:
- Around line 45-50: The code casts `definition` to `QueryDefinition` when
calling evaluateQueryServerFn inside useQuery; instead validate `definition`
first (use the existing isQueryDefinition(definition) guard or the Zod schema's
parse()) and only call evaluateQueryServerFn with a properly typed value.
Concretely, compute a typedDefinition (e.g., if (isQueryDefinition(definition))
typedDefinition = definition; else try schema.parse(definition) or set
enabled=false), update the useQuery `enabled` to depend on that check, and pass
typedDefinition to evaluateQueryServerFn (reference: useQuery,
outlineQueryKeys.evaluation, evaluateQueryServerFn, isQueryDefinition).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9d1177b-5ff3-48b0-a3ee-805bc3583697
📒 Files selected for processing (2)
apps/nxus-editor/src/components/outline/node-block.tsxapps/nxus-editor/src/components/outline/query-results.tsx
20px square icon button (same height as supertag badge inner), no text label, right after the content text. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tent Place the icon-only Configure button (h-6 w-6, Sliders icon) as a sibling after NodeContent in the node row. Remove afterContent slot from NodeContent — not needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| const hasFields = node.fields.length > 0 | ||
| const isExpandable = hasChildren || hasFields | ||
| const primaryTagColor = node.supertags[0]?.color ?? null | ||
| const [workbenchOpen, setWorkbenchOpen] = useState(false) |
There was a problem hiding this comment.
🔴 useState hook called after conditional early return violates React Rules of Hooks
The useState(false) call at line 212 is placed after the if (!node) return null early return at line 206. This violates React's Rules of Hooks, which require hooks to be called in the same order on every render. If a node is deleted from the Zustand store while this NodeBlock component is still mounted, the component will re-render with node === undefined, hit the early return, and skip the useState call — causing React to throw "Rendered fewer hooks than expected". The same issue applies to the pre-existing useOutlineStore(useShallow(...)) at line 224, but the newly added useState at line 212 is the hook introduced by this PR.
Fix: move useState before the early return
Move const [workbenchOpen, setWorkbenchOpen] = useState(false) before line 206 (if (!node) return null), alongside the other hook calls (lines 24–47). The useOutlineStore(useShallow(...)) at line 224 should also be moved above the early return for correctness.
Prompt for agents
In apps/nxus-editor/src/components/outline/node-block.tsx, the useState hook at line 212 and the useOutlineStore(useShallow(...)) hook at lines 224-234 are both placed AFTER the conditional early return at line 206 (if (!node) return null). This violates React's Rules of Hooks.
To fix:
1. Move `const [workbenchOpen, setWorkbenchOpen] = useState(false)` from line 212 to somewhere before line 206, e.g. after line 47 (after the useOutlineSync() call) or alongside the other hook declarations around lines 24-36.
2. Also move the `sortedChildren` hook (lines 224-234) to before line 206, since it's also a hook call after a conditional return. This was pre-existing but should be fixed as well.
3. All non-hook computations that depend on `node` (lines 208-220) should remain after the early return guard.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes