# Code Review: jarq - Interactive JSON Query Tool
**Review Date:** 2026-01-28
**Reviewer:** Claude Opus 4.5
**Codebase Version:** 0.7.3
---
## Executive Summary
The jarq codebase is generally well-structured with clean separation of concerns. The core filter evaluation system is well-tested with extensive unit tests covering parsing and evaluation. The architecture demonstrates good understanding of performance considerations (virtualized rendering, async loading, pipe caching).
However, there are several areas that could benefit from improvement in terms of testability, maintainability, and potential bugs. This review identifies 20+ issues across 7 categories with severity and complexity ratings.
### Rating Legend
**Severity:**
- š“ **Critical** - Could cause data loss, crashes, or security issues
- š **High** - Significant impact on reliability or maintainability
- š” **Medium** - Moderate impact, should be addressed
- š¢ **Low** - Minor issues, nice-to-have improvements
**Complexity:**
- ā¬ **High** - Requires significant refactoring or architectural changes
- ā¬ **Medium** - Moderate effort, localized changes
- ā½ **Low** - Simple fix, minimal changes required
---
## 1. Potential Bugs & Issues
### 1.1 Race Condition in Worker Cancellation
| š” Medium | ā½ Low | `worker.rs:79-82` |
```rust
// Skip if newer request waiting (implicit cancellation)
if !request_rx.is_empty() {
continue;
}
```
**Issue:** TOCTOU (Time-of-check to time-of-use) race condition. Between checking `is_empty()` and receiving the next request, another request could be enqueued. While not catastrophic, this can lead to unnecessary skipping of valid requests.
**Impact:** Occasionally, a filter evaluation might be skipped when it shouldn't be, causing stale results to display briefly.
**Recommendation:** Accept the race as benign (document it), or use a more sophisticated request tracking mechanism with sequence numbers that are checked after evaluation completes.
---
### 1.2 Silent Clipboard Failures ā
RESOLVED
| š” Medium | ā½ Low | `app/mod.rs:324-344` |
~~**Issue:** No user feedback when clipboard operations fail.~~
**Resolution:** Added status message system:
- `App` now has `status_message` field with auto-expiring messages (2 seconds)
- `copy_filter_to_clipboard` shows success message with copied command
- Clipboard errors show error message
- Status message appears above the filter input line (white text)
**Bonus:** Extracted all UI colors into `src/theme.rs` module:
- Centralized color definitions for JSON syntax, filter input, status bar, help popup, etc.
- Uses `THEME` static for easy access throughout UI code
- Foundation for future theme customization
---
### 1.3 Unconstrained Cache Growth
| š High | ā¬ Medium | `worker.rs:115-120` |
```rust
if let Ok(ref values) = result {
cache = Some(PipeCache {
filter_text: req.filter_text.clone(),
values: Arc::new(values.clone()),
slurp_mode: req.slurp_mode,
});
}
```
**Issue:** If the filter produces very large intermediate results, these are cached indefinitely. The cache clones the evaluation result (`Arc::new(values.clone())`), effectively doubling memory usage for that intermediate result.
**Impact:** For filters like `.[] | .` on a large array, the cache stores all intermediate values. Combined with the 3-5x memory overhead of `simd_json::OwnedValue`, a 128MB file could result in 500MB+ cached, plus the original result sent to the app.
**Why this is inherent:** The result needs to exist in two places - the cache (for future suffix evaluation) and the app (for display). Since filter evaluation produces new owned `Vec<Value>` data, sharing via `Arc` alone doesn't help: we'd need `Arc` at evaluation time, but then suffix evaluation still produces new owned results that face the same problem.
**Recommendation:** Memory-budget caching - accept the clone but skip caching when estimated size exceeds a threshold:
```rust
const CACHE_SIZE_LIMIT: usize = 50_000; // ~100MB assuming 2KB avg per Value
if let Ok(ref values) = result {
if values.len() <= CACHE_SIZE_LIMIT {
cache = Some(PipeCache { ... });
} else {
cache = None; // Too large to cache safely
}
}
```
This preserves caching for the common case (small-to-medium results where it's most useful) while preventing OOM on pathological cases. Document that pipe caching has memory overhead proportional to intermediate result size.
---
### 1.4 Potential Integer Underflow in Navigation
| š¢ Low | ā½ Low | `text_buffer.rs:157-158` |
```rust
**Issue:** The `i > 1` check prevents underflow, but the logic is fragile and easy to break during maintenance. The compound condition is hard to reason about.
**Recommendation:** Refactor to use `chars.get(i.wrapping_sub(2))` with explicit `Option` handling, or extract to a helper function with clearer semantics.
---
### 1.5 Incomplete String Escape Handling in Breakpoints ā
PARTIALLY RESOLVED
| š” Medium | ā¬ Medium | `filter/mod.rs`, `text_buffer.rs` |
~~**Issue:** The string detection logic doesn't handle:~~
~~- Escaped backslashes before quotes (`"\\"` followed by `"`)~~
~~- Unicode escapes that might contain quote-like sequences~~
~~**Impact:** Error position highlighting could be incorrect for filters with complex string literals.~~
**Resolution (Error Highlighting):** Implemented proper AST span tracking using `nom_locate`:
- Parser now tracks source positions for each AST node via `Span { start, end }`
- Eval errors set position from `filter.span.start` instead of heuristic scanning
- Worker adjusts error positions when using pipe cache (suffix evaluation)
- Removed unused `find_eval_error_position` function
**Remaining Issue (Navigation):** The `text_buffer.rs:compute_navigation_positions()` function still uses the same heuristic character scanning for Alt+Left/Right word jumping. While less critical (navigation just needs "good enough" stopping points vs. precise error positions), it could be improved to use AST spans for more accurate navigation boundaries.
**Future Enhancement:** Consider using AST spans for navigation positions:
- Would require parsing on text changes (or caching AST)
- Challenge: incomplete/invalid filters won't parse, breaking navigation mid-typing
- Possible hybrid: use AST when available, fall back to heuristics for invalid input
---
### 1.6 Inconsistent `has()` Behavior for Arrays
| š¢ Low | ā½ Low | `filter/eval.rs:258-268` |
```rust
Value::Array(arr) => {
if let Ok(idx) = key.parse::<usize>() {
Ok(vec![Value::Static(StaticNode::Bool(idx < arr.len()))])
} else {
Ok(vec![Value::Static(StaticNode::Bool(false))])
}
}
```
**Issue:** `has("-1")` returns `false` because negative numbers fail `parse::<usize>()`. In jq, `has(-1)` on an array checks if the last element exists (negative indexing).
**Recommendation:** Parse as `i64` first, then handle negative indices like the `Index` filter does.
---
## 2. Code Structure & Maintainability Issues
### 2.1 God Struct: `App`
| š” Medium | ā¬ High | `app/mod.rs:40-68` |
The `App` struct has 16 fields spanning multiple concerns:
```rust
pub struct App {
// Input/Loading (4 fields)
input_values: Option<Arc<Vec<Value>>>,
loading_state: LoadingState,
loader: Option<Loader>,
// Filter editing (4 fields)
filter_buffer: TextBuffer,
filter_error: Option<FilterError>,
pending_filter_update: Option<Instant>,
partial_eval_mode: bool,
// Output (2 fields)
output_values: Vec<Value>,
total_lines: usize,
// Mode flags (2 fields)
editing_filter: bool,
eval_paused: bool,
// Worker (1 field)
worker: Worker,
// Grouped state (4 fields)
help: HelpState,
eval: EvalTracker,
viewport: Viewport,
display: DisplayOptions,
}
```
**Issue:** Single struct orchestrating loading, editing, evaluation, display, and help. Changes to any subsystem risk affecting others.
**Recommendation:** Consider a more modular architecture:
- `InputManager` - handles loading and input values
- `FilterEditor` - manages filter text, validation, partial eval
- `EvalCoordinator` - worker communication, caching, debouncing
- `App` becomes a thin coordinator
---
### 2.2 Duplicated `type_name` Functions ā
RESOLVED
| š¢ Low | ā½ Low | Multiple files |
~~Three separate `type_name` implementations existed:~~
- ~~`filter/eval.rs:225-234`~~
- ~~`filter/builtins/mod.rs:14-23`~~
- ~~`error.rs:58-68` (inline in `Display` impl)~~
**Resolution:** Centralized to `src/utils.rs`:
- Created `utils.rs` with single `pub fn type_name()`
- All usages now import directly from `crate::utils::type_name`
---
### 2.3 Large Monolithic `eval.rs` File
| š” Medium | ā¬ High | `filter/eval.rs` (1724 lines) |
The evaluation logic file contains:
- Main `eval` function (220+ line match statement)
- All function implementations (`eval_map`, `eval_select`, etc.)
- Comparison logic
- 900+ lines of tests
**Recommendation:** Split into focused modules:
```
filter/
eval/
mod.rs - Main eval dispatch
accessor.rs - Field, Index, Slice, Iterate
functions.rs - Map, Select, SortBy, etc.
comparisons.rs - Compare, BoolOp
construction.rs - Array, Object construction
```
---
### 2.4 Inconsistent Error Handling Patterns ā
RESOLVED
| š” Medium | ā¬ Medium | Multiple files |
~~Different error patterns across the codebase:~~
- ~~`loader.rs` uses `Result<_, String>`~~
- ~~`filter/` uses `Result<_, FilterError>` with proper typed errors~~
- `main.rs` uses `anyhow::Result` (appropriate for binary entry point)
**Resolution:** Standardized on `thiserror`-derived errors:
- Converted `FilterError`, `ParseError`, `EvalError` to use `#[derive(thiserror::Error)]`
- Added `LoadError` enum with `Io`, `ParseLine`, `EmptyInput`, `NoValidJson` variants
- Updated `loader.rs` to use `LoadError` instead of `String`
- Updated `LoadingState::Failed` to hold `LoadError`
---
## 3. Testability Issues
### 3.1 Untestable TUI Code
| š High | ā¬ Medium | `main.rs:163-203` |
```rust
fn run_tui_loop(app: &mut App) -> Result<()> {
// Direct terminal I/O - impossible to unit test
let mut terminal = Terminal::new(CrosstermBackend::new(stdout))?;
loop {
terminal.draw(|frame| ui::render(frame, app))?;
if event::poll(POLL_TIMEOUT)? {
match event::read()? {
Event::Key(key) => match app.handle_key(key) { ... }
}
}
app.tick();
}
}
```
**Issue:** Core application loop directly interacts with terminal I/O, making it impossible to unit test state transitions.
**Recommendation:** Extract a pure `update` function:
```rust
enum AppEvent {
Key(KeyEvent),
Tick,
Resize(u16, u16),
}
fn update(app: &mut App, event: AppEvent) -> ControlFlow<()> {
// Pure logic, no I/O - fully testable
}
```
---
### 3.2 No Tests for `App` Logic
| š High | ā¬ Medium | `app/mod.rs` |
The core `App` struct has no unit tests. Untested behaviors include:
- Debounce timing logic (`pending_filter_update`)
- Eval cancellation (`cancel_eval`)
- Mode transitions (navigation ā filter editing)
- Partial eval mode toggling
- Slurp mode interactions
**Recommendation:** Add focused unit tests:
```rust
#[cfg(test)]
mod tests {
#[test]
fn test_debounce_schedules_update() {
let mut app = App::new(vec![json!({"a": 1})]);
app.filter_buffer.insert('x');
app.mark_filter_dirty();
assert!(app.pending_filter_update.is_some());
}
#[test]
fn test_cancel_clears_pending() {
// ...
}
}
```
---
### 3.3 UI Rendering Not Tested
| š” Medium | ā¬ Medium | `ui.rs` |
Only basic virtualized rendering has tests. Untested:
- Main `render` function
- `render_filter_input` with error highlighting
- `render_help_popup` scrolling
- Status bar content
**Recommendation:** Use `ratatui`'s `TestBackend`:
```rust
#[test]
fn test_filter_error_rendering() {
let backend = TestBackend::new(80, 24);
let mut terminal = Terminal::new(backend).unwrap();
let mut app = App::new(vec![json!(1)]);
app.filter_error = Some(FilterError::Parse(...));
terminal.draw(|f| render(f, &mut app)).unwrap();
let buffer = terminal.backend().buffer();
assert!(buffer_contains(buffer, "Error:"));
}
```
---
### 3.4 Worker Thread Not Unit Tested
| š” Medium | ā¬ Medium | `worker.rs` |
The worker's pipe caching logic (`PipeCache::get_suffix`) is tested only indirectly through integration tests.
**Recommendation:** Extract and test the caching logic in isolation:
```rust
#[cfg(test)]
mod tests {
#[test]
fn test_pipe_cache_suffix_detection() {
let cache = PipeCache {
filter_text: ".[] | .name".to_string(),
values: Arc::new(vec![]),
slurp_mode: false,
};
assert_eq!(cache.get_suffix(".[] | .name | length", false), Some("length"));
assert_eq!(cache.get_suffix(".[] | .other", false), None);
}
}
```
---
## 4. Performance Concerns
### 4.1 Excessive Cloning in Slurp Mode
| š High | ā¬ Medium | `worker.rs:142-148` |
```rust
fn prepare_inputs(req: &EvalRequest) -> Arc<Vec<Value>> {
if req.slurp_mode {
Arc::new(vec![Value::Array(Box::new((*req.inputs).clone()))]) // Full deep clone
} else {
Arc::clone(&req.inputs)
}
}
```
**Issue:** When slurp mode is enabled, the entire input is deep-cloned. For large files, this doubles memory usage during evaluation.
**Impact:** A 500MB JSON file in memory becomes 1GB+ during slurp mode evaluation.
**Recommendation:** Consider lazy wrapping or COW (copy-on-write) semantics. At minimum, document the memory implications of slurp mode.
---
### 4.2 O(nĀ²) Algorithm in `filter_breakpoints`
| š¢ Low | ā½ Low | `filter/mod.rs:108` |
```rust
if !breakpoints.contains(&i) { // O(n) search on every iteration
breakpoints.push(i);
}
```
**Issue:** Linear search on every character for long filters. A filter with 1000 characters could result in ~500K comparisons.
**Recommendation:** Use a `HashSet` for deduplication:
```rust
let mut breakpoints: HashSet<usize> = HashSet::new();
breakpoints.insert(0);
// ... collect breakpoints ...
let mut result: Vec<_> = breakpoints.into_iter().collect();
result.sort();
```
---
### 4.3 Repeated Navigation Position Computation
| š¢ Low | ā¬ Medium | `text_buffer.rs:198-234` |
```rust
pub fn jump_word_back(&mut self) {
let positions = self.navigation_positions(); // Recomputed every jump
// ...
}
```
**Issue:** `navigation_positions()` rescans the entire text on every cursor move, even when text hasn't changed.
**Recommendation:** Cache navigation positions and invalidate on text modification:
```rust
struct TextBuffer {
text: String,
cursor: usize,
nav_cache: Option<Vec<usize>>, // Invalidated on insert/delete
}
```
---
## 5. Missing Features / Code Gaps
### 5.1 No Graceful Worker Thread Shutdown
| š” Medium | ā½ Low | `worker.rs` |
```rust
pub struct Worker {
request_tx: Sender<EvalRequest>,
result_rx: Receiver<EvalResult>,
_handle: JoinHandle<()>, // Never joined
}
```
**Issue:** The worker thread is orphaned on app exit. While not problematic for short-lived CLI usage, it's poor hygiene and could cause issues if jarq were used as a library.
**Recommendation:** Implement graceful shutdown:
```rust
impl Drop for Worker {
fn drop(&mut self) {
// Dropping the sender signals the worker to exit
// (recv() will return Err when all senders are dropped)
}
}
```
Or use an explicit shutdown channel for immediate termination.
---
### 5.2 No Loader Thread Cancellation
| š¢ Low | ā¬ Medium | `loader.rs` |
If a user quits while loading a large file, the loader thread continues reading until completion.
**Recommendation:** Add a cancellation mechanism using an `AtomicBool` or channel.
---
### 5.3 No Configuration File Support
| š¢ Low | ā¬ High | N/A |
Users cannot customize:
- Default display options (wrap, raw, compact)
- Keybindings
- Color scheme
- Debounce duration
**Recommendation:** Consider adding `~/.config/jarq/config.toml` support in a future version.
---
## 6. Code Style & Cleanup
### 6.1 Dead Code
| š¢ Low | ā½ Low | `main.rs:157-161` |
```rust
#[allow(dead_code)]
fn run_interactive(values: Vec<simd_json::OwnedValue>) -> Result<()> {
let mut app = App::new(values);
run_tui_loop(&mut app)
}
```
**Issue:** Function is explicitly marked dead but retained. Per CLAUDE.md guidelines, unused code should be removed.
**Recommendation:** Remove the function entirely.
---
### 6.2 Magic Numbers
| š¢ Low | ā½ Low | Multiple files |
Undocumented constants:
- `DEBOUNCE_DURATION: 250ms` (documented but not configurable)
- `PARALLEL_THRESHOLD: 10_000` (appears in `filter/mod.rs:28` and `filter/eval.rs:11`)
- `POLL_TIMEOUT: 50ms`
**Recommendation:** Centralize constants in a `config.rs` module with documentation explaining the rationale for each value.
---
### 6.3 Inconsistent Use of `#[allow(...)]`
| š¢ Low | ā½ Low | Multiple files |
Some files use `#[allow(clippy::too_many_arguments)]` without attempting to refactor. Example: `ui.rs:597`.
**Recommendation:** Either refactor the function signatures (e.g., use a config struct) or add comments explaining why the many arguments are necessary.
---
## 7. Positive Observations
### 7.1 Excellent Parser Test Coverage
The `filter/parser/mod.rs` contains 100+ test cases covering:
- All syntax variations
- Edge cases (empty strings, unicode, escapes)
- Error conditions
- Operator precedence
### 7.2 Clean AST Design
The `Filter` enum is well-structured and exhaustive:
```rust
pub enum Filter {
Identity,
Field(String),
Index(i64),
Slice(Option<i64>, Option<i64>),
// ... all variants clearly named
}
```
### 7.3 Efficient Virtualized Rendering
The line counting and viewport rendering in `ui.rs` correctly handles large files by only rendering visible lines. The separation of `count_total_lines` and `render_viewport` is elegant.
### 7.4 Proper Panic Hook Handling
`main.rs:170-175` correctly installs a panic hook to restore terminal state:
```rust
let original_hook = panic::take_hook();
let _ = io::stdout().execute(LeaveAlternateScreen);
original_hook(panic_info);
}));
```
### 7.5 Unicode-Aware Text Buffer
The `TextBuffer` correctly handles multi-byte characters by tracking cursor position in characters, not bytes.
### 7.6 Comprehensive Builtin Implementation
All common jq builtins are implemented with correct semantics:
- `length`, `keys`, `values`, `type`
- `sort`, `reverse`, `unique`, `flatten`
- `first`, `last`, `min`, `max`, `add`
- `to_entries`, `from_entries`, `with_entries`
---
## Summary Table
| 1.1 | Worker race condition | š” Medium | ā½ Low | Bug |
| 1.2 | ā
~~Silent clipboard failures~~ | š” Medium | ā½ Low | Bug |
| 1.3 | Unconstrained cache growth | š High | ā¬ Medium | Bug |
| 1.4 | Integer underflow risk | š¢ Low | ā½ Low | Bug |
| 1.5 | ā
~~Incomplete string escape handling~~ (nav pending) | š” Medium | ā¬ Medium | Bug |
| 1.6 | Inconsistent `has()` for arrays | š¢ Low | ā½ Low | Bug |
| 2.1 | God struct `App` | š” Medium | ā¬ High | Structure |
| 2.2 | ā
~~Duplicated `type_name`~~ | š¢ Low | ā½ Low | Structure |
| 2.3 | Large monolithic `eval.rs` | š” Medium | ā¬ High | Structure |
| 2.4 | ā
~~Inconsistent error handling~~ | š” Medium | ā¬ Medium | Structure |
| 3.1 | Untestable TUI code | š High | ā¬ Medium | Testability |
| 3.2 | No `App` unit tests | š High | ā¬ Medium | Testability |
| 3.3 | UI rendering not tested | š” Medium | ā¬ Medium | Testability |
| 3.4 | Worker not unit tested | š” Medium | ā¬ Medium | Testability |
| 4.1 | Excessive cloning in slurp | š High | ā¬ Medium | Performance |
| 4.2 | O(nĀ²) in filter_breakpoints | š¢ Low | ā½ Low | Performance |
| 4.3 | Repeated nav computation | š¢ Low | ā¬ Medium | Performance |
| 5.1 | No worker shutdown | š” Medium | ā½ Low | Missing |
| 5.2 | No loader cancellation | š¢ Low | ā¬ Medium | Missing |
| 6.1 | Dead code | š¢ Low | ā½ Low | Cleanup |
| 6.2 | Magic numbers | š¢ Low | ā½ Low | Cleanup |
---
## Recommended Priority Order
### Immediate (High Impact, Low Effort)
1. Remove dead code (`main.rs:157-161`)
2. ā
~~Add clipboard failure feedback~~
3. ā
~~Centralize `type_name` function~~
4. Fix O(nĀ²) in `filter_breakpoints`
### Short-Term (High Impact)
1. Add `App` unit tests
2. Add cache size limits to worker
3. Implement worker graceful shutdown
4. Fix slurp mode memory duplication
### Medium-Term (Architectural)
1. Extract testable TUI update function
2. Split `eval.rs` into modules
3. ā
~~Standardize error handling~~
4. Add UI rendering tests
### Long-Term (Nice to Have)
1. Refactor `App` struct
2. Add configuration file support
3. Cache navigation positions
4. Add loader cancellation
---
*End of Code Review*