Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThe dependency branch for aw-client-rust changed in Cargo.toml. In src/main.rs, a setting named always_active_pattern is retrieved and passed into DesktopQueryParams. An import alias for Ordering was added, and a comparison reference was updated to use the alias. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Main as main.rs
participant Client as ActivityWatch Client
participant Server as ActivityWatch Server
User->>Main: Run application
Main->>Client: get_setting("always_active_pattern")
Client-->>Main: Option<String> value
Note over Main: Build DesktopQueryParams<br/>including always_active_pattern (if present)
Main->>Server: query(desktop, DesktopQueryParams{...})
Server-->>Main: Results
Note over Main: Sort results using Ordering alias
Main-->>User: Display processed results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip You can disable sequence diagrams in the walkthrough.Disable the ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Cargo.toml (1)
19-19: Pin the git dependency and verify crate resolution from forked repoDepending on a moving branch increases build non-determinism and can break consumers unexpectedly. Also, since
aw-client-rustis sourced from a forkedaw-server-rustrepo, please verify that the workspace actually contains a package namedaw-client-ruston branchalways-active-pattern.
- Pin to a specific commit via
rev = "<commit-sha>"for reproducibility.- Consider adding a comment explaining why the fork/branch is required and when it can be reverted.
- If the repo hosts multiple crates in a workspace (likely), ensure Cargo resolves the
aw-client-rustpackage correctly (nopathoverride needed).Example (illustrative only—use the actual commit SHA you depend on):
-aw-client-rust = {git = "https://github.com/0xbrayo/aw-server-rust", branch="always-active-pattern"} +aw-client-rust = { git = "https://github.com/0xbrayo/aw-server-rust", branch = "always-active-pattern", rev = "<locked-commit-sha>" }You can verify resolution locally with:
cargo update -p aw-client-rustcargo tree -i aw-client-rustsrc/main.rs (1)
17-17: Drop the type alias and use total_cmp for deterministic float sortingThe alias
cmpOrderingis non-idiomatic (type aliases are usuallyUpperCamelCase) and exists solely to referenceOrdering::Equal. Usingf64::total_cmpyields a total ordering (robust in presence of NaN) and removes the need for the alias entirely.Apply this diff to simplify:
-use std::cmp::Ordering as cmpOrdering;- categories.sort_by(|a, b| b.1.partial_cmp(&a.1).unwrap_or(cmpOrdering::Equal)); + categories.sort_by(|a, b| b.1.total_cmp(&a.1));If you prefer to keep the current approach, at least rename the alias to
CmpOrderingfor style consistency and adjust the usage accordingly.Also applies to: 1038-1038
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)src/main.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint and Format Check
- GitHub Check: Build (Windows)
- GitHub Check: Build (Linux ARM64)
- GitHub Check: Build (macOS)
- GitHub Check: Build (Linux x64)
- GitHub Check: Build (Windows)
- GitHub Check: Build (Linux ARM64)
🔇 Additional comments (1)
src/main.rs (1)
478-479: Threading the new parameter through DesktopQueryParams looks goodThe new
always_active_patternfield is correctly plumbed intoDesktopQueryParams. Once the safe extraction above is applied, this will gracefully handle absent settings.
| let always_active_pattern = Some( | ||
| client | ||
| .get_setting("always_active_pattern") | ||
| .unwrap() | ||
| .to_string(), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Avoid panic and incorrect JSON stringification when reading always_active_pattern
unwrap() will panic if the setting is missing or the server is unavailable. Additionally, to_string() on a serde_json::Value yields JSON (e.g., quoted strings) rather than the plain string content. Prefer a safe extraction to Option<String>.
Apply this diff to make it robust and correct:
- let always_active_pattern = Some(
- client
- .get_setting("always_active_pattern")
- .unwrap()
- .to_string(),
- );
+ let always_active_pattern = match client.get_setting("always_active_pattern") {
+ Ok(v) => v.as_str().map(|s| s.to_owned()),
+ Err(e) => {
+ log::debug!("always_active_pattern not set or unavailable: {}", e);
+ None
+ }
+ };🤖 Prompt for AI Agents
In src/main.rs around lines 459 to 465, replace the current unwrap()+to_string()
usage with a non-panicking conversion that yields an Option<String>; call
client.get_setting("always_active_pattern"), convert any Err to None with .ok()
(or if it already returns Option, use it directly), then .and_then(|v|
v.as_str().map(|s| s.to_owned())), assigning the result to let
always_active_pattern: Option<String> so missing settings or errors produce None
and the JSON Value is unwrapped to the raw string content.
0xbrayo
force-pushed
the
always-active-pattern
branch
from
ee09313 to
3e07089
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main.rs (1)
459-466: Avoid panic and incorrect JSON stringification when reading always_active_patternUsing expect(...) will terminate the process if the setting is absent or the server is unavailable, and Value::to_string() returns JSON (quoted) rather than the raw string. Convert to Option via as_str() and handle errors gracefully.
Apply this diff to Lines 459-466:
- // Should never fail - let always_active_pattern = Some( - client - .get_setting("always_active_pattern") - .expect("failed to fetch always_active_pattern") - .to_string(), - ); + // Best-effort fetch; treat missing/unavailable as None and unwrap JSON string to a plain String + let always_active_pattern = match client.get_setting("always_active_pattern") { + Ok(v) => v.as_str().map(|s| s.to_owned()), + Err(e) => { + log::debug!("always_active_pattern not set or unavailable: {}", e); + None + } + };
🧹 Nitpick comments (2)
src/main.rs (2)
1039-1040: Prefer f64::total_cmp to avoid NaN pitfalls and drop the Ordering aliaspartial_cmp returns None for NaN; defaulting to Equal can yield unstable ordering. total_cmp provides a total order for f64 and is available on stable. It also lets you remove the cmpOrdering alias entirely.
Apply this diff to Line 1039:
- categories.sort_by(|a, b| b.1.partial_cmp(&a.1).unwrap_or(cmpOrdering::Equal)); + categories.sort_by(|a, b| b.1.total_cmp(&a.1));
17-17: Remove the non-idiomatic cmpOrdering aliasThe alias adds noise and CamelCase spelling is unconventional for a type alias. With the sort change to total_cmp, this import is no longer needed.
Apply this diff to Line 17:
-use std::cmp::Ordering as cmpOrdering; +// (removed) no longer needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)src/main.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build (Windows)
- GitHub Check: Build (Linux ARM64)
- GitHub Check: Build (Linux x64)
- GitHub Check: Lint and Format Check
- GitHub Check: Build (Linux ARM64)
- GitHub Check: Lint and Format Check
- GitHub Check: Build (Windows)
- GitHub Check: Build (Linux x64)
🔇 Additional comments (1)
src/main.rs (1)
479-480: Confirm DesktopQueryParams expects Option for always_active_patternGiven the upstream change, verify the field type matches Option (or whatever the new API expects). If it expects a regex or serde_json::Value, adapt the conversion accordingly. Also decide whether empty strings should be treated as None.
Docstrings generation was requested by @0xbrayo. * #10 (comment) The following files were modified: * `src/main.rs`
|
Note Generated docstrings for this pull request at #11 |
1 participant
Summary by CodeRabbit
New Features
Chores