Fix shortcuts implementation#73
Conversation
…tcuts for note actions (duplicate notes)
|
Someone is attempting to deploy a commit to the Dhanush Nehru's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@DhanushNehru kindly review the open PRs |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the shortcuts implementation by adding a confirmation dialog for note deletion and reorganizing the duplicate note shortcut from Ctrl/Cmd+D to Ctrl/Cmd+Shift+D to avoid conflicts. Additionally, it improves the authentication modal with password visibility toggles and confirmation fields.
- Added delete confirmation dialog to prevent accidental note deletion
- Moved duplicate note shortcut to Ctrl/Cmd+Shift+D and added Shift+D as alternative
- Enhanced authentication modal with password visibility controls and confirmation field
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/pages/Index.tsx | Implements delete confirmation dialog, reorganizes keyboard shortcuts, and updates shortcut documentation |
| src/components/ui/AuthModal.tsx | Adds password visibility toggles and confirm password field for registration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (isShift && !isCtrlOrMeta) { | ||
| switch (key) { | ||
| case "d": { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (activeNoteId) { | ||
| handleDuplicateNote(activeNoteId); | ||
| toast.success("Note duplicated"); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate code: The Shift+D case (lines 162-170) contains identical logic to the Ctrl/Cmd+Shift+D case (lines 147-155). Consider extracting this logic into a shared function to avoid code duplication.
| <kbd className="px-2 py-0.5 bg-muted rounded border border-border font-mono"> | ||
| Ctrl+Shift+D | ||
| </kbd> | ||
| </div> |
There was a problem hiding this comment.
The shortcut documentation only shows 'Ctrl+Shift+D' but the code also supports 'Shift+D' alone. The documentation should reflect both available shortcuts or clarify which is the primary one.
| <kbd className="px-2 py-0.5 bg-muted rounded border border-border font-mono"> | |
| Ctrl+Shift+D | |
| </kbd> | |
| </div> | |
| <div className="flex gap-2"> | |
| <kbd className="px-2 py-0.5 bg-muted rounded border border-border font-mono"> | |
| Ctrl+Shift+D | |
| </kbd> | |
| <span className="text-xs text-muted-foreground">or</span> | |
| <kbd className="px-2 py-0.5 bg-muted rounded border border-border font-mono"> | |
| Shift+D | |
| </kbd> | |
| </div> |
|
@DhanushNehru All suggestions reviewed and fixed |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@DhanushNehru kindly merge now |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| <div className="flex items-center justify-between gap-4"> | ||
| <span>Duplicate note</span> | ||
| <div className="flex items-center gap-2"> | ||
| <kbd className="px-2 py-0.5 bg-muted rounded border border-border font-mono"> | ||
| Shift+D | ||
| </kbd> | ||
| <span className="text-muted-foreground">or</span> | ||
| <kbd className="px-2 py-0.5 bg-muted rounded border border-border font-mono"> | ||
| Cmd/Ctrl+Shift+D | ||
| </kbd> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The keyboard shortcut documentation shows 'Shift+D' as an option for duplicating notes, but this shortcut is not implemented in the handleKeyDown function. Only 'Ctrl/Cmd+Shift+D' is actually handled.
|
|
||
| <div | ||
| className="w-16 h-0.5 mx-auto my-2 rounded-full bg-gradient-to-r from-orange-500 via-orange-400 to-orange-100" | ||
| ></div> |
There was a problem hiding this comment.
The empty div tag should be self-closing for consistency with JSX best practices.
| ></div> | |
| /> |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const isShift = event.shiftKey; | ||
|
|
||
| // Handle Ctrl/Cmd + key combinations | ||
| if (isCtrlOrMeta && !isShift) { |
There was a problem hiding this comment.
The keyboard shortcut handling logic has an issue where isShift is extracted but the condition !isShift in line 118 will prevent Shift+key combinations from being processed in the first block, even though some shortcuts might legitimately use Shift modifiers.
| if (isCtrlOrMeta && !isShift) { | |
| if (isCtrlOrMeta) { |
| <div | ||
| className="w-16 h-0.5 mx-auto my-2 rounded-full bg-gradient-to-r from-orange-500 via-orange-400 to-orange-100" | ||
| ></div> |
There was a problem hiding this comment.
[nitpick] The self-closing div tag should use the more conventional <div></div> format instead of <div></div> for better readability and consistency with JSX conventions.
|
Can you attach a video based on current deployed version and your branch in local |
…atchpad-scribe into fix-shortcuts-implementation
|
@DhanushNehru I've resolved the merge conflic here as well |
Add delete confirmation dialog for shortcut delete action and fix duplicate note shortcut action