Skip to content

fix(cli): render sub-millisecond timer inputs correctly in dora node info - #2859

Merged
trunk-io[bot] merged 1 commit into
mainfrom
claude/dreamy-bardeen-so9lfs-node-info-timer
Aug 11, 2026
Merged

fix(cli): render sub-millisecond timer inputs correctly in dora node info#2859
trunk-io[bot] merged 1 commit into
mainfrom
claude/dreamy-bardeen-so9lfs-node-info-timer

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Issue

build_input_info in binaries/cli/src/command/node/info.rs reconstructed every timer input source by hand as dora/timer/millis/{interval.as_millis()}.

Duration::as_millis() floors to 0 for any sub-millisecond interval, so a valid high-rate timer input such as dora/timer/hz/2000 (500 µs) was displayed by dora node info as dora/timer/millis/0 — a nonsensical zero-interval source that no longer round-trips through the descriptor parser. Whole-Hz timers like dora/timer/hz/3 (~333 ms) were also rendered lossily as millis/333.

Fix

Use InputMapping's canonical Display impl (libraries/message/src/config.rs) instead of re-deriving the source string. Display already selects the coarsest exact unit (secs/millis/micros/nanos) and is the form the descriptor round-trips through. The User and Logs arms of the old hand-rolled match rendered identically to Display anyway, so this also removes a small reimplementation of the trait — build_input_info now just calls input.mapping.to_string().

Validation

  • New unit tests were unnecessary: the sub-millisecond round-trip is already covered by dora-message's existing timer_subms_interval_roundtrips test, which guarantees Display never truncates.
  • cargo test -p dora-cli, cargo clippy -p dora-cli --all-targets -- -D warnings, and cargo fmt --all -- --check all pass.
  • Full workspace test suite green except for pre-existing zenoh/ros2 networking tests that require IPv6 (unavailable in the CI sandbox) — unrelated to this CLI-only change.

🤖 This is a machine-generated pull request opened by Claude Code. A human maintainer should review before merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_0142J7B2578ixrh77kNDNJry


Generated by Claude Code

… info`

`build_input_info` reconstructed every timer input source as
`dora/timer/millis/{interval.as_millis()}`. `Duration::as_millis()`
floors to 0 for any sub-millisecond interval, so a valid high-rate
timer input such as `dora/timer/hz/2000` (500µs) was displayed as
`dora/timer/millis/0` — a nonsensical zero-interval source that no
longer round-trips through the descriptor parser. Whole-Hz timers like
`dora/timer/hz/3` were also rendered lossily as `millis/333`.

Use `InputMapping`'s canonical `Display` instead of re-deriving the
source string by hand. `Display` already picks the coarsest exact unit
(secs/millis/micros/nanos) and is the form the descriptor round-trips
through; the `User` and `Logs` arms rendered identically to it anyway,
so this also removes a small hand-rolled reimplementation of the trait.

The sub-millisecond round-trip is already covered by dora-message's
`timer_subms_interval_roundtrips` test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0142J7B2578ixrh77kNDNJry
@trunk-io

trunk-io Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

😎 Merged successfully - details.

Copy link
Copy Markdown
Collaborator Author

Automated review run summary

This PR is one of a batch opened by an automated Claude Code review pass over origin/main (84709f7). The pass reviewed the mavlink2-bridge, arrow-convert, message, core, node/operator, daemon, coordinator, telemetry, and CLI subsystems. Each finding below was verified by hand against current main before acting. All PRs are machine-generated drafts — a human maintainer should review before merging.

Opened PRs

Correctness

Robustness / security

Rejected findings (verified, deliberately not filed)

  • dora logs raw .txt files bypass all filters — real (Medium), but the correct fix synthesizes timestamps/levels and changes output ordering; a behavior change that needs maintainer design input, not an autonomous patch.
  • dora cluster upgrade reconnect check false-positive — a stale still-connected daemon can satisfy the post-restart wait; real, but a correct fix (disconnect-then-reconnect / connection-identity tracking) is design-level and too speculative to patch blind.
  • dora cluster up coordinator addr 0.0.0.0/loopback unvalidated for multi-machine clusters — plausible guard but borderline feature-add; low confidence it isn't intentional.
  • --hub-override split_once('=') mis-parses an =1.2.3 version in the key — low; only yields a confusing error, and a version in the override key is meaningless anyway.
  • Duplicate --hub-override keys silently overwrite — very low footgun.
  • dora top stale table selection not re-clamped after the node list shrinks — cosmetic; ratatui clamps at render time, no panic.
  • dora hub publish version auto-detect fails for version.workspace = true crates — fails gracefully with an actionable "pass --version" message; a real fix requires resolving the workspace-root Cargo.toml at the committed rev.
  • dora hub fetch dataflow-vs-package detection is extension-case-sensitive (.YAML) — cosmetic UX only.
  • merge_env doc-vs-behavior mismatch; ROS2 multi-topic validation gap (core) — not correctness bugs (cosmetic / caught later by the bridge binary).
  • mavlink2-bridge / arrow-convert / node & operator APIs — reviewed field-by-field; no solid findings. This code is heavily hardened with regression history.

Validation

Per-PR: cargo fmt --all -- --check, cargo clippy -p dora-cli --all-targets -- -D warnings, and cargo test -p dora-cli all green. Combined diff additionally run through the /simplify gate (which prompted collapsing the #2859 helper into the canonical Display and broadening #2861 from id-only to labels). The full workspace test suite is green except for 4 pre-existing zenoh/ros2 networking tests that require IPv6 (tcp/[::]:0 — unavailable in this sandbox) and are unrelated to these CLI-only changes.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

Automated review by Claude — this is a fully automated review; no human has vetted it.

No issues found. Delegating to InputMapping's Display is the correct fix: it picks the coarsest exact unit and produces the canonical source string the descriptor parser round-trips, so dora/timer/hz/2000 (500µs) no longer collapses to dora/timer/millis/0. The removed User/Logs match arms rendered identically to Display, so this is behavior-preserving for those cases while removing a small reimplementation.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

@phil-opp the Trunk merge queue failed for this PR.

See the Trunk merge-status comment for details.

Posted as a new comment so GitHub sends an email — Trunk's sticky comment is edited in place and won't trigger a notification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants