Skip to content

fix: add confirm shortcut to welcome CTA#2206

Merged
arnestrickmann merged 1 commit into
mainfrom
emdash/cmd-enter-on-start-shipping-button
May 23, 2026
Merged

fix: add confirm shortcut to welcome CTA#2206
arnestrickmann merged 1 commit into
mainfrom
emdash/cmd-enter-on-start-shipping-button

Conversation

@arnestrickmann
Copy link
Copy Markdown
Contributor

Summary

  • Reuses the shared ConfirmButton for the onboarding welcome Start shipping CTA.
  • Shows the configured confirm shortcut on the button and lets Mod+Enter activate it.

Impact

The welcome screen now matches other confirm actions and supports Cmd+Return on macOS.

Validation

  • pnpm run format
  • pnpm run lint
  • pnpm run typecheck
  • pnpm run test

@arnestrickmann arnestrickmann marked this pull request as ready for review May 23, 2026 20:10
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 23, 2026

Greptile Summary

This PR adds keyboard shortcut support to the welcome screen's "Start shipping" CTA by wiring up the configured confirm hotkey directly in WelcomeScreen, and displaying it with BoundShortcut.

  • Rather than adopting ConfirmButton (which uses useHotkey globally), the author registers a capture-phase document.addEventListener with stopImmediatePropagation() to prevent Mod+Enter from leaking into the concurrently-mounted <Workspace /> and triggering its confirm handlers — addressing the known hotkey-leakage risk.
  • BoundShortcut is rendered inside the button for visual parity with other confirm actions; the underlying settings key is read once in the component and once more internally by BoundShortcut, a minor duplication with no runtime impact.

Confidence Score: 5/5

Safe to merge — the change is self-contained, correctly scoped to the welcome overlay, and the capture-phase interception prevents hotkey leakage into the Workspace.

The new keydown handler is registered in the capture phase with stopImmediatePropagation(), which prevents the confirm hotkey from reaching any Workspace listeners while the welcome overlay is shown. Cleanup is handled correctly in the effect, and the fallback when the hotkey is unset is guarded.

No files require special attention.

Important Files Changed

Filename Overview
src/renderer/app/welcome.tsx Adds capture-phase keydown listener and BoundShortcut badge to the welcome CTA; deliberately avoids useHotkey to prevent leakage into the concurrently-mounted Workspace.

Sequence Diagram

sequenceDiagram
    participant User
    participant Document as document (capture)
    participant WelcomeHandler as WelcomeScreen keydown handler
    participant WorkspaceHotkeys as Workspace useHotkey listeners (bubble)

    User->>Document: keydown (Mod+Enter)
    Document->>WelcomeHandler: capture phase (fires first)
    WelcomeHandler->>WelcomeHandler: matchesKeyboardEvent?
    alt matches confirm hotkey
        WelcomeHandler->>WelcomeHandler: stopImmediatePropagation()
        WelcomeHandler->>User: onGetStarted() → window.location.reload()
        Note over WorkspaceHotkeys: Bubble phase never reached
    else does not match
        WelcomeHandler-->>Document: propagate
        Document->>WorkspaceHotkeys: bubble phase listeners fire normally
    end
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
src/renderer/app/welcome.tsx:37
**Unstable `onGetStarted` reference causes unnecessary effect churn**

`onGetStarted` is passed as `() => window.location.reload()` inline in `App.tsx`, so it gets a new reference on every `AppContent` render. Because it's listed as an effect dependency, the cleanup/re-register cycle runs on every render of the parent while the welcome view is shown (query refetches, etc.). The effect is cheap, so this is harmless in practice, but wrapping the prop at the call site with `useCallback` would make the dependency stable and is the idiomatic fix.

Reviews (2): Last reviewed commit: "fix: add confirm shortcut to welcome CTA" | Re-trigger Greptile

Comment thread src/renderer/app/welcome.tsx Outdated
Comment on lines +80 to +88
<ConfirmButton
onClick={onGetStarted}
size="sm"
className={
effectiveTheme === 'emdark' ? 'bg-gray-200 text-gray-900 hover:bg-gray-300' : ''
}
>
Start shipping
</Button>
</ConfirmButton>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Global hotkey leakage through the overlay

ConfirmButton registers the confirm hotkey (Mod+Enter) globally via useHotkey. In App.tsx the welcome screen is rendered alongside <Workspace /> (not instead of it), so all ConfirmButton instances currently mounted in the workspace also have the same hotkey registered. Pressing Mod+Enter on the welcome screen will fire every one of those handlers simultaneously — including potentially destructive confirm buttons (e.g., "Delete item") — before window.location.reload() takes effect. The overlay has no way to suppress or capture keyboard events from the background tree.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/app/welcome.tsx
Line: 80-88

Comment:
**Global hotkey leakage through the overlay**

`ConfirmButton` registers the confirm hotkey (`Mod+Enter`) globally via `useHotkey`. In `App.tsx` the welcome screen is rendered *alongside* `<Workspace />` (not instead of it), so all `ConfirmButton` instances currently mounted in the workspace also have the same hotkey registered. Pressing `Mod+Enter` on the welcome screen will fire every one of those handlers simultaneously — including potentially destructive confirm buttons (e.g., "Delete item") — before `window.location.reload()` takes effect. The overlay has no way to suppress or capture keyboard events from the background tree.

How can I resolve this? If you propose a fix, please make it concise.

@arnestrickmann arnestrickmann force-pushed the emdash/cmd-enter-on-start-shipping-button branch from 3883667 to b900398 Compare May 23, 2026 20:16
@arnestrickmann
Copy link
Copy Markdown
Contributor Author

@greptile

@arnestrickmann arnestrickmann merged commit 2f47077 into main May 23, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant