Skip to content

refactor: route all operation progress through a new icp-events crate - #725

Open
raymondk wants to merge 4 commits into
mainfrom
rk/events
Open

refactor: route all operation progress through a new icp-events crate#725
raymondk wants to merge 4 commits into
mainfrom
rk/events

Conversation

@raymondk

Copy link
Copy Markdown
Collaborator

Supersedes #709, which landed the same inversion as one change. This PR reaches the same end state through four reviewable slices (one commit each), each keeping the full test suite green and pinning output parity against the existing integration tests along the way.

Why

Progress reporting is what welded the operations layer to the terminal: operations constructed ProgressManagers, drove indicatif bars, formatted user-facing strings, and printed batch failure dumps, while the Build/Synchronize ports in crates/icp took a tokio::sync::mpsc::Sender<String> that only existed because the binary had a progress bar to feed.

What

New crates/icp-events: typed, serde::Serialize events (TaskStarted/StepStarted/Output/Progress/StepCompleted/TaskCompleted, with typed TaskKind payloads per operation) and cheap-clone emitter handles (ReporterTaskReporterStepReporter). Sends are sync and non-blocking; with no receiver they are no-ops, so tests and headless callers need no wiring. Errors stay on the operations' Result path — TaskOutcome::Failed carries display data only.

New crates/icp-cli/src/render/: the presentation layer. InteractiveRenderer reproduces the existing indicatif UX (spinners, step headers, rolling output window, byte bars for snapshot transfers, deferred failure dumps — absorbing MultiStepProgressBar::dump_output); PlainRenderer covers --debug. Commands drive a renderer per operation phase via rendered() / rendered_task().

Push-down: Build, Synchronize, ScriptRunner, and the sync-plugin runtime's run_plugin take a StepReporter in place of Option<Sender<String>> — no Option, a null reporter instead. The plugin runtime's live output forwarding becomes lossless (events instead of best-effort try_send).

Commit chain (intended to be reviewed commit by commit, merged unsquashed):

  1. icp-events crate + renderers + the build pipeline end to end
  2. sync pipeline: trait push-down into icp and icp-sync-plugin, temporary line-channel bridges deleted, retained plugin stderr and failure cause chains modeled as outcome data
  3. deploy's single-action phases (create/install/settings/env-vars/Candid check) + the rendered() helper; no ProgressManager left in operations or deploy
  4. snapshot byte transfers: Progress { position } events and renderer-owned byte bars

Notes for review

  • Output parity is pinned by the existing build_tests/sync_tests exact-format assertions; the only test edits are the tracing-target renames (icp::progress:icp::render::plain:).
  • One deliberate behavior change: under --debug, snapshot transfer bars no longer draw (previously they ignored the debug flag); --debug output is now consistent across all commands.
  • Network commands' spinners and migrate_id's status spinner are command-owned UX and intentionally stay direct — commands are the presentation layer.
  • Events serialize with stable naming from day one, so a streaming --json renderer (RFC: document --json output schemas in --help and/or generated docs #493) is purely additive later.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QUZHfx2ng97WM2j9XoA3rr

raymondk and others added 4 commits August 19, 2026 19:51
Operations now communicate with the presentation layer through typed
events instead of driving progress bars directly. New icp-events crate
carries the task/step/output vocabulary and the Reporter handles; the
new icp-cli render module owns all wording, indicatif bars, and failure
replay (absorbing MultiStepProgressBar::dump_output). The Build trait
and the low-level script/wasm helpers in the core crate take a
StepReporter in place of Option<Sender<String>>; the sync path bridges
into its legacy line channel until it is ported.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QUZHfx2ng97WM2j9XoA3rr
The Synchronize and ScriptRunner traits, and the sync-plugin runtime's
run_plugin, take a StepReporter in place of Option<Sender<String>>,
which removes the temporary line-channel bridges entirely — the plugin
runtime's live output forwarding is now lossless (events instead of
best-effort try_send). Retained plugin stderr rides on
TaskOutcome::Succeeded and failure cause chains on TaskOutcome::Failed,
so the renderers own printing both; sync_many drops its ProgressManager
and eprintln calls. MultiStepProgressBar and its step channel are now
unused and deleted from progress.rs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QUZHfx2ng97WM2j9XoA3rr
Install, settings sync, binding env vars, and the Candid compatibility
check take a Reporter instead of a debug flag; their per-canister
Failure structs, progress bars, and batch error! dumps move into the
renderers (new TaskKinds carry the data, TaskOutcome::Skipped covers
the not-an-upgrade Candid case). Deploy's create phase emits Create
task events instead of driving bars, so no ProgressManager remains in
deploy or the operations layer. A render::rendered() helper wraps the
channel/renderer lifecycle that every phase and command was repeating.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QUZHfx2ng97WM2j9XoA3rr
The blob upload/download operations take a TaskReporter and emit the
new Progress { position } event in place of driving an indicatif bar;
TaskKind::SnapshotTransfer declares the direction, blob, and byte
total, and the interactive renderer builds the byte-bar style (moved
from create_transfer_progress_bar) for that kind. The snapshot
upload/download commands run each blob through the new rendered_task
helper. Under --debug the plain renderer shows no live transfer bar,
consistent with the rest of the CLI (the bar previously drew there).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QUZHfx2ng97WM2j9XoA3rr
Copilot AI balanced review requested due to automatic review settings August 20, 2026 23:25
@raymondk
raymondk requested a review from a team as a code owner August 20, 2026 23:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces typed operation-progress events and CLI-owned renderers, decoupling core operations from terminal presentation.

Changes:

  • Adds the icp-events event and reporter API.
  • Routes build, sync, deploy, and snapshot progress through renderers.
  • Replaces output channels and operation-owned progress bars.

Reviewed changes

Copilot reviewed 36 out of 37 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
Cargo.toml Registers icp-events.
Cargo.lock Locks new dependencies.
crates/icp-events/Cargo.toml Defines the events crate.
crates/icp-events/src/lib.rs Implements events and reporters.
crates/icp/Cargo.toml Adds events dependency.
crates/icp/src/canister/wasm.rs Reports WASM resolution events.
crates/icp/src/canister/script.rs Streams script output through reporters.
crates/icp/src/canister/build/mod.rs Updates the build interface.
crates/icp/src/canister/build/script.rs Reports script-build output.
crates/icp/src/canister/build/prebuilt.rs Reports prebuilt-WASM progress.
crates/icp/src/canister/sync/mod.rs Updates the synchronization interface.
crates/icp/src/canister/sync/script.rs Updates script-runner reporting.
crates/icp/src/canister/sync/plugin.rs Passes reporters into plugins.
crates/icp-sync-plugin/Cargo.toml Adds events dependency.
crates/icp-sync-plugin/src/runtime.rs Emits typed plugin output.
crates/icp-cli/Cargo.toml Adds events dependency.
crates/icp-cli/src/main.rs Registers the render module.
crates/icp-cli/src/progress.rs Removes migrated multi-step rendering.
crates/icp-cli/src/render/mod.rs Adds renderer orchestration and logging.
crates/icp-cli/src/render/plain.rs Adds debug-mode rendering.
crates/icp-cli/src/render/interactive.rs Adds interactive progress rendering.
crates/icp-cli/src/operations/build.rs Emits build task events.
crates/icp-cli/src/operations/sync.rs Emits sync task events.
crates/icp-cli/src/operations/install.rs Emits installation events.
crates/icp-cli/src/operations/settings.rs Emits settings-update events.
crates/icp-cli/src/operations/binding_env_vars.rs Emits environment-update events.
crates/icp-cli/src/operations/candid_compat.rs Emits compatibility-check events.
crates/icp-cli/src/operations/bundle.rs Uses event-driven builds.
crates/icp-cli/src/operations/snapshot_transfer.rs Emits byte positions.
crates/icp-cli/src/commands/build.rs Runs builds with a renderer.
crates/icp-cli/src/commands/sync.rs Runs sync with a renderer.
crates/icp-cli/src/commands/deploy.rs Renders deploy phases.
crates/icp-cli/src/commands/project/bundle.rs Renders bundle builds.
crates/icp-cli/src/commands/canister/snapshot/upload.rs Renders upload progress.
crates/icp-cli/src/commands/canister/snapshot/download.rs Renders download progress.
crates/icp-cli/tests/build_tests.rs Updates tracing targets.
crates/icp-cli/tests/sync_tests.rs Updates tracing targets.
Suppressed comments (1)

crates/icp-cli/src/render/interactive.rs:65

  • The spinner is inserted into MultiProgress before its prefix and initial message are set. This reintroduces the unlabeled first-frame race described in the superseded implementation: configure the detached bar fully, add it, and only then start the ticker.
                        let bar = self.multi_progress.add(
                            ProgressBar::new_spinner()
                                .with_style(make_style(TICK_EMPTY, COLOR_REGULAR)),
                        );
                        bar.set_prefix(format!("[{}]", task.canister()));

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +55 to +58
let bar = self.multi_progress.add(ProgressBar::new(*total_bytes));
bar.set_style(transfer_style());
bar.set_prefix(transfer_label(blob));
bar
Comment on lines +99 to +102
view.header = step_header(view.log.kind(), number, total, &label);
view.window = RollingLines::new(LIVE_WINDOW_LINES);
view.log.start_step(view.header.clone());
view.bar.set_message(view.header.clone());
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants