New Cropper across the app #1079
Conversation
…move haptics toggle
|
Warning Rate limit exceeded@richiemcilroy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 49 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 Tauri permission for closing webviews; removes haptics from persisted settings and normalizes haptic enum JSON casing; makes two windows non-resizable and adjusts CaptureArea macOS level; replaces legacy canvas/box/CropAreaRenderer with a new Cropper API and updates dependent UI/routes; adds an error-boundary “Try Close” action and expands icon typings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CaptureAreaUI as CaptureArea UI
participant Cropper
participant MenuAPI as Menu(API)
participant MainWindow
rect rgb(245,248,255)
note over CaptureAreaUI: Initialize Cropper and per-display state
CaptureAreaUI->>Cropper: mount(ref, { initialCrop, aspectRatio, snapToRatio })
Cropper-->>CaptureAreaUI: onCropChange(bounds)
end
User->>CaptureAreaUI: Click Aspect Ratio / Options
CaptureAreaUI->>MenuAPI: createMenu(items from createCropOptionsMenuItems)
MenuAPI-->>User: Popup at cursor/anchor
User-->>MenuAPI: Select ratio / toggle snap
MenuAPI-->>CaptureAreaUI: Selection callback
CaptureAreaUI->>Cropper: setCropProperty / setCrop / animateTo
alt Confirm valid
User->>CaptureAreaUI: Confirm
CaptureAreaUI->>MainWindow: setCaptureAreaPending (with bounds)
else Cancel
User->>CaptureAreaUI: Cancel
CaptureAreaUI->>MainWindow: setCaptureAreaPending(false)
end
sequenceDiagram
autonumber
participant User
participant CapErrorBoundary
participant Webview as getCurrentWebviewWindow()
participant OSWindow as window
User->>CapErrorBoundary: Click "Try Close"
CapErrorBoundary->>Webview: close()
Webview-->>CapErrorBoundary: resolved
CapErrorBoundary->>OSWindow: window.close()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 8
🧹 Nitpick comments (6)
apps/desktop/src/routes/capture-area.tsx (1)
97-98: Drop stray debug logThe
console.log("Hei")slipped in from debugging. Please remove it before shipping.apps/desktop/src/routes/editor/Editor.tsx (1)
322-343: Consider extracting menu positioning logic.The menu positioning logic in
showCropOptionsMenucould be reused across the app for consistent menu placement.Consider extracting this into a utility function:
// In a utils file: export async function showMenuBelowElement(menu: Menu, element: HTMLElement, yOffset = 40) { const rect = element.getBoundingClientRect(); const pos = new LogicalPosition(rect.x, rect.y + yOffset); await menu.popup(pos); await menu.close(); }apps/desktop/src/components/Cropper.tsx (1)
298-307: Add more descriptive error messages.The error messages could be more helpful by including the actual constraint values.
Enhance the error messages:
-throw new Error( - `Cropper error: minSize.x (${min.x}) cannot be greater than maxSize.x (${max.x}).`, -); +throw new Error( + `Cropper constraint error: minSize.x (${min.x}px) exceeds maxSize.x (${max.x}px). Please adjust the size constraints.`, +);apps/desktop/src/routes/target-select-overlay.tsx (1)
439-443: Consider extracting the menu positioning logic.This menu positioning pattern is repeated. Consider creating a shared utility.
Extract to a shared utility as suggested in the Editor.tsx review to maintain consistency across the application.
apps/desktop/src-tauri/src/windows.rs (1)
571-575: macOS: Verify NSPopUpMenuWindowLevel is correct for CaptureArea stackingSwitching to NSPopUpMenuWindowLevel should place the capture area above normal windows but below critical system UI. Please verify stacking with camera, overlay, and menus to ensure no regressions across Spaces/mission control.
If crate::platform::set_window_level expects a numeric CGWindowLevel, confirm that passing objc2_app_kit::NSPopUpMenuWindowLevel matches the expected type on your target toolchain. If needed, cast explicitly.
apps/desktop/src/routes/window-capture-occluder.tsx (1)
8-13: Deduplicate window focusing logicgetAllWindows().then(...setFocus()) is called twice. Extract into a small helper or run once (e.g., onMount) to avoid repeated invokes on reactive updates.
Also applies to: 29-35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/desktop/src-tauri/capabilities/default.json(1 hunks)apps/desktop/src-tauri/src/general_settings.rs(0 hunks)apps/desktop/src-tauri/src/platform/mod.rs(2 hunks)apps/desktop/src-tauri/src/windows.rs(2 hunks)apps/desktop/src/components/CapErrorBoundary.tsx(2 hunks)apps/desktop/src/components/CropAreaRenderer.tsx(0 hunks)apps/desktop/src/components/Cropper.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/(main).tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/settings/general.tsx(0 hunks)apps/desktop/src/routes/capture-area.tsx(3 hunks)apps/desktop/src/routes/editor/ConfigSidebar.tsx(2 hunks)apps/desktop/src/routes/editor/Editor.tsx(6 hunks)apps/desktop/src/routes/target-select-overlay.tsx(7 hunks)apps/desktop/src/routes/window-capture-occluder.tsx(1 hunks)apps/desktop/src/utils/box.ts(0 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
💤 Files with no reviewable changes (4)
- apps/desktop/src/utils/box.ts
- apps/desktop/src/components/CropAreaRenderer.tsx
- apps/desktop/src/routes/(window-chrome)/settings/general.tsx
- apps/desktop/src-tauri/src/general_settings.rs
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: Do not manually import icons in the desktop app; rely on auto-imported icons
In the desktop app, use @tanstack/solid-query for server state management
Files:
apps/desktop/src/routes/window-capture-occluder.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/(main).tsxapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsxapps/desktop/src/components/Cropper.tsxapps/desktop/src/routes/capture-area.tsxapps/desktop/src/components/CapErrorBoundary.tsxapps/desktop/src/routes/editor/Editor.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src/routes/window-capture-occluder.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src-tauri/src/windows.rsapps/desktop/src/routes/(window-chrome)/(main).tsxpackages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src-tauri/src/platform/mod.rsapps/desktop/src/routes/editor/ConfigSidebar.tsxapps/desktop/src/components/Cropper.tsxapps/desktop/src/routes/capture-area.tsxapps/desktop/src/components/CapErrorBoundary.tsxapps/desktop/src/routes/editor/Editor.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/routes/window-capture-occluder.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/(main).tsxpackages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsxapps/desktop/src/components/Cropper.tsxapps/desktop/src/routes/capture-area.tsxapps/desktop/src/components/CapErrorBoundary.tsxapps/desktop/src/routes/editor/Editor.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/window-capture-occluder.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/(main).tsxpackages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsxapps/desktop/src/components/Cropper.tsxapps/desktop/src/routes/capture-area.tsxapps/desktop/src/components/CapErrorBoundary.tsxapps/desktop/src/routes/editor/Editor.tsx
**/tauri.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated IPC bindings file tauri.ts
Do not edit auto-generated files named
tauri.ts.
Files:
apps/desktop/src/utils/tauri.ts
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/platform/mod.rs
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:17:47.407Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-22T14:17:47.407Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Do not manually import icons in the desktop app; rely on auto-imported icons
Applied to files:
packages/ui-solid/src/auto-imports.d.ts
🧬 Code graph analysis (5)
apps/desktop/src/routes/target-select-overlay.tsx (3)
apps/desktop/src/components/Cropper.tsx (6)
CropBounds(21-26)CROP_ZERO(27-27)CropperRef(219-229)Ratio(74-74)createCropOptionsMenuItems(1476-1503)Cropper(231-1253)apps/desktop/src/store.ts (1)
generalSettingsStore(61-62)apps/desktop/src/utils/tauri.ts (1)
LogicalPosition(398-398)
apps/desktop/src/components/Cropper.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
commands(7-269)
apps/desktop/src/routes/capture-area.tsx (3)
apps/desktop/src/utils/tauri.ts (2)
DisplayId(368-368)LogicalPosition(398-398)apps/desktop/src/components/Cropper.tsx (6)
CropBounds(21-26)CropperRef(219-229)CROP_ZERO(27-27)Ratio(74-74)createCropOptionsMenuItems(1476-1503)Cropper(231-1253)apps/desktop/src/utils/queries.ts (1)
createOptionsQuery(91-128)
apps/desktop/src/components/CapErrorBoundary.tsx (1)
packages/ui-solid/src/Button.tsx (1)
Button(40-50)
apps/desktop/src/routes/editor/Editor.tsx (4)
apps/desktop/src/components/Cropper.tsx (6)
CropperRef(219-229)CROP_ZERO(27-27)Ratio(74-74)createCropOptionsMenuItems(1476-1503)CropBounds(21-26)Cropper(231-1253)apps/desktop/src/utils/tauri.ts (1)
LogicalPosition(398-398)apps/desktop/src/utils/composeEventHandlers.ts (1)
composeEventHandlers(20-28)apps/desktop/src/routes/editor/ui.tsx (1)
EditorButton(353-418)
⏰ 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: Vercel Agent Review
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (23)
packages/ui-solid/src/auto-imports.d.ts (1)
71-92: Icon typings update looks consistentThe new Lucide icon globals line up alphabetically with the existing block and follow the established import typing convention. LGTM.
apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
662-666: Verified matching emittersetCaptureAreaPending. Listener and emitter use the same channel; no action required.apps/desktop/src/routes/editor/Editor.tsx (3)
345-369: Good use of composition for NumberField inputs!The BoundInput component elegantly encapsulates the NumberField configuration and event handling, reducing duplication across the four input fields.
484-494: Verify crop bounds conversion consistency. Ensure bounds.x/y map correctly to position.x/y and bounds.width/height to size.x/y, matching the editor’s expected coordinate space.
2-2: Confirm NumberField import path The NumberField component exists in @kobalte/core v0.13 and the import from"@kobalte/core/number-field"is correct.apps/desktop/src/components/Cropper.tsx (8)
93-95: Platform-specific haptic feedback implementation looks good.The haptic feedback is correctly limited to macOS and uses the appropriate command interface.
175-187: Excellent handling of aspect ratio constraints!The code correctly calculates effective min/max sizes when an aspect ratio is enforced, preventing invalid size combinations.
267-273: Good animation frame management.The stopAnimation function properly cleans up animation frames and synchronizes display bounds.
595-626: Well-structured CropperRef implementation.The ref implementation provides a clean API surface with appropriate methods for external control. The conditional handling of function vs object refs is correct.
873-932: Comprehensive keyboard control implementation!The keyboard action loop correctly handles arrow key navigation with appropriate modifiers (Shift for larger steps, Meta/Ctrl for resizing, Alt for center-origin resize).
984-1011: Smart use of requestAnimationFrame for DOM updates.Using RAF for updating element styles prevents layout thrashing and ensures smooth visual updates. The cleanup of previous frame IDs prevents memory leaks.
1265-1338: Robust aspect ratio resize logic.The
computeAspectRatioResizefunction correctly handles aspect-constrained resizing with proper boundary checking and ratio maintenance.
1476-1503: Well-designed menu item creation utility.The
createCropOptionsMenuItemsfunction provides a clean interface for generating aspect ratio menu items with proper type safety.apps/desktop/src/routes/target-select-overlay.tsx (4)
292-292: Good platform-aware safe margin for macOS notch.The TOP_SAFE_MARGIN correctly accounts for the macOS notch with 40px margin while using 10px for other platforms.
294-331: Excellent controls positioning logic!The dynamic positioning algorithm intelligently places controls based on available space with clear priority: below > above > inside. The boundary checking prevents UI elements from being cut off.
248-271: Clean integration of crop options menu.The showCropOptionsMenu function properly integrates with the Cropper's menu system and handles the reset action appropriately.
355-364: Proper Cropper integration with all necessary callbacks.The Cropper is correctly configured with all required props and callbacks for the area selection mode.
apps/desktop/src-tauri/src/windows.rs (1)
543-543: Making CaptureArea non-resizable aligns with UX goalsSetting resizable(false) for the capture-area window matches the unified cropper behavior and prevents accidental window resizes. LGTM.
apps/desktop/src-tauri/src/platform/mod.rs (1)
13-14: Enum casing normalization via serde is correctAdding #[serde(rename_all = "camelCase")] aligns Rust enum variants with the updated TS string literals. This preserves TAURI invoke compatibility while keeping NS APIs fed with repr casts.
Confirm tauri-specta regenerated bindings (see tauri.ts) and there are no downstream string-case mismatches.
Also applies to: 23-24
apps/desktop/src/utils/tauri.ts (2)
378-378: GeneralSettingsStore type update acknowledgedhapticsEnabled removal is reflected here. Ensure any UI checks (like the Editor sidebar) no longer rely on it.
389-390: Haptic enums now lowercase — matches Rust serde camelCaseTypes switched to "alignment" | "levelChange" | "generic" and "default" | "now" | "drawCompleted". Ensure all call sites use lowercase (e.g., commands.performHapticFeedback("alignment","now")).
Also, please confirm this file is generated by tauri-specta and not manually edited, per repo policy. As per coding guidelines.
apps/desktop/src-tauri/capabilities/default.json (1)
27-27: Allowing resource close looks goodAdding core:resources:allow-close should unblock programmatic close flows (e.g., context menus). Validate on all windows where menus/resources are created to ensure no permission denials in logs.
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
898-899: Ignore async fix:ostype()is synchronous in plugin-os v2
type()returns anOsTypestring (e.g."macos") synchronously, sohapticsEnabledis correctly computed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/desktop/src/routes/capture-area.tsx (1)
98-107: Use the validated bounds for saving and optionsYou validate
currentBoundsbut then read fromcrop()which can drift. Use the same snapshot for consistency.Apply this diff:
- const b = crop(); + const b = currentBounds; @@ - position: { x: b.x, y: b.y }, - size: { width: b.width, height: b.height }, + position: { x: b.x, y: b.y }, + size: { width: b.width, height: b.height },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/capture-area.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: Do not manually import icons in the desktop app; rely on auto-imported icons
In the desktop app, use @tanstack/solid-query for server state management
Files:
apps/desktop/src/routes/capture-area.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src/routes/capture-area.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/routes/capture-area.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/capture-area.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/capture-area.tsx (3)
apps/desktop/src/utils/tauri.ts (2)
DisplayId(368-368)LogicalPosition(398-398)apps/desktop/src/components/Cropper.tsx (6)
CropBounds(21-26)CropperRef(219-229)CROP_ZERO(27-27)Ratio(74-74)createCropOptionsMenuItems(1476-1503)Cropper(231-1253)apps/desktop/src/utils/queries.ts (1)
createOptionsQuery(91-128)
⏰ 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: Vercel Agent Review
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (1)
apps/desktop/src/routes/capture-area.tsx (1)
59-66: Fix debounce misuse:createScheduledis invoked without a callback (Confirm can stay disabled)You’re calling
scheduled()with no callback and using its return as a boolean. That enqueues a noop and never re-computes validity. Port the debounce to wrap the validity computation and store it in a signal.Apply this diff:
-const scheduled = createScheduled((fn) => debounce(fn, 50)); - -const isValid = createMemo((p: boolean = true) => { - const b = crop(); - return scheduled() - ? b.width >= MIN_SIZE.width && b.height >= MIN_SIZE.height - : p; -}); +const scheduled = createScheduled((fn) => debounce(fn, 50)); +const [isValid, setIsValid] = createSignal(true); +createEffect(() => { + const b = crop(); + scheduled(() => { + setIsValid(b.width >= MIN_SIZE.width && b.height >= MIN_SIZE.height); + }); +});Also add the missing import:
-import { createMemo, createSignal, onCleanup, onMount, Show } from "solid-js"; +import { createEffect, createMemo, createSignal, onCleanup, onMount, Show } from "solid-js";
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/routes/window-capture-occluder.tsx (1)
8-13: Move focus side-effects out of render and dedupe callsCalling getAllWindows().setFocus() during render (twice) can cause flicker and unnecessary work. Run it once in onMount and remove the inner call.
-import { Show, Suspense } from "solid-js"; +import { Show, Suspense, onMount } from "solid-js"; @@ - getAllWindows().then((w) => - w.forEach((w) => { - if (w.label === "camera" || w.label === "in-progress-recording") - w.setFocus(); - }), - ); + onMount(() => { + getAllWindows().then((ws) => { + for (const w of ws) { + if (w.label === "camera" || w.label === "in-progress-recording") { + w.setFocus(); + } + } + }); + }); @@ - getAllWindows().then((w) => - w.forEach((w) => { - if (w.label === "camera" || w.label === "in-progress-recording") - w.setFocus(); - }), - ); + // focus handled in onMountAlso applies to: 29-34
🧹 Nitpick comments (7)
apps/desktop/src/routes/window-capture-occluder.tsx (1)
42-46: Remove inline comments per coding guidelinesInline JSX comments were added; the repository guidelines prohibit inline/block comments in TS/TSX/JS/JSX.
As per coding guidelines
- {/* Top blind */} @@ - {/* Bottom blind */} @@ - {/* Left blind */} @@ - {/* Right blind */}Also applies to: 47-51, 52-60, 61-69
apps/desktop/src/routes/editor/Editor.tsx (1)
354-361: Guard NumberField raw valueonRawValueChange can emit null/NaN during typing; setCropProperty expects a number. Guard to avoid propagating invalid values.
- onRawValueChange={(v) => { - cropperRef?.setCropProperty(props.field, v); - }} + onRawValueChange={(v) => { + if (typeof v === "number" && Number.isFinite(v)) { + cropperRef?.setCropProperty(props.field, v); + } + }}apps/desktop/src/components/Cropper.tsx (1)
112-116: Remove inline comments to conform with repo rulesNew inline comments were added; repo policy disallows inline/block/doc comments in TS/TSX/JS/JSX.
As per coding guidelines
Also applies to: 783-785, 1052-1056, 1063-1066, 1079-1087
apps/desktop/src/routes/target-select-overlay.tsx (4)
441-445: Use currentTarget and a generic Element cast for menu positioninge.target may be a child (e.g., SVG). Use currentTarget to position reliably.
- function showMenu(menu: Promise<Menu>, e: UIEvent) { + function showMenu(menu: Promise<Menu>, e: UIEvent) { e.stopPropagation(); - const rect = (e.target as HTMLDivElement).getBoundingClientRect(); + const rect = (e.currentTarget as Element).getBoundingClientRect(); menu.then((menu) => menu.popup(new LogicalPosition(rect.x, rect.y + 40))); }
247-249: Hide controls while interacting (state unused)shouldHideControls is set via onInteraction but not used. Apply it to opacity to avoid hover conflicts during drag/resize.
- class="fixed z-50 transition-opacity" + class="fixed z-50 transition-opacity" + classList={{ "opacity-0 pointer-events-none": shouldHideControls() }}Also applies to: 339-355
146-151: Remove inline comments per coding rulesInline comments were added; repo policy disallows them in TS/TSX/JS/JSX.
As per coding guidelines
Also applies to: 286-295
115-117: Keyboard default prevention is globalPreventing all keydown defaults on document can affect assistive tech and OS shortcuts. Consider scoping to relevant keys or element.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/components/Cropper.tsx(1 hunks)apps/desktop/src/routes/editor/Editor.tsx(6 hunks)apps/desktop/src/routes/target-select-overlay.tsx(6 hunks)apps/desktop/src/routes/window-capture-occluder.tsx(1 hunks)apps/desktop/src/utils/canvas.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/utils/canvas.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: Do not manually import icons in the desktop app; rely on auto-imported icons
In the desktop app, use @tanstack/solid-query for server state management
Files:
apps/desktop/src/routes/window-capture-occluder.tsxapps/desktop/src/components/Cropper.tsxapps/desktop/src/routes/editor/Editor.tsxapps/desktop/src/routes/target-select-overlay.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src/routes/window-capture-occluder.tsxapps/desktop/src/components/Cropper.tsxapps/desktop/src/routes/editor/Editor.tsxapps/desktop/src/routes/target-select-overlay.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/routes/window-capture-occluder.tsxapps/desktop/src/components/Cropper.tsxapps/desktop/src/routes/editor/Editor.tsxapps/desktop/src/routes/target-select-overlay.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/window-capture-occluder.tsxapps/desktop/src/components/Cropper.tsxapps/desktop/src/routes/editor/Editor.tsxapps/desktop/src/routes/target-select-overlay.tsx
🧬 Code graph analysis (4)
apps/desktop/src/routes/window-capture-occluder.tsx (1)
crates/scap-targets/src/bounds.rs (4)
position(15-17)position(42-44)size(19-21)size(46-48)
apps/desktop/src/components/Cropper.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
commands(7-269)
apps/desktop/src/routes/editor/Editor.tsx (4)
apps/desktop/src/components/Cropper.tsx (6)
CropperRef(219-229)CROP_ZERO(27-27)Ratio(74-74)createCropOptionsMenuItems(1476-1503)CropBounds(21-26)Cropper(231-1253)apps/desktop/src/utils/tauri.ts (1)
LogicalPosition(398-398)apps/desktop/src/utils/composeEventHandlers.ts (1)
composeEventHandlers(20-28)apps/desktop/src/routes/editor/ui.tsx (1)
EditorButton(353-418)
apps/desktop/src/routes/target-select-overlay.tsx (3)
apps/desktop/src/components/Cropper.tsx (6)
CropBounds(21-26)CROP_ZERO(27-27)CropperRef(219-229)Ratio(74-74)createCropOptionsMenuItems(1476-1503)Cropper(231-1253)apps/desktop/src/store.ts (1)
generalSettingsStore(61-62)apps/desktop/src/utils/tauri.ts (1)
LogicalPosition(398-398)
⏰ 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: Vercel Agent Review
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (1)
apps/desktop/src/routes/editor/Editor.tsx (1)
304-308: Confirm display presence in first segmentAccess to firstSegment.display assumes it exists. If there are recordings without display, this throws.
Would you like a defensive guard with early return in the dialog branch?
| if (props.ref) { | ||
| const cropperRef: CropperRef = { | ||
| reset: () => { | ||
| const bounds = computeInitialBounds(); | ||
| setRawBoundsAndAnimate(bounds); | ||
| setAspectState("snapped", null); | ||
| }, | ||
| fill, | ||
| setCropProperty: (field, value) => { | ||
| setAspectState("snapped", null); | ||
| setRawBoundsConstraining( | ||
| boundsToRaw({ ...realBounds(), [field]: value }), | ||
| { x: 0, y: 0 }, | ||
| ); | ||
| }, | ||
| setCrop: (value, origin) => | ||
| setRawBoundsConstraining( | ||
| boundsToRaw( | ||
| typeof value === "function" ? value(rawBounds()) : value, | ||
| ), | ||
| origin, | ||
| ), | ||
| get bounds() { | ||
| return realBounds; | ||
| }, | ||
| animateTo: (real, durationMs) => | ||
| setRawBoundsAndAnimate(boundsToRaw(real), undefined, durationMs), | ||
| }; | ||
|
|
||
| if (typeof props.ref === "function") props.ref(cropperRef); | ||
| else props.ref = cropperRef; | ||
| } |
There was a problem hiding this comment.
Ref pattern is unsafe; avoid assigning to props
Assigning to props.ref won’t set the parent variable. Accept only a callback ref and invoke it.
- ref?: CropperRef | ((ref: CropperRef) => void);
+ ref?: (ref: CropperRef) => void;
@@
- if (props.ref) {
- const cropperRef: CropperRef = {
+ if (props.ref) {
+ const cropperRef: CropperRef = {
/* ... */
- };
-
- if (typeof props.ref === "function") props.ref(cropperRef);
- else props.ref = cropperRef;
+ };
+ props.ref(cropperRef);
}Follow up: update all callsites to pass a callback ref (see Editor and target-select-overlay). As per coding guidelines
Also applies to: 231-247
| let cropperRef: CropperRef | undefined; | ||
| const [crop, setCrop] = createSignal(CROP_ZERO); | ||
| const [aspect, setAspect] = createSignal<Ratio | null>(null); | ||
|
|
There was a problem hiding this comment.
Fix ref wiring: CropperRef never set with current usage
Passing ref={cropperRef} to a custom component doesn’t assign the variable in the parent. Use a callback ref so cropperRef gets the instance.
- let cropperRef: CropperRef | undefined;
+ let cropperRef: CropperRef | undefined;
@@
- <Cropper
- ref={cropperRef}
+ <Cropper
+ ref={(r) => (cropperRef = r)}Optionally, tighten Cropper’s prop to ref?: (ref: CropperRef) => void to prevent this in the future. Based on learnings
Also applies to: 463-471
| <Button | ||
| variant="white" | ||
| size="xs" | ||
| class="flex items-center justify-center text-center rounded-full h-[2rem] w-[2rem] border focus:border-blue-9" | ||
| onMouseDown={showCropOptionsMenu} | ||
| onClick={showCropOptionsMenu} | ||
| > |
There was a problem hiding this comment.
Avoid double-triggering menu: onMouseDown + onClick
Both handlers call showCropOptionsMenu, causing duplicate popups. Keep one (prefer onClick for accessibility).
- <Button
+ <Button
variant="white"
size="xs"
class="flex items-center justify-center text-center rounded-full h-[2rem] w-[2rem] border focus:border-blue-9"
- onMouseDown={showCropOptionsMenu}
onClick={showCropOptionsMenu}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| variant="white" | |
| size="xs" | |
| class="flex items-center justify-center text-center rounded-full h-[2rem] w-[2rem] border focus:border-blue-9" | |
| onMouseDown={showCropOptionsMenu} | |
| onClick={showCropOptionsMenu} | |
| > | |
| <Button | |
| variant="white" | |
| size="xs" | |
| class="flex items-center justify-center text-center rounded-full h-[2rem] w-[2rem] border focus:border-blue-9" | |
| onClick={showCropOptionsMenu} | |
| > |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Editor.tsx around lines 402 to 408, the Button
currently registers both onMouseDown and onClick calling showCropOptionsMenu
which causes duplicate menu triggers; remove the onMouseDown prop and only keep
the onClick handler (prefer onClick for accessibility) so showCropOptionsMenu is
invoked once when activated; ensure no other code relies on onMouseDown for this
button and run the UI to verify a single popup appears.
| let cropperRef: CropperRef | undefined; | ||
|
|
There was a problem hiding this comment.
Fix ref wiring for CropperRef (parent variable not assigned)
Same pattern as Editor: ref={cropperRef} won’t set the variable. Use a callback ref.
- let cropperRef: CropperRef | undefined;
+ let cropperRef: CropperRef | undefined;
@@
- <Cropper
- ref={cropperRef}
+ <Cropper
+ ref={(r) => (cropperRef = r)}Also applies to: 357-366
🤖 Prompt for AI Agents
In apps/desktop/src/routes/target-select-overlay.tsx around lines 242-243 (and
similarly at lines 357-366), the local variable declaration "let cropperRef:
CropperRef | undefined;" is never assigned because using JSX prop
ref={cropperRef} does not wire the ref; replace the current ref usage with a
callback ref that assigns the element to the parent variable (e.g., ref={el =>
(cropperRef = el)}), update both occurrences, and ensure type safety by
narrowing or asserting the assigned value to CropperRef so the parent variable
is populated for later use.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/components/Cropper.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: Do not manually import icons in the desktop app; rely on auto-imported icons
In the desktop app, use @tanstack/solid-query for server state management
Files:
apps/desktop/src/components/Cropper.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src/components/Cropper.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/components/Cropper.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/components/Cropper.tsx
⏰ 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: Vercel Agent Review
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (1)
apps/desktop/src/components/Cropper.tsx (1)
233-234: Use callback refs instead of mutatingprops.refSolid props are immutable; reassigning
props.refis ignored, so consumers never receive the instance when they pass an object ref. Restrict the prop to a callback and invoke it directly to align with the framework expectations. As per coding guidelines.- ref?: CropperRef | ((ref: CropperRef) => void); + ref?: (ref: CropperRef) => void; @@ - if (props.ref) { - const cropperRef: CropperRef = { + if (props.ref) { + const cropperRef: CropperRef = { @@ - if (typeof props.ref === "function") props.ref(cropperRef); - else props.ref = cropperRef; + props.ref(cropperRef); }Also applies to: 592-623
| const shouldTriggerHaptic = ostype() === "macos"; | ||
| function triggerHaptic() { | ||
| if (shouldTriggerHaptic) commands.performHapticFeedback("alignment", null); | ||
| } |
There was a problem hiding this comment.
Fix async OS detection before gating haptics
ostype() from @tauri-apps/plugin-os returns a promise, so this equality check always yields false and haptics never fire on macOS. Resolve the promise once and gate the feedback on the resolved value.
-const shouldTriggerHaptic = ostype() === "macos";
-function triggerHaptic() {
- if (shouldTriggerHaptic) commands.performHapticFeedback("alignment", null);
-}
+let shouldTriggerHaptic = false;
+
+ostype()
+ .then((platform) => {
+ shouldTriggerHaptic = platform === "macos";
+ })
+ .catch(() => {
+ shouldTriggerHaptic = false;
+ });
+
+function triggerHaptic() {
+ if (!shouldTriggerHaptic) return;
+ void commands.performHapticFeedback("alignment", null);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shouldTriggerHaptic = ostype() === "macos"; | |
| function triggerHaptic() { | |
| if (shouldTriggerHaptic) commands.performHapticFeedback("alignment", null); | |
| } | |
| let shouldTriggerHaptic = false; | |
| ostype() | |
| .then((platform) => { | |
| shouldTriggerHaptic = platform === "macos"; | |
| }) | |
| .catch(() => { | |
| shouldTriggerHaptic = false; | |
| }); | |
| function triggerHaptic() { | |
| if (!shouldTriggerHaptic) return; | |
| void commands.performHapticFeedback("alignment", null); | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src/components/Cropper.tsx around lines 92-95, ostype() is
asynchronous so the current equality check always fails and haptics never fire;
fix by resolving the promise once and gating haptics on the resolved value
(e.g., on mount call ostype().then(value => set a local state/ref like isMac =
value === "macos" or store in a module-scoped variable, handle errors), then
change triggerHaptic to use that cached boolean (or make triggerHaptic async and
await ostype() if you prefer), ensuring you don't call ostype() repeatedly and
you handle promise rejection.
…ng after size constraint checks
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/components/Cropper.tsx(1 hunks)apps/desktop/src/routes/target-select-overlay.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: Do not manually import icons in the desktop app; rely on auto-imported icons
In the desktop app, use @tanstack/solid-query for server state management
Files:
apps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/components/Cropper.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/components/Cropper.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/components/Cropper.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/components/Cropper.tsx
🧬 Code graph analysis (2)
apps/desktop/src/routes/target-select-overlay.tsx (3)
apps/desktop/src/components/Cropper.tsx (6)
CropBounds(21-26)CROP_ZERO(27-27)CropperRef(216-226)Ratio(74-74)createCropOptionsMenuItems(1473-1500)Cropper(228-1250)apps/desktop/src/store.ts (1)
generalSettingsStore(61-62)apps/desktop/src/utils/tauri.ts (1)
LogicalPosition(398-398)
apps/desktop/src/components/Cropper.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
commands(7-269)
⏰ 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: Vercel Agent Review
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (5)
apps/desktop/src/components/Cropper.tsx (3)
92-95: Fix async OS detection before gating hapticsThe
ostype()from@tauri-apps/plugin-osreturns a promise but is being compared synchronously, so this equality check always yieldsfalseand haptics never fire on macOS. Resolve the promise once and gate the feedback on the resolved value.
201-214: Bug: width/height clamps are discardedThe
slideBoundsIntoContainerfunction clamps width and height into local variables but returns the original bounds object, losing those clamps.Apply this diff to fix the issue:
- return { ...bounds, x, y }; + return { ...bounds, x, y, width, height };
592-623: Ref pattern is unsafe; avoid assigning to propsAssigning to
props.refwon't set the parent variable. Accept only a callback ref and invoke it.Apply this diff:
- ref?: CropperRef | ((ref: CropperRef) => void); + ref?: (ref: CropperRef) => void; @@ - if (props.ref) { - const cropperRef: CropperRef = { + if (props.ref) { + const cropperRef: CropperRef = { /* ... */ - }; - - if (typeof props.ref === "function") props.ref(cropperRef); - else props.ref = cropperRef; + }; + props.ref(cropperRef); }apps/desktop/src/routes/target-select-overlay.tsx (2)
242-243: Fix ref wiring for CropperRef (parent variable not assigned)The ref assignment
ref={cropperRef}won't set the variable. Use a callback ref.Apply this diff:
- let cropperRef: CropperRef | undefined; + let cropperRef: CropperRef | undefined; @@ - <Cropper - ref={cropperRef} + <Cropper + ref={(r) => (cropperRef = r)}
489-490: Prevent duplicate menu popupsBoth onMouseDown and onClick trigger showMenu causing duplicate menu popups.
Apply this diff:
- onMouseDown={(e) => showMenu(menuModes(), e)} - onClick={(e) => showMenu(menuModes(), e)} + onClick={(e) => showMenu(menuModes(), e)} @@ - onMouseDown={(e) => showMenu(preRecordingMenu(), e)} - onClick={(e) => showMenu(preRecordingMenu(), e)} + onClick={(e) => showMenu(preRecordingMenu(), e)}Also applies to: 497-498
| checked: !options.aspect, | ||
| action: () => options.onAspectSet(null), | ||
| } satisfies CheckMenuItemOptions, | ||
| ...COMMON_RATIOS.map( | ||
| (ratio) => | ||
| ({ | ||
| text: `${ratio[0]}:${ratio[1]}`, | ||
| checked: options.aspect === ratio, |
There was a problem hiding this comment.
🧩 Analysis chain
Correct satisfies usage with proper TypeScript version
The satisfies operator was introduced in TypeScript v4.9. The current usage ratio === ratio in Line 1489 appears incorrect. It should compare the option's aspect ratio against the current ratio for the checked state.
Apply this diff to fix the comparison:
- checked: options.aspect === ratio,
+ checked: options.aspect?.[0] === ratio[0] && options.aspect?.[1] === ratio[1],🌐 Web query:
What is the latest stable version of TypeScript and does it support the satisfies keyword in version 4.9+?
💡 Result:
Latest stable TypeScript: v5.9.2 (released July 31, 2025). [1]
Yes — the satisfies operator was introduced in TypeScript 4.9 and is available in 4.9 and later. [2]
Sources:
- TypeScript GitHub releases (v5.9.2). [1]
- TypeScript 4.9 release notes — "The
satisfiesOperator". [2]
🏁 Script executed:
#!/bin/bash
# Locate TypeScript version in package.json
rg -n '"typescript"\s*:' -g 'package.json'Length of output: 484
🏁 Script executed:
#!/bin/bash
# Find definition of COMMON_RATIOS
rg -n "COMMON_RATIOS" -C3 --glob '*.ts*'Length of output: 1531
🏁 Script executed:
#!/bin/bash
# Find onAspectSet usage and definition context
rg -n "onAspectSet" -C5 --glob 'apps/desktop/src/components/Cropper.tsx'Length of output: 1807
Compare tuple values for checked state
options.aspect === ratio tests reference identity and won’t match tuples returned by findClosestRatio. Change to a value comparison:
- checked: options.aspect === ratio,
+ checked: options.aspect?.[0] === ratio[0] && options.aspect?.[1] === ratio[1],apps/desktop/src/components/Cropper.tsx:1489
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| checked: !options.aspect, | |
| action: () => options.onAspectSet(null), | |
| } satisfies CheckMenuItemOptions, | |
| ...COMMON_RATIOS.map( | |
| (ratio) => | |
| ({ | |
| text: `${ratio[0]}:${ratio[1]}`, | |
| checked: options.aspect === ratio, | |
| ...COMMON_RATIOS.map( | |
| (ratio) => | |
| ({ | |
| text: `${ratio[0]}:${ratio[1]}`, | |
| checked: | |
| options.aspect?.[0] === ratio[0] && | |
| options.aspect?.[1] === ratio[1], | |
| action: () => options.onAspectSet(ratio), | |
| } satisfies CheckMenuItemOptions) | |
| ) |
🤖 Prompt for AI Agents
apps/desktop/src/components/Cropper.tsx lines ~1482-1489: the checked expression
uses reference equality (options.aspect === ratio) which fails for separately
created tuples; replace it with a value comparison that handles null/undefined,
e.g. check that Array.isArray(options.aspect) and compare elements
(options.aspect[0] === ratio[0] && options.aspect[1] === ratio[1]) or use a
small equality helper (or JSON.stringify) to compare the two tuples by value so
the correct menu item is marked checked.
| const MARGIN_BELOW = 16; | ||
| const MARGIN_TOP_OUTSIDE = 16; | ||
| const MARGIN_TOP_INSIDE = 28; | ||
| const TOP_SAFE_MARGIN = ostype() === "macos" ? 40 : 10; // keep clear of notch on MacBooks |
There was a problem hiding this comment.
Fix async OS detection for macOS safe margin
Similar to the Cropper component, ostype() returns a promise but is compared synchronously. This will always be false, making the macOS safe margin ineffective.
Apply this diff to fix the async OS detection:
+ let isMacOS = false;
+ ostype()
+ .then((platform) => {
+ isMacOS = platform === "macos";
+ })
+ .catch(() => {
+ isMacOS = false;
+ });
+
- const TOP_SAFE_MARGIN = ostype() === "macos" ? 40 : 10;
+ const TOP_SAFE_MARGIN = () => isMacOS ? 40 : 10;And update the usage in Line 312:
- if (aboveY >= TOP_SAFE_MARGIN) {
+ if (aboveY >= TOP_SAFE_MARGIN()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const TOP_SAFE_MARGIN = ostype() === "macos" ? 40 : 10; // keep clear of notch on MacBooks | |
| // … earlier in the component | |
| let isMacOS = false; | |
| ostype() | |
| .then((platform) => { | |
| isMacOS = platform === "macos"; | |
| }) | |
| .catch(() => { | |
| isMacOS = false; | |
| }); | |
| const TOP_SAFE_MARGIN = () => isMacOS ? 40 : 10; // keep clear of notch on MacBooks | |
| // … later around line 312 | |
| if (aboveY >= TOP_SAFE_MARGIN()) { | |
| // … | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/target-select-overlay.tsx around line 292 (and update
usage at line 312), ostype() is async and returns a Promise, but the code uses
it synchronously (ostype() === "macos"), so change to await the result or use
.then to resolve it before comparison; e.g., make the containing function async
(or compute the OS in useEffect), await ostype(), set a local state/variable for
the OS, then compute TOP_SAFE_MARGIN based on that resolved value and update the
later usage at line 312 to reference the resolved OS/state value.
This RP removes the two previous cropper implementation and replaces them with a new, feature rich, more efficient unified cropper component.
The new cropper no longer uses a canvas and consumes much less resources in comparison.
Changes:
core:resources:allow-closefor properly closing context menus.With this, I tried having it resemble most of the functionality that is available in the cropper that's part of macOS's native Screenshot app.
Summary by CodeRabbit
New Features
Improvements
UI
Settings