Backlog items surfaced during the second PR review of #162 (S03 Chunk Detail View and Spec Browser). The critical and structural issues were fixed inline; these are doc, comment, and minor code quality improvements.
Items
detail_spec_note field doc (#2)
The doc says "Diagnostic reason when detail_spec is None" but the field is also written on history load failure when detail_spec is Some. Update to reflect both uses, e.g.:
General-purpose diagnostic message for the chunk detail view. Set when either spec loading or history loading fails. Rendered as the block title warning when spec is present; rendered as the fallback paragraph when spec is absent.
join_results doc missing run = None case (#3)
The most common real-world case (run is None because no gate history exists yet) is not documented. Add as the first bullet:
run is None (no gate history) → all criteria return None (Pending)
ChunkDetail Esc arm lacks preservation comment (#4)
detail_milestone and detail_list_state are intentionally not cleared on Esc (D099 — cursor position preserved). Without a comment a future maintainer could break this. Add:
// Intentionally do NOT clear detail_milestone or detail_list_state:
// returning preserves the user's chunk cursor position (D099).
draw_chunk_detail doc only describes happy path (#5)
Current: "Render the chunk detail screen with a table of criteria and their results."
The function has two modes. Update to describe both the table path (spec=Some) and the paragraph fallback (spec=None).
.last().unwrap() safety annotation (#6)
Ok(ids) if !ids.is_empty() => {
let run_id = ids.last().unwrap().clone();
Use .expect("non-empty: guarded by !ids.is_empty() above") to make the invariant explicit at the call site.
up_down_in_milestone_detail weak assertion (#7)
assert!(after_down > initial_selection) passes for any forward move. Pin to concrete indices:
assert_eq!(app.detail_list_state.selected(), Some(1), "Down from 0 must select 1");
assert_eq!(app.detail_list_state.selected(), Some(0), "Up must wrap back to 0");
Validation comment in setup_project incomplete (#8)
// Validation requires at least one criterion with a `cmd` or `path` field.
Missing third accepted form. Fix:
// Validation requires at least one criterion with a `cmd` field, `path` field,
// or `kind = "AgentReport"` — at least one criterion must be executable.
Error vs. info styling in draw_chunk_detail (suggestion)
"Legacy flat spec…" (informational) and "Failed to load spec…" (error) both render .dim(). Errors should use .fg(Color::Red); info should stay dim. Could detect by prefix or add a separate detail_spec_error: Option<String> field.
detail_run unnecessary clone (suggestion)
let run_id = ids.last().unwrap().clone();
match history::load(&assay_dir, &chunk_slug, &run_id) {
history::load accepts &str; ids.last().unwrap() (a &String) already coerces. Drop the .clone() and the binding.
Discovered during second parallel review of PR #162. All critical and structural issues already addressed; these are low-risk doc/comment/minor code cleanups safe to batch into S05 or a dedicated polish pass.
Backlog items surfaced during the second PR review of #162 (S03 Chunk Detail View and Spec Browser). The critical and structural issues were fixed inline; these are doc, comment, and minor code quality improvements.
Items
detail_spec_notefield doc (#2)The doc says "Diagnostic reason when
detail_specisNone" but the field is also written on history load failure whendetail_specisSome. Update to reflect both uses, e.g.:join_resultsdoc missingrun = Nonecase (#3)The most common real-world case (
runisNonebecause no gate history exists yet) is not documented. Add as the first bullet:ChunkDetail
Escarm lacks preservation comment (#4)detail_milestoneanddetail_list_stateare intentionally not cleared on Esc (D099 — cursor position preserved). Without a comment a future maintainer could break this. Add:draw_chunk_detaildoc only describes happy path (#5)Current: "Render the chunk detail screen with a table of criteria and their results."
The function has two modes. Update to describe both the table path (spec=Some) and the paragraph fallback (spec=None).
.last().unwrap()safety annotation (#6)Use
.expect("non-empty: guarded by !ids.is_empty() above")to make the invariant explicit at the call site.up_down_in_milestone_detailweak assertion (#7)assert!(after_down > initial_selection)passes for any forward move. Pin to concrete indices:Validation comment in
setup_projectincomplete (#8)// Validation requires at least one criterion with a `cmd` or `path` field.Missing third accepted form. Fix:
Error vs. info styling in
draw_chunk_detail(suggestion)"Legacy flat spec…"(informational) and"Failed to load spec…"(error) both render.dim(). Errors should use.fg(Color::Red); info should stay dim. Could detect by prefix or add a separatedetail_spec_error: Option<String>field.detail_rununnecessary clone (suggestion)history::loadaccepts&str;ids.last().unwrap()(a&String) already coerces. Drop the.clone()and the binding.Discovered during second parallel review of PR #162. All critical and structural issues already addressed; these are low-risk doc/comment/minor code cleanups safe to batch into S05 or a dedicated polish pass.