feat: allow field/arg toggling when selection set is empty#39
feat: allow field/arg toggling when selection set is empty#39david-on-github wants to merge 4 commits into
Conversation
Fixes #24. GraphQL's parser rejects empty selection sets ({ }), which previously caused all sidebar buttons to go disabled when the query editor contained one. - Changed DEFAULT_QUERY to empty string so the editor starts parseable - Added patchEmptySelectionSets() helper (negative lookbehind to avoid touching ObjectValue args like filter: {}) - Added findRootFieldSelectionSet() text scanner to locate a root field's selection set without parsing - toggleFieldInQuery, toggleArgInQuery, canToggleInputType, ensureArgAndToggleInputField all fall back to patch-then-restore when parse() throws on an empty selection set - Field insertion into empty selection sets derives indentation from the close-brace's actual line position, avoiding stray blank lines - getCursorContext falls back to a text scan for root operation detection when the query is unparseable, so the sidebar auto-navigates correctly when the cursor is inside an empty selection set - Removed disabled={!inQuery} from the Fields section buttons in SchemaExplorer so fields can be clicked before a selection set exists - Fixed React duplicate key warning in DashboardView peers list
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughQuery builder logic gains parse-failure recovery for empty GraphQL selection sets ( ChangesEmpty Selection Set Query Toggling
Peers List Rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SchemaExplorer
participant QueryBuilder
participant GraphQLParser
SchemaExplorer->>QueryBuilder: toggleFieldInQuery(query, field)
QueryBuilder->>GraphQLParser: parse(query)
GraphQLParser-->>QueryBuilder: parse error (empty selection set)
QueryBuilder->>QueryBuilder: patchEmptySelectionSets(query)
QueryBuilder->>GraphQLParser: parse(patched query)
GraphQLParser-->>QueryBuilder: AST
QueryBuilder->>QueryBuilder: apply toggle, restore original braces
QueryBuilder-->>SchemaExplorer: updated query
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
defradb-explorer | 4cd2778 | Commit Preview URL Branch Preview URL |
Jul 05 2026, 05:21 PM |
Covers the catch-block repair logic in toggleFieldInQuery, toggleArgInQuery, isRootFieldInQuery, canToggleInputType, ensureArgAndToggleInputField, and getCursorContext for queries containing empty selection sets that parse() rejects. Also removes the dead '!ssInfo' fallback path from toggleFieldInQuery that would incorrectly build a fresh document for garbled input.
Extends the empty-selection-set fallback to cover the case where the
cursor is inside an ObjectValue arg (e.g. filter: {}) while the query
also has an empty selection set.
- getCursorContext catch block now text-scans each root field's arg list
for input-object args and returns insertObject when the cursor is
found inside one, enabling sidebar auto-navigation to the filter type
- toggleInputObjectFieldAtOffset gains a patchEmptySelectionSets
fallback so clicking filter fields actually modifies the query
- getInputObjectFieldsAtOffset gains the same fallback so toggled
fields show as selected after insertion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/lib/queryBuilder.ts`:
- Around line 244-262: findRootFieldSelectionSet() currently matches the first
textual rootFieldName anywhere in the query, which can target nested or
unrelated occurrences; update the fallback scan to track brace/operation depth
and only accept rootFieldName when it appears in the top-level selection set.
Keep the existing selection parsing logic in src/lib/queryBuilder.ts, but gate
the RegExp match and subsequent scan on root-level context so fallback toggles
patch the correct selection set.
- Line 235: The patch in patchEmptySelectionSets() is too broad because it
rewrites any empty braces that are not after : or [, so it still mutates empty
object values after commas. Update queryBuilder.ts to use a more context-aware
scan (or extend the matcher) so only true empty selection sets are patched, and
exclude object/list value positions such as commas and default-value contexts;
keep the fix localized to patchEmptySelectionSets() and its return query.replace
logic.
- Line 235: The fallback-path changes in queryBuilder are tripping ESLint: clean
up the regex in the query replacement logic by removing the unnecessary escaping
in the character class, and update the empty catch blocks in the related
fallback sections to include explicit comments so they are not flagged. Apply
the same lint-safe cleanup in the affected fallback helpers around the
queryBuilder flow, including the logic near the query.replace call and the empty
catches referenced in the review.
- Around line 1335-1342: The argument lookup in queryBuilder’s field parsing is
scanning beyond the current field’s parentheses, so later arguments can be
incorrectly associated with the current field. In the logic around the
`query[pos] === '('` block, first find the matching closing parenthesis for the
current field, then restrict the `argMatch` search to that slice instead of
`query.slice(argListStart)`. Keep the fix local to this parser path so
`fieldDef.args`, `getNamedType`, and `isInputObjectType` still behave the same.
- Line 159: The placeholder restoration in queryBuilder is using a first-match
replace, which can target the wrong __typename occurrence or miss additional
patched spots. Update the logic around the toggled query reconstruction to
restore only the specific empty selection set that was originally patched,
preferably by tracking the patched range(s) or re-locating the exact selection
set before replacement. Apply the same fix to the other __typename restore sites
in queryBuilder so each patched placeholder is matched deterministically rather
than by the first string occurrence.
In `@src/views/DashboardView.tsx`:
- Around line 38-39: The peers list in DashboardView should use a stable unique
key instead of the array index. Update the map in DashboardView.tsx to key each
row by p.id, matching the existing PeersView.tsx pattern, and keep the row
rendering tied to the peer identity rather than position. If duplicate key
warnings were the original concern, fix the source of non-unique peer ids
instead of switching to index-based keys.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96e545cf-0aaa-41a4-9cb8-25a6cc14b746
📒 Files selected for processing (5)
src/components/SchemaExplorer.tsxsrc/lib/queryBuilder.test.tssrc/lib/queryBuilder.tssrc/views/DashboardView.tsxsrc/views/QueryView.tsx
💤 Files with no reviewable changes (1)
- src/components/SchemaExplorer.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Workers Builds: defradb-explorer
🧰 Additional context used
🪛 ast-grep (0.44.1)
src/lib/queryBuilder.ts
[warning] 243-243: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${rootFieldName}\\b)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 1330-1330: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${fieldName}\\b)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 1339-1339: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${arg.name}\\s*:\\s*\\{)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 1361-1361: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${fieldName}\\s*(?:\\([^)]*\\))?\\s*\\{, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 ESLint
src/lib/queryBuilder.ts
[error] 235-235: Unnecessary escape character: [.
(no-useless-escape)
[error] 784-784: Unnecessary escape character: [.
(no-useless-escape)
[error] 788-788: Empty block statement.
(no-empty)
[error] 966-966: Empty block statement.
(no-empty)
[error] 1012-1012: Unnecessary escape character: [.
(no-useless-escape)
[error] 1013-1013: Empty block statement.
(no-empty)
🪛 OpenGrep (1.23.0)
src/lib/queryBuilder.ts
[ERROR] 244-244: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 1331-1331: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 1340-1340: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 1364-1364: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (3)
src/views/QueryView.tsx (1)
49-49: LGTM!src/lib/queryBuilder.test.ts (1)
20-21: LGTM!Also applies to: 969-1073
src/lib/queryBuilder.ts (1)
423-431: LGTM!
| const m = new RegExp(`\\b${rootFieldName}\\b`).exec(query) | ||
| if (!m) return null | ||
| let i = m.index + m[0].length | ||
| while (i < query.length && /[ \t]/.test(query[i])) i++ | ||
| if (query[i] === '(') { | ||
| let d = 1; i++ | ||
| while (i < query.length && d > 0) { | ||
| if (query[i] === '(') d++; else if (query[i] === ')') d-- | ||
| i++ | ||
| } | ||
| } | ||
| while (i < query.length && /\s/.test(query[i])) i++ | ||
| if (i >= query.length || query[i] !== '{') return null | ||
| const start = i; let d = 1; i++ | ||
| while (i < query.length && d > 0) { | ||
| if (query[i] === '{') d++; else if (query[i] === '}') d-- | ||
| i++ | ||
| } | ||
| return { start, end: i, text: query.slice(start, i) } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Constrain the fallback scan to root-level fields.
findRootFieldSelectionSet() returns the first textual rootFieldName anywhere in the query. A nested Post { } or a same-name token before the actual root field can make fallback toggles patch the wrong selection set or fail to recover. Track operation/brace depth and only accept matches in the operation’s top-level selection set.
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 244-244: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 `@src/lib/queryBuilder.ts` around lines 244 - 262, findRootFieldSelectionSet()
currently matches the first textual rootFieldName anywhere in the query, which
can target nested or unrelated occurrences; update the fallback scan to track
brace/operation depth and only accept rootFieldName when it appears in the
top-level selection set. Keep the existing selection parsing logic in
src/lib/queryBuilder.ts, but gate the RegExp match and subsequent scan on
root-level context so fallback toggles patch the correct selection set.
| {peers!.map((p, i) => ( | ||
| <div key={i} className={styles.peerRow}> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Revert to keying by p.id instead of array index.
Using the index as a React key for a dynamic, externally-fetched peers list is an anti-pattern — it breaks reconciliation when peers connect/disconnect or reorder, causing stale DOM/state per row. p.id is already used as a stable, unique key in PeersView.tsx for the same peer shape, so this should be consistent here rather than regressing to index-based keys.
🔧 Proposed fix
- {peers!.map((p, i) => (
- <div key={i} className={styles.peerRow}>
+ {peers!.map(p => (
+ <div key={p.id} className={styles.peerRow}>If the intent was to fix a duplicate-key warning caused by non-unique p.id values, the actual fix should address the source of duplicate ids rather than discarding stable keys.
📝 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.
| {peers!.map((p, i) => ( | |
| <div key={i} className={styles.peerRow}> | |
| {peers!.map(p => ( | |
| <div key={p.id} className={styles.peerRow}> |
🤖 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 `@src/views/DashboardView.tsx` around lines 38 - 39, The peers list in
DashboardView should use a stable unique key instead of the array index. Update
the map in DashboardView.tsx to key each row by p.id, matching the existing
PeersView.tsx pattern, and keep the row rendering tied to the peer identity
rather than position. If duplicate key warnings were the original concern, fix
the source of non-unique peer ids instead of switching to index-based keys.
ensureArgAndToggleInputField was silently no-oping when the query had an
empty selection set and the target ObjectValue was nested (e.g. filter: {
title: {} }). hasInputObjectInQuery used its own patchEmptySelectionSets
fallback to find the type without throwing, then toggleInputObjectField
caught the parse error and returned the query unchanged — no exception was
raised, so the outer try/catch never fell through to the patch+retry path.
Fix: compare the return value of run(query) against the original query and
only return early if something actually changed; otherwise fall through to
the patched retry path.
Adds tests for both toggleInputObjectFieldAtOffset and
ensureArgAndToggleInputField with a nested empty ObjectValue and empty SS.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Fixes #24
Summary
Test plan