feat: multi-column Y-axis plot for series comparison

[SpollaL]

Summary

• Adds a new PlotPickY mode that lets users select multiple Y-axis columns before picking X
• PlotState.y_col replaced with y_cols: Vec<usize> to support multi-series rendering
• Histogram plot type is disabled when multiple Y columns are selected (only Line/Bar cycle)
• QA script (qa.sh) updated to cover the new multi-Y selection flow

Test plan

• Press p in Normal mode → enters PlotPickY, navigate columns with ←/→ (or h/l), toggle selection with Space
• Press Enter to advance to PlotPickX, Esc to go back to PlotPickY, Esc again to cancel
• Single-Y selection: Line → Bar → Histogram cycle still works with t
• Multi-Y selection: t cycles only Line ↔ Bar (no Histogram)
• Run bash qa.sh and verify exit 0
• Run cargo test

🤖 Generated with Claude Code

[SpollaL]

@claude review this merge request

[claude]

Claude finished @SpollaL's task in 2m 45s —— View job

---

PR Review: feat/multi-column Y-axis plot for series comparison

• Gather context and read changed files
• Review src/app.rs changes
• Review src/events.rs changes
• Review src/ui.rs changes
• Review qa.sh and .githooks/pre-commit changes
• Summarize findings

---

Overall this is a well-structured feature. The state machine changes are clean, the color palette integration is consistent, and the QA script coverage is thorough. A couple of issues to address before merging.

Bugs

1. first_len should come from nonempty, not all_series — src/ui.rs:1150

// current — wrong if first Y col is non-numeric
let first_len = all_series.first().map(|(d, _)| d.len()).unwrap_or(0);

// fix
let first_len = nonempty.first().map(|(_, d)| d.len()).unwrap_or(0);

If the first selected Y column has all-null or non-numeric data its series will be empty, making first_len = 0. Categorical X labels then won't render even for other valid series in the same plot. nonempty is computed a few lines later; first_len should use it instead.

Fix this →

---

2. Y-axis title regresses to "Value" for single-Y plots — src/ui.rs:1278

// current — always generic
.title("Value")

// fix — restore column name for single-Y, generic for multi
.title(if app.plot.y_cols.len() == 1 {
    app.headers[app.plot.y_cols[0]].as_str()
} else {
    "Value"
})

Previously single-series plots showed the column name on the Y axis. That's now gone even when only one column is selected. The legend only appears for multi-Y, so single-Y users lose the axis label with no replacement.

Fix this →

---

Minor / Nits

3. Duplicate cycle_hint logic — src/ui.rs:477 and src/ui.rs:1176

The cycle_hint string is computed identically in get_bar() and directly inside render_plot(). In Plot mode ui() returns early before get_bar() is called, so the Mode::Plot arm in get_bar() is actually unreachable dead code — render_plot() owns the status bar. The duplication won't cause a runtime bug today, but the two copies can drift. Worth either removing the dead Mode::Plot arm from get_bar() and shortcut_bar(), or adding a comment noting that Plot mode renders its own bar.

---

4. No unit tests for PlotPickY toggle — src/app_tests.rs

The Space-toggle logic in events.rs (add/remove from y_cols) is a new non-trivial invariant — e.g. toggling the same column twice should return to the original state. There are no tests for this. Given the existing test pattern in app_tests.rs, a small test in the plot_tests module would guard against future regressions.

---

5. CLAUDE.md PlotState table is stale

The architecture table in CLAUDE.md still shows y_col and doesn't list PlotPickY in the Mode variants section. Not blocking, but keeping the docs in sync with the code is worth a one-line update.

---

What looks good

• State machine transitions are clean: Normal → PlotPickY → PlotPickX → Plot, with Esc walking back correctly at each step
• Pre-selecting the current column on p is ergonomic; the status bar shows the live selection list clearly
• series_color() maps series indices to the Catppuccin Mocha palette consistently with the rest of the codebase
• Legend bounds-checking in render_plot_legend() prevents overflow on small terminals
• Series color indices are stable over the nonempty filter (empty series don't shift the color of later ones)
• Histogram correctly disabled for multi-Y (both in event handling and in the status bar hint)
• QA script coverage (J1–J4) is thorough and well-labelled
• Pre-commit hook sourcing $HOME/.cargo/env is the right fix for CI environments where cargo isn't in PATH

[SpollaL]

Claude finished @SpollaL's task in 2m 45s —— View job

PR Review: feat/multi-column Y-axis plot for series comparison

• Gather context and read changed files
• Review src/app.rs changes
• Review src/events.rs changes
• Review src/ui.rs changes
• Review qa.sh and .githooks/pre-commit changes
• Summarize findings

Overall this is a well-structured feature. The state machine changes are clean, the color palette integration is consistent, and the QA script coverage is thorough. A couple of issues to address before merging.

Bugs

1. first_len should come from nonempty, not all_series — src/ui.rs:1150

// current — wrong if first Y col is non-numeric
let first_len = all_series.first().map(|(d, _)| d.len()).unwrap_or(0);

// fix
let first_len = nonempty.first().map(|(_, d)| d.len()).unwrap_or(0);

If the first selected Y column has all-null or non-numeric data its series will be empty, making first_len = 0. Categorical X labels then won't render even for other valid series in the same plot. nonempty is computed a few lines later; first_len should use it instead.

Fix this →

2. Y-axis title regresses to "Value" for single-Y plots — src/ui.rs:1278

// current — always generic
.title("Value")

// fix — restore column name for single-Y, generic for multi
.title(if app.plot.y_cols.len() == 1 {
    app.headers[app.plot.y_cols[0]].as_str()
} else {
    "Value"
})

Previously single-series plots showed the column name on the Y axis. That's now gone even when only one column is selected. The legend only appears for multi-Y, so single-Y users lose the axis label with no replacement.

Fix this →

Minor / Nits

3. Duplicate cycle_hint logic — src/ui.rs:477 and src/ui.rs:1176

The cycle_hint string is computed identically in get_bar() and directly inside render_plot(). In Plot mode ui() returns early before get_bar() is called, so the Mode::Plot arm in get_bar() is actually unreachable dead code — render_plot() owns the status bar. The duplication won't cause a runtime bug today, but the two copies can drift. Worth either removing the dead Mode::Plot arm from get_bar() and shortcut_bar(), or adding a comment noting that Plot mode renders its own bar.

4. No unit tests for PlotPickY toggle — src/app_tests.rs

The Space-toggle logic in events.rs (add/remove from y_cols) is a new non-trivial invariant — e.g. toggling the same column twice should return to the original state. There are no tests for this. Given the existing test pattern in app_tests.rs, a small test in the plot_tests module would guard against future regressions.

5. CLAUDE.md PlotState table is stale

The architecture table in CLAUDE.md still shows y_col and doesn't list PlotPickY in the Mode variants section. Not blocking, but keeping the docs in sync with the code is worth a one-line update.

What looks good

• State machine transitions are clean: Normal → PlotPickY → PlotPickX → Plot, with Esc walking back correctly at each step
• Pre-selecting the current column on p is ergonomic; the status bar shows the live selection list clearly
• series_color() maps series indices to the Catppuccin Mocha palette consistently with the rest of the codebase
• Legend bounds-checking in render_plot_legend() prevents overflow on small terminals
• Series color indices are stable over the nonempty filter (empty series don't shift the color of later ones)
• Histogram correctly disabled for multi-Y (both in event handling and in the status bar hint)
• QA script coverage (J1–J4) is thorough and well-labelled
• Pre-commit hook sourcing $HOME/.cargo/env is the right fix for CI environments where cargo isn't in PATH

@claude address this issues

[claude]

Claude finished @SpollaL's task in 5m 14s —— View job

---

Addressing the review issues.

Tasks

• Gather context and read relevant files
• Fix Bug 1: first_len uses all_series instead of nonempty (src/ui.rs)
• Fix Bug 2: Y-axis title always shows "Value" for single-Y plots (src/ui.rs)
• Fix Minor 3: Remove dead Mode::Plot arm from get_bar() / shortcut_bar()
• Fix Minor 4: Add unit tests for PlotPickY Space-toggle logic (src/app_tests.rs)
• Fix Minor 5: Update CLAUDE.md PlotState table and Mode variants
• Run cargo test and cargo clippy
• Commit and push

[SpollaL]

Claude finished @SpollaL's task in 5m 14s —— View job

Addressing the review issues.

Tasks

• Gather context and read relevant files
• Fix Bug 1: first_len uses all_series instead of nonempty (src/ui.rs)
• Fix Bug 2: Y-axis title always shows "Value" for single-Y plots (src/ui.rs)
• Fix Minor 3: Remove dead Mode::Plot arm from get_bar() / shortcut_bar()
• Fix Minor 4: Add unit tests for PlotPickY Space-toggle logic (src/app_tests.rs)
• Fix Minor 5: Update CLAUDE.md PlotState table and Mode variants
• Run cargo test and cargo clippy
• Commit and push

@claude have you committed this changes?

[claude]

Claude finished @SpollaL's task in 3m 57s —— View job

---

All 5 review issues have been addressed and pushed in commit aa61c2c.
Here's a summary of what was done:

Tasks

• Check git status — no changes had been committed by the previous run
• Bug 1: first_len now uses nonempty (moved before first_len) — categorical X labels render correctly even when the first Y column has no numeric data (src/ui.rs)
• Bug 2: Y-axis title restored to column name for single-Y plots; only falls back to "Value" when multiple Y columns are selected (src/ui.rs)
• Minor 3: Removed dead Mode::Plot arms from get_bar() and shortcut_bar(), replaced with unreachable!() to document the invariant that ui() returns early before these functions are called in Plot mode (src/ui.rs)
• Minor 4: Added 4 unit tests for PlotPickY Space-toggle logic to the plot_tests module: add column, remove column, double-toggle restores state, multi-column independence (src/app_tests.rs)
• Minor 5: Updated CLAUDE.md — PlotState table now shows y_cols (not y_col), and PlotPickY added to the Mode variants list (CLAUDE.md)
• Commit and push — aa61c2c on feat/multi-y-plot

Note: cargo test and cargo clippy require approval in this environment — CI will validate on the pushed commit.
|