feat: linux back-end abstraction#17
Merged
Merged
Conversation
Reviewer's GuideIntroduce a generic capture backend abstraction for the Linux grim+Hyprland implementation, add a serializable commands layer for future IPC/Tauri integration, and extend the UI/settings with a close-after-save workflow and safer image handling and diagnostics tweaks. Sequence diagram for capture execution via commands surfacesequenceDiagram
participant Caller
participant Commands as CommandsModule
participant SettingsSvc as SettingsServiceModule
participant CaptureSvc as CaptureServiceModule
participant CaptureMod as CaptureModule
participant PlatformCap as PlatformCaptureModule
participant Backend as CaptureBackend
Caller->>Commands: capture(request)
activate Commands
alt request.settings is None
Commands->>SettingsSvc: load_or_default()
SettingsSvc-->>Commands: Settings
else request.settings provided
Note over Commands: Use provided Settings
end
Commands->>CaptureSvc: run(mode, Settings)
activate CaptureSvc
CaptureSvc->>CaptureMod: capture(mode)
activate CaptureMod
CaptureMod->>PlatformCap: current_backend()
PlatformCap-->>CaptureMod: backend
Note over CaptureMod,Backend: capture_with_backend(mode, backend)
alt mode Region
CaptureMod->>Backend: capture_fullscreen()
Backend-->>CaptureMod: fullscreen_png
CaptureMod-->>CaptureSvc: CaptureResult
else mode Fullscreen
CaptureMod->>Backend: capture_fullscreen()
Backend-->>CaptureMod: png
CaptureMod-->>CaptureSvc: CaptureResult
else mode Window
CaptureMod->>Backend: active_window_geometry()
Backend-->>CaptureMod: geometry
CaptureMod->>Backend: capture_region(geometry)
Backend-->>CaptureMod: png
CaptureMod-->>CaptureSvc: CaptureResult
end
CaptureSvc-->>Commands: CaptureExecution
deactivate CaptureSvc
Commands-->>Caller: CaptureCommandResponse
deactivate Commands
Class diagram for the new Linux capture backend abstractionclassDiagram
class CaptureBackend {
<<trait>>
+capture_fullscreen() Result~Vec_u8~
+capture_region(geometry str) Result~Vec_u8~
+active_window_geometry() Result~String~
}
class ScreenshotTool {
<<trait>>
+capture_region(geometry str) Result~Vec_u8~
+capture_fullscreen() Result~Vec_u8~
}
class ActiveWindowGeometrySource {
<<trait>>
+active_window_geometry() Result~String~
}
class GrimCli {
+capture_region(geometry str) Result~Vec_u8~
+capture_fullscreen() Result~Vec_u8~
}
class HyprctlCli {
+active_window_geometry() Result~String~
}
class GrimHyprlandBackend {
+screenshot_tool
+active_window_source
+new(screenshot_tool S, active_window_source W) GrimHyprlandBackend~S_W~
+capture_fullscreen() Result~Vec_u8~
+capture_region(geometry str) Result~Vec_u8~
+active_window_geometry() Result~String~
}
class PlatformCaptureModule {
+current_backend() CaptureBackend
+platform_name() str
+backend_name() str
+mode_supported(mode CaptureMode) bool
}
class CaptureModule {
+capture(mode CaptureMode) Result~CaptureResult~
+capture_with_backend(mode CaptureMode, backend CaptureBackend) Result~CaptureResult~
}
class CaptureResult {
+png_data Vec_u8
}
class FullscreenCaptureModule {
+capture(backend CaptureBackend) Result~CaptureResult~
}
class RegionCaptureModule {
+capture(backend CaptureBackend) Result~CaptureResult~
}
class WindowCaptureModule {
+capture(backend CaptureBackend) Result~CaptureResult~
}
GrimCli ..|> ScreenshotTool
HyprctlCli ..|> ActiveWindowGeometrySource
GrimHyprlandBackend ..|> CaptureBackend
PlatformCaptureModule --> GrimHyprlandBackend : current_backend
CaptureModule --> PlatformCaptureModule : uses current_backend
CaptureModule --> FullscreenCaptureModule : delegates fullscreen
CaptureModule --> RegionCaptureModule : delegates region
CaptureModule --> WindowCaptureModule : delegates window
FullscreenCaptureModule --> CaptureBackend : capture_fullscreen
RegionCaptureModule --> CaptureBackend : capture_fullscreen
WindowCaptureModule --> CaptureBackend : active_window_geometry
WindowCaptureModule --> CaptureBackend : capture_region
Class diagram for the new commands IPC surface and settings changesclassDiagram
class CaptureMode
class Settings {
+default_save_location PathBuf
+copy_to_clipboard bool
+close_after_copy bool
+close_after_save bool
+open_after_save bool
+open_editor bool
+default_capture_mode CaptureMode
}
class CaptureCommandRequest {
+mode CaptureMode
+settings Option~Settings~
+include_png Option~bool~
}
class CaptureCommandResponse {
+mode CaptureMode
+png_base64 Option~String~
+png_byte_len usize
+copied_to_clipboard bool
+saved_path Option~String~
+backend CaptureBackendInfo
}
class CaptureBackendInfo {
+platform String
+backend String
+region_supported bool
+fullscreen_supported bool
+window_supported bool
}
class MissingDependencyInfo {
+tool String
+required_for String
+install_command Option~String~
+workaround Option~String~
}
class CommandError {
+code String
+title String
+message String
+missing_dependencies Vec~MissingDependencyInfo~
}
class CommandsModule {
+capture(request CaptureCommandRequest) CommandResult~CaptureCommandResponse~
+load_settings() CommandResult~Settings~
+save_settings(settings Settings) CommandResult~()~
+default_settings() Settings
+doctor_report() String
+capture_backend_info() CaptureBackendInfo
+capture_inner(request CaptureCommandRequest) AppResult~CaptureCommandResponse~
}
class CaptureExecution {
+mode CaptureMode
+capture CaptureResult
+copied_to_clipboard bool
+saved_path Option~PathBuf~
}
class CaptureServiceModule {
+run(mode CaptureMode, settings Settings) AppResult~CaptureExecution~
}
class SettingsServiceModule {
+load_or_default() AppResult~Settings~
+save(settings Settings) AppResult~()~
+default_settings() Settings
}
class DiagnosticsModule {
+doctor_report() String
}
class PlatformCaptureModule {
+platform_name() str
+backend_name() str
+mode_supported(mode CaptureMode) bool
}
class AppError {
+code() String
+title() String
+feedback_body() String
+missing_dependencies() Option~MissingDependenciesError~
}
class MissingDependenciesError {
+items Vec~MissingDependency~
}
class MissingDependency {
+tool String
+required_for String
+install_command Option~String~
+workaround Option~String~
}
class AppWindowRun {
+run(capture CaptureResult, settings Settings, current_mode CaptureMode)
}
class EditorCanvas {
+render_png() Result~Vec_u8~
+mark_saved()
}
class AppWindowSaveFlow {
+prompt_save_as(canvas EditorCanvas, settings Settings, status_label Label, app Application, current_mode CaptureMode)
}
CommandsModule --> CaptureServiceModule : capture_inner uses run
CommandsModule --> SettingsServiceModule : load_settings save_settings default_settings
CommandsModule --> DiagnosticsModule : doctor_report
CommandsModule --> PlatformCaptureModule : capture_backend_info
CommandError .. CommandResult
CommandError <-- AppError : From<AppError>
CommandError --> MissingDependencyInfo : contains
MissingDependencyInfo <-- MissingDependency : converted from
AppWindowRun --> Settings : reads close_after_save
AppWindowSaveFlow --> Settings : reads close_after_save
Settings ..> SettingsServiceModule : persisted via save
CaptureServiceModule --> CaptureModule : uses capture
Class diagram for diagnostics and tray repair status changesclassDiagram
class PortalRepairResult {
+attempted bool
+repaired bool
+message String
}
class DiagnosticsModule {
+repair_portals() PortalRepairResult
+doctor_report() String
+colorize_doctor_report(report str) String
}
class TrayLauncher {
+run_launcher(settings Settings) AppResult~()~
}
TrayLauncher --> DiagnosticsModule : repair_portals
class TrayRepairStatusFlow {
+update_status_label(result PortalRepairResult)
}
TrayRepairStatusFlow --> PortalRepairResult : reads attempted repaired message
class ColorizedDoctorReport {
+colorize_doctor_report(report str) String
}
ColorizedDoctorReport --> DiagnosticsModule : formats doctor report
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR refactors the capture system to use pluggable backend abstractions, adds a "Close After Save" UI toggle for batch automation workflows, introduces a serializable command API layer for future Tauri IPC integration, and makes minor diagnostic and canvas improvements. ChangesCapture System Abstraction
Service Commands API
Close After Save Feature
Miscellaneous Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR spans multiple independent feature areas (capture abstraction, service API, UI toggle) with significant structural changes across the capture subsystem, introduces new trait abstractions and implementations, adds a new service layer with serialization contracts, and requires understanding the temporal flow of refactoring to verify correct delegation and test coverage. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Now that capture is abstracted behind
CaptureBackend, the user-facing error contexts incapture::fullscreen/windowstill hard-codegrim/Hyprland in their messages; consider updating these strings (or deriving them from the backend) so they stay accurate if a different backend is plugged in. - In
platform::capture,platform_name,backend_name, andmode_supportedare currently hard-coded for the linux/grim-hyprland backend and marked#[allow(dead_code)]; if these are intended as a stable capability surface (e.g., forcommands), it may be clearer to remove thedead_codeallowances and/or gate them withcfgso they don’t accidentally get out of sync when additional platforms/backends are added.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that capture is abstracted behind `CaptureBackend`, the user-facing error contexts in `capture::fullscreen`/`window` still hard-code `grim`/Hyprland in their messages; consider updating these strings (or deriving them from the backend) so they stay accurate if a different backend is plugged in.
- In `platform::capture`, `platform_name`, `backend_name`, and `mode_supported` are currently hard-coded for the linux/grim-hyprland backend and marked `#[allow(dead_code)]`; if these are intended as a stable capability surface (e.g., for `commands`), it may be clearer to remove the `dead_code` allowances and/or gate them with `cfg` so they don’t accidentally get out of sync when additional platforms/backends are added.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
.gitignore (1)
41-41: ⚡ Quick winThese
.gitignorepatterns are unnecessary for this project.This repository is a Rust and Go project (evidenced by
Cargo.lock,/target/, and Go build patterns). Neitherpackage.jsonnorbun.lockexist in the repository, making these patterns no-ops. The patterns in the "Local agent/tooling installs" section appear to be preemptively added or carried over from another project's configuration.If Node.js tooling is genuinely needed for local development, keep these patterns. Otherwise, consider removing the unnecessary entries to keep
.gitignoreclean and relevant.🤖 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 @.gitignore at line 41, Remove the unnecessary Node.js ignore patterns from the ".gitignore": locate the "Local agent/tooling installs" section and delete the entries for "/package.json" and any "bun.lock" (or other Node-specific) patterns since this repo is Rust/Go and those files don't exist; if Node tooling is intended for local dev, instead add a brief comment explaining why these Node entries are present so future readers understand their purpose.src/editor/canvas.rs (1)
529-541: ⚡ Quick winAdd a
#[cfg(test)]unit test for the unpremultiplication logic.The unpremultiply closure refactoring changes the behaviour of the save path (
render_png→cairo_surface_to_rgba_image). Per the coding guidelines, focused unit tests should be added inline for save-path logic changes.At minimum, three representative cases should be exercised: fully transparent (
a == 0), fully opaque (a == 255, channels pass through unchanged), and a partial-alpha pixel to confirm thechannel * 255 / alphaformula.✅ Suggested inline test skeleton
+#[cfg(test)] +mod tests { + use super::*; + use cairo::{Format, ImageSurface}; + + fn make_argb32_surface(b: u8, g: u8, r: u8, a: u8) -> ImageSurface { + let stride = Format::ARgb32.stride_for_width(1).unwrap(); + let mut data = vec![0u8; stride as usize]; + data[0] = b; + data[1] = g; + data[2] = r; + data[3] = a; + ImageSurface::create_for_data(data, Format::ARgb32, 1, 1, stride).unwrap() + } + + #[test] + fn unpremultiplies_fully_transparent_pixel() { + let mut surface = make_argb32_surface(0, 0, 0, 0); + let img = cairo_surface_to_rgba_image(&mut surface).unwrap(); + assert_eq!(img.get_pixel(0, 0).0, [0, 0, 0, 0]); + } + + #[test] + fn unpremultiplies_fully_opaque_pixel() { + // Premultiplied opaque red: b=0, g=0, r=200, a=255 + let mut surface = make_argb32_surface(0, 0, 200, 255); + let img = cairo_surface_to_rgba_image(&mut surface).unwrap(); + assert_eq!(img.get_pixel(0, 0).0, [200, 0, 0, 255]); + } + + #[test] + fn unpremultiplies_partial_alpha_pixel() { + // Premultiplied 50% transparent red: r_pre = 128 (≈ 255*0.5), a = 128 + // Expected straight r ≈ 255 + let mut surface = make_argb32_surface(0, 0, 128, 128); + let img = cairo_surface_to_rgba_image(&mut surface).unwrap(); + let [r, g, b, a] = img.get_pixel(0, 0).0; + assert_eq!([g, b, a], [0, 0, 128]); + assert!(r >= 254, "unpremultiplied r should be ~255, got {r}"); + } +}As per coding guidelines: "Write focused Rust unit tests for parser behavior, save-path logic, and error recovery when changing those flows."
🤖 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 `@src/editor/canvas.rs` around lines 529 - 541, Add a focused #[cfg(test)] unit test for the unpremultiplication logic used in cairo_surface_to_rgba_image: cover the three cases (a == 0 returns 0 for channels, a == 255 passes channels through unchanged, and a representative partial-alpha case verifying channel * 255 / alpha rounding/limits). If the unpremultiply closure is not directly callable from tests, extract it to a small private helper fn (e.g., unpremultiply_channel(channel: u16, a: u16) -> u8) in the same module so tests can call it deterministically; add test functions under #[cfg(test)] mod tests that assert expected outputs for the three cases.src/platform/linux/hyprctl.rs (1)
38-41: 💤 Low valueConsider removing the
#[allow(dead_code)]free-function wrapper.
active_window_geometry()is unreachable frommainin this binary crate and the#[allow(dead_code)]just papers over that. If no external consumer needs it, removing the wrapper is simpler than suppressing the lint; if future callers are expected, a short doc-comment explaining the intent would make the suppression self-evident.🗑️ Proposed removal (if no planned consumers)
-#[allow(dead_code)] -pub fn active_window_geometry() -> Result<String> { - HyprctlCli.active_window_geometry() -}🤖 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 `@src/platform/linux/hyprctl.rs` around lines 38 - 41, The free-function wrapper active_window_geometry() is marked with #[allow(dead_code)] but is unused in the binary; remove the wrapper to avoid hiding dead-code with an attribute. Delete the pub fn active_window_geometry() wrapper and leave callers to call HyprctlCli::active_window_geometry() (or add a short doc-comment on the wrapper if you intend it to be a public API), ensuring no external references rely on the removed symbol before committing.src/platform/linux/grim.rs (1)
50-58: 💤 Low valueConsider removing the
#[allow(dead_code)]free-function wrappers.Both
capture_regionandcapture_fullscreenare suppressed dead code. Same pattern asactive_window_geometry()inhyprctl.rs— if there are no planned callers, removing them eliminates the suppression noise. If they're intentionally kept as an ergonomic fallback API, a short doc-comment is worth adding to clarify intent.🗑️ Proposed removal (if no planned consumers)
-#[allow(dead_code)] -pub fn capture_region(geometry: &str) -> Result<Vec<u8>> { - GrimCli.capture_region(geometry) -} - -#[allow(dead_code)] -pub fn capture_fullscreen() -> Result<Vec<u8>> { - GrimCli.capture_fullscreen() -}🤖 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 `@src/platform/linux/grim.rs` around lines 50 - 58, The two free-function wrappers capture_region and capture_fullscreen in grim.rs are annotated with #[allow(dead_code)] and should either be removed if unused or documented if kept as a public ergonomic fallback: delete the entire functions (including their #[allow(dead_code)] annotations and bodies) when there are no planned callers, or replace the #[allow(dead_code)] with a short doc-comment above capture_region and capture_fullscreen explaining they are retained as an ergonomic API surface (e.g., "Ergonomic fallback wrappers around GrimCli methods for callers that prefer free functions") to justify keeping them.src/capture/fullscreen.rs (1)
5-8: ⚡ Quick winMake the error context backend-neutral.
This path is now backend-agnostic, but the message still hardcodes
grim, so the first alternate backend will emit misleading diagnostics.♻️ Suggested cleanup
pub fn capture<B: CaptureBackend>(backend: &B) -> Result<CaptureResult> { let png_data = backend .capture_fullscreen() - .context("grim failed to capture fullscreen")?; + .context("capture backend failed to capture fullscreen")?; Ok(CaptureResult { png_data }) }🤖 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 `@src/capture/fullscreen.rs` around lines 5 - 8, The error context in capture() is backend-specific ("grim failed..."); update the context to be backend-neutral so any CaptureBackend implementation generates accurate diagnostics. In the function capture with the CaptureBackend generic and its capture_fullscreen() call, replace the hardcoded "grim failed to capture fullscreen" context with a generic message like "failed to capture fullscreen" (or include the backend type name if available) so errors remain accurate for all backends.src/capture/window.rs (1)
34-38: ⚡ Quick winAssert the forwarded geometry, not just the call order.
FakeBackend::capture_regionignores itsgeometryargument today, so this test would still pass ifcapture()sent an empty or stale string. Recording the received geometry closes the main regression gap in this abstraction change.♻️ Suggested test tightening
struct FakeBackend { calls: RefCell<Vec<&'static str>>, + seen_geometry: RefCell<Option<String>>, geometry_result: Option<String>, region_result: Option<Vec<u8>>, } @@ - fn capture_region(&self, _geometry: &str) -> Result<Vec<u8>> { + fn capture_region(&self, geometry: &str) -> Result<Vec<u8>> { self.calls.borrow_mut().push("capture_region"); + *self.seen_geometry.borrow_mut() = Some(geometry.to_string()); self.region_result .clone() .ok_or_else(|| anyhow!("region capture failed")) } @@ assert_eq!(result.png_data, vec![9, 8, 7]); assert_eq!( backend.calls.borrow().as_slice(), &["active_window_geometry", "capture_region"] ); + assert_eq!(backend.seen_geometry.borrow().as_deref(), Some("5,6 7x8")); }Also applies to: 49-63
🤖 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 `@src/capture/window.rs` around lines 34 - 38, FakeBackend::capture_region currently ignores its geometry argument so tests only verify call order; update the fake to record the forwarded geometry and assert it in tests. Modify FakeBackend::capture_region to push or store the received geometry string (e.g., add a geometry_received field or push a "capture_region:<geometry>" into calls) instead of only pushing "capture_region", and update the tests that call capture() (and the similar cases around the other region lines) to assert the recorded geometry equals the expected value; ensure capture_region still returns region_result as before if present.src/capture/mod.rs (1)
63-84: ⚡ Quick winMissing test for
CaptureMode::Region.
FullscreenandWindowpaths are covered, butRegionis not. Per the coding guidelines, capture-flow changes warrant a test for every changed path.region::capture(backend)callsbackend.capture_fullscreen()(as shown insrc/capture/region.rs), so the test is straightforward.✅ Proposed additional test
+ #[test] + fn region_capture_uses_fullscreen_backend_for_screenshot() { + let backend = FakeBackend::default(); + + let capture = capture_with_backend(CaptureMode::Region, &backend).unwrap(); + + assert_eq!(capture.png_data, vec![1, 2, 3]); + assert_eq!(backend.calls.borrow().as_slice(), &["capture_fullscreen"]); + }As per coding guidelines: "Write focused Rust unit tests for parser behavior, save-path logic, and error recovery when changing those flows."
🤖 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 `@src/capture/mod.rs` around lines 63 - 84, Add a unit test for the CaptureMode::Region path: create a FakeBackend, call capture_with_backend(CaptureMode::Region, &backend).unwrap(), assert capture.png_data equals the expected fullscreen bytes (same as fullscreen test) and assert backend.calls.borrow().as_slice() contains the single call "capture_fullscreen" (or the exact call name used by region::capture). Place this alongside the existing fullscreen/window tests and follow their naming/structure (e.g., fn region_capture_uses_fullscreen_backend()).
🤖 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 @.gitignore:
- Line 42: Remove the /bun.lock entry from .gitignore (or, if there is a
deliberate reason to exclude it, add a short comment above that line explaining
why it must remain ignored) so that the bun.lock lockfile is committed for
reproducible builds; reference the existing /bun.lock ignore entry to locate and
update the file.
In `@src/app/window.rs`:
- Around line 285-298: The save click handler's new close-after-save branch
isn't covered by tests; extract the post-save behavior into a small helper
function (e.g., handle_post_save or post_save_actions) that takes the saved
path, cfg (or the close_after_save bool) and app handle, move the logic that
calls c.mark_saved(), export::maybe_open_saved_path(), sets status_label text
and calls app.quit() into that helper, and then update the
save_btn.connect_clicked closure to call this helper; add #[cfg(test)] unit
tests next to the helper that exercise both branches (close_after_save = true
and false), verifying that mark_saved and maybe_open_saved_path are invoked and
that app.quit() would be triggered only when expected (use a testable app stub
or flag to observe quit).
In `@src/platform/capture.rs`:
- Around line 10-12: Guard the linux module declaration with conditional
compilation (e.g., add #[cfg(target_os = "linux")] to the pub mod linux; line in
src/platform/mod.rs) and then update src/platform/capture.rs so
current_backend() has two platform-specific implementations: the existing one
compiled only on linux (#[cfg(target_os = "linux")] pub fn current_backend() ->
impl CaptureBackend { crate::platform::linux::backend::current_backend() }) and
a #[cfg(not(target_os = "linux"))] fallback implementation that returns a
suitable non-Linux backend or stub (implement or call your project’s
Noop/Default/Fallback CaptureBackend) so builds on non-Linux targets succeed
without pulling Linux-only dependencies.
In `@src/services/capture.rs`:
- Around line 37-40: Add a rust-version = "1.81" entry to the project's
Cargo.toml so toolchains prior to Rust 1.81 (which don't support the stabilized
#[expect] attribute used in src/services/capture.rs) will fail with a clear
message; update the top-level [package] section to include rust-version = "1.81"
to declare the MSRV requirement.
---
Nitpick comments:
In @.gitignore:
- Line 41: Remove the unnecessary Node.js ignore patterns from the ".gitignore":
locate the "Local agent/tooling installs" section and delete the entries for
"/package.json" and any "bun.lock" (or other Node-specific) patterns since this
repo is Rust/Go and those files don't exist; if Node tooling is intended for
local dev, instead add a brief comment explaining why these Node entries are
present so future readers understand their purpose.
In `@src/capture/fullscreen.rs`:
- Around line 5-8: The error context in capture() is backend-specific ("grim
failed..."); update the context to be backend-neutral so any CaptureBackend
implementation generates accurate diagnostics. In the function capture with the
CaptureBackend generic and its capture_fullscreen() call, replace the hardcoded
"grim failed to capture fullscreen" context with a generic message like "failed
to capture fullscreen" (or include the backend type name if available) so errors
remain accurate for all backends.
In `@src/capture/mod.rs`:
- Around line 63-84: Add a unit test for the CaptureMode::Region path: create a
FakeBackend, call capture_with_backend(CaptureMode::Region, &backend).unwrap(),
assert capture.png_data equals the expected fullscreen bytes (same as fullscreen
test) and assert backend.calls.borrow().as_slice() contains the single call
"capture_fullscreen" (or the exact call name used by region::capture). Place
this alongside the existing fullscreen/window tests and follow their
naming/structure (e.g., fn region_capture_uses_fullscreen_backend()).
In `@src/capture/window.rs`:
- Around line 34-38: FakeBackend::capture_region currently ignores its geometry
argument so tests only verify call order; update the fake to record the
forwarded geometry and assert it in tests. Modify FakeBackend::capture_region to
push or store the received geometry string (e.g., add a geometry_received field
or push a "capture_region:<geometry>" into calls) instead of only pushing
"capture_region", and update the tests that call capture() (and the similar
cases around the other region lines) to assert the recorded geometry equals the
expected value; ensure capture_region still returns region_result as before if
present.
In `@src/editor/canvas.rs`:
- Around line 529-541: Add a focused #[cfg(test)] unit test for the
unpremultiplication logic used in cairo_surface_to_rgba_image: cover the three
cases (a == 0 returns 0 for channels, a == 255 passes channels through
unchanged, and a representative partial-alpha case verifying channel * 255 /
alpha rounding/limits). If the unpremultiply closure is not directly callable
from tests, extract it to a small private helper fn (e.g.,
unpremultiply_channel(channel: u16, a: u16) -> u8) in the same module so tests
can call it deterministically; add test functions under #[cfg(test)] mod tests
that assert expected outputs for the three cases.
In `@src/platform/linux/grim.rs`:
- Around line 50-58: The two free-function wrappers capture_region and
capture_fullscreen in grim.rs are annotated with #[allow(dead_code)] and should
either be removed if unused or documented if kept as a public ergonomic
fallback: delete the entire functions (including their #[allow(dead_code)]
annotations and bodies) when there are no planned callers, or replace the
#[allow(dead_code)] with a short doc-comment above capture_region and
capture_fullscreen explaining they are retained as an ergonomic API surface
(e.g., "Ergonomic fallback wrappers around GrimCli methods for callers that
prefer free functions") to justify keeping them.
In `@src/platform/linux/hyprctl.rs`:
- Around line 38-41: The free-function wrapper active_window_geometry() is
marked with #[allow(dead_code)] but is unused in the binary; remove the wrapper
to avoid hiding dead-code with an attribute. Delete the pub fn
active_window_geometry() wrapper and leave callers to call
HyprctlCli::active_window_geometry() (or add a short doc-comment on the wrapper
if you intend it to be a public API), ensuring no external references rely on
the removed symbol before committing.
🪄 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: 3c666ec2-1c34-4185-8906-3e9818e0dbd9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.gitignoreCargo.tomlsrc/app/tray.rssrc/app/window.rssrc/capture/fullscreen.rssrc/capture/mod.rssrc/capture/region.rssrc/capture/window.rssrc/diagnostics/mod.rssrc/editor/canvas.rssrc/main.rssrc/platform/capture.rssrc/platform/linux/backend.rssrc/platform/linux/grim.rssrc/platform/linux/hyprctl.rssrc/platform/linux/mod.rssrc/platform/mod.rssrc/services/capture.rssrc/services/commands.rssrc/services/mod.rssrc/settings/config.rs
| /.claude/ | ||
| /node_modules/ | ||
| /package.json | ||
| /bun.lock |
There was a problem hiding this comment.
Reconsider ignoring bun.lock.
Lock files like bun.lock ensure that all developers and CI/CD pipelines use identical dependency versions, which is critical for reproducible builds. Ignoring it can lead to "works on my machine" issues and inconsistent behavior across environments.
Best practice is to commit lock files to version control. If there's a specific reason to ignore it (e.g., it's generated by a local development tool and not part of the build process), please document that reasoning.
🔧 Proposed fix
/.agents/
/.claude/
/node_modules/
-/package.json
-/bun.lock🤖 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 @.gitignore at line 42, Remove the /bun.lock entry from .gitignore (or, if
there is a deliberate reason to exclude it, add a short comment above that line
explaining why it must remain ignored) so that the bun.lock lockfile is
committed for reproducible builds; reference the existing /bun.lock ignore entry
to locate and update the file.
Comment on lines
+285
to
+298
| let app = app.clone(); | ||
| save_btn.connect_clicked(move |_| { | ||
| if let Ok(png) = c.render_png() { | ||
| let cfg = s.borrow(); | ||
| if let Ok(path) = export::save_capture(&png, &cfg, current_mode) { | ||
| let close_after_save = cfg.close_after_save; | ||
| c.mark_saved(); | ||
| let _ = export::maybe_open_saved_path(&path, &cfg); | ||
| status_label | ||
| .set_text(&format!("Saved annotated image to {}.", path.display())); | ||
| eprintln!("Saved annotated image: {}", path.display()); | ||
| if close_after_save { | ||
| app.quit(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Please add coverage for the new close-after-save branches.
Both save paths now have extra quit behavior, but there’s no inline regression coverage for either branch. Even a small extracted post-save helper would make this much easier to test and would keep future save-flow changes safer. As per coding guidelines, "Add Rust unit tests inline under #[cfg(test)] blocks near the code they validate" and "Write focused Rust unit tests for parser behavior, save-path logic, and error recovery when changing those flows".
Also applies to: 803-845
🤖 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 `@src/app/window.rs` around lines 285 - 298, The save click handler's new
close-after-save branch isn't covered by tests; extract the post-save behavior
into a small helper function (e.g., handle_post_save or post_save_actions) that
takes the saved path, cfg (or the close_after_save bool) and app handle, move
the logic that calls c.mark_saved(), export::maybe_open_saved_path(), sets
status_label text and calls app.quit() into that helper, and then update the
save_btn.connect_clicked closure to call this helper; add #[cfg(test)] unit
tests next to the helper that exercise both branches (close_after_save = true
and false), verifying that mark_saved and maybe_open_saved_path are invoked and
that app.quit() would be triggered only when expected (use a testable app stub
or flag to observe quit).
Comment on lines
+10
to
+12
| pub fn current_backend() -> impl CaptureBackend { | ||
| crate::platform::linux::backend::current_backend() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the linux platform module is conditionally compiled
rg -n 'cfg.*target_os.*linux' --type rust -B2 -A2
# Also check how platform::linux is declared/re-exported
rg -n 'mod linux' --type rust -B1 -A3Repository: remcostoeten/kiekje
Length of output: 139
🏁 Script executed:
cat -n src/platform/mod.rsRepository: remcostoeten/kiekje
Length of output: 109
🏁 Script executed:
# Check if the linux directory/files have inline cfg guards
find src/platform/linux -name "*.rs" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -10 {}'Repository: remcostoeten/kiekje
Length of output: 1508
🏁 Script executed:
# Check if current_backend is currently callable and used, and on what platforms
rg -n 'current_backend' --type rust -B2 -A2Repository: remcostoeten/kiekje
Length of output: 880
🏁 Script executed:
# Search more broadly for any cfg attributes in mod.rs files
rg -n '#\[cfg' src/platform/ --type rustRepository: remcostoeten/kiekje
Length of output: 250
🏁 Script executed:
# Check project scope in Cargo.toml
cat -n Cargo.toml | head -30Repository: remcostoeten/kiekje
Length of output: 1042
🏁 Script executed:
# Look for any platform-specific code structure (e.g., different backends)
ls -la src/platform/Repository: remcostoeten/kiekje
Length of output: 345
The linux module declaration in src/platform/mod.rs is not conditionally compiled.
The code currently exposes pub mod linux; unconditionally on line 2 of src/platform/mod.rs. This means the entire Linux platform module—including platform-specific dependencies like grim, hyprctl, and system bindings—is available on all targets. To support the multi-platform abstraction, the module declaration itself must be guarded, and then the fallback in current_backend() should handle other platforms.
🔧 Proposed fix
In src/platform/mod.rs:
pub mod capture;
+#[cfg(target_os = "linux")]
pub mod linux;In src/platform/capture.rs:
+#[cfg(target_os = "linux")]
pub fn current_backend() -> impl CaptureBackend {
crate::platform::linux::backend::current_backend()
}
+
+#[cfg(not(target_os = "linux"))]
+compile_error!("No capture backend is available for this platform.");🤖 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 `@src/platform/capture.rs` around lines 10 - 12, Guard the linux module
declaration with conditional compilation (e.g., add #[cfg(target_os = "linux")]
to the pub mod linux; line in src/platform/mod.rs) and then update
src/platform/capture.rs so current_backend() has two platform-specific
implementations: the existing one compiled only on linux (#[cfg(target_os =
"linux")] pub fn current_backend() -> impl CaptureBackend {
crate::platform::linux::backend::current_backend() }) and a #[cfg(not(target_os
= "linux"))] fallback implementation that returns a suitable non-Linux backend
or stub (implement or call your project’s Noop/Default/Fallback CaptureBackend)
so builds on non-Linux targets succeed without pulling Linux-only dependencies.
Comment on lines
+37
to
+40
| #[expect( | ||
| clippy::too_many_arguments, | ||
| reason = "test hook injection keeps the production workflow free of trait-object plumbing" | ||
| )] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if Cargo.toml exists and view its current contents
cat Cargo.toml | head -30Repository: remcostoeten/kiekje
Length of output: 832
🏁 Script executed:
# Check if src/services/capture.rs exists and view lines 37-40
if [ -f "src/services/capture.rs" ]; then
echo "File exists. Viewing lines 37-40:"
sed -n '37,40p' src/services/capture.rs
else
echo "File not found at src/services/capture.rs"
# Try to find it
find . -name "capture.rs" -type f
fiRepository: remcostoeten/kiekje
Length of output: 237
🌐 Web query:
Rust #[expect] macro stabilization version release
💡 Result:
Rust’s #[expect] macro/attribute for lints (from RFC 2383 “lint_reasons”) was stabilized in Rust 1.81.0.[1][2] The Rust 1.81.0 release blog states that “1.81 stabilizes … a new lint level, expect” and introduces #[expect(clippy::…)] for expecting a lint to occur, warning if it doesn’t.[1] The official Rust 1.81.0 release notes also include “Stabilize #[expect] for lints (RFC 2383)”.[2] For context: the stabilization work corresponds to the “Let’s #[expect] some lints: Stabilize lint_reasons” PR, and that PR lists milestone 1.81.0.[3]
Citations:
- 1: https://blog.rust-lang.org/2024/09/05/Rust-1.81.0/
- 2: https://github.com/rust-lang/rust/releases/tag/1.81.0
- 3: Let's
#[expect]some lints: Stabilizelint_reasons(RFC 2383) rust-lang/rust#120924
Add rust-version to Cargo.toml to declare Rust 1.81 requirement.
#[expect] was stabilized in Rust 1.81 and allows explicitly noting that a particular lint should occur, warning if it doesn't. The project currently has no rust-version key in Cargo.toml, so contributors using a toolchain older than 1.81 will get an opaque compile error. Pinning the MSRV makes the requirement explicit.
📌 Proposed addition to `Cargo.toml`
edition = "2021"
+rust-version = "1.81"🤖 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 `@src/services/capture.rs` around lines 37 - 40, Add a rust-version = "1.81"
entry to the project's Cargo.toml so toolchains prior to Rust 1.81 (which don't
support the stabilized #[expect] attribute used in src/services/capture.rs) will
fail with a clear message; update the top-level [package] section to include
rust-version = "1.81" to declare the MSRV requirement.
Resolves merge conflicts in capture module files by adopting master's &dyn CaptureBackend approach while preserving the generic GrimHyprlandBackend from the feature branch. Removes the redundant platform/capture.rs abstraction layer since CaptureBackend now lives directly in capture/mod.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
Describe what this PR changes and why.
Testing
cargo check --all-targets --all-featurescargo fmt --all -- --check(cd cmd/screeny-tui && go test ./...)(cd cmd/screeny-tui && go vet ./...)Checklist
Summary by Sourcery
Introduce a pluggable capture backend abstraction for Linux, extend capture services with a serializable command API, and add new user options around post-save behavior.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Chores