Repository navigation
fix: restore Cmd+S save and title caret in bookmarks panel - #4066
Merged
Merged
Conversation
- Pass empty tagsToIgnore to the bookmarks header useHotkeys so mod+S fires even when focus is in an input (restores pre-mantine behavior) - Place the caret at the start of the title input when the add/edit bookmark modal opens via base-ui initialFocus - Add e2e tests reproducing both bugs Fixes #4064 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closed
2 tasks done
PR Summary by QodoRestore Cmd+S save hotkey and title caret behavior in Bookmarks panel
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
![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 found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="apps/extension/tests/specs/bookmarks.spec.ts" line_range="100-109" />
<code_context>
await expect(dialog).toBeHidden();
});
+ test('should focus the title input with caret at start when opening the edit modal', async ({
+ bookmarksPage,
+ }) => {
+ const panel = new BookmarksPanel(bookmarksPage);
+ await panel.ensureAtRoot();
+
+ const dialog = await panel.openEditBookmarkDialog(
+ TEST_BOOKMARKS.REACT_DOCS
+ );
+ const titleInput = dialog.getByTestId('bookmark-title-input');
+ await expect(titleInput).toBeFocused();
+
+ const selection = await titleInput.evaluate((el) => ({
+ start: (el as HTMLInputElement).selectionStart,
+ end: (el as HTMLInputElement).selectionEnd,
+ }));
+ expect(selection.start).toBe(0);
+ expect(selection.end).toBe(0);
+
+ await panel.closeDialog();
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen caret-position test to explicitly cover non-empty edited titles
This only checks the caret position; it doesn’t verify we’re in a true edit scenario with a pre-populated title. Please also assert that the title input has the expected non-empty value (e.g. `toHaveValue(TEST_BOOKMARKS.REACT_DOCS.title)` or at least `not.toBe('')`) so the caret behaviour is exercised on an actual edited bookmark rather than an empty field.
```suggestion
const dialog = await panel.openEditBookmarkDialog(
TEST_BOOKMARKS.REACT_DOCS
);
const titleInput = dialog.getByTestId('bookmark-title-input');
await expect(titleInput).toHaveValue(TEST_BOOKMARKS.REACT_DOCS.title);
await expect(titleInput).toBeFocused();
const selection = await titleInput.evaluate((el) => ({
start: (el as HTMLInputElement).selectionStart,
end: (el as HTMLInputElement).selectionEnd,
}));
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review by Qodo
1. Input ref not forwarded
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe bookmark edit dialog now focuses its title input and places the caret at position zero when opened. The bookmarks header save shortcut is configured to trigger while focus is inside an input. End-to-end tests cover both the initial title selection behavior and saving from the bookmarks search input. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 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 `@apps/extension/tests/specs/bookmarks.spec.ts`:
- Around line 445-462: Update the keyboard shortcut invocation in the “should
save via Cmd+S while focus is in the search input” test to use Playwright’s
cross-platform ControlOrMeta+s shortcut instead of Meta+s, preserving the
existing focus and saved-state assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c5cbca3-6854-4504-ac13-9d9df9cd5f74
📒 Files selected for processing (3)
apps/extension/src/entrypoints/popup/panels/BookmarksPanel/components/BookmarkAddEditDialog.tsxapps/extension/src/entrypoints/popup/panels/BookmarksPanel/components/BookmarksHeader.tsxapps/extension/tests/specs/bookmarks.spec.ts
- Skip the panel Cmd+S save when focus is inside a dialog so an in-progress bookmark form edit isn't dropped from the saved snapshot - Assert the edited title's value in the caret-at-start e2e test - Use ControlOrMeta+s in the Cmd+S e2e test for cross-platform CI Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Extension version is updated from |
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.
Check if the Pull Request fulfils these requirements
Summary by Sourcery
Restore expected save hotkey behaviour and title field focus in the bookmarks panel edit dialog.
Enhancements:
Tests:
Greptile Summary
This PR restores two regression fixes in the bookmarks panel: Cmd+S now fires even when focus is in the search input by passing
[]astagsToIgnoretouseHotkeys, with a[role="dialog"]guard preventing spurious saves when any dialog is open; and the bookmark edit dialog now focuses the title input with the caret at position 0 via aninitialFocuscallback and auseRef.Confidence Score: 5/5
The changes are narrowly scoped and well-tested; the dialog guard correctly prevents the hotkey from interfering with in-progress edits across all dialog types.
Both fixes are tightly scoped with matching E2E test coverage. The [role=dialog] guard is the correct idiomatic check and covers all dialogs in the component tree. No new code paths are introduced that could cause data loss or incorrect behavior.
No files require special attention.
Reviews (3): Last reviewed commit: "Update package.json" | Re-trigger Greptile