Refine NSys timeline visualization#7
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds density-bin aggregation and related metrics to timeline SVG rendering, updates query/response types and fingerprints, introduces CLI flags and validation, expands unit tests, and updates docs, RFCs, examples, and version/release metadata to ship the wire-version and density rollout. ChangesTimeline density rendering and contract alignment
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/veloq-vis/src/render/text.rs`:
- Around line 67-83: fit_interval_label currently ignores policy.min_label_px
and doesn't enforce policy.max_chars (aside from the zero-disable check); update
fit_interval_label to first treat policy.max_chars == 0 as before, then treat
the minimum visible width as the max of INTERVAL_LABEL_MIN_WIDTH_PX and
policy.min_label_px when comparing against raw_width, compute available_width as
before, then apply policy.max_chars by truncating label to at most
policy.max_chars characters (preserving any needed ellipsis logic if desired)
before measuring width with estimate_text_width(INTERVAL_LABEL_FONT_PX); set the
returned "truncated" flag if either the label was truncated to max_chars or its
measured width exceeds available_width, and return the possibly-truncated String
and the flag.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05b38517-4943-43c9-9994-e4e29f5c1869
⛔ Files ignored due to path filters (6)
Cargo.lockis excluded by!**/*.lockdocs/assets/social-card.pngis excluded by!**/*.pngdocs/assets/social-card.svgis excluded by!**/*.svgexamples/reports/nsys-timeline-vis/figures/hybrid-decode-overlap.svgis excluded by!**/*.svgexamples/reports/nsys-timeline-vis/figures/hybrid-prefill-pipeline.svgis excluded by!**/*.svgexamples/reports/nsys-timeline-vis/figures/tp-decode-contrast.svgis excluded by!**/*.svg
📒 Files selected for processing (34)
.agents/skills/nsys-profile-analysis/SKILL.md.agents/skills/nsys-profile-analysis/references/cookbook.md.agents/skills/nsys-profile-analysis/references/limitations.md.claude-plugin/marketplace.json.claude-plugin/plugin.json.codex-plugin/plugin.jsonAGENTS.mdCHANGELOG.mdCargo.tomlREADME.mdcrates/nsys/veloq-nsys-query/src/viz_timeline/fingerprint.rscrates/nsys/veloq-nsys-query/src/viz_timeline/mod.rscrates/nsys/veloq-nsys-query/src/viz_timeline/types.rscrates/nsys/veloq-nsys-query/tests/wire_format_smoke.rscrates/nsys/veloq-nsys/src/cli.rscrates/nsys/veloq-nsys/src/commands/mod.rscrates/nsys/veloq-nsys/src/error.rscrates/nsys/veloq-nsys/src/help.rscrates/nsys/veloq-nsys/src/views/basic.rscrates/veloq-vis/src/model.rscrates/veloq-vis/src/render/mod.rscrates/veloq-vis/src/render/painter.rscrates/veloq-vis/src/render/style.rscrates/veloq-vis/src/render/tests.rscrates/veloq-vis/src/render/text.rsdocs/index.htmldocs/rfc/RFC-0009.mdexamples/README.mdexamples/reports/nsys-timeline-vis/README.mdexamples/reports/nsys-timeline-vis/evidence/summary.jsonexamples/reports/nsys-timeline-vis/report.mdgov/rfc/RFC-0009/clauses/C-COMMAND-SURFACE.tomlgov/work/2026-06-10-add-timeline-density-rendering.tomltypos.toml
Comment on lines
67
to
83
| pub(super) fn fit_interval_label( | ||
| label: &str, | ||
| raw_width: f64, | ||
| policy: &VizLabelPolicy, | ||
| ) -> Option<(String, bool)> { | ||
| if raw_width < policy.min_label_px || policy.max_chars == 0 { | ||
| if policy.max_chars == 0 { | ||
| return None; | ||
| } | ||
| // 8px horizontal padding, ~6.4px average character width. | ||
| let width_chars = ((raw_width - 8.0) / 6.4).floor(); | ||
| if width_chars < 4.0 { | ||
| // Text is clipped to the interval bar, so the string can be longer than | ||
| // the visible span. This threshold only prevents noisy one-letter labels. | ||
| if raw_width < INTERVAL_LABEL_MIN_WIDTH_PX { | ||
| return None; | ||
| } | ||
| let max_chars = policy.max_chars.min(width_chars as usize); | ||
| Some(truncate_label(label, max_chars)) | ||
| let available_width = (raw_width - INTERVAL_LABEL_PADDING_X).max(0.0); | ||
| let truncated = estimate_text_width(label, INTERVAL_LABEL_FONT_PX) > available_width; | ||
| Some((label.to_string(), truncated)) | ||
| } |
There was a problem hiding this comment.
Honor min_label_px and max_chars in label fitting
Line 72-Line 83 currently ignores policy.min_label_px and does not apply policy.max_chars (except the zero-disable case), which makes --min-label-px/--max-label-chars effectively non-functional at the render boundary.
Suggested fix
pub(super) fn fit_interval_label(
label: &str,
raw_width: f64,
policy: &VizLabelPolicy,
) -> Option<(String, bool)> {
if policy.max_chars == 0 {
return None;
}
- // Text is clipped to the interval bar, so the string can be longer than
- // the visible span. This threshold only prevents noisy one-letter labels.
- if raw_width < INTERVAL_LABEL_MIN_WIDTH_PX {
+ let min_width = policy.min_label_px.max(INTERVAL_LABEL_MIN_WIDTH_PX);
+ if raw_width < min_width {
return None;
}
- let available_width = (raw_width - INTERVAL_LABEL_PADDING_X).max(0.0);
- let truncated = estimate_text_width(label, INTERVAL_LABEL_FONT_PX) > available_width;
- Some((label.to_string(), truncated))
+ let (fitted, truncated_by_chars) = truncate_label(label, policy.max_chars);
+ let available_width = (raw_width - INTERVAL_LABEL_PADDING_X).max(0.0);
+ let clipped_by_width = estimate_text_width(&fitted, INTERVAL_LABEL_FONT_PX) > available_width;
+ Some((fitted, truncated_by_chars || clipped_by_width))
}🤖 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/veloq-vis/src/render/text.rs` around lines 67 - 83, fit_interval_label
currently ignores policy.min_label_px and doesn't enforce policy.max_chars
(aside from the zero-disable check); update fit_interval_label to first treat
policy.max_chars == 0 as before, then treat the minimum visible width as the max
of INTERVAL_LABEL_MIN_WIDTH_PX and policy.min_label_px when comparing against
raw_width, compute available_width as before, then apply policy.max_chars by
truncating label to at most policy.max_chars characters (preserving any needed
ellipsis logic if desired) before measuring width with
estimate_text_width(INTERVAL_LABEL_FONT_PX); set the returned "truncated" flag
if either the label was truncated to max_chars or its measured width exceeds
available_width, and return the possibly-truncated String and the flag.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/veloq-vis/src/render/text.rs (1)
67-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPolicy max_chars still not enforced (partial past-review fix).
Line 74 now correctly honors
policy.min_label_px, but the function still does not enforcepolicy.max_chars. Users who setmax_charsin the label policy expect character-count truncation before width-based clipping. Applytruncate_label(label, policy.max_chars)before the width estimation at line 79, and combine both truncation signals in the returned boolean.Suggested fix
pub(super) fn fit_interval_label( label: &str, raw_width: f64, policy: &VizLabelPolicy, ) -> Option<(String, bool)> { + if policy.max_chars == 0 { + return None; + } // Text is clipped to the interval bar, so the string can be longer than // the visible span. This threshold only prevents noisy one-letter labels. let min_visible_width = INTERVAL_LABEL_MIN_WIDTH_PX.max(policy.min_label_px); if raw_width < min_visible_width { return None; } + let (fitted, truncated_by_chars) = truncate_label(label, policy.max_chars); let available_width = (raw_width - INTERVAL_LABEL_PADDING_X).max(0.0); - let truncated = estimate_text_width(label, INTERVAL_LABEL_FONT_PX) > available_width; - Some((label.to_string(), truncated)) + let clipped_by_width = estimate_text_width(&fitted, INTERVAL_LABEL_FONT_PX) > available_width; + Some((fitted, truncated_by_chars || clipped_by_width)) }🤖 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/veloq-vis/src/render/text.rs` around lines 67 - 81, The function fit_interval_label currently ignores policy.max_chars; before estimating width in fit_interval_label, apply truncate_label(label, policy.max_chars) to get a pre-truncated string, then use estimate_text_width on that truncated string (with INTERVAL_LABEL_FONT_PX) and compute the width-based truncated flag as before; finally return the truncated string and a combined boolean (true if either truncate_label shortened the input or the width check set truncated) so both character-count and pixel-based truncation are reported.
🤖 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.
Duplicate comments:
In `@crates/veloq-vis/src/render/text.rs`:
- Around line 67-81: The function fit_interval_label currently ignores
policy.max_chars; before estimating width in fit_interval_label, apply
truncate_label(label, policy.max_chars) to get a pre-truncated string, then use
estimate_text_width on that truncated string (with INTERVAL_LABEL_FONT_PX) and
compute the width-based truncated flag as before; finally return the truncated
string and a combined boolean (true if either truncate_label shortened the input
or the width check set truncated) so both character-count and pixel-based
truncation are reported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1023e603-cca3-4a40-973d-0a72bf647086
⛔ Files ignored due to path filters (6)
Cargo.lockis excluded by!**/*.lockdocs/assets/social-card.pngis excluded by!**/*.pngdocs/assets/social-card.svgis excluded by!**/*.svgexamples/reports/nsys-timeline-vis/figures/hybrid-decode-overlap.svgis excluded by!**/*.svgexamples/reports/nsys-timeline-vis/figures/hybrid-prefill-pipeline.svgis excluded by!**/*.svgexamples/reports/nsys-timeline-vis/figures/tp-decode-contrast.svgis excluded by!**/*.svg
📒 Files selected for processing (50)
.agents/skills/nsys-profile-analysis/SKILL.md.agents/skills/nsys-profile-analysis/references/cookbook.md.agents/skills/nsys-profile-analysis/references/limitations.md.claude-plugin/marketplace.json.claude-plugin/plugin.json.codex-plugin/plugin.jsonAGENTS.mdCHANGELOG.mdCargo.tomlREADME.mdcrates/nsys/veloq-nsys-query/src/viz_timeline/fingerprint.rscrates/nsys/veloq-nsys-query/src/viz_timeline/mod.rscrates/nsys/veloq-nsys-query/src/viz_timeline/types.rscrates/nsys/veloq-nsys-query/tests/wire_format_smoke.rscrates/nsys/veloq-nsys/src/cli.rscrates/nsys/veloq-nsys/src/commands/mod.rscrates/nsys/veloq-nsys/src/error.rscrates/nsys/veloq-nsys/src/help.rscrates/nsys/veloq-nsys/src/source.rscrates/nsys/veloq-nsys/src/views/basic.rscrates/veloq-vis/src/model.rscrates/veloq-vis/src/render/mod.rscrates/veloq-vis/src/render/painter.rscrates/veloq-vis/src/render/style.rscrates/veloq-vis/src/render/tests.rscrates/veloq-vis/src/render/text.rscrates/veloq/tests/cli_smoke/root.rsdocs/index.htmldocs/rfc/RFC-0001.mddocs/rfc/RFC-0002.mddocs/rfc/RFC-0003.mddocs/rfc/RFC-0004.mddocs/rfc/RFC-0005.mddocs/rfc/RFC-0006.mddocs/rfc/RFC-0007.mddocs/rfc/RFC-0008.mddocs/rfc/RFC-0009.mdexamples/README.mdexamples/reports/nsys-timeline-vis/README.mdexamples/reports/nsys-timeline-vis/evidence/summary.jsonexamples/reports/nsys-timeline-vis/report.mdgov/releases.tomlgov/rfc/RFC-0006/clauses/C-ARTIFACTS.tomlgov/rfc/RFC-0006/clauses/C-COMPATIBILITY.tomlgov/rfc/RFC-0006/clauses/C-SOURCE-IDENTITY.tomlgov/rfc/RFC-0006/rfc.tomlgov/rfc/RFC-0009/clauses/C-COMMAND-SURFACE.tomlgov/rfc/RFC-0009/clauses/C-COMPATIBILITY.tomlgov/work/2026-06-10-add-timeline-density-rendering.tomltypos.toml
✅ Files skipped from review due to trivial changes (18)
- typos.toml
- gov/rfc/RFC-0009/clauses/C-COMPATIBILITY.toml
- gov/rfc/RFC-0006/rfc.toml
- .claude-plugin/plugin.json
- .claude-plugin/marketplace.json
- gov/releases.toml
- docs/rfc/RFC-0003.md
- docs/rfc/RFC-0007.md
- docs/rfc/RFC-0001.md
- examples/README.md
- docs/rfc/RFC-0002.md
- examples/reports/nsys-timeline-vis/evidence/summary.json
- docs/rfc/RFC-0005.md
- docs/rfc/RFC-0004.md
- CHANGELOG.md
- docs/rfc/RFC-0008.md
- examples/reports/nsys-timeline-vis/README.md
- .agents/skills/nsys-profile-analysis/references/cookbook.md
🚧 Files skipped from review as they are similar to previous changes (11)
- gov/rfc/RFC-0009/clauses/C-COMMAND-SURFACE.toml
- docs/index.html
- crates/nsys/veloq-nsys-query/tests/wire_format_smoke.rs
- gov/work/2026-06-10-add-timeline-density-rendering.toml
- crates/nsys/veloq-nsys/src/cli.rs
- crates/nsys/veloq-nsys/src/error.rs
- crates/nsys/veloq-nsys-query/src/viz_timeline/fingerprint.rs
- crates/nsys/veloq-nsys/src/views/basic.rs
- crates/veloq-vis/src/render/painter.rs
- crates/nsys/veloq-nsys/src/commands/mod.rs
- crates/veloq-vis/src/render/mod.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores