Add UI automation scenarios and CI e2e coverage#25
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a UI automation subsystem and CLI/launch wiring to run TOML scenarios that drive the app: select requests, send them, await conditions (status/text/delay), and capture screenshots. Introduces AutomationRuntime with scenario/step parsing, pending-wait evaluation, and snapshot persistence; new automation-related Message variants and update handling; LaunchOptions/AutomationOptions and cli parsing; E2E tests, scenarios, fixtures, and a GitHub Actions e2e workflow; and a new image dependency. Lifecycle, subscription, and run paths are extended to load and drive automation when configured. Sequence Diagram(s)sequenceDiagram
participant CLI
participant App as App Lifecycle
participant AR as AutomationRuntime
participant UI as UI / Update Loop
participant FS as Filesystem
participant Net as Network
CLI->>App: run(LaunchOptions with AutomationOptions)
App->>AR: AutomationRuntime::load(AutomationOptions)
AR->>FS: read scenario TOML
FS-->>AR: scenario data
App->>UI: include automation_subscription()
UI->>UI: emit AutomationPoll messages
UI->>AR: handle_automation_pulse() / drive_automation()
AR->>AR: evaluate current step
alt select_request
AR->>App: resolve_request_selector()
App-->>AR: request id or none
end
alt send
AR->>UI: request send Task
UI->>Net: perform HTTP request
Net-->>UI: response delivered
UI-->>AR: response recorded
end
alt wait_for_status/text/millis
AR->>AR: wait_satisfied() / timeout handling
end
alt screenshot
UI->>App: capture screenshot
App->>AR: handle_automation_screenshot(screenshot)
AR->>FS: write screenshot file
end
alt completed
AR->>FS: write snapshot JSON
AR->>App: complete_automation()
else failed
AR->>FS: write failure snapshot
AR->>App: fail_automation(reason)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
src/app/view/response.rs (1)
302-307: Consider lazily initialising thevoid_tagsset.The
HashSetis rebuilt from a fixed literal array on everypretty_htmlinvocation. While this is not a hot path, it could be trivially avoided with astd::sync::OnceLock.♻️ Proposed refactor — static OnceLock
+use std::sync::OnceLock; + +fn void_tags() -> &'static std::collections::HashSet<&'static str> { + static VOID_TAGS: OnceLock<std::collections::HashSet<&'static str>> = OnceLock::new(); + VOID_TAGS.get_or_init(|| { + [ + "area", "base", "br", "col", "embed", "hr", "img", "input", "link", "meta", "param", + "source", "track", "wbr", + ] + .into_iter() + .collect() + }) +}Then replace the local binding in
pretty_htmland threadvoid_tags()through the call chain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/view/response.rs` around lines 302 - 307, Replace the local rebuilding of void_tags in pretty_html with a lazily-initialized static using std::sync::OnceLock: create a function (e.g., void_tags()) that uses a static OnceLock<HashSet<&'static str>> to initialize the HashSet once from the literal array and return a reference to it, then update pretty_html to call void_tags() (and propagate that reference through any helper functions it calls) instead of constructing the HashSet on every invocation..github/workflows/ci.yml (1)
44-49: Add strict Clippy enforcement to CI baseline.Given the Rust lint policy, CI should run clippy with warnings denied so regressions are caught before merge.
Suggested workflow update
- name: Build run: cargo build --release --locked + - name: Clippy (strict) + run: cargo clippy --all-targets --all-features -- -D warnings + - name: Test run: cargo test --lockedAs per coding guidelines "**/*.rs
: Runcargo clippy --all-targets --all-features -- -D warnings(orcargo clippy-strict`) before committing to enforce the strict lint baseline".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 44 - 49, Update the CI workflow to run Clippy with strict enforcement by adding a step (e.g., named "Clippy" placed after the "Build" step and before "Test") that executes cargo clippy --all-targets --all-features -- -D warnings (or cargo clippy-strict) so warnings are denied in CI; ensure the new step mirrors existing workspace flags (if any) and fails the job on lint regressions.tests/ui/scenarios/smoke.toml (1)
15-17: Prefer condition-based wait aftersendto reduce CI flakiness.A fixed sleep here can race slower CI runs; waiting for a response condition before the final screenshot will be more stable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ui/scenarios/smoke.toml` around lines 15 - 17, The test uses a fixed sleep step ([[step]] with action "wait_for_millis" and value 1200) which can cause CI flakiness; replace this hard-coded wait with a condition-based wait that checks for the expected response after the preceding send (e.g., waiting for a DOM selector/text/state or an explicit "response_received" condition) so the final screenshot is taken only once the app has reached the expected state; locate the [[step]] having action "wait_for_millis" and change it to the appropriate condition-based wait action used elsewhere in your test harness.tests/e2e_automation.rs (2)
126-172: Consider asserting on process stderr even on success.The test checks
output.status.success()and prints stdout/stderr on failure, which is good. One minor improvement: checking that stderr is empty (or contains only expected output) on success would catch unexpected warnings from the automation runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e_automation.rs` around lines 126 - 172, In the test function automation_navigation_scenario_emits_screenshots_and_state, after asserting output.status.success() add an assertion that the child process produced no unexpected stderr (e.g. assert that output.stderr is empty or matches an expected pattern) by checking output.stderr (or String::from_utf8_lossy(&output.stderr)) and failing with a helpful message that includes the stderr contents; this ensures run_scenario’s runtime warnings are caught even when the process exit is success.
8-28: Snapshot deserialization structs use a permissive subset — verify serde defaults.These structs only capture a subset of the fields from
AutomationStateSnapshot(defined insrc/app/automation.rs). This works because serde's default behavior for structs ignores unknown fields. This is fine, but if#[serde(deny_unknown_fields)]is ever added to the serialization side or a field name changes, these tests will silently break. A#[allow(dead_code)]or a comment noting the intentional subset would clarify the design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e_automation.rs` around lines 8 - 28, The test deserialization structs (SnapshotProgress, SelectedRequest, StateSnapshot) intentionally cover only a subset of AutomationStateSnapshot but lack any indication of that intent; add an explicit comment above each struct (or a single comment above the group) stating they intentionally mirror a subset and rely on serde ignoring unknown fields, and annotate the structs with #[allow(dead_code)] to make the intent explicit and avoid future silent failures if fields change or deny_unknown_fields is introduced; ensure you reference the existing struct names (SnapshotProgress, SelectedRequest, StateSnapshot) when adding the comment/attribute so reviewers and future readers understand this is deliberate.src/app/automation.rs (4)
611-631:TextPresentwait checks three sources — document this breadth.
wait_satisfiedforTextPresentsearches the status line, response body raw, and the response viewer text. This is a broad net and could match unintended content (e.g., "No response yet" in the viewer). A brief comment explaining why all three sources are checked would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/automation.rs` around lines 611 - 631, Add a brief inline comment above the PendingWait::TextPresent match arm in the wait_satisfied method explaining that TextPresent intentionally checks three places—status_line, response.body.raw(), and response_viewer.text()—to catch text that may appear in different rendering contexts (status summary, raw body, or formatted viewer) and note the risk of matching viewer placeholder text; update the comment near wait_satisfied / PendingWait::TextPresent / status_line / response.body.raw() / response_viewer.text() so future maintainers understand why all three sources are searched.
44-51: Silently merging[[step]]and[[steps]]TOML keys may cause confusion.Both
stepandstepsare accepted and merged. A user who typos[[steps]]instead of[[step]](or vice versa) won't get an error — the steps just get appended. If only one key is the intended canonical form, consider warning or erroring on the other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/automation.rs` around lines 44 - 51, The code currently silently merges parsed.step and parsed.steps into all_steps (variables parsed.step, parsed.steps, and all_steps) which hides typos; update the logic in the function in automation.rs to detect when both parsed.step and parsed.steps are present and return an Err (or log a clear warning) that instructs the user to use the canonical key (choose which key your project prefers, e.g., require [[step]] and reject [[steps]]), and only if exactly one of them is present proceed to extend into all_steps and continue the existing empty-check against scenario_path; include the conflicting-key names in the error message so users can correct the TOML.
421-429:take()/re-assign pattern forautomationis sound but repetitive.The
self.automation.take()→drive_automation(&mut runtime)→self.automation = Some(runtime)pattern appears inhandle_automation_pulse,handle_automation_window_resolved, andhandle_automation_screenshot. This is correct (avoids double-borrow ofself), but the repetition across three methods is a maintenance risk.Consider extracting a helper like
with_automation(&mut self, f: impl FnOnce(&mut Self, &mut AutomationRuntime) -> Task<Message>) -> Task<Message>to reduce duplication.Sketch
fn with_automation( &mut self, f: impl FnOnce(&mut Self, &mut AutomationRuntime) -> Task<Message>, ) -> Task<Message> { let Some(mut runtime) = self.automation.take() else { return Task::none(); }; let task = f(self, &mut runtime); self.automation = Some(runtime); task }Then each handler becomes:
pub(super) fn handle_automation_pulse(&mut self) -> Task<Message> { self.with_automation(|app, runtime| app.drive_automation(runtime)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/automation.rs` around lines 421 - 429, Extract the repeated take/return pattern around self.automation into a helper method (e.g., with_automation) that takes &mut self and a closure FnOnce(&mut Self, &mut AutomationRuntime) -> Task<Message>, so each handler (handle_automation_pulse, handle_automation_window_resolved, handle_automation_screenshot) can call self.with_automation(|s, runtime| s.drive_automation(runtime)) or the appropriate method; inside with_automation perform the current pattern (let Some(mut runtime) = self.automation.take() else return Task::none(); let task = f(self, &mut runtime); self.automation = Some(runtime); task) to avoid double-borrows and remove duplicated code around the automation field and drive_automation calls.
694-752:complete_automationandfail_automationshare duplicated exit logic.Both methods: set
runtime.done, update status, write snapshot, optionally update status again, checkexit_when_done, and either close window or returnTask::none(). The duplication is significant.Consider extracting shared finalization
fn finalize_automation( &mut self, runtime: &mut AutomationRuntime, outcome: SnapshotOutcome, base_message: &str, ) -> Task<Message> { runtime.done = true; self.update_status_with_missing(base_message); match self.write_automation_state_snapshot(runtime, &outcome) { Ok(Some(path)) => { self.update_status_with_missing(&format!( "{base_message} (state: {})", path.display() )); } Ok(None) => {} Err(err) => eprintln!("automation: {err}"), } if let SnapshotOutcome::Failed(reason) = &outcome { eprintln!("automation failed: {reason}"); } if runtime.exit_when_done { runtime.window_id.map_or_else( || window::latest().map(Message::AutomationWindowResolved), window::close::<Message>, ) } else { Task::none() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/automation.rs` around lines 694 - 752, complete_automation and fail_automation duplicate finalization steps; extract the shared logic into a new method (e.g., finalize_automation) that takes (&mut self, runtime: &mut AutomationRuntime, outcome: SnapshotOutcome, base_message: &str) and does: set runtime.done, call update_status_with_missing(base_message), call write_automation_state_snapshot(runtime, &outcome) and on Ok(Some(path)) update_status_with_missing with "(state: {})", on Err(...) eprintln the error, if outcome is SnapshotOutcome::Failed(reason) eprintln!("automation failed: {reason}"), and finally return either the window closing Task (using runtime.window_id.map_or_else(|| window::latest().map(Message::AutomationWindowResolved), window::close::<Message>)) when runtime.exit_when_done is true or Task::none(); then replace the bodies of complete_automation and fail_automation to call finalize_automation with appropriate outcome and base_message.src/main.rs (1)
27-32: Include the previously-set path in error message for better debugging.On line 30,
_existingcontains the previously-set path but is discarded. Sinceset_state_file_override()returnsResult<(), PathBuf>, the error value would be useful diagnostic information if this condition triggers unexpectedly.Suggested improvement
if let Some(path) = launch.state_file.clone() - && let Err(_existing) = state::set_state_file_override(path) + && let Err(existing) = state::set_state_file_override(path) { - eprintln!("state file override was already configured"); + eprintln!( + "state file override was already configured ({})", + existing.display() + ); std::process::exit(2); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 27 - 32, The error currently discards the previously-set path returned from state::set_state_file_override when launch.state_file is Some; update the error handling to capture the Err(existing) value and include that PathBuf in the diagnostic message (e.g., print the existing path alongside "state file override was already configured") so the previously-set path is visible when that branch runs; locate the pattern using launch.state_file and state::set_state_file_override and change _existing to a named binding used in the eprintln invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 65-67: Update the README E2E command so it matches the
locked/artifact-enabled invocation used in CI/local scripts: replace the current
line that uses "ZAGEL_E2E=1 cargo test --test e2e_automation -- --nocapture"
with the invocation that includes cargo's --locked flag and the
artifact-enabling environment/flag used elsewhere (preserving ZAGEL_E2E and the
--test e2e_automation -- --nocapture parts) so docs mirror the reproducible
invocation used in CI.
In `@src/app/lifecycle.rs`:
- Around line 242-253: If AutomationRuntime::load(automation_options) fails
while launch.automation is Some, don't silently continue; either
return/propagate the Err from this function up to main or explicitly exit with a
non-zero code. Modify the match in lifecycle (where AutomationRuntime::load is
called) so on Err(err) you call
app.update_status_with_missing(&format!("Automation disabled: {err}")) and then
either (a) convert/return Err(err) from the surrounding function so the caller
(main) can handle termination, or (b) call a deterministic exit path (e.g., set
an overall failure state or trigger the same code path used for
--exit-when-done) so the process terminates with a non-zero status instead of
creating a GUI-only run; ensure references to app.automation,
app.automation_start_task(), Task::batch, and launch.automation are preserved
while performing this propagation/exit.
In `@src/cli.rs`:
- Around line 67-73: In next_path, ensure the token pulled from iter is not
another flag: after obtaining raw =
iter.next().ok_or(CliError::MissingValue(flag))?, convert raw to &str (via
raw.to_string_lossy() or raw.to_str()) and if it starts_with('-') return
Err(CliError::MissingValue(flag)); otherwise pass it to resolve_path(raw). Apply
the same check to the other path-consuming helper used around the 174-183 region
so any flag-like token is treated as MissingValue(flag) instead of being
accepted as a path.
In `@tests/e2e_automation.rs`:
- Around line 97-119: The run_scenario helper currently calls
Command::new(binary).output() and can block forever; change run_scenario to
spawn the child instead, set .stdout(Stdio::piped()).stderr(Stdio::piped()),
then use a timeout (e.g. Duration::from_secs(60)) to wait for the child (e.g.
via wait_timeout::ChildExt::wait_timeout or an equivalent manual loop); if the
wait times out, kill the child (child.kill()), wait for it to exit, then read
the piped stdout/stderr and construct/return the Output-like result so tests
won't hang; update references to Command::new(binary).output() inside
run_scenario accordingly.
In `@tests/ui/fixtures/workspace/requests/sample.http`:
- Around line 1-6: The fixture currently targets external endpoints "GET
https://httpbin.org/get" and "GET https://httpbin.org/uuid"; replace those with
the harness-owned local test stub endpoints (for example use the harness base
URL variable or localhost test server: e.g. {{TEST_SERVER}}/get and
{{TEST_SERVER}}/uuid or http://localhost:<port>/get and /uuid) and keep the
Accept: application/json header unchanged so CI uses the local stub instead of
httpbin.org.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 44-49: Update the CI workflow to run Clippy with strict
enforcement by adding a step (e.g., named "Clippy" placed after the "Build" step
and before "Test") that executes cargo clippy --all-targets --all-features -- -D
warnings (or cargo clippy-strict) so warnings are denied in CI; ensure the new
step mirrors existing workspace flags (if any) and fails the job on lint
regressions.
In `@src/app/automation.rs`:
- Around line 611-631: Add a brief inline comment above the
PendingWait::TextPresent match arm in the wait_satisfied method explaining that
TextPresent intentionally checks three places—status_line, response.body.raw(),
and response_viewer.text()—to catch text that may appear in different rendering
contexts (status summary, raw body, or formatted viewer) and note the risk of
matching viewer placeholder text; update the comment near wait_satisfied /
PendingWait::TextPresent / status_line / response.body.raw() /
response_viewer.text() so future maintainers understand why all three sources
are searched.
- Around line 44-51: The code currently silently merges parsed.step and
parsed.steps into all_steps (variables parsed.step, parsed.steps, and all_steps)
which hides typos; update the logic in the function in automation.rs to detect
when both parsed.step and parsed.steps are present and return an Err (or log a
clear warning) that instructs the user to use the canonical key (choose which
key your project prefers, e.g., require [[step]] and reject [[steps]]), and only
if exactly one of them is present proceed to extend into all_steps and continue
the existing empty-check against scenario_path; include the conflicting-key
names in the error message so users can correct the TOML.
- Around line 421-429: Extract the repeated take/return pattern around
self.automation into a helper method (e.g., with_automation) that takes &mut
self and a closure FnOnce(&mut Self, &mut AutomationRuntime) -> Task<Message>,
so each handler (handle_automation_pulse, handle_automation_window_resolved,
handle_automation_screenshot) can call self.with_automation(|s, runtime|
s.drive_automation(runtime)) or the appropriate method; inside with_automation
perform the current pattern (let Some(mut runtime) = self.automation.take() else
return Task::none(); let task = f(self, &mut runtime); self.automation =
Some(runtime); task) to avoid double-borrows and remove duplicated code around
the automation field and drive_automation calls.
- Around line 694-752: complete_automation and fail_automation duplicate
finalization steps; extract the shared logic into a new method (e.g.,
finalize_automation) that takes (&mut self, runtime: &mut AutomationRuntime,
outcome: SnapshotOutcome, base_message: &str) and does: set runtime.done, call
update_status_with_missing(base_message), call
write_automation_state_snapshot(runtime, &outcome) and on Ok(Some(path))
update_status_with_missing with "(state: {})", on Err(...) eprintln the error,
if outcome is SnapshotOutcome::Failed(reason) eprintln!("automation failed:
{reason}"), and finally return either the window closing Task (using
runtime.window_id.map_or_else(||
window::latest().map(Message::AutomationWindowResolved),
window::close::<Message>)) when runtime.exit_when_done is true or Task::none();
then replace the bodies of complete_automation and fail_automation to call
finalize_automation with appropriate outcome and base_message.
In `@src/app/view/response.rs`:
- Around line 302-307: Replace the local rebuilding of void_tags in pretty_html
with a lazily-initialized static using std::sync::OnceLock: create a function
(e.g., void_tags()) that uses a static OnceLock<HashSet<&'static str>> to
initialize the HashSet once from the literal array and return a reference to it,
then update pretty_html to call void_tags() (and propagate that reference
through any helper functions it calls) instead of constructing the HashSet on
every invocation.
In `@src/main.rs`:
- Around line 27-32: The error currently discards the previously-set path
returned from state::set_state_file_override when launch.state_file is Some;
update the error handling to capture the Err(existing) value and include that
PathBuf in the diagnostic message (e.g., print the existing path alongside
"state file override was already configured") so the previously-set path is
visible when that branch runs; locate the pattern using launch.state_file and
state::set_state_file_override and change _existing to a named binding used in
the eprintln invocation.
In `@tests/e2e_automation.rs`:
- Around line 126-172: In the test function
automation_navigation_scenario_emits_screenshots_and_state, after asserting
output.status.success() add an assertion that the child process produced no
unexpected stderr (e.g. assert that output.stderr is empty or matches an
expected pattern) by checking output.stderr (or
String::from_utf8_lossy(&output.stderr)) and failing with a helpful message that
includes the stderr contents; this ensures run_scenario’s runtime warnings are
caught even when the process exit is success.
- Around line 8-28: The test deserialization structs (SnapshotProgress,
SelectedRequest, StateSnapshot) intentionally cover only a subset of
AutomationStateSnapshot but lack any indication of that intent; add an explicit
comment above each struct (or a single comment above the group) stating they
intentionally mirror a subset and rely on serde ignoring unknown fields, and
annotate the structs with #[allow(dead_code)] to make the intent explicit and
avoid future silent failures if fields change or deny_unknown_fields is
introduced; ensure you reference the existing struct names (SnapshotProgress,
SelectedRequest, StateSnapshot) when adding the comment/attribute so reviewers
and future readers understand this is deliberate.
In `@tests/ui/scenarios/smoke.toml`:
- Around line 15-17: The test uses a fixed sleep step ([[step]] with action
"wait_for_millis" and value 1200) which can cause CI flakiness; replace this
hard-coded wait with a condition-based wait that checks for the expected
response after the preceding send (e.g., waiting for a DOM selector/text/state
or an explicit "response_received" condition) so the final screenshot is taken
only once the app has reached the expected state; locate the [[step]] having
action "wait_for_millis" and change it to the appropriate condition-based wait
action used elsewhere in your test harness.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.github/workflows/ci.ymlCargo.tomlREADME.mdsrc/app/automation.rssrc/app/hotkeys.rssrc/app/lifecycle.rssrc/app/messages.rssrc/app/mod.rssrc/app/update.rssrc/app/view/response.rssrc/app/view/workspace.rssrc/cli.rssrc/launch.rssrc/main.rssrc/state.rstests/e2e_automation.rstests/ui/fixtures/workspace/requests/sample.httptests/ui/scenarios/rest_send_status.tomltests/ui/scenarios/smoke.tomltests/ui/scenarios/snapshot_only.tomltests/ui/scenarios/ui_navigation.toml
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/app/lifecycle.rs (1)
242-254: Deadupdate_status_with_missingcall beforeprocess::exit.Line 249's
update_status_with_missing(...)sets a UI status string, butstd::process::exit(2)on line 251 terminates the process immediately — the status will never be rendered. Theeprintln!on line 250 is the only output the user will see.♻️ Remove dead code
Err(err) => { - app.update_status_with_missing(&format!("Automation disabled: {err}")); eprintln!("automation: {err}"); std::process::exit(2); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/lifecycle.rs` around lines 242 - 254, The call to app.update_status_with_missing(...) right before std::process::exit(2) is dead code because process::exit terminates immediately; remove the update_status_with_missing(...) invocation in the Err branch of AutomationRuntime::load so only the eprintln! and exit remain (leave AutomationRuntime::load, eprintln!, and std::process::exit(2) logic intact); if the intent was to surface the status to the UI instead of exiting, replace the exit with non‑terminating error flow instead of reintroducing update_status_with_missing.src/app/automation.rs (3)
207-215: Accepting both[[step]]and[[steps]]silently merges entries — potential user confusion.Having two TOML array table names (
stepandsteps) that get merged on line 44–45 means a user who writes[[steps]](plural) alongside[[step]](singular) gets both combined without warning, and a user who accidentally uses the wrong key still "works" but may not realize the other entries exist. If this is intentional for flexibility, a doc comment would help. If not, consider accepting only one canonical key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/automation.rs` around lines 207 - 215, ScenarioFile's automatic deserialization silently merges the two fields step and steps; implement a custom Deserialize for ScenarioFile (replacing #[derive(Deserialize)] on ScenarioFile) that reads both optional arrays (RawStep) and returns an error if both are present/non-empty (or alternatively chooses a single canonical field), so callers like the code invoking ScenarioFile deserialization get a clear failure instead of silently merged entries; update the struct (ScenarioFile) and its Deserialize impl to enforce the single-key constraint and surface a clear error message mentioning "step" vs "steps".
697-755: Duplicated exit logic betweencomplete_automationandfail_automation.Lines 718–725 and 747–754 are identical — both resolve the window ID and either close the window or return
Task::none(). The snapshot-write-and-log pattern (703–717 vs 731–745) is also very similar. A small shared helper (e.g.,fn finish_automation(&mut self, runtime, outcome) -> Task<Message>) would reduce this duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/automation.rs` around lines 697 - 755, Extract the duplicated end-of-run behavior from complete_automation and fail_automation into a shared helper like finish_automation(&mut self, runtime: &mut AutomationRuntime, outcome: SnapshotOutcome, status_msg: &str) -> Task<Message>: inside the helper call write_automation_state_snapshot(runtime, &outcome), log errors to stderr and call update_status_with_missing with the provided status_msg and the optional state path, then perform the exit_when_done logic using runtime.window_id.map_or_else(|| window::latest().map(Message::AutomationWindowResolved), window::close::<Message>) or return Task::none(); replace the duplicated blocks in complete_automation and fail_automation with calls to this helper, passing SnapshotOutcome::Completed or SnapshotOutcome::Failed(reason.to_string()) and the appropriate status messages, reusing existing methods write_automation_state_snapshot, update_status_with_missing, window::latest, window::close, and Task::none.
359-361: UseTask::doneinstead of theimmediatehelper.Iced 0.14.0 provides
Task::done(message)which creates a task that immediately resolves to a message. This is a cleaner replacement for theimmediatehelper function, which can be removed entirely and replaced with inlineTask::done(...)calls at its three call sites (lines 365, 371, and 385).♻️ Suggested simplification
-fn immediate(message: Message) -> Task<Message> { - Task::perform(async move { message }, |message| message) +fn immediate(message: Message) -> Task<Message> { + Task::done(message) }Or inline
Task::done(...)at call sites and remove the helper entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/automation.rs` around lines 359 - 361, Replace the custom helper fn immediate(message: Message) -> Task<Message> and all its usages with Iced's built-in Task::done(message): remove the immediate function definition and update each call site that currently calls immediate(some_message) to use Task::done(some_message) inline (the three places that reference immediate in this module), ensuring imports remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/automation.rs`:
- Around line 207-215: ScenarioFile's automatic deserialization silently merges
the two fields step and steps; implement a custom Deserialize for ScenarioFile
(replacing #[derive(Deserialize)] on ScenarioFile) that reads both optional
arrays (RawStep) and returns an error if both are present/non-empty (or
alternatively chooses a single canonical field), so callers like the code
invoking ScenarioFile deserialization get a clear failure instead of silently
merged entries; update the struct (ScenarioFile) and its Deserialize impl to
enforce the single-key constraint and surface a clear error message mentioning
"step" vs "steps".
- Around line 697-755: Extract the duplicated end-of-run behavior from
complete_automation and fail_automation into a shared helper like
finish_automation(&mut self, runtime: &mut AutomationRuntime, outcome:
SnapshotOutcome, status_msg: &str) -> Task<Message>: inside the helper call
write_automation_state_snapshot(runtime, &outcome), log errors to stderr and
call update_status_with_missing with the provided status_msg and the optional
state path, then perform the exit_when_done logic using
runtime.window_id.map_or_else(||
window::latest().map(Message::AutomationWindowResolved),
window::close::<Message>) or return Task::none(); replace the duplicated blocks
in complete_automation and fail_automation with calls to this helper, passing
SnapshotOutcome::Completed or SnapshotOutcome::Failed(reason.to_string()) and
the appropriate status messages, reusing existing methods
write_automation_state_snapshot, update_status_with_missing, window::latest,
window::close, and Task::none.
- Around line 359-361: Replace the custom helper fn immediate(message: Message)
-> Task<Message> and all its usages with Iced's built-in Task::done(message):
remove the immediate function definition and update each call site that
currently calls immediate(some_message) to use Task::done(some_message) inline
(the three places that reference immediate in this module), ensuring imports
remain unchanged.
In `@src/app/lifecycle.rs`:
- Around line 242-254: The call to app.update_status_with_missing(...) right
before std::process::exit(2) is dead code because process::exit terminates
immediately; remove the update_status_with_missing(...) invocation in the Err
branch of AutomationRuntime::load so only the eprintln! and exit remain (leave
AutomationRuntime::load, eprintln!, and std::process::exit(2) logic intact); if
the intent was to surface the status to the UI instead of exiting, replace the
exit with non‑terminating error flow instead of reintroducing
update_status_with_missing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.mdsrc/app/automation.rssrc/app/lifecycle.rssrc/cli.rstests/e2e_automation.rstests/ui/fixtures/workspace/requests/sample.http
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- tests/e2e_automation.rs
- src/cli.rs
- tests/ui/fixtures/workspace/requests/sample.http
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
55-80: Duplicated setup steps; consider a composite action or reusable workflow.The Checkout → Install Rust → Rust cache → apt dependencies sequence is copy-pasted from
build-test. Any future change (e.g., adding a new system library) must be applied in both places.A local composite action (e.g.,
.github/actions/setup-linux/action.yml) or a reusable workflow would centralise this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 55 - 80, The CI workflow duplicates the "Checkout", "Install Rust", "Rust cache" and "Linux system dependencies" steps already present in build-test; extract these steps into a single reusable unit (either a local composite action like .github/actions/setup-linux/action.yml or a reusable workflow) and replace the duplicated blocks in ci.yml and build-test with a single uses: reference to that unit; specifically factor out the apt-get install list and the actions/checkout@v4, dtolnay/rust-toolchain@stable and Swatinem/rust-cache@v2 invocations, give the composite action a clear name (e.g., setup-linux) and update ci.yml to call it instead of repeating the steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 82-85: Add a timeout to the "Run automation e2e tests" step (the
step that executes "xvfb-run -a env ZAGEL_E2E=1
ZAGEL_E2E_ARTIFACTS_DIR=artifacts/e2e cargo test --locked --test e2e_automation
-- --nocapture --test-threads=1") to prevent hung UI tests from blocking the
runner; update the workflow step to include a timeout-minutes value (or apply
timeout-minutes at the job level) with a sensible limit (e.g., 30–60 minutes) so
the runner is aborted if the step exceeds that duration.
- Line 53: The e2e-automation job is currently gated on the entire matrix job
"build-test", which blocks E2E if any non-Linux matrix leg fails; split out the
Linux matrix leg into its own job (e.g., create a new job named
"build-test-linux" that runs the same steps on ubuntu-latest) and change the
dependency in e2e-automation from needs: build-test to needs: build-test-linux
so E2E only requires the Linux build; update or remove the Linux entry from the
original "build-test" matrix as needed to avoid duplicate runs.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 55-80: The CI workflow duplicates the "Checkout", "Install Rust",
"Rust cache" and "Linux system dependencies" steps already present in
build-test; extract these steps into a single reusable unit (either a local
composite action like .github/actions/setup-linux/action.yml or a reusable
workflow) and replace the duplicated blocks in ci.yml and build-test with a
single uses: reference to that unit; specifically factor out the apt-get install
list and the actions/checkout@v4, dtolnay/rust-toolchain@stable and
Swatinem/rust-cache@v2 invocations, give the composite action a clear name
(e.g., setup-linux) and update ci.yml to call it instead of repeating the steps.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.ymlREADME.md
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
50-84: Consider extracting the shared setup into a composite action to reduce duplication.
build-test-linuxande2e-automationshare four setup steps verbatim (checkout, install Rust, Rust cache, and a nearly identical apt-get list). As the dep list grows, keeping them in sync manually is error-prone. A local composite action (e.g.,.github/actions/linux-setup/action.yml) accepting an optionalextra-packagesinput for the e2e-specific additions (libxkbcommon-x11-0,xvfb) would eliminate the duplication.Also applies to: 92-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 50 - 84, Extract the shared setup steps used by the build-test-linux and e2e-automation jobs into a local composite action (e.g., .github/actions/linux-setup/action.yml) that encapsulates the Checkout step, the dtolnay/rust-toolchain install, the Swatinem/rust-cache step, and the apt-get install list; add an optional input named extra-packages (string/array) so callers can append e2e-only packages like libxkbcommon-x11-0 and xvfb, then replace the duplicated steps in the build-test-linux and e2e-automation jobs with a single uses: ./github/actions/linux-setup call and pass extra-packages where needed.
27-42: Remove the now-unreachable "Linux system dependencies" step frombuild-test.With
ubuntu-latestremoved from the matrix (line 15),runner.os == 'Linux'is never true in this job. The entire step is dead and can be deleted.🧹 Proposed cleanup
- - name: Linux system dependencies - if: runner.os == 'Linux' - run: | - sudo apt-get update - sudo apt-get install -y --no-install-recommends \ - pkg-config \ - libudev-dev \ - libx11-dev \ - libxrandr-dev \ - libxinerama-dev \ - libxcursor-dev \ - libxi-dev \ - libxkbcommon-dev \ - libwayland-dev \ - libxcb-shape0-dev \ - libxcb-xfixes0-dev - - name: Build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 27 - 42, The "Linux system dependencies" step in the build-test job is now unreachable because the matrix no longer includes ubuntu-latest, so remove the entire step block named "Linux system dependencies" (the run block guarded by if: runner.os == 'Linux') from the .github/workflows/ci.yml; delete the step to clean up dead CI code and avoid confusion while leaving other steps and the build-test job intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 50-84: Extract the shared setup steps used by the build-test-linux
and e2e-automation jobs into a local composite action (e.g.,
.github/actions/linux-setup/action.yml) that encapsulates the Checkout step, the
dtolnay/rust-toolchain install, the Swatinem/rust-cache step, and the apt-get
install list; add an optional input named extra-packages (string/array) so
callers can append e2e-only packages like libxkbcommon-x11-0 and xvfb, then
replace the duplicated steps in the build-test-linux and e2e-automation jobs
with a single uses: ./github/actions/linux-setup call and pass extra-packages
where needed.
- Around line 27-42: The "Linux system dependencies" step in the build-test job
is now unreachable because the matrix no longer includes ubuntu-latest, so
remove the entire step block named "Linux system dependencies" (the run block
guarded by if: runner.os == 'Linux') from the .github/workflows/ci.yml; delete
the step to clean up dead CI code and avoid confusion while leaving other steps
and the build-test job intact.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.ymlsrc/app/view/response.rssrc/app/view/workspace.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/view/workspace.rs
- src/app/view/response.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
xvfband uploads generated artifacts for debuggingTesting
Summary by CodeRabbit
New Features
Documentation
Tests
Chores