Add &field candidate completion for JMESPath function arguments - #113
Conversation
- Show field candidates when &field placeholder is active (sort_by, max_by, min_by, map) - Remove placeholder text immediately on entering candidate mode - Fix cursor position: stays between & and ) while Tab-cycling candidates - Ctrl+W treats &field) as one unit and preserves & on deletion - Suppress misleading green hint in &partial mode - Add ampFieldCursorPos helper and ampFieldPartial/ampFieldCandidates functions - Update ChangeLog, README, and CLI help with new features and key bindings
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5a3b5f479
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ampIdx := strings.LastIndex(suffix, "&"); ampIdx >= 0 && strings.Contains(suffix, "(") { | ||
| // &field typing inside function argument (e.g. "sort_by(@, &na)"). | ||
| // Replace everything after the last "&" with the selected field name. | ||
| absAmpIdx := strings.LastIndex(qs, "&") | ||
| _ = e.query.StringSet(qs[:absAmpIdx+1] + selected + ")") |
There was a problem hiding this comment.
This branch rewrites everything after the last & whenever the pipe suffix contains (, which also matches completed expressions like . | max_by(@, &age). while selecting the next field candidate. In that case, confirming name becomes . | max_by(@, &name) instead of appending .name, so users cannot continue navigation from a max_by/sort_by result and silently change query semantics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 402e7ef. Replaced the raw strings.LastIndex(suffix, "&") >= 0 && strings.Contains(suffix, "(") guard with ampFieldPartial(suffix), which only returns true when the cursor is genuinely inside an active &partial edit (i.e. the portion after & consists purely of identifier characters or is empty). This means already-completed expressions like max_by(@, &age).field no longer trigger the replacement when the user is selecting the next navigation field.
| if result.Interface() == nil { | ||
| if baseExpr2, hasPipe2 := baseExprBeforePipe(qs); hasPipe2 { | ||
| suffix2 := pipeSuffix(qs) | ||
| if partial, ok := ampFieldPartial(suffix2); ok { | ||
| return jm.ampFieldCandidates(baseExpr2, partial) |
There was a problem hiding this comment.
The null-result fallback now always returns ampFieldCandidates, even during confirmed evaluation (confirm=true). That means a query like .stats | max_by(@, &missing_field) can no longer produce its actual null result when the user exits; instead it returns the base array used for suggestions, which is incorrect output corruption in final execution mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 402e7ef. Added !confirm guard to both places where the ampFieldCandidates fallback can fire: (1) the null-result check after evalJMESPath succeeds, and (2) the error-recovery path when evalJMESPath fails (e.g. max_by(@, &missing) causes a type error from go-jmespath). In the error case the function now returns early with the parse error rather than silently returning the base array. A test covers this: GetFilteredData(q, true) on . | max_by(@, &missing) now returns a non-nil error instead of the suggestion base array.
| case '&': | ||
| // Inside a function call: "&field)" is one deletion unit; keep the "&". | ||
| // e.g. "max_by(@, &base_stat)" → "max_by(@, &" | ||
| if parenDepth > 0 && bracketDepth == 0 { | ||
| return expr[:i+1] |
There was a problem hiding this comment.
The new & deletion rule triggers for any ampersand inside parentheses, including string literals, so Ctrl+W on expressions such as . | contains(@, 'a&b') truncates to . | contains(@, 'a& instead of deleting one logical segment. This introduces malformed intermediate queries for normal string searches containing &.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 402e7ef. Added string-literal tracking to removeLastJMESPathSegment: when scanning right-to-left, single/double quote characters toggle an inString flag, and the & case is skipped while inside a literal. A test covers this: removeLastJMESPathSegment("contains(@, 'a&b')") now returns "" (full expression removed as one segment) instead of truncating at the &.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 849faf6f74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ampIdx := strings.LastIndex(suffix, "&"); ampIdx >= 0 && strings.Contains(suffix, "(") { | ||
| // &field typing inside function argument (e.g. "sort_by(@, &na)"). | ||
| // Replace everything after the last "&" with the selected field name. | ||
| absAmpIdx := strings.LastIndex(qs, "&") | ||
| _ = e.query.StringSet(qs[:absAmpIdx+1] + selected + ")") |
There was a problem hiding this comment.
Selecting a normal field candidate after a completed function expression can now rewrite the function argument instead of appending navigation, because this branch only checks for "(" and any "&" in the pipe suffix. For example, from . | max_by(@, &age). choosing name becomes . | max_by(@, &name) rather than . | max_by(@, &age).name, silently changing query semantics. Fresh evidence: in this commit, confirmCandidate at engine.go still unconditionally enters amp-replacement mode based on strings.LastIndex(suffix, "&") plus strings.Contains(suffix, "(").
Useful? React with 👍 / 👎.
| if result.Interface() == nil { | ||
| if baseExpr2, hasPipe2 := baseExprBeforePipe(qs); hasPipe2 { | ||
| suffix2 := pipeSuffix(qs) | ||
| if partial, ok := ampFieldPartial(suffix2); ok { | ||
| return jm.ampFieldCandidates(baseExpr2, partial) |
There was a problem hiding this comment.
The null-result fallback now returns ampFieldCandidates even when confirm=true, so final execution can emit the base array used for suggestions instead of the true evaluated result. A query like .stats | max_by(@, &missing_field) should resolve to null on Enter, but this path substitutes candidate context data and corrupts output. Fresh evidence: in json_manager.go, the result.Interface() == nil branch always returns jm.ampFieldCandidates(...) without checking confirm.
Useful? React with 👍 / 👎.
| case '&': | ||
| // Inside a function call: "&field)" is one deletion unit; keep the "&". | ||
| // e.g. "max_by(@, &base_stat)" → "max_by(@, &" | ||
| if parenDepth > 0 && bracketDepth == 0 { | ||
| return expr[:i+1] |
There was a problem hiding this comment.
Backward-segment deletion now treats any & inside parentheses as an amp-field boundary, including ampersands in string literals, so Ctrl+W can truncate valid expressions into malformed ones (for example, . | contains(@, 'a&b') can be cut back to an unterminated literal). This impacts normal typing flows whenever string arguments contain &. Fresh evidence: removeLastJMESPathSegment returns at any & with parenDepth > 0, without checking whether that character is inside a quoted literal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
- Fix confirmCandidate: use ampFieldPartial() to guard & replacement, preventing it from firing on already-completed expressions like max_by(@, &age).field when navigating the result - Fix null-result ampField fallback: skip when confirm=true so that a confirmed query propagates JMESPath errors instead of returning the base array used for suggestions - Fix removeLastJMESPathSegment: track string literals so that & inside quotes (e.g. contains(@, 'a&b')) does not trigger the &-stop rule
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
&fieldplaceholder (e.g.sort_by(@, &field)), field candidates from the base array are automatically shown for Tab-cycling or partial typing&and)for clean inline editingCtrl+Wtreats&field)as one deletion unit and stops at&, preserving it (e.g.max_by(@, &base_stat)→max_by(@, &)&partialmode (hint was appearing after))&and)while Tab-cycling field candidatesChanges
json_manager.go: AddampFieldPartial(detects&partialpattern) andampFieldCandidates(returns field candidates from base array); suppress inline hint in&partialmodeengine.go: Fix placeholder deletion cursor position (placeholderStartinstead ofquery.Length()); addampFieldCursorPoshelper for Tab/Shift+Tab; extendremoveLastJMESPathSegmentto treat&field)as a unit and preserve&terminal.go: Change placeholder color fromColorBluetoColorCyan(lighter)engine_test.go,json_manager_test.go: Tests forampFieldPartial,ampFieldCandidates, cursor position after placeholder deletion,ampFieldCursorPos,removeLastJMESPathSegmentwith&field, anddeleteWordBackwardwith&fieldChangeLog,README.md,cmd/jid/jid.go: Document new behavior and update CLI helpTest plan
go test ./...passes.stats | sort_by(@, &→ field candidates appear; Tab cycles; selection inserts correctlyCtrl+Wonmax_by(@, &base_stat)→max_by(@, &&filters candidates correctly@codex review