refactor(asusctl): modularize CLI handlers into dedicated modules and fix service status guards - #238
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds modular asusctl handlers for anime, fan curves, platform, SCSI, and slash commands, then updates control-center D-Bus proxies, mocks, and version diagnostics to use revised platform interfaces. Changesasusctl command handlers
Control-center interface updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
coderabbitai
Bot
added
asusctl
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
asusctl/src/main.rs (1)
102-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
conn: &Connectionover suppressing the lint.
do_parsedonly ever passes&connto handlers, so taking the borrow removes the need for#[allow(clippy::needless_pass_by_value)].🤖 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 `@asusctl/src/main.rs` around lines 102 - 108, Update do_parsed to accept conn as &Connection instead of Connection, remove the #[allow(clippy::needless_pass_by_value)] attribute, and preserve the existing handler calls by passing the borrowed connection through.
🤖 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 `@asusctl/src/anime_cli.rs`:
- Around line 205-291: The argument checks in the anime and platform handlers
run inside per-device loops, causing repeated warnings and partial updates
before returning. In asusctl/src/anime_cli.rs lines 205-291, hoist the image,
pixel-image, gif, pixel-gif path.is_empty() checks and SetBuiltins
builtins.set.is_none() check before the for proxy in animes loop, returning
before proxy work; in asusctl/src/platform_cli.rs lines 344-348, move the
power.command let Some(...) else validation before the for aura in aura loop and
remove the empty “Commands available” print.
- Around line 170-177: Update the anime type selection around get_anime_type so
cmd.override_type is applied unconditionally whenever provided, regardless of
whether detection returns Unsupported. Retain the warning only for an
Unsupported detected type when no override is supplied.
In `@asusctl/src/platform_cli.rs`:
- Line 72: Move the rog_dbus::find_iface_blocking import from its current
location into the file’s existing top-level import block, without changing its
usage.
- Around line 289-300: Update the loop over AuraProxyBlocking devices to track
whether any eligible device was handled instead of returning immediately from
the first match. Call handle_led_power_1_do_1866 for every old or TUF laptop,
then emit the existing warning only after iteration when no matching device was
found, preserving the function’s successful return behavior.
- Around line 512-561: The handle_armoury_command function repeats the armoury
interface lookup in each subcommand arm. Resolve attrs once before matching on
cmd.command, then reuse it in List, Get, and Set; optionally extract the shared
Get/Set attribute search while preserving their distinct read versus mutation
behavior.
- Around line 482-496: Update the possible-values output block around the `p.0`
checks to always emit a terminating newline after printing the closing `]`,
before the `has_default` branch prints `default:` or the blank line. Preserve
the existing comma and closing-bracket formatting.
- Around line 118-135: Construct a single PlatformProxyBlocking instance before
the match on cmd.command, then reuse that proxy in the Limit, OneShot, and Info
arms. Remove the repeated PlatformProxyBlocking::new(conn)? calls while
preserving each arm’s existing operations and error propagation.
In `@asusctl/src/scsi_cli.rs`:
- Around line 46-47: Update the scsi command flow around find_iface_blocking and
the cmd.list handling so --list is processed before D-Bus interface discovery
and succeeds without asusd or the SCSI interface. Only call find_iface_blocking
when device options are supplied, while preserving existing behavior for
device-specific operations.
In `@asusctl/src/slash_cli.rs`:
- Around line 82-83: Update the no-argument branch in the slash command handler
to return immediately after emitting the missing-argument warning. Ensure the
early return occurs before opening the system connection or constructing the
Slash proxy, while preserving the existing usage message.
---
Outside diff comments:
In `@asusctl/src/main.rs`:
- Around line 102-108: Update do_parsed to accept conn as &Connection instead of
Connection, remove the #[allow(clippy::needless_pass_by_value)] attribute, and
preserve the existing handler calls by passing the borrowed connection through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 50e31e4d-5cc3-4278-bb3c-8ae52471e7fa
📒 Files selected for processing (9)
asusctl/src/anime_cli.rsasusctl/src/fan_curve_cli.rsasusctl/src/main.rsasusctl/src/platform_cli.rsasusctl/src/scsi_cli.rsasusctl/src/slash_cli.rsrog-control-center/src/main.rsrog-control-center/src/mocking.rsrog-control-center/src/zbus_proxies.rs
📜 Review details
🔇 Additional comments (15)
rog-control-center/src/zbus_proxies.rs (2)
1-5: LGTM!Also applies to: 71-89
91-91: 🎯 Functional CorrectnessNo compatibility alias needed. No live Rust callers still reference
find_iface; the only remaining occurrence is a commented example.> Likely an incorrect or invalid review comment.rog-control-center/src/mocking.rs (1)
5-6: LGTM!Also applies to: 98-125, 137-137, 152-154, 165-165, 198-202
rog-control-center/src/main.rs (1)
83-91: LGTM!asusctl/src/anime_cli.rs (3)
2-5: LGTM!Also applies to: 35-37
295-325: LGTM!Also applies to: 327-375
199-203: 🎯 Functional CorrectnessUse a zeroed buffer for
clear.AnimeDataBuffer::new()starts with0u8, and the other clear path also sendsvec![0u8; ...];255would light the whole panel.> Likely an incorrect or invalid review comment.asusctl/src/fan_curve_cli.rs (1)
2-2: LGTM!Also applies to: 46-119
asusctl/src/platform_cli.rs (5)
1-19: LGTM!Also applies to: 21-70
74-112: LGTM!Also applies to: 140-217
219-276: LGTM!Also applies to: 306-333, 350-388
390-434: LGTM!
436-481: LGTM!Also applies to: 500-510
asusctl/src/main.rs (1)
1-19: LGTM!Also applies to: 95-100, 117-131
asusctl/src/slash_cli.rs (1)
2-2: LGTM!Also applies to: 142-149
8196d47 to
7998c1a
Compare
7998c1a to
95f3669
Compare
…ync into rog-dbus
…le values newline
…atus and configuration output
95f3669 to
1015b1f
Compare
1 participant
Description
This PR modularizes the monolithic
asusctlCLI entry point into focused, single-responsibility modules and adds daemon service guards to improve CLI resilience and user error reporting.Key Changes
CLI Modularization (
asusctl):asusctl/src/main.rs(removing ~970 lines of monolithic code) into dedicated handler modules:platform_cli.rs: Platform settings (fan profiles, CPU governor/power limits, charge thresholds, Panel OD).anime_cli.rs: AniMe Matrix display configuration and image/gif rendering handlers.fan_curve_cli.rs: Custom fan curve editing and upload subcommands.scsi_cli.rs: External SCSI LED lighting subcommands.slash_cli.rs: Slash lighting bar control subcommands.Service Guards & Error Handling:
check_service_status) before dispatching D-Bus calls, preventing cryptic D-Bus timeout errors whenasusdis inactive.Control Center Mocking & Proxies:
rog-control-centermocking andzbusproxies to align with the modularized CLI subcommand signatures.