Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
github-actions
bot
added
rust
mrshmllow
force-pushed
the
push-lszyzzpnqukm
branch
from
9760a93 to
58fe700
Compare
mrshmllow
changed the title
mrshmllow
force-pushed
the
push-lszyzzpnqukm
branch
from
6674a30 to
f09bfc7
Compare
mrshmllow
force-pushed
the
push-lszyzzpnqukm
branch
from
f09bfc7 to
4d48b8d
Compare
mrshmllow
force-pushed
the
push-lszyzzpnqukm
branch
from
fef4b45 to
2e1317e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/core/src/commands/mod.rs (1)
194-225: Activity lifecycle events look correct, but consider trackingResultdata.The
LogMessage::StartandLogMessage::Stophandling correctly emits UI activity events. However,LogMessage::Resultat line 224 is silently ignored.In Nix's internal-json log format,
Resultmessages carry completion status/data for activities. If you want to display download progress, build success/failure, or similar metrics in the status bar, you may need to:
- Track pending activities by
id- Update activity state when
Resultis received- Send the accumulated result with
ActivityEndIf this is intentional (e.g., you only need start/stop signals for now), consider adding a brief comment explaining why
Resultis ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/commands/mod.rs` around lines 194 - 225, LogMessage::Result is currently swallowed; update the handler so results for activities are captured and forwarded instead of ignored: maintain a map of pending activities keyed by id (created in the LogMessage::Start branch where UI_SENDER sends ActivityBegin for node task_name via create_activity_category), update that map when LogMessage::Result arrives (using the same id) and send a UiMessage::ActivityEnd (as in the LogMessage::Stop branch) including the accumulated result data, then remove the id from the map; if you intentionally want to ignore results, add a short comment on LogMessage::Result explaining why.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/core/src/status.rs`:
- Around line 262-263: The status rendering currently returns early if
self.node_statuses.is_empty() and later ignores the nix_activities entry keyed
by None (and explicitly drops None), which hides ActivityBegin events with node:
None or activities recorded before a node row exists; update the rendering logic
to not return early when node_statuses is empty, include the global/unattributed
bucket by preserving and iterating the nix_activities entry for the None key (do
not remove it at lines that drop None), and render a global row for
nix_activities.get(&None) alongside per-node rows so ActivityBegin { node: None,
.. } events and pre-node activities are shown; adjust any loops that currently
only iterate over node_statuses (e.g., where rows are built at lines 281-289) to
also handle the None key and ensure the global bucket is formatted like other
node rows.
- Around line 406-416: The clear() method currently only clears based on the
saved logical newline count (previous_number_of_lines) and does not reset that
counter or account for soft-wrapped rows, so wrapped output can be left behind
or higher output erased; update clear() to reset previous_number_of_lines to 0
after wiping the screen and change the logic that computes
previous_number_of_lines (the code path that sets previous_number_of_lines when
rendering status lines and on UiMessage::Clear) to measure rendered terminal
rows (e.g., compute line wraps by clamping to the terminal width or using a
function that computes displayed rows per string) rather than raw .lines() count
so clearing and subsequent refreshes use the actual number of terminal rows.
---
Nitpick comments:
In `@crates/core/src/commands/mod.rs`:
- Around line 194-225: LogMessage::Result is currently swallowed; update the
handler so results for activities are captured and forwarded instead of ignored:
maintain a map of pending activities keyed by id (created in the
LogMessage::Start branch where UI_SENDER sends ActivityBegin for node task_name
via create_activity_category), update that map when LogMessage::Result arrives
(using the same id) and send a UiMessage::ActivityEnd (as in the
LogMessage::Stop branch) including the accumulated result data, then remove the
id from the map; if you intentionally want to ignore results, add a short
comment on LogMessage::Result explaining why.
🪄 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: 21f46ab9-3095-4143-9ca1-58364fbee7ff
📒 Files selected for processing (17)
CHANGELOG.mdcrates/cli/src/cli.rscrates/cli/src/main.rscrates/core/src/commands/common.rscrates/core/src/commands/mod.rscrates/core/src/commands/noninteractive.rscrates/core/src/commands/pty/mod.rscrates/core/src/commands/pty/output.rscrates/core/src/hive/executor.rscrates/core/src/hive/mod.rscrates/core/src/hive/steps/activate.rscrates/core/src/hive/steps/build.rscrates/core/src/hive/steps/evaluate.rscrates/core/src/hive/steps/keys.rscrates/core/src/hive/steps/ping.rscrates/core/src/hive/steps/push.rscrates/core/src/status.rs
| if self.node_statuses.is_empty() { | ||
| return String::new(); |
There was a problem hiding this comment.
Render the unattributed/global activity bucket.
Line 262 returns early when there are no node_statuses, Line 281 only renders rows that come from node_statuses, and Lines 374-376 explicitly drop the None key from nix_activities. Any ActivityBegin { node: None, .. } event—or any activity recorded before its node row exists—will be tracked but never shown, so the status bar underreports real Nix work.
Also applies to: 281-289, 374-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/core/src/status.rs` around lines 262 - 263, The status rendering
currently returns early if self.node_statuses.is_empty() and later ignores the
nix_activities entry keyed by None (and explicitly drops None), which hides
ActivityBegin events with node: None or activities recorded before a node row
exists; update the rendering logic to not return early when node_statuses is
empty, include the global/unattributed bucket by preserving and iterating the
nix_activities entry for the None key (do not remove it at lines that drop
None), and render a global row for nix_activities.get(&None) alongside per-node
rows so ActivityBegin { node: None, .. } events and pre-node activities are
shown; adjust any loops that currently only iterate over node_statuses (e.g.,
where rows are built at lines 281-289) to also handle the None key and ensure
the global bucket is formatted like other node rows.
| pub fn clear<T: std::io::Write>(&self, writer: &mut T) { | ||
| if !self.show_progress { | ||
| return; | ||
| } | ||
|
|
||
| let _ = write!(writer, "{}", cursor::Save); | ||
| // let _ = write!(writer, "{}", cursor::Down(1)); | ||
| let _ = write!(writer, "{}", cursor::Left(999)); | ||
| let _ = write!(writer, "{}", clear::CurrentLine); | ||
| for _ in 0..self.previous_number_of_lines { | ||
| let _ = write!(writer, "\r{}{}", clear::CurrentLine, cursor::Up(1)); | ||
| } | ||
|
|
||
| let _ = write!(writer, "\r{}", clear::CurrentLine); | ||
| let _ = writer.flush(); |
There was a problem hiding this comment.
Track rendered screen rows, not just logical lines().
Lines 411-415 clear only the newline count saved at Line 439, and clear() leaves that count intact after the wipe. After UiMessage::Clear, or whenever one of these longer rows soft-wraps in a narrow terminal, the next refresh can either erase output above the status block or leave wrapped fragments behind. Please reset the counter after clearing and base it on rendered terminal rows (or clamp the rendered width before writing).
Also applies to: 428-439
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/core/src/status.rs` around lines 406 - 416, The clear() method
currently only clears based on the saved logical newline count
(previous_number_of_lines) and does not reset that counter or account for
soft-wrapped rows, so wrapped output can be left behind or higher output erased;
update clear() to reset previous_number_of_lines to 0 after wiping the screen
and change the logic that computes previous_number_of_lines (the code path that
sets previous_number_of_lines when rendering status lines and on
UiMessage::Clear) to measure rendered terminal rows (e.g., compute line wraps by
clamping to the terminal width or using a function that computes displayed rows
per string) rather than raw .lines() count so clearing and subsequent refreshes
use the actual number of terminal rows.
1 participant
Closes #405
Summary by CodeRabbit
Release Notes
New Features
UI/UX