From 32783080f1f7360191269b35a817f2ba11c266d2 Mon Sep 17 00:00:00 2001 From: Taylor Eernisse Date: Fri, 6 Feb 2026 09:35:02 -0500 Subject: [PATCH] fix(timeline): report true total_events in robot JSON meta The robot JSON envelope's meta.total_events field was incorrectly reporting events.len() (the post-limit count), making it identical to meta.showing. This defeated the purpose of having both fields. Changes across the pipeline to fix this: - collect_events now returns (Vec, usize) where the second element is the total event count before truncation - TimelineResult gains a total_events_before_limit field (serde-skipped) so the value flows cleanly from collect through to the renderer - main.rs passes the real total instead of the events.len() workaround Additional cleanup in this pass: - Derive PartialEq/Eq/PartialOrd/Ord on TimelineEventType, replacing the hand-rolled event_type_discriminant() function. Variant declaration order now defines sort tiebreak, documented in a doc comment. - Validate --since input with a proper LoreError::Other instead of silently treating invalid values as None - Fix ANSI-aware tag column padding with console::pad_str (colored tags like "[merged]" were misaligned because ANSI escapes consumed width) - Remove dead print_timeline_json and infer_max_depth functions that were superseded by print_timeline_json_with_meta Co-Authored-By: Claude Opus 4.6 --- src/cli/commands/timeline.rs | 49 +++++++++++--------------------- src/core/timeline.rs | 31 ++++++-------------- src/core/timeline_collect.rs | 29 +++++++++++-------- src/main.rs | 5 +--- tests/timeline_pipeline_tests.rs | 6 ++-- 5 files changed, 47 insertions(+), 73 deletions(-) diff --git a/src/cli/commands/timeline.rs b/src/cli/commands/timeline.rs index 4ad6e3f..478a87b 100644 --- a/src/cli/commands/timeline.rs +++ b/src/cli/commands/timeline.rs @@ -1,9 +1,9 @@ -use console::style; +use console::{Alignment, pad_str, style}; use serde::Serialize; use crate::Config; use crate::core::db::create_connection; -use crate::core::error::Result; +use crate::core::error::{LoreError, Result}; use crate::core::paths::get_db_path; use crate::core::project::resolve_project; use crate::core::time::{ms_to_iso, parse_since}; @@ -38,7 +38,17 @@ pub fn run_timeline(config: &Config, params: &TimelineParams) -> Result Result Result Vec { // ─── Robot JSON output ─────────────────────────────────────────────────────── /// Render timeline as robot-mode JSON in {ok, data, meta} envelope. -pub fn print_timeline_json(result: &TimelineResult, total_events_before_limit: usize) { - let output = TimelineJsonEnvelope { - ok: true, - data: TimelineDataJson::from_result(result), - meta: TimelineMetaJson { - search_mode: "lexical".to_owned(), - expansion_depth: infer_max_depth(&result.expanded_entities), - expand_mentions: false, // caller should pass this, but we infer from data - total_entities: result.seed_entities.len() + result.expanded_entities.len(), - total_events: total_events_before_limit, - evidence_notes_included: count_evidence_notes(&result.events), - unresolved_references: result.unresolved_references.len(), - showing: result.events.len(), - }, - }; - - match serde_json::to_string(&output) { - Ok(json) => println!("{json}"), - Err(e) => eprintln!("Error serializing timeline JSON: {e}"), - } -} - -/// Extended version that accepts explicit meta values from the caller. pub fn print_timeline_json_with_meta( result: &TimelineResult, total_events_before_limit: usize, @@ -482,10 +471,6 @@ struct TimelineMetaJson { showing: usize, } -fn infer_max_depth(expanded: &[ExpandedEntityRef]) -> u32 { - expanded.iter().map(|e| e.depth).max().unwrap_or(0) -} - fn count_evidence_notes(events: &[TimelineEvent]) -> usize { events .iter() diff --git a/src/core/timeline.rs b/src/core/timeline.rs index 27b0dee..2f41310 100644 --- a/src/core/timeline.rs +++ b/src/core/timeline.rs @@ -26,7 +26,7 @@ impl PartialEq for TimelineEvent { fn eq(&self, other: &Self) -> bool { self.timestamp == other.timestamp && self.entity_id == other.entity_id - && self.event_type_discriminant() == other.event_type_discriminant() + && self.event_type == other.event_type } } @@ -43,31 +43,15 @@ impl Ord for TimelineEvent { self.timestamp .cmp(&other.timestamp) .then_with(|| self.entity_id.cmp(&other.entity_id)) - .then_with(|| { - self.event_type_discriminant() - .cmp(&other.event_type_discriminant()) - }) - } -} - -impl TimelineEvent { - fn event_type_discriminant(&self) -> u8 { - match &self.event_type { - TimelineEventType::Created => 0, - TimelineEventType::StateChanged { .. } => 1, - TimelineEventType::LabelAdded { .. } => 2, - TimelineEventType::LabelRemoved { .. } => 3, - TimelineEventType::MilestoneSet { .. } => 4, - TimelineEventType::MilestoneRemoved { .. } => 5, - TimelineEventType::Merged => 6, - TimelineEventType::NoteEvidence { .. } => 7, - TimelineEventType::CrossReferenced { .. } => 8, - } + .then_with(|| self.event_type.cmp(&other.event_type)) } } /// Per spec Section 3.3. Serde tagged enum for JSON output. -#[derive(Debug, Clone, Serialize)] +/// +/// Variant declaration order defines the sort order within a timestamp+entity +/// tiebreak (Created < StateChanged < LabelAdded < ... < CrossReferenced). +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize)] #[serde(tag = "kind", rename_all = "snake_case")] pub enum TimelineEventType { Created, @@ -133,6 +117,9 @@ pub struct UnresolvedRef { pub struct TimelineResult { pub query: String, pub events: Vec, + /// Total events before the `--limit` was applied (for meta.total_events vs meta.showing). + #[serde(skip)] + pub total_events_before_limit: usize, pub seed_entities: Vec, pub expanded_entities: Vec, pub unresolved_references: Vec, diff --git a/src/core/timeline_collect.rs b/src/core/timeline_collect.rs index 86ad386..62da76f 100644 --- a/src/core/timeline_collect.rs +++ b/src/core/timeline_collect.rs @@ -17,7 +17,7 @@ pub fn collect_events( evidence_notes: &[TimelineEvent], since_ms: Option, limit: usize, -) -> Result> { +) -> Result<(Vec, usize)> { let mut all_events: Vec = Vec::new(); // Collect events for seed entities @@ -41,10 +41,13 @@ pub fn collect_events( all_events.retain(|e| e.timestamp >= since); } + // Capture total before applying limit (for meta.total_events vs meta.showing) + let total_before_limit = all_events.len(); + // Apply limit all_events.truncate(limit); - Ok(all_events) + Ok((all_events, total_before_limit)) } /// Collect all events for a single entity. @@ -480,7 +483,7 @@ mod tests { let issue_id = insert_issue(&conn, project_id, 1); let seeds = vec![make_entity_ref("issue", issue_id, 1)]; - let events = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); + let (events, _) = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); assert_eq!(events.len(), 1); assert!(matches!(events[0].event_type, TimelineEventType::Created)); assert_eq!(events[0].timestamp, 1000); @@ -498,7 +501,7 @@ mod tests { insert_state_event(&conn, project_id, Some(issue_id), None, "reopened", 4000); let seeds = vec![make_entity_ref("issue", issue_id, 1)]; - let events = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); + let (events, _) = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); // Created + 2 state changes = 3 assert_eq!(events.len(), 3); @@ -523,7 +526,7 @@ mod tests { insert_state_event(&conn, project_id, None, Some(mr_id), "merged", 5000); let seeds = vec![make_entity_ref("merge_request", mr_id, 10)]; - let events = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); + let (events, _) = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); // Should have Created + Merged (not Created + StateChanged{merged} + Merged) let merged_count = events @@ -548,7 +551,7 @@ mod tests { insert_label_event(&conn, project_id, Some(issue_id), None, "add", None, 2000); let seeds = vec![make_entity_ref("issue", issue_id, 1)]; - let events = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); + let (events, _) = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); let label_event = events.iter().find(|e| { matches!(&e.event_type, TimelineEventType::LabelAdded { label } if label == "[deleted label]") @@ -565,7 +568,7 @@ mod tests { insert_milestone_event(&conn, project_id, Some(issue_id), None, "add", None, 2000); let seeds = vec![make_entity_ref("issue", issue_id, 1)]; - let events = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); + let (events, _) = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); let ms_event = events.iter().find(|e| { matches!(&e.event_type, TimelineEventType::MilestoneSet { milestone } if milestone == "[deleted milestone]") @@ -585,7 +588,7 @@ mod tests { let seeds = vec![make_entity_ref("issue", issue_id, 1)]; // Since 4000: should exclude Created (1000) and closed (3000) - let events = collect_events(&conn, &seeds, &[], &[], Some(4000), 100).unwrap(); + let (events, _) = collect_events(&conn, &seeds, &[], &[], Some(4000), 100).unwrap(); assert_eq!(events.len(), 1); assert_eq!(events[0].timestamp, 5000); } @@ -612,7 +615,7 @@ mod tests { make_entity_ref("issue", issue_id, 1), make_entity_ref("merge_request", mr_id, 10), ]; - let events = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); + let (events, _) = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); // Verify chronological order for window in events.windows(2) { @@ -638,8 +641,10 @@ mod tests { } let seeds = vec![make_entity_ref("issue", issue_id, 1)]; - let events = collect_events(&conn, &seeds, &[], &[], None, 5).unwrap(); + let (events, total) = collect_events(&conn, &seeds, &[], &[], None, 5).unwrap(); assert_eq!(events.len(), 5); + // 20 state changes + 1 created = 21 total before limit + assert_eq!(total, 21); } #[test] @@ -666,7 +671,7 @@ mod tests { }]; let seeds = vec![make_entity_ref("issue", issue_id, 1)]; - let events = collect_events(&conn, &seeds, &[], &evidence, None, 100).unwrap(); + let (events, _) = collect_events(&conn, &seeds, &[], &evidence, None, 100).unwrap(); let note_event = events.iter().find(|e| { matches!( @@ -688,7 +693,7 @@ mod tests { insert_state_event(&conn, project_id, None, Some(mr_id), "merged", 5000); let seeds = vec![make_entity_ref("merge_request", mr_id, 10)]; - let events = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); + let (events, _) = collect_events(&conn, &seeds, &[], &[], None, 100).unwrap(); let merged = events .iter() diff --git a/src/main.rs b/src/main.rs index cb89872..bda60f4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1416,12 +1416,9 @@ fn handle_timeline( let result = run_timeline(&config, ¶ms)?; if robot_mode { - // total_events_before_limit: the result already has events truncated, - // but we can compute it from the pipeline if needed. For now, use events.len() - // since collect_events already applied the limit internally. print_timeline_json_with_meta( &result, - result.events.len(), + result.total_events_before_limit, params.depth, params.expand_mentions, ); diff --git a/tests/timeline_pipeline_tests.rs b/tests/timeline_pipeline_tests.rs index a2317ce..5181b39 100644 --- a/tests/timeline_pipeline_tests.rs +++ b/tests/timeline_pipeline_tests.rs @@ -170,7 +170,7 @@ fn pipeline_seed_expand_collect_end_to_end() { assert!(total_entities >= 2, "Should have at least issue + MR"); // COLLECT: gather all events - let events = collect_events( + let (events, _) = collect_events( &conn, &seed_result.seed_entities, &expand_result.expanded_entities, @@ -224,7 +224,7 @@ fn pipeline_empty_query_produces_empty_result() { let expand_result = expand_timeline(&conn, &seed_result.seed_entities, 1, false, 100).unwrap(); assert!(expand_result.expanded_entities.is_empty()); - let events = collect_events( + let (events, _) = collect_events( &conn, &seed_result.seed_entities, &expand_result.expanded_entities, @@ -259,7 +259,7 @@ fn pipeline_since_filter_excludes_old_events() { let expand_result = expand_timeline(&conn, &seed_result.seed_entities, 0, false, 100).unwrap(); // Collect with since=5000: should exclude Created(1000) and closed(2000) - let events = collect_events( + let (events, _) = collect_events( &conn, &seed_result.seed_entities, &expand_result.expanded_entities,