New recording flow #831
New recording flow #831
Conversation
|
Warning Rate limit exceeded@Brendonovich has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 14 seconds before requesting another review. ⌛ 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 (5)
WalkthroughAdds a cross-platform target-select overlay system: new Tauri commands/events/types, per-display overlay windows with cursor tracking and Escape handling, display/window APIs in a new displays crate, a SolidJS overlay and new recording flow UI, settings and binding updates, plus dependency and minor UI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend (overlay / new-main)
participant Tauri as Tauri Runtime
participant Disp as cap-displays
participant Win as Window Manager
participant GS as Global Shortcut
UI->>Tauri: open_target_select_overlays()
Tauri->>Disp: enumerate displays
loop per display
Tauri->>Win: create TargetSelectOverlay window (display_id)
Tauri->>Tauri: spawn cursor-tracking task -> emit TargetUnderCursor
Tauri->>GS: register Escape shortcut (once)
end
UI-->>Tauri: close_target_select_overlays() or OnEscapePress
Tauri->>Win: close overlay windows
Tauri->>GS: unregister Escape when no overlays remain
sequenceDiagram
participant OS as OS Keyboard
participant GS as Global Shortcut
participant Tauri as Tauri Runtime
participant UI as Frontend Overlay
OS-->>GS: Escape pressed
GS->>Tauri: deliver Escape event
Tauri->>UI: emit OnEscapePress
UI->>Tauri: request close_target_select_overlays()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Suggested labels
Poem
✨ 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 (
|
… into new-recording-flow
… into new-recording-flow
|
Merging with the experimental feature toggled, until recording mode selection is resolved + new design is settled on. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/desktop/src-tauri/src/general_settings.rs (1)
83-87: Fix incorrect serde predicate and formalize deprecations
- The
skip_serializing_if = "no"predicate never skips serialization becausenoalways returnsfalse. If the intent is to skip when the value isfalse, the predicate must check the value.- The deprecation notes are comments. Use Rust’s
#[deprecated]attribute so the compiler warns on use.Apply this diff:
- // #[deprecated = "can be removed when native camera preview is ready"] + #[deprecated(note = "can be removed when native camera preview is ready")] #[serde( default = "default_enable_native_camera_preview", - skip_serializing_if = "no" + skip_serializing_if = "is_false" )] pub enable_native_camera_preview: bool, @@ - // #[deprecated = "can be removed when new recording flow is the default"] + #[deprecated(note = "can be removed when new recording flow is the default")] #[serde( default = "default_enable_new_recording_flow", - skip_serializing_if = "no" + skip_serializing_if = "is_false" )] pub enable_new_recording_flow: bool, @@ -fn no(_: &bool) -> bool { - false -} +fn is_false(value: &bool) -> bool { + !*value +}Also applies to: 91-96, 111-114
apps/desktop/src-tauri/src/windows.rs (1)
243-247: Replace magic window-level numbers with named constantsUse named constants to document intent and avoid “50”/“45” magic numbers. This was flagged previously.
Apply these diffs in-place:
- crate::platform::set_window_level(window.as_ref().window(), 50); + crate::platform::set_window_level(window.as_ref().window(), NEW_RECORDING_FLOW_WINDOW_LEVEL);- crate::platform::set_window_level(window.as_ref().window(), 45); + crate::platform::set_window_level(window.as_ref().window(), TARGET_SELECT_OVERLAY_WINDOW_LEVEL);Add the constants near the top of this file:
#[cfg(target_os = "macos")] const NEW_RECORDING_FLOW_WINDOW_LEVEL: i32 = 50; #[cfg(target_os = "macos")] const TARGET_SELECT_OVERLAY_WINDOW_LEVEL: i32 = 45;Also applies to: 283-285
🧹 Nitpick comments (4)
packages/ui-solid/src/auto-imports.d.ts (2)
62-63: Nit: Inconsistent quoting style (double vs single).The rest of the file uses single quotes, while these two lines use double quotes. If you prefer consistency (even in generated files), align them to single quotes or update the generator config.
Apply this diff if you choose to normalize:
- const IconIcBaselineMonitor: typeof import("~icons/ic/baseline-monitor.jsx")["default"] - const IconIcRoundSearch: typeof import("~icons/ic/round-search.jsx")["default"] + const IconIcBaselineMonitor: typeof import('~icons/ic/baseline-monitor.jsx')['default'] + const IconIcRoundSearch: typeof import('~icons/ic/round-search.jsx')['default']Note: since this file is generated, prefer fixing at the plugin/config level if possible.
86-90: Ergonomics: Extremely long/duplicated component names; consider aliasing.Declarations like
IconMaterialSymbolsLightScreenshotFrame2MaterialSymbolsScreenshotFrame2Roundedare verbose and contain duplicated segments. To keep JSX more readable, consider aliasing via a local module (or adjusting the icon resolver naming strategy) and importing those aliases where used.Example alias module outside the generated file (e.g., packages/ui-solid/src/icons.ts):
export { default as IconScreenshotFrameRounded } from '~icons/material-symbols/screenshot-frame2-rounded.jsx' export { default as IconScreenshotFrameLight } from '~icons/material-symbols-light/screenshot-frame2.jsx' export { default as IconClose } from '~icons/lucide/x.jsx' export { default as IconMonitor } from '~icons/mdi/monitor.jsx'Then use
<IconScreenshotFrameRounded />, etc., instead of relying on the long auto-imported globals. This keeps the generated d.ts intact while improving call sites.apps/desktop/src-tauri/src/general_settings.rs (1)
101-106: Replace TODO with explicit intent or tracked issueThe TODO suggests platform-conditional defaulting. Consider referencing an issue ID or adding a short rationale to avoid confusion later.
apps/desktop/src-tauri/src/windows.rs (1)
226-232: Nit: avoid redundant reference when fetching settingsPass
appdirectly;&appintroduces an extra borrow.- let new_recording_flow = GeneralSettingsStore::get(&app) + let new_recording_flow = GeneralSettingsStore::get(app) .ok() .flatten() .map(|s| s.enable_new_recording_flow) .unwrap_or_default();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/desktop/package.json(1 hunks)apps/desktop/src-tauri/Cargo.toml(1 hunks)apps/desktop/src-tauri/src/general_settings.rs(2 hunks)apps/desktop/src-tauri/src/hotkeys.rs(3 hunks)apps/desktop/src-tauri/src/lib.rs(10 hunks)apps/desktop/src-tauri/src/windows.rs(10 hunks)apps/desktop/src/routes/(window-chrome)/(main).tsx(2 hunks)apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx(3 hunks)apps/desktop/src/routes/(window-chrome)/settings/general.tsx(1 hunks)apps/desktop/src/utils/tauri.ts(11 hunks)crates/displays/src/platform/win.rs(2 hunks)crates/media/src/feeds/camera.rs(1 hunks)packages/ui-solid/src/auto-imports.d.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/desktop/package.json
- apps/desktop/src-tauri/Cargo.toml
- crates/media/src/feeds/camera.rs
- apps/desktop/src/routes/(window-chrome)/(main).tsx
- apps/desktop/src-tauri/src/hotkeys.rs
- apps/desktop/src/routes/(window-chrome)/settings/general.tsx
- crates/displays/src/platform/win.rs
- apps/desktop/src/utils/tauri.ts
- apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx
- apps/desktop/src-tauri/src/lib.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/desktop/src-tauri/src/windows.rs (5)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
cap_displays(51-54)crates/displays/src/lib.rs (5)
s(65-65)s(86-86)s(147-147)s(168-168)from_id(27-29)apps/desktop/src-tauri/src/lib.rs (4)
None(2428-2428)app(1190-1191)app(2183-2183)app(2211-2211)apps/desktop/src-tauri/src/general_settings.rs (1)
get(172-183)apps/desktop/src-tauri/src/platform/macos/mod.rs (1)
set_window_level(15-24)
apps/desktop/src-tauri/src/general_settings.rs (1)
apps/desktop/src/utils/tauri.ts (1)
PostDeletionBehaviour(394-394)
⏰ 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). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (14)
packages/ui-solid/src/auto-imports.d.ts (1)
60-60: LGTM: New global icon declaration is correct.The added
IconFa6SolidDisplaydeclaration matches the established pattern and path resolution.apps/desktop/src-tauri/src/general_settings.rs (2)
107-110: LGTM: sensible default for experimental flowDefaulting
enable_new_recording_flowtocfg!(debug_assertions)aligns with “experimental on dev, off on prod.”
150-153: LGTM: defaults wired through helper fnsUsing
default_enable_*helpers inDefaultis clean and consistent with serde defaults.apps/desktop/src-tauri/src/windows.rs (11)
4-10: LGTM: additional imports required for new overlay flowNew imports are consistent with added functionality.
31-31: LGTM: derive additions on CapWindowIdDeriving
Clone,Deserialize, andTypeis appropriate for routing and TS bridging.
40-40: LGTM: add TargetSelectOverlay variantThe new
CapWindowId::TargetSelectOverlay { display_id }cleanly represents per-display overlays.
76-81: LGTM: FromStr branch for target-select-overlayParsing
display_idviaDisplayId::from_stris straightforward; errors are handled.
98-100: LGTM: Display impl for target-select-overlayString format matches the FromStr branch, maintaining round-trip symmetry.
156-158: LGTM: exclude overlays from macOS traffic lightsCorrect to return
Nonefor overlays to avoid drawing traffic controls.
184-190: LGTM: ShowCapWindow variant for TargetSelectOverlayThis keeps the show-path aligned with the new CapWindowId variant.
194-205: LGTM: avoid duplicate editor windowsBorrowing
selfto accessproject_pathand tracking IDs prevents duplicates.
233-242: Confirm always-on-top + all-workspaces policy for MainSetting the Main window
.always_on_top(true)and.visible_on_all_workspaces(true)unconditionally may degrade UX (e.g., pinning across virtual desktops) and differs from the macOS-specific window-level handling used elsewhere.
- If the goal is Windows-only z-order behavior, consider gating:
.always_on_top(cfg!(target_os = "windows"))- or tie it to the feature flag.
- If intended globally, ignore this.
Proposed adjustment:
- .always_on_top(true) - .visible_on_all_workspaces(true) + .always_on_top(cfg!(target_os = "windows")) + .visible_on_all_workspaces(cfg!(target_os = "windows"))
253-289: LGTM: per-display overlay window configSizing/positioning to display bounds, per-display URL param, focus management spawn, and taskbar skipping look correct. macOS window-level handling is consistent with the rest of the codebase.
624-626: LGTM: id mapping for TargetSelectOverlayCorrectly maps ShowCapWindow to CapWindowId with cloned
display_id.
TODO:
MainandNewMaininto one window with different URL based on feature flagopen_target_select_overlaysinside needs a windows alternativeTauri window spawning in the incorrect sizeSeems to be working for me now when I switch back and forth withmainbut might come back with more testing?Escapekey from other applications?Summary by CodeRabbit
New Features
Improvements
Bug Fixes