test(viewer): timeline proptest surface (WBS-6.2 #433) - #433
Conversation
Adds `crates/sl-viewer/tests/properties_viewer_timeline.rs` with
16 proptest properties pinning the timeline pure-helper reductions:
* `group_by_day` partitions every entry into exactly one group (no
losses), orders groups chronologically, and labels empty-day groups
with the literal `"(unknown date)"` string.
* `normalize_widths` produces one width per input entry, all in
`[MIN_PX, MAX_PX]`; empty or all-zero inputs collapse to MIN_PX;
the entry with the max `token_count` always renders at MAX_PX.
* `model_hue` is deterministic and lands in `[0, 359]`.
* `model_color` is deterministic and matches `hsl(<hue>, 60%, 55%)`.
* `TimelineEntry::from_bundle`:
* `day` is the leading 10 chars of `created_at` (else empty).
* `goal` falls back to `"(no goal)"` when no Intent slice
carries a string `goal` body.
* `model` falls back to `"unknown"` when no Context slice
carries a string `model` body.
* `source_id` carries through unchanged.
* `message_count` equals slice count; `has_acceptance` /
`has_contract` reflect kind presence (any-of).
* `token_count` falls back to 0 when no Intent slice carries a
numeric `user_turn_count`.
Updates WBS-6.2 evidence list, TRACEABILITY.json, and CHANGELOG.
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughSummaryThe PR adds 16 property-based tests for It updates WBS-6.2 evidence, Must FixNone identified. Should FixNone identified. ConsiderConsider running the full workspace test suite before merge. Approve / Request ChangesApprove. The PR adds focused test coverage without changing public APIs. WalkthroughThis change adds 16 property-based tests for ChangesTimeline property-test coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/sl-viewer/tests/properties_viewer_timeline.rs`:
- Around line 129-139: Update the property test group_by_day_partitions_entries
to compare the flattened grouped entries against the original entries by full
entry identity, not just length. Sort or otherwise count equivalent
TimelineEntry keys on both sides, then assert the multisets match so dropped or
duplicated entries are detected.
- Around line 246-250: Constrain the light_tokens generator in
normalize_widths_max_token_renders_max_px so it never exceeds heavy_tokens,
while retaining the existing upper bound and allowing zero where intended. Keep
the rest of the property setup unchanged.
In `@docs/ops/WBS.md`:
- Line 32: Update the WBS-6.2 references so properties_viewer_timeline.rs is
attributed to Wave-32 PR `#433`, matching CHANGELOG.md and the PR objective. Keep
the existing `#432` attribution only for the related prior work and remove
timeline properties from that reference.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 724e0389-e7b6-44f3-9232-053d90790331
📒 Files selected for processing (4)
CHANGELOG.mdcrates/sl-viewer/tests/properties_viewer_timeline.rsdocs/ops/TRACEABILITY.jsondocs/ops/WBS.md
📜 Review details
⏰ Context from checks skipped due to timeout. (20)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: race smoke + channel/cancel model · windows-latest
- GitHub Check: cargo deny check
- GitHub Check: Dependency Review
- GitHub Check: browser e2e · axe · responsive · visual
- GitHub Check: rootless-only matrix · SelfCheck
- GitHub Check: signing hard · SelfCheck
- GitHub Check: sl-viewer help · unit tests
- GitHub Check: alloc profile hard · dhat smoke
- GitHub Check: exotic check · x86_64-unknown-linux-musl
- GitHub Check: session-ledger build · windows-latest
- GitHub Check: sl-daemon build · macos-latest
- GitHub Check: jemalloc default-on · windows default build
- GitHub Check: sl-daemon build · windows-latest
- GitHub Check: sl-viewer macOS app · artifact
- GitHub Check: jemalloc default-on · unix default build
- GitHub Check: tsan permutation · race_model
- GitHub Check: Summary
- GitHub Check: browser e2e · axe · responsive · visual
- GitHub Check: prepare
⚠️ CI failures not shown inline (2)
GitHub Actions: Trunk Check / Lint & Format: test(viewer): timeline proptest surface (WBS-6.2 #433)
Conclusion: failure
##[group]Run cat >>$GITHUB_ENV <<EOF
�[36;1mcat >>$GITHUB_ENV <<EOF�[0m
�[36;1mGITHUB_***REDACTED_SECRET_ASSIGNMENT***
�[36;1mTRUNK_LAUNCHER_QUIET=false�[0m
�[36;1mEOF�[0m
�[36;1m�[0m
�[36;1m# First arg is field to fetch, second arg is default value or empty�[0m
�[36;1mpayload() {�[0m
�[36;1m if [ $# -lt 2 ]; then�[0m
�[36;1m DEFAULT_VALUE=empty�[0m
�[36;1m else�[0m
�[36;1m DEFAULT_VALUE=\"$2\"�[0m
�[36;1m fi�[0m
�[36;1m if command -v jq >/dev/null; then�[0m
�[36;1m jq -r ".inputs.payload | fromjson | .$1 // ${DEFAULT_VALUE}" ${TEST_GITHUB_EVENT_PATH:-${GITHUB_EVENT_PATH}}�[0m
�[36;1m else�[0m
�[36;1m echo "::error::jq not installed on system!"�[0m
GitHub Actions: Trunk Check / 0_Lint & Format.txt: test(viewer): timeline proptest surface (WBS-6.2 #433)
Conclusion: failure
##[group]Run cat >>$GITHUB_ENV <<EOF
�[36;1mcat >>$GITHUB_ENV <<EOF�[0m
�[36;1mGITHUB_***REDACTED_SECRET_ASSIGNMENT***
�[36;1mTRUNK_LAUNCHER_QUIET=false�[0m
�[36;1mEOF�[0m
�[36;1m�[0m
�[36;1m# First arg is field to fetch, second arg is default value or empty�[0m
�[36;1mpayload() {�[0m
�[36;1m if [ $# -lt 2 ]; then�[0m
�[36;1m DEFAULT_VALUE=empty�[0m
�[36;1m else�[0m
�[36;1m DEFAULT_VALUE=\"$2\"�[0m
�[36;1m fi�[0m
�[36;1m if command -v jq >/dev/null; then�[0m
�[36;1m jq -r ".inputs.payload | fromjson | .$1 // ${DEFAULT_VALUE}" ${TEST_GITHUB_EVENT_PATH:-${GITHUB_EVENT_PATH}}�[0m
�[36;1m else�[0m
�[36;1m echo "::error::jq not installed on system!"�[0m
🧰 Additional context used
📓 Path-based instructions (5)
*
📄 CodeRabbit inference engine (AGENTS.md)
*: Perform feature work in a git worktree under.claude/worktrees/, created fromorigin/mainon a branch named<type>/<topic>, rather than working directly onmain.
Do not make direct commits to protectedmain; use a pull request.
Do not usegit reset --hard,git stash, orgit cleanin worktrees.
Do not use--no-verifyor bypass hooks without operator approval.
Do not work on a branch or worktree another actor is using.
Files:
CHANGELOG.md
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Use the Rust toolchain pinned inrust-toolchain.toml; the workspace MSRV is Rust 1.85.
Validate Rust workspace changes with the prescribed locked build, all-features test suite, Clippy, and rustfmt checks where applicable.
Files:
crates/sl-viewer/tests/properties_viewer_timeline.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Fix Clippy warnings; do not add
#[allow]unless it includes a tracking-issue comment.
Files:
crates/sl-viewer/tests/properties_viewer_timeline.rs
crates/sl-viewer/**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
crates/sl-viewer/**/*.{rs,toml}: Thesl-viewercrate uses Dioxus 0.6; use the Dioxus CLI/toolchain for desktop development and bundling.
Usecargo check -p sl-vieweras the fast inner-loop check for viewer changes.
Files:
crates/sl-viewer/tests/properties_viewer_timeline.rs
crates/sl-viewer/**/*
📄 CodeRabbit inference engine (AGENTS.md)
When packaging the macOS viewer, account for the documented Electrobun/Dioxus code-signing requirements.
Files:
crates/sl-viewer/tests/properties_viewer_timeline.rs
🪛 LanguageTool
docs/ops/WBS.md
[uncategorized] ~32-~32: The official name of this software platform is spelled with a capital “H”.
Context: ...; fuzz/fuzz_targets/jsonl_ingest.rs; .github/workflows/ci.yml; .github/workflows/b...
(GITHUB)
[uncategorized] ~32-~32: The official name of this software platform is spelled with a capital “H”.
Context: ...ingest.rs; .github/workflows/ci.yml; .github/workflows/bench-gate.yml; docs/ops/pe...
(GITHUB)
🔇 Additional comments (1)
crates/sl-viewer/tests/properties_viewer_timeline.rs (1)
22-26: 📐 Maintainability & Code QualityVerify the required Rust workspace validation.
Run the pinned toolchain with the locked build, all-features test suite, Clippy, and rustfmt checks. Also run
cargo check -p sl-viewer --lockedfor this viewer change.As per coding guidelines, “Validate Rust workspace changes with the prescribed locked build, all-features test suite, Clippy, and rustfmt checks where applicable.”
Source: Coding guidelines
| fn group_by_day_partitions_entries( | ||
| entries in prop::collection::vec(entry_strategy(), 0..12), | ||
| ) { | ||
| let groups = group_by_day(&entries); | ||
| let flat: Vec<TimelineEntry> = | ||
| groups.iter().flat_map(|(_, v)| v.iter()).cloned().collect(); | ||
| prop_assert_eq!( | ||
| flat.len(), | ||
| entries.len(), | ||
| "every input entry must appear in exactly one group", | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Compare the flattened entries, not only their count.
Equal lengths do not prove partitioning. A defective implementation can drop one entry and duplicate another entry while preserving flat.len(). Compare sorted full entry keys, or per-entry multiplicities, with the input collection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sl-viewer/tests/properties_viewer_timeline.rs` around lines 129 - 139,
Update the property test group_by_day_partitions_entries to compare the
flattened grouped entries against the original entries by full entry identity,
not just length. Sort or otherwise count equivalent TimelineEntry keys on both
sides, then assert the multisets match so dropped or duplicated entries are
detected.
| fn normalize_widths_max_token_renders_max_px( | ||
| // First entry: heavy. Rest: light. | ||
| heavy_tokens in 1u64..1_000_000, | ||
| light_tokens in 0u64..1000, | ||
| rest in 0usize..6, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Constrain light_tokens to the heavy-entry range.
light_tokens can exceed heavy_tokens. For heavy_tokens = 1, light_tokens = 2, and rest = 1, Line 278 is less than MAX_PX, so this property fails.
Proposed fix
- light_tokens in 0u64..1000,
+ light_tokens in 0u64..heavy_tokens,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sl-viewer/tests/properties_viewer_timeline.rs` around lines 246 - 250,
Constrain the light_tokens generator in
normalize_widths_max_token_renders_max_px so it never exceeds heavy_tokens,
while retaining the existing upper bound and allowing zero where intended. Keep
the rest of the property setup unchanged.
| | WBS-5.1 | P5 deterministic dedup merge and crash/lost-work recovery E2E | done | machine | `src/domain/merge.rs`; `src/domain/worklog.rs`; `tests/merge_recovery.rs` | FR-011; T-024, T-035; C03 | | ||
| | WBS-6.1 | P6 85% coverage gate and deterministic golden corpus | done | machine | `.github/workflows/ci.yml`; `tests/okf_golden.rs`; `tests/fixtures/okf/` | T-037, T-038; C01, C08 | | ||
| | WBS-6.2 | P6 property tests, fuzzing, race checks, and enforced performance budgets | partial | machine | `tests/properties.rs`; `crates/sl-viewer/tests/properties_viewer.rs`; `crates/sl-viewer/tests/properties_viewer_theme_url.rs`; `crates/sl-viewer/tests/properties_viewer_unfinished_tab.rs`; `fuzz/fuzz_targets/okf_roundtrip.rs`; `fuzz/fuzz_targets/jsonl_ingest.rs`; `.github/workflows/ci.yml`; `.github/workflows/bench-gate.yml`; `docs/ops/perf-baseline.json`; `scripts/bench-gate.ps1`; `benches/pipeline.rs`; `tests/loom_model.rs` | DESIGN P6 residual; C00 L6-L8; C07 L66-L68; C08 L74; perf-budget enforced Wave-26 #223; p95 latency enforced Wave-30 #256; FSM properties Wave-31 #261; soft loom Wave-31 #264; viewer corpus_paths/parquet/settings properties #425; viewer theme + daemon_url properties #427; viewer unfinished_tab properties + fuzz/rootless CI drift fixes #428; full loom/shuttle unpaid | | ||
| | WBS-6.2 | P6 property tests, fuzzing, race checks, and enforced performance budgets | partial | machine | `tests/properties.rs`; `crates/sl-viewer/tests/properties_viewer.rs`; `crates/sl-viewer/tests/properties_viewer_theme_url.rs`; `crates/sl-viewer/tests/properties_viewer_unfinished_tab.rs`; `crates/sl-viewer/tests/properties_viewer_timeline.rs`; `fuzz/fuzz_targets/okf_roundtrip.rs`; `fuzz/fuzz_targets/jsonl_ingest.rs`; `.github/workflows/ci.yml`; `.github/workflows/bench-gate.yml`; `docs/ops/perf-baseline.json`; `scripts/bench-gate.ps1`; `benches/pipeline.rs`; `tests/loom_model.rs` | DESIGN P6 residual; C00 L6-L8; C07 L66-L68; C08 L74; perf-budget enforced Wave-26 #223; p95 latency enforced Wave-30 #256; FSM properties Wave-31 #261; soft loom Wave-31 #264; viewer corpus_paths/parquet/settings properties #425; viewer theme + daemon_url properties #427; viewer unfinished_tab properties + fuzz/rootless CI drift fixes #428; viewer bundle_diff + timeline properties + web_exports/hmetic-pin cleanups #432; full loom/shuttle unpaid | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the timeline-property change reference.
CHANGELOG.md Line 21 and the PR objective assign the timeline properties to #433. This row assigns timeline properties to #432. Associate properties_viewer_timeline.rs with #433, and retain #432 only for its related prior work.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~32-~32: The official name of this software platform is spelled with a capital “H”.
Context: ...; fuzz/fuzz_targets/jsonl_ingest.rs; .github/workflows/ci.yml; .github/workflows/b...
(GITHUB)
[uncategorized] ~32-~32: The official name of this software platform is spelled with a capital “H”.
Context: ...ingest.rs; .github/workflows/ci.yml; .github/workflows/bench-gate.yml; docs/ops/pe...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/ops/WBS.md` at line 32, Update the WBS-6.2 references so
properties_viewer_timeline.rs is attributed to Wave-32 PR `#433`, matching
CHANGELOG.md and the PR objective. Keep the existing `#432` attribution only for
the related prior work and remove timeline properties from that reference.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| /// `created_at.len() >= 10`, else empty. | ||
| #[test] | ||
| fn from_bundle_day_is_leading_10_chars_or_empty( | ||
| prefix in "[0-9TZ:.+-]{10,24}", |
There was a problem hiding this comment.
WARNING: prefix generator "[0-9TZ:.+-]{10,24}" always produces strings of length ≥ 10, so the else { String::new() } branch in the expected value (line 329) is dead code. The test never exercises the short-string fallback where day should be empty. Consider using "[0-9TZ:.+-]{0,24}" and adjusting the assertion to cover both branches.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| let expected = if intent_has_numeric { | ||
| cb.bundles | ||
| .iter() | ||
| .find(|b| b.kind == BundleKind::Intent) |
There was a problem hiding this comment.
WARNING: expected is computed using the same .find(|b| b.kind == BundleKind::Intent) algorithm as the implementation (timeline.rs:40). This makes the success-path check a tautology — a bug in bundle selection (e.g., picking the wrong Intent bundle among multiples) would not be caught. Define expected from the specification rather than mirroring the implementation.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| }); | ||
| } | ||
| let groups = group_by_day(&entries); | ||
| let unknown = groups.iter().find(|(d, _)| d == "(unknown date)").expect("unknown group exists"); |
There was a problem hiding this comment.
SUGGESTION: Using expect in a proptest property converts a soft assertion failure into a panic, losing the minimal counterexample. Prefer prop_assert! for property-test invariants to get shrinking on failure.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 86.4K · Output: 15.2K · Cached: 402.9K |
User description
Summary
Adds
crates/sl-viewer/tests/properties_viewer_timeline.rswith 16 proptest properties pinning the timeline pure-helper reductions (WBS-6.2 #433).group_by_day(3 properties)day)"(unknown date)"normalize_widths(4 properties)[MIN_PX, MAX_PX](24–240 px)MIN_PXtoken_countentry renders atMAX_PXmodel_hue/model_color(3 properties)model_huelands in[0, 359]model_colormatcheshsl(<hue>, 60%, 55%)exactlyTimelineEntry::from_bundle(6 properties)dayis the leading 10 chars ofcreated_at(else empty)goalfalls back to"(no goal)"when no Intent slice carries a stringgoalmodelfalls back to"unknown"when no Context slice carries a stringmodelsource_idcarries through unchangedmessage_countequals slice count;has_acceptance/has_contractreflect kind presencetoken_countfalls back to 0 when no Intent slice carries a numericuser_turn_countValidation
cargo test -p sl-viewer --test properties_viewer_timeline --features "desktop parquet" --locked— 16 passed (×8 proptest cases each)cargo clippy -p sl-viewer --test properties_viewer_timeline --features "desktop parquet" --locked -- -D warnings— cleancargo fmt --all --check— cleanWBS / TRACEABILITY
WBS-6.2 evidence list and
TRACEABILITY.jsongaincrates/sl-viewer/tests/properties_viewer_timeline.rs. Status stayspartial(fuzzing cadence, full loom/shuttle, perf-budget gates remain). CHANGELOG Unreleased documents the new surface.CodeAnt-AI Description
Add property coverage for the viewer timeline
What Changed
"(unknown date)", and widths stay within the supported rangeImpact
✅ Fewer timeline grouping regressions✅ Consistent timeline bar sizing✅ Clearer fallback behavior for incomplete timeline data💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.