feat(phase-1): allow user to customize keybindings#7163
Open
shubh-bruno wants to merge 8 commits intousebruno:mainfrom
Open
feat(phase-1): allow user to customize keybindings#7163shubh-bruno wants to merge 8 commits intousebruno:mainfrom
shubh-bruno wants to merge 8 commits intousebruno:mainfrom
Conversation
shubh-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a full in-app keybindings editor with validation and OS-aware merging, centralizes hotkey lifecycle in a refactored HotkeysProvider, expands default key mappings and Electron zoom IPC/menu, augments sidebar keyboard handlers, and adds extensive Playwright tests and small test-id tweaks. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as "Keybindings Editor (Renderer)"
participant Validator as "Validation Logic"
participant Prefs as "Preferences Store"
participant Hotkeys as "HotkeysProvider"
participant Mousetrap as "Mousetrap"
User->>UI: Edit/save keybinding
UI->>Validator: Validate combo (reserved, OS rules, duplicates)
Validator-->>UI: Validation result
alt valid
UI->>Prefs: Persist custom binding
Prefs-->>Hotkeys: Preferences updated
Hotkeys->>Hotkeys: Merge defaults + user bindings
Hotkeys->>Mousetrap: Convert to mousetrap combos and bind/unbind
Mousetrap-->>UI: Hotkey active
else invalid
UI-->>User: Show error tooltip
end
sequenceDiagram
actor User
participant Menu as "Electron Menu"
participant Main as "Main Process"
participant Renderer as "Renderer Process"
User->>Menu: Click "Zoom In"
Menu->>Main: Invoke click handler (access focused BrowserWindow)
Main->>Main: Adjust webContents zoom (+0.5)
Main->>Renderer: (IPC or webContents) apply zoom change
Renderer-->>User: UI zoom changed
sequenceDiagram
participant Hotkeys as "HotkeysProvider"
participant EventBus as "Window Event Bus"
participant Collection as "Collection Component"
Hotkeys->>EventBus: Emit 'clone-item-open'
EventBus->>Collection: Event dispatched
Collection->>Collection: Check isFocusedRef
alt focused
Collection->>Collection: Open clone modal
else not focused
Collection-->>Hotkeys: Ignore event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (7)
packages/bruno-app/src/providers/Hotkeys/keyMappings.js (1)
128-148:getMergedKeyBindingsis recomputed on every call togetKeyBindingsForActionAllOS.Each action lookup triggers a full merge of all bindings. In the Hotkeys provider (which calls
getKeyBindingsForActionAllOSper action), this means N full merges for N actions. Not a blocking issue at the current scale, but worth noting if the action list grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/Hotkeys/keyMappings.js` around lines 128 - 148, getKeyBindingsForActionAllOS currently calls getMergedKeyBindings on every invocation causing O(N^2) work when the Hotkeys provider asks per-action; fix by computing the merged bindings once and reusing them: either change the Hotkeys provider to call getMergedKeyBindings(userKeyBindings) once and pass the resulting merged object into getKeyBindingsForActionAllOS (add a new parameter like mergedBindings) or memoize getMergedKeyBindings (e.g., cache by reference or userKeyBindings identity) so repeated calls return the same merged object; update references to getKeyBindingsForActionAllOS and any calls from the Hotkeys provider to use the new parameter or rely on the memoized result.packages/bruno-electron/src/store/preferences.js (1)
60-104:keyBindingsdefaults duplicated across electron store and app-sidekeyMappings.js.The exact same binding definitions exist in
packages/bruno-app/src/providers/Hotkeys/keyMappings.js(DEFAULT_KEY_BINDINGS). If a new action is added or a default shortcut changes, both files must be updated in lockstep — a classic DRY violation.Consider extracting the default keybindings into a shared package/module and importing from both sides.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/store/preferences.js` around lines 60 - 104, Duplicate default key bindings exist between keyBindings (in preferences.js) and DEFAULT_KEY_BINDINGS (in keyMappings.js); extract them to a single shared module and import from both places. Create a shared export (e.g., defaultKeyBindings) containing the same shape (platform keys mac/windows and name) and replace the inline object in preferences.js (keyBindings) and the app-side DEFAULT_KEY_BINDINGS to import that shared symbol; ensure function/class references that consume these values (e.g., anywhere expecting keyBindings or DEFAULT_KEY_BINDINGS) still receive the same object shape and update any TypeScript types or tests to reference the new module.packages/bruno-app/src/components/Preferences/Keybindings/index.js (2)
186-201: Redundant guard onDEFAULT_KEY_BINDINGS.Line 189:
DEFAULT_KEY_BINDINGSis a module-level constant object — this check is always truthy and adds no value. It's also placed aftergetDefaultRowKeysString(action)has already dereferenced it on line 188.Suggested fix
const isRowDirty = (action) => { const current = getCurrentRowKeysString(action); const def = getDefaultRowKeysString(action); - if (!DEFAULT_KEY_BINDINGS) return false; return current !== def; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js` around lines 186 - 201, Remove the redundant runtime guard on DEFAULT_KEY_BINDINGS inside isRowDirty: delete the `if (!DEFAULT_KEY_BINDINGS) return false;` check (it’s a module-level constant and already dereferenced by getDefaultRowKeysString), so isRowDirty should simply compare getCurrentRowKeysString(action) !== getDefaultRowKeysString(action). Leave hasDirtyRows/useMemo as-is (it already iterates Object.keys(DEFAULT_KEY_BINDINGS)); if you want safety, perform any validation at module init rather than inside isRowDirty.
464-469: Orphan<Tooltip>— never anchored to any element.This
<Tooltip id="kb-editing-error-tooltip">has noanchorSelectprop, so it will never attach to or display next to any element. The per-row tooltips (lines 536–544) are the ones actually used. Remove this dead JSX.Suggested fix
- <Tooltip - id="kb-editing-error-tooltip" - place="bottom-start" - opacity={1} - className="kb-tooltip kb-tooltip--error" - /> -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js` around lines 464 - 469, Remove the orphan Tooltip JSX with id="kb-editing-error-tooltip" since it has no anchorSelect and is never attached; delete the <Tooltip id="kb-editing-error-tooltip" ... /> element (the standalone tooltip block) and ensure that only the per-row Tooltip instances (the tooltips used within each row) remain; also search for any references to "kb-editing-error-tooltip" and remove or update them if found.packages/bruno-app/src/components/Sidebar/Sections/CollectionsSection/index.js (1)
48-63:setTimeout+document.querySelectorfor focusing is fragile.If the search input takes longer than 50ms to mount, or if the class name
.collection-search-inputchanges, focus silently fails. Consider using a ref or a callback-based approach for more reliable post-render focus.That said, the event listener setup and cleanup are correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/Sidebar/Sections/CollectionsSection/index.js` around lines 48 - 63, The current handleSidebarSearch in useEffect relies on setTimeout + document.querySelector('.collection-search-input') which is fragile; replace this with a React ref/callback: create a ref (e.g., searchInputRef) or a focus callback in the CollectionsSection and pass it into the Search/Input component (rather than querying by class), call setShowSearch(true) as before, then in a useEffect/useLayoutEffect that depends on showSearch or in the input's ref callback, call searchInputRef.current.focus() (remove the setTimeout and DOM query). Update references to the class selector and the handleSidebarSearch logic so focusing is driven by the ref (preserve the event listener add/remove in useEffect).packages/bruno-app/src/providers/Hotkeys/index.js (2)
333-372:useSelectorsubscriptions for modal context cause unnecessary re-renders.
tabs,collections, andactiveTabUid(lines 334–336) are selected solely to derivecurrentCollectionfor theNewRequestmodal'scollectionUidprop. Every tab or collection state change triggers a re-render of the entire provider tree, even when no modal is open.Consider computing
currentCollectionlazily (e.g., inside the modal-open handler viastore.getState()) or wrapping the modal section in a separate component that subscribes independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/Hotkeys/index.js` around lines 333 - 372, HotkeysProvider is subscribing to tabs, collections and activeTabUid (via useSelector) purely to derive currentCollection, causing unnecessary re-renders; remove those top-level selectors and stop computing currentCollection on every render in HotkeysProvider, and instead compute the collection lazily when opening the New Request modal (e.g., inside the handler that calls setShowNewRequestModal) using the redux store directly (store.getState()) or move the modal UI into a separate component (e.g., NewRequestModal) that calls useSelector for tabs/collections/activeTabUid itself; update getCurrentCollection/currentCollection usage accordingly so bindAllHotkeys and the provider render path no longer depend on tabs/collections/activeTabUid.
85-87: Static analysis:forEachcallback implicitly returns a value.Biome flags this because
unbindHotkeycan return the result ofMousetrap.unbind(). Harmless sinceforEachignores return values, but trivially fixable.Suggested fix
function unbindAllHotkeys(userKeyBindings) { - BOUND_ACTIONS.forEach((action) => unbindHotkey(action, userKeyBindings)); + BOUND_ACTIONS.forEach((action) => { unbindHotkey(action, userKeyBindings); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/Hotkeys/index.js` around lines 85 - 87, The forEach callback in unbindAllHotkeys implicitly returns the value from unbindHotkey (which can return Mousetrap.unbind()), triggering static analysis; change the implementation to explicitly ignore the return value—e.g., replace the BOUND_ACTIONS.forEach(...) with an explicit loop (for (const action of BOUND_ACTIONS) { unbindHotkey(action, userKeyBindings); }) or wrap the call with void to discard the result—so unbindAllHotkeys, BOUND_ACTIONS, and unbindHotkey remain the same but no value is implicitly returned from the callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js`:
- Around line 219-278: The subset-conflict logic in validateCombo is too strict
for Mousetrap because it treats modifier differences as conflicts; update the
loop that compares arr and otherKeys (inside validateCombo) to only treat one
combo as a conflict if their modifier sets are identical (compute modifier sets
from arr and otherKeys via existing isModifier/isOnlyModifiers helpers) before
performing the subset checks, and add a short inline comment near those checks
explaining that Mousetrap treats combos with different modifier sets as distinct
so we only block true subset conflicts with the same modifiers; if you intended
the stricter behavior instead, replace the change with a concise comment
explaining that rationale.
- Line 223: The string literal returned when sig is falsy uses a smart quote;
change it to a standard straight apostrophe to match project style and other
occurrences: update the return in the function that checks sig (the line
returning { code: ERROR.EMPTY, message: 'Shortcut can’t be empty.' }) to use a
straight apostrophe (e.g. 'Shortcut can\'t be empty.') so it matches the style
used elsewhere (see other uses of ERROR.EMPTY and escaped straight apostrophe).
In
`@packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js`:
- Around line 596-599: The useSelector hook (and derived variables
userKeyBindings, hasCustomCopyBinding, hasCustomPasteBinding) is defined after
early returns, violating Rules of Hooks; move the useSelector((state) =>
state.app.preferences) call and the lines that compute userKeyBindings,
hasCustomCopyBinding and hasCustomPasteBinding up to the top of the
CollectionItem component alongside the other hooks (near where other hooks are
declared, before any conditional returns such as the searchText filter), and
remove the original declarations at lines 596–599 so hooks are invoked in a
consistent order on every render.
- Around line 126-157: The event handlers registered in the useEffect
(handleCloneItemOpen, handleCopyItemOpen, handlePasteItemOpen) close over
mutable props/state because handleCopyItem and handlePasteItem are captured once
on mount, causing stale operations when
item/collection/collectionUid/isFolder/dispatch change; fix by either memoizing
handleCopyItem and handlePasteItem with useCallback and adding them (and any
other used values) to the useEffect dependency array, or keep the effect deps
empty but store current handler references in refs (e.g., copyHandlerRef.current
= handleCopyItem, pasteHandlerRef.current = handlePasteItem) and call those refs
inside the window event callbacks while still using isFocusedRef for focus
checks, then update the refs wherever the handlers change so the event listeners
always invoke the latest logic.
In `@packages/bruno-electron/src/app/menu-template.js`:
- Around line 66-92: The menu click handlers currently use
focusedWindow.webContents.send(...) which targets the renderer but your zoom
handlers are registered in the main process, causing an IPC direction mismatch;
change each menu click (the "Reset Zoom", "Zoom In", "Zoom Out" handlers where
BrowserWindow.getFocusedWindow() is used) to manipulate zoom directly in the
main process by getting the focusedWindow and calling its
webContents.getZoomLevel() and webContents.setZoomLevel(...) (or
setZoomFactor(...)) to implement reset/in/out behavior, rather than sending
renderer messages; ensure you remove or stop relying on webContents.send(...)
for these channels so main-process handlers (e.g., the existing ipcMain.handle
for renderer:zoom-in) are not bypassed.
- Around line 66-92: The IPC channel names used in the menu click handlers are
mismatched with the listeners; update the click handlers in the menu items (the
'Reset Zoom', 'Zoom In', and 'Zoom Out' entries in menu-template.js) to send
'main:zoom-reset', 'main:zoom-in', and 'main:zoom-out' respectively (instead of
'main:menu-zoom-*') so they match the handlers in
packages/bruno-electron/src/index.js and restore zoom functionality.
- Around line 64-65: There are two consecutive `{ type: 'separator' }` entries
in the View menu template causing an extra empty gap; remove one of the
duplicate `{ type: 'separator' }` objects in the menu template array (the View
menu block) so only a single separator remains between the adjacent menu groups.
In `@packages/bruno-electron/src/index.js`:
- Around line 195-196: The comment beside the zoomFactor option is incorrect:
zoomFactor: 1.0 only sets the initial page zoom (and is the default), it does
not disable zoom shortcuts. Either remove the misleading comment or update it to
state it sets the initial zoom; if the intent was to disable zoom shortcuts,
implement a handler on the BrowserWindow's webContents (e.g.,
BrowserWindow/webContents 'before-input-event' to intercept Ctrl/Cmd + '+', '-'
and '0' or use webFrame.setZoomLevelLimits in the preload) and remove or replace
the zoomFactor line if redundant.
- Around line 247-250: The change removed the platform guard around the ipcMain
handler for 'renderer:window-close', altering macOS behavior; either restore the
original guard using isWindows/isLinux so macOS ignores the close request, or
implement macOS-specific behavior by checking !isWindows && !isLinux and calling
mainWindow.hide() (or app.hide()) instead of mainWindow.close(); if the macOS
behavior change was intentional, delete the commented-out guard entirely to
avoid dead code.
- Around line 227-245: There are duplicate ipcMain handlers
(ipcMain.on('main:zoom-in' / 'main:zoom-out' / 'main:zoom-reset')) that mirror
the existing renderer handlers (ipcMain.on('renderer:zoom-in' /
'renderer:zoom-out' / 'renderer:reset-zoom')) and the global shortcut uses a
different step (+1); remove the duplicate 'main:zoom-*' handlers and consolidate
zoom behavior by introducing a single ZOOM_STEP constant (e.g., 0.5) used by the
remaining handlers (renderer:zoom-in, renderer:zoom-out, renderer:reset-zoom)
and by the globalShortcut.register callback that currently adjusts
mainWindow.webContents.setZoomLevel(currentZoom + 1), so update it to use
ZOOM_STEP (and -ZOOM_STEP) and always reference
mainWindow.webContents.getZoomLevel()/setZoomLevel accordingly to keep zoom
behavior consistent.
In `@packages/bruno-electron/src/store/preferences.js`:
- Around line 60-104: Add a Yup schema entry for keyBindings inside
preferencesSchema to validate the structure currently defined in the default
keyBindings object: ensure keyBindings is a Yup.object().shape(...) (or
Yup.record-style object) whose values are objects with required string fields
mac, windows, and name (use Yup.string().required()), and permit only that shape
to prevent corrupted/unknown bindings when saving preferences; update the
preferencesSchema symbol accordingly so that keyBindings is validated alongside
the other preference fields.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js`:
- Around line 1099-1106: The test currently comments out the assertion that the
environment editor opened, so it only checks the shortcut was saved but not that
it triggered the editor; restore and use the previously commented checks by
locating the tab with page.locator('.request-tab').filter({ hasText: 'Global
Environments' }) and asserting await expect(envTab).toBeVisible() after sending
the keyboard shortcut via page.keyboard.press(`${modifier}+KeyW`), and keep the
short wait (or replace waitForTimeout with a waitFor condition) to ensure the
tab has time to appear before the assertion.
- Around line 604-609: The test uses the wrong modifier when closing the
Preferences tab: replace the Cmd/Ctrl combo sent via
page.keyboard.press(`${modifier}+KeyT+KeyC`) with the actual custom binding for
closeTab (Alt+T+C) by either simulating the Alt chord with
page.keyboard.down('Alt'), page.keyboard.press('KeyT'),
page.keyboard.press('KeyC'), page.keyboard.up('Alt') or by closing the tab
through the UI using the existing locator (page.locator('.request-tab').filter({
hasText: 'Preferences' }).locator('.close-icon-container').click()); update the
code that references page.keyboard.press and the modifier variable accordingly
so the test triggers the new closeTab binding.
- Around line 764-778: The test simulates the custom "Close All Tabs" shortcut
by sending Alt+W+A (via page.keyboard.down/up) but still asserts three
'.request-tab' elements after the keypress; update the test to either (A) assert
the expected behavior like the default-binding test by changing the final
expectation on page.locator('.request-tab') to toHaveCount(1), or (B) if the
custom binding should differ, fix the key simulation to match the app's
registered shortcut (ensure the sequence and modifiers match how the app
listens) and then assert the correct resulting count; reference the keyboard
sequence code and the page.locator('.request-tab') assertion to locate the
change.
- Line 92: The test file contains stray double semicolons after statements
(e.g., the pattern "await page.waitForTimeout(200); ;") which are harmless but
untidy; remove the extra trailing semicolon occurrences so each statement ends
with a single ";" (search for ";;" or instances of "await page.waitForTimeout"
and similar lines in preference-shortcuts-edit.spec.js) and run the
linter/formatter to ensure no other duplicate-semicolon artifacts remain.
- Line 632: Remove the leftover debug statement console.log('Veried : 1') from
the test file; locate the statement in preference-shortcuts-edit.spec.js (the
console.log with the misspelled "Veried : 1") and delete that line so the test
contains no stray debug output.
- Line 3: Remove the unused Electron import to avoid runtime resolution
failures: delete the require('electron') line that imports BrowserWindow (the
unused symbol BrowserWindow) from
tests/shortcuts/preference-shortcuts-edit.spec.js and any references to
BrowserWindow in that test; if the test needs Electron functionality instead,
replace it with Playwright-compatible helpers or mocks rather than requiring
'electron'. Ensure linter passes after removal.
- Around line 5-2162: The tests overuse page.waitForTimeout and lack test.step
instrumentation, and the helper getError is defined but unused; update the key
interaction sequences (e.g., customize keybinding → verify → invoke shortcut) to
be wrapped in test.step calls, replace arbitrary await page.waitForTimeout(...)
with targeted awaits such as await expect(locator).toBeVisible(),
locator.waitFor(), or specific element state checks (use helpers
openKeybindingsTab, getEditBtn, getInput, getResetBtn as anchors to find the
flows), and either remove the unused getError helper or start using it where
keybinding error assertions are expected; ensure each replaced timeout waits for
the relevant locator/state to avoid flakiness and add concise test.step
descriptions for major actions.
---
Nitpick comments:
In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js`:
- Around line 186-201: Remove the redundant runtime guard on
DEFAULT_KEY_BINDINGS inside isRowDirty: delete the `if (!DEFAULT_KEY_BINDINGS)
return false;` check (it’s a module-level constant and already dereferenced by
getDefaultRowKeysString), so isRowDirty should simply compare
getCurrentRowKeysString(action) !== getDefaultRowKeysString(action). Leave
hasDirtyRows/useMemo as-is (it already iterates
Object.keys(DEFAULT_KEY_BINDINGS)); if you want safety, perform any validation
at module init rather than inside isRowDirty.
- Around line 464-469: Remove the orphan Tooltip JSX with
id="kb-editing-error-tooltip" since it has no anchorSelect and is never
attached; delete the <Tooltip id="kb-editing-error-tooltip" ... /> element (the
standalone tooltip block) and ensure that only the per-row Tooltip instances
(the tooltips used within each row) remain; also search for any references to
"kb-editing-error-tooltip" and remove or update them if found.
In
`@packages/bruno-app/src/components/Sidebar/Sections/CollectionsSection/index.js`:
- Around line 48-63: The current handleSidebarSearch in useEffect relies on
setTimeout + document.querySelector('.collection-search-input') which is
fragile; replace this with a React ref/callback: create a ref (e.g.,
searchInputRef) or a focus callback in the CollectionsSection and pass it into
the Search/Input component (rather than querying by class), call
setShowSearch(true) as before, then in a useEffect/useLayoutEffect that depends
on showSearch or in the input's ref callback, call
searchInputRef.current.focus() (remove the setTimeout and DOM query). Update
references to the class selector and the handleSidebarSearch logic so focusing
is driven by the ref (preserve the event listener add/remove in useEffect).
In `@packages/bruno-app/src/providers/Hotkeys/index.js`:
- Around line 333-372: HotkeysProvider is subscribing to tabs, collections and
activeTabUid (via useSelector) purely to derive currentCollection, causing
unnecessary re-renders; remove those top-level selectors and stop computing
currentCollection on every render in HotkeysProvider, and instead compute the
collection lazily when opening the New Request modal (e.g., inside the handler
that calls setShowNewRequestModal) using the redux store directly
(store.getState()) or move the modal UI into a separate component (e.g.,
NewRequestModal) that calls useSelector for tabs/collections/activeTabUid
itself; update getCurrentCollection/currentCollection usage accordingly so
bindAllHotkeys and the provider render path no longer depend on
tabs/collections/activeTabUid.
- Around line 85-87: The forEach callback in unbindAllHotkeys implicitly returns
the value from unbindHotkey (which can return Mousetrap.unbind()), triggering
static analysis; change the implementation to explicitly ignore the return
value—e.g., replace the BOUND_ACTIONS.forEach(...) with an explicit loop (for
(const action of BOUND_ACTIONS) { unbindHotkey(action, userKeyBindings); }) or
wrap the call with void to discard the result—so unbindAllHotkeys,
BOUND_ACTIONS, and unbindHotkey remain the same but no value is implicitly
returned from the callback.
In `@packages/bruno-app/src/providers/Hotkeys/keyMappings.js`:
- Around line 128-148: getKeyBindingsForActionAllOS currently calls
getMergedKeyBindings on every invocation causing O(N^2) work when the Hotkeys
provider asks per-action; fix by computing the merged bindings once and reusing
them: either change the Hotkeys provider to call
getMergedKeyBindings(userKeyBindings) once and pass the resulting merged object
into getKeyBindingsForActionAllOS (add a new parameter like mergedBindings) or
memoize getMergedKeyBindings (e.g., cache by reference or userKeyBindings
identity) so repeated calls return the same merged object; update references to
getKeyBindingsForActionAllOS and any calls from the Hotkeys provider to use the
new parameter or rely on the memoized result.
In `@packages/bruno-electron/src/store/preferences.js`:
- Around line 60-104: Duplicate default key bindings exist between keyBindings
(in preferences.js) and DEFAULT_KEY_BINDINGS (in keyMappings.js); extract them
to a single shared module and import from both places. Create a shared export
(e.g., defaultKeyBindings) containing the same shape (platform keys mac/windows
and name) and replace the inline object in preferences.js (keyBindings) and the
app-side DEFAULT_KEY_BINDINGS to import that shared symbol; ensure
function/class references that consume these values (e.g., anywhere expecting
keyBindings or DEFAULT_KEY_BINDINGS) still receive the same object shape and
update any TypeScript types or tests to reference the new module.
Comment on lines
219
to
278
| const validateCombo = (action, arrRaw) => { | ||
| const arr = uniqSorted(arrRaw); | ||
| const sig = comboSignature(arr); | ||
|
|
||
| if (!sig) return { code: ERROR.EMPTY, message: 'Shortcut can’t be empty.' }; | ||
| if (isOnlyModifiers(arr)) | ||
| return { code: ERROR.ONLY_MODIFIERS, message: 'Add a non-modifier key (e.g. Ctrl + K).' }; | ||
|
|
||
| // OS-specific must-have modifier rule | ||
| if (!hasRequiredModifier(os, arr)) { | ||
| return { | ||
| code: ERROR.MISSING_REQUIRED_MOD, | ||
| message: | ||
| os === 'mac' | ||
| ? 'macOS shortcuts must include at least one modifier (command/alt/shift/ctrl).' | ||
| : 'Windows shortcuts must include at least one modifier (ctrl/alt/shift).' | ||
| }; | ||
| } | ||
|
|
||
| // OS reserved | ||
| if (RESERVED_BY_OS[os]?.has(sig)) | ||
| return { code: ERROR.RESERVED, message: 'This shortcut is reserved by the OS.' }; | ||
|
|
||
| // No duplicates (across all other actions) | ||
| if (buildUsedSignatures(action).has(sig)) | ||
| return { code: ERROR.DUPLICATE, message: 'That shortcut is already in use.' }; | ||
|
|
||
| // Check for subset conflicts (e.g., Cmd+A conflicts with Cmd+Z+A) | ||
| for (const [otherAction, binding] of Object.entries(keyBindings)) { | ||
| if (otherAction === action) continue; | ||
| const otherKeysStr = binding?.[os]; | ||
| if (!otherKeysStr) continue; | ||
|
|
||
| const otherKeys = fromKeysString(otherKeysStr); | ||
|
|
||
| // Check if current is a subset of other (current is shorter) | ||
| if (arr.length < otherKeys.length) { | ||
| const isSubset = arr.every((k) => otherKeys.includes(k)); | ||
| if (isSubset) { | ||
| return { | ||
| code: ERROR.CONFLICT, | ||
| message: `Conflicts with "${binding.name}" (${otherKeys.join(' + ')}). Remove the longer shortcut first.` | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // Check if other is a subset of current (current is longer) | ||
| if (arr.length > otherKeys.length) { | ||
| const isSubset = otherKeys.every((k) => arr.includes(k)); | ||
| if (isSubset) { | ||
| return { | ||
| code: ERROR.CONFLICT, | ||
| message: `Conflicts with "${binding.name}" (${otherKeys.join(' + ')}). Remove that shortcut first.` | ||
| }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }; |
Contributor
There was a problem hiding this comment.
Subset-conflict validation may be overly restrictive for Mousetrap.
The conflict check (lines 254–274) flags Ctrl+K as conflicting with Ctrl+Shift+K because one is a "subset" of the other. Mousetrap treats these as distinct combos — modifiers are part of the match, so ctrl+k will not fire when ctrl+shift+k is pressed. This could prevent users from assigning perfectly valid non-overlapping shortcuts.
Consider whether this restriction is intentional. If it is, a brief inline comment explaining the rationale would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js` around
lines 219 - 278, The subset-conflict logic in validateCombo is too strict for
Mousetrap because it treats modifier differences as conflicts; update the loop
that compares arr and otherKeys (inside validateCombo) to only treat one combo
as a conflict if their modifier sets are identical (compute modifier sets from
arr and otherKeys via existing isModifier/isOnlyModifiers helpers) before
performing the subset checks, and add a short inline comment near those checks
explaining that Mousetrap treats combos with different modifier sets as distinct
so we only block true subset conflicts with the same modifiers; if you intended
the stricter behavior instead, replace the change with a concise comment
explaining that rationale.
Contributor
There was a problem hiding this comment.
Inconsistent apostrophe character (smart quote vs. straight quote).
Line 223 uses a Unicode right single quotation mark (', U+2019) in 'Shortcut can't be empty.', while line 405 uses an escaped straight apostrophe \'. Pick one style — straight apostrophe is standard in JS strings.
Suggested fix
- if (!sig) return { code: ERROR.EMPTY, message: 'Shortcut can\u2019t be empty.' };
+ if (!sig) return { code: ERROR.EMPTY, message: 'Shortcut can\'t be empty.' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js` at line
223, The string literal returned when sig is falsy uses a smart quote; change it
to a standard straight apostrophe to match project style and other occurrences:
update the return in the function that checks sig (the line returning { code:
ERROR.EMPTY, message: 'Shortcut can’t be empty.' }) to use a straight apostrophe
(e.g. 'Shortcut can\'t be empty.') so it matches the style used elsewhere (see
other uses of ERROR.EMPTY and escaped straight apostrophe).
packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js
Show resolved
Hide resolved
packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js
Outdated
Show resolved
Hide resolved
Comment on lines
64
to
65
| { type: 'separator' }, | ||
| { role: 'resetzoom' }, | ||
| { role: 'zoomin' }, | ||
| { role: 'zoomout' }, | ||
| { type: 'separator' }, |
Contributor
There was a problem hiding this comment.
Duplicate separator.
Two consecutive { type: 'separator' } entries produce an empty gap in the View menu.
Proposed fix
{ role: 'toggledevtools' },
{ type: 'separator' },
- { type: 'separator' },
{📝 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.
Suggested change
| { type: 'separator' }, | |
| { role: 'resetzoom' }, | |
| { role: 'zoomin' }, | |
| { role: 'zoomout' }, | |
| { type: 'separator' }, | |
| { type: 'separator' }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/app/menu-template.js` around lines 64 - 65, There
are two consecutive `{ type: 'separator' }` entries in the View menu template
causing an extra empty gap; remove one of the duplicate `{ type: 'separator' }`
objects in the menu template array (the View menu block) so only a single
separator remains between the adjacent menu groups.
Comment on lines
604
to
609
Contributor
There was a problem hiding this comment.
Wrong modifier used to close the preferences tab after customizing closeTab.
Line 608 uses ${modifier}+KeyT+KeyC (i.e., Cmd/Ctrl+T+C), but closeTab was just re-bound to Alt+T+C. The old default Cmd/Ctrl+W was unbound. This line should either use the new custom combo (Alt+T+C via keyboard.down/up) or close the tab via the UI (click the close icon).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js` around lines 604 - 609,
The test uses the wrong modifier when closing the Preferences tab: replace the
Cmd/Ctrl combo sent via page.keyboard.press(`${modifier}+KeyT+KeyC`) with the
actual custom binding for closeTab (Alt+T+C) by either simulating the Alt chord
with page.keyboard.down('Alt'), page.keyboard.press('KeyT'),
page.keyboard.press('KeyC'), page.keyboard.up('Alt') or by closing the tab
through the UI using the existing locator (page.locator('.request-tab').filter({
hasText: 'Preferences' }).locator('.close-icon-container').click()); update the
code that references page.keyboard.press and the modifier variable accordingly
so the test triggers the new closeTab binding.
Comment on lines
764
to
778
Contributor
There was a problem hiding this comment.
Suspicious assertion: customized "Close All Tabs" appears to not actually close anything.
Line 764 asserts 3 request tabs before pressing the customized Alt+W+A. Line 777 still expects 3 tabs afterwards (with comment "Overview / Global Environments / Preferences"). Compare with the default-binding test (line 699) which expects count to drop to 1. Either the customized binding isn't working as expected, or the assertion is wrong.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js` around lines 764 - 778,
The test simulates the custom "Close All Tabs" shortcut by sending Alt+W+A (via
page.keyboard.down/up) but still asserts three '.request-tab' elements after the
keypress; update the test to either (A) assert the expected behavior like the
default-binding test by changing the final expectation on
page.locator('.request-tab') to toHaveCount(1), or (B) if the custom binding
should differ, fix the key simulation to match the app's registered shortcut
(ensure the sequence and modifiers match how the app listens) and then assert
the correct resulting count; reference the keyboard sequence code and the
page.locator('.request-tab') assertion to locate the change.
Comment on lines
1099
to
1106
Contributor
There was a problem hiding this comment.
Commented-out assertions — test doesn't verify the environment editor actually opened.
Lines 1099–1101 comment out the assertion that the environment tab is visible after pressing the customized shortcut. Without this check, the test only verifies the keybinding was saved, not that it actually works.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js` around lines 1099 - 1106,
The test currently comments out the assertion that the environment editor
opened, so it only checks the shortcut was saved but not that it triggered the
editor; restore and use the previously commented checks by locating the tab with
page.locator('.request-tab').filter({ hasText: 'Global Environments' }) and
asserting await expect(envTab).toBeVisible() after sending the keyboard shortcut
via page.keyboard.press(`${modifier}+KeyW`), and keep the short wait (or replace
waitForTimeout with a waitFor condition) to ensure the tab has time to appear
before the assertion.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bruno-app/src/components/StatusBar/index.js (1)
5-6: 🛠️ Refactor suggestion | 🟠 MajorDead imports:
MousetrapandgetKeyBindingsForActionAllOSare no longer used.These two imports were only consumed by the old
openGlobalSearchimplementation. Now that it dispatches aCustomEvent, they're unused and should be removed.🧹 Proposed cleanup
-import Mousetrap from 'mousetrap'; -import { getKeyBindingsForActionAllOS } from 'providers/Hotkeys/keyMappings';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/StatusBar/index.js` around lines 5 - 6, Remove the dead imports Mousetrap and getKeyBindingsForActionAllOS from the module since they are no longer referenced after replacing the old openGlobalSearch implementation; locate the import lines importing Mousetrap and getKeyBindingsForActionAllOS in packages/bruno-app/src/components/StatusBar/index.js and delete those import entries (leaving any other imports intact) and run a quick lint/compile to ensure no residual references to Mousetrap or getKeyBindingsForActionAllOS remain (also verify openGlobalSearch now dispatches a CustomEvent and does not rely on them).packages/bruno-electron/src/index.js (1)
462-467:⚠️ Potential issue | 🟠 MajorZoom step of
+1here is inconsistent with±0.5everywhere else.The global shortcut at Line 466 increments zoom by
1, while all IPC handlers (Lines 228–278) use0.5. This creates a jarring UX whereCtrl+=zooms twice as aggressively as the menu/shortcut-driven zoom. Unify with a constant.Proposed fix
+const ZOOM_STEP = 0.5; + // Register the global shortcuts app.on('browser-window-focus', () => { // Quick fix for Electron issue `#29996`: https://github.com/electron/electron/issues/29996 globalShortcut.register('Ctrl+=', () => { - mainWindow.webContents.setZoomLevel(mainWindow.webContents.getZoomLevel() + 1); + mainWindow.webContents.setZoomLevel(mainWindow.webContents.getZoomLevel() + ZOOM_STEP); }); });Then reuse
ZOOM_STEPin the IPC handlers as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/index.js` around lines 462 - 467, The global shortcut increments zoom by 1 which conflicts with the 0.5 step used elsewhere; introduce a single ZOOM_STEP constant (set to 0.5) and use it in the globalShortcut.register callback (replace the hardcoded +1 with ZOOM_STEP applied to mainWindow.webContents.getZoomLevel()/setZoomLevel) and update the IPC zoom handlers (the handlers that increment/decrement zoom) to also use ZOOM_STEP so all zoom changes are consistent across globalShortcut.register, mainWindow.webContents.setZoomLevel/getZoomLevel and the IPC zoom handlers.
🧹 Nitpick comments (1)
packages/bruno-electron/src/index.js (1)
228-233: Missingnullguard onmainWindowis inconsistent across handlers.The new
main:zoom-*handlers guard withif (mainWindow && mainWindow.webContents), but the existingrenderer:zoom-in/outhandlers at Lines 270–278 and the global shortcut at Line 466 accessmainWindow.webContentsdirectly without a guard. Either all handlers should guard or none should — pick one convention. SincemainWindowis module-scoped and these handlers only fire after the window is created, the guards here are likely unnecessary noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/index.js` around lines 228 - 233, The new handlers ipcMain.on('main:zoom-in') and ipcMain.on('main:zoom-out') add redundant null checks for mainWindow and mainWindow.webContents that are inconsistent with the existing renderer:zoom-in/renderer:zoom-out handlers and the global shortcut usage; remove the if (mainWindow && mainWindow.webContents) guard in those handlers and directly call mainWindow.webContents.getZoomLevel()/setZoomLevel(...) so behavior and convention match the other zoom handlers and the global shortcut that already assume mainWindow exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/bruno-app/src/components/StatusBar/index.js`:
- Around line 5-6: Remove the dead imports Mousetrap and
getKeyBindingsForActionAllOS from the module since they are no longer referenced
after replacing the old openGlobalSearch implementation; locate the import lines
importing Mousetrap and getKeyBindingsForActionAllOS in
packages/bruno-app/src/components/StatusBar/index.js and delete those import
entries (leaving any other imports intact) and run a quick lint/compile to
ensure no residual references to Mousetrap or getKeyBindingsForActionAllOS
remain (also verify openGlobalSearch now dispatches a CustomEvent and does not
rely on them).
In `@packages/bruno-electron/src/index.js`:
- Around line 462-467: The global shortcut increments zoom by 1 which conflicts
with the 0.5 step used elsewhere; introduce a single ZOOM_STEP constant (set to
0.5) and use it in the globalShortcut.register callback (replace the hardcoded
+1 with ZOOM_STEP applied to mainWindow.webContents.getZoomLevel()/setZoomLevel)
and update the IPC zoom handlers (the handlers that increment/decrement zoom) to
also use ZOOM_STEP so all zoom changes are consistent across
globalShortcut.register, mainWindow.webContents.setZoomLevel/getZoomLevel and
the IPC zoom handlers.
---
Duplicate comments:
In `@packages/bruno-electron/src/index.js`:
- Around line 248-251: The commented-out platform guard in the ipcMain listener
for 'renderer:window-close' is dead code; either remove the comment line
entirely or restore the platform-specific behavior by reintroducing the guard
that checks isWindows/isLinux inside the ipcMain.on('renderer:window-close',
...) handler and conditionally call mainWindow.hide() on macOS (i.e., when not
isWindows and not isLinux) and mainWindow.close() otherwise; update the ipcMain
handler around the 'renderer:window-close' listener accordingly and ensure
isWindows/isLinux symbols are referenced or removed consistently.
- Around line 196-197: Reviewer flagged webviewTag: true in the BrowserWindow
webPreferences as a potential security risk and the zoomFactor comment as
misleading; verify whether the app actually uses <webview> and if not remove
webviewTag from webPreferences, otherwise document and constrain its usage
(ensure loaded content is trusted, enable appropriate webview sandboxing
options), and update the zoomFactor comment to accurately state its purpose;
search for occurrences using the webviewTag symbol and <webview> usage in
renderers and then either remove the setting and switch to BrowserView/iframe
with webContents or keep it with added security controls and clearer comment
next to the zoomFactor and webPreferences configuration.
- Around line 227-246: The three ipcMain.on handlers for "main:zoom-in",
"main:zoom-out", and "main:zoom-reset" duplicate the existing
"renderer:zoom-in", "renderer:zoom-out", and "renderer:reset-zoom" logic and
also use a different signaling pattern; remove the duplicate ipcMain.on blocks
and consolidate by wiring the "main:*" channels to the same implementations used
by the renderer handlers (or replace the "main:*" registrations with
ipcMain.handle calls that call the same zoom logic). Specifically, locate the
existing renderer handlers (ipcMain.handle('renderer:zoom-in' /
'renderer:zoom-out' / 'renderer:reset-zoom')) and either: 1) register equivalent
ipcMain.handle('main:zoom-in' / 'main:zoom-out' / 'main:zoom-reset') that invoke
the same code path that uses mainWindow.webContents.getZoomLevel()/setZoomLevel,
or 2) remove the duplicated main: handlers and map incoming main:* messages to
call the renderer:* handlers internally; ensure you use ipcMain.handle
consistently and reference mainWindow.webContents within the shared handler.
---
Nitpick comments:
In `@packages/bruno-electron/src/index.js`:
- Around line 228-233: The new handlers ipcMain.on('main:zoom-in') and
ipcMain.on('main:zoom-out') add redundant null checks for mainWindow and
mainWindow.webContents that are inconsistent with the existing
renderer:zoom-in/renderer:zoom-out handlers and the global shortcut usage;
remove the if (mainWindow && mainWindow.webContents) guard in those handlers and
directly call mainWindow.webContents.getZoomLevel()/setZoomLevel(...) so
behavior and convention match the other zoom handlers and the global shortcut
that already assume mainWindow exists.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js (1)
580-601:⚠️ Potential issue | 🔴 CriticalRules of Hooks violation —
useCallbackcalled after early returns.Lines 478–488 contain conditional
return nullstatements, butuseCallbackat Lines 580 and 586 runs after them. React requires hooks to execute in the same order on every render. WhensearchTextfilters out this item, these hooks are skipped, which will cause runtime errors.Move both
useCallbackdeclarations (and the ref assignments at Lines 603–605) above the early returns — near the other hooks at the top of the component.🐛 Proposed fix — move hooks above the early return block
+ const handleCopyItem = useCallback(() => { + dispatch(copyRequest(item)); + const itemType = isFolder ? 'Folder' : 'Request'; + toast.success(`${itemType} copied`); + }, [dispatch, item, isFolder]); + + const handlePasteItem = useCallback(() => { + let targetFolderUid = item.uid; + if (!isFolder) { + const parentFolder = findParentItemInCollection(collection, item.uid); + targetFolderUid = parentFolder ? parentFolder.uid : null; + } + dispatch(pasteItem(collectionUid, targetFolderUid)) + .then(() => { + toast.success('Item pasted successfully'); + }) + .catch((err) => { + toast.error(err ? err.message : 'An error occurred while pasting the item'); + }); + }, [dispatch, collection, item, isFolder, collectionUid]); + + // Update refs whenever handlers change + copyHandlerRef.current = handleCopyItem; + pasteHandlerRef.current = handlePasteItem; + const className = classnames('flex flex-col w-full', { 'is-sidebar-dragging': isSidebarDragging }); if (searchText && searchText.length) { // ... early returns ... } // ... other non-hook code ... - const handleCopyItem = useCallback(() => { - dispatch(copyRequest(item)); - const itemType = isFolder ? 'Folder' : 'Request'; - toast.success(`${itemType} copied`); - }, [dispatch, item, isFolder]); - - const handlePasteItem = useCallback(() => { - let targetFolderUid = item.uid; - if (!isFolder) { - const parentFolder = findParentItemInCollection(collection, item.uid); - targetFolderUid = parentFolder ? parentFolder.uid : null; - } - dispatch(pasteItem(collectionUid, targetFolderUid)) - .then(() => { - toast.success('Item pasted successfully'); - }) - .catch((err) => { - toast.error(err ? err.message : 'An error occurred while pasting the item'); - }); - }, [dispatch, collection, item, isFolder, collectionUid]); - - // Update refs whenever handlers change - copyHandlerRef.current = handleCopyItem; - pasteHandlerRef.current = handlePasteItem;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js` around lines 580 - 601, The useCallback hooks handleCopyItem and handlePasteItem (which dispatch copyRequest and pasteItem and reference findParentItemInCollection, dispatch, collection, item, isFolder, collectionUid) and the related ref assignments must be moved above any conditional early returns so hooks run unconditionally and in consistent order; relocate those useCallback declarations and the ref initializations to the top of the component near the other hooks (keeping their current dependency arrays) and then keep the existing logic for early returns below them.
🧹 Nitpick comments (3)
packages/bruno-app/src/components/Preferences/Keybindings/index.js (1)
464-469: Unused top-levelTooltipcomponent.This
Tooltipwithid="kb-editing-error-tooltip"is rendered at lines 464-469 but has noanchorSelectand is never referenced. The per-row tooltips (lines 536-544) each use their ownid. Remove this dead element.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js` around lines 464 - 469, Remove the unused top-level Tooltip element with id="kb-editing-error-tooltip" since it has no anchorSelect and is never referenced; locate the JSX Tooltip component instance (Tooltip id="kb-editing-error-tooltip") inside the Keybindings component and delete that element so only the per-row tooltips (the ones around lines where individual rows create their own Tooltip ids) remain.packages/bruno-app/src/providers/Hotkeys/index.js (2)
338-356: Redundant double-unbind on keybinding changes.The
prevKeyBindingsRefunbind (lines 341-346) duplicates the work done by the effect's cleanup function (lines 352-355). WhenuserKeyBindingschanges from A → B:
- Cleanup fires →
unbindAllHotkeys(A)✓- New effect →
unbindAllHotkeys(A)again via ref (no-op but wasteful)The ref is unnecessary since the effect already depends on
[userKeyBindings]and the cleanup captures the correct closure. Consider removing the ref-based unbind and keeping only the cleanup return.Simplified version
- const prevKeyBindingsRef = useRef(undefined); ... useEffect(() => { - const prevBindings = prevKeyBindingsRef.current; - if (prevBindings !== undefined) { - unbindAllHotkeys(prevBindings); - } bindAllHotkeys(userKeyBindings); - prevKeyBindingsRef.current = userKeyBindings; return () => { unbindAllHotkeys(userKeyBindings); }; }, [userKeyBindings]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/Hotkeys/index.js` around lines 338 - 356, The effect currently double-unbinds by using prevKeyBindingsRef plus the cleanup; remove the prevKeyBindingsRef logic and its conditional call to unbindAllHotkeys so the effect only calls bindAllHotkeys(userKeyBindings) and its cleanup returns unbindAllHotkeys(userKeyBindings). Specifically, delete references to prevKeyBindingsRef and the block that reads/unbinds prevBindings, keep bindAllHotkeys(userKeyBindings) and the return cleanup that calls unbindAllHotkeys(userKeyBindings) inside the useEffect that depends on userKeyBindings.
84-86: Static analysis: forEach callback implicitly returns a value.Biome flags the implicit return from the arrow in
BOUND_ACTIONS.forEach(...). Wrap the body in braces to make the void intent explicit.Suggested fix
function unbindAllHotkeys(userKeyBindings) { - BOUND_ACTIONS.forEach((action) => unbindHotkey(action, userKeyBindings)); + BOUND_ACTIONS.forEach((action) => { unbindHotkey(action, userKeyBindings); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/Hotkeys/index.js` around lines 84 - 86, The forEach callback in unbindAllHotkeys currently uses a concise arrow that implicitly returns a value; change the callback to a block-bodied arrow to make the void intent explicit: update BOUND_ACTIONS.forEach((action) => unbindHotkey(action, userKeyBindings)) to use braces and an explicit statement body so it reads like BOUND_ACTIONS.forEach((action) => { unbindHotkey(action, userKeyBindings); }); this keeps behavior the same while satisfying the static analyzer that the callback returns void (refer to the unbindAllHotkeys function, BOUND_ACTIONS.forEach call, and unbindHotkey).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js`:
- Around line 280-294: persistToPreferences is setting the key-binding "name" to
the camelCase action when no existing preference exists, which overwrites the
human-readable name from DEFAULT_KEY_BINDINGS on first edit; change the fallback
so the name is taken from DEFAULT_KEY_BINDINGS for that action if present (e.g.,
use preferences?.keyBindings?.[action]?.name ||
DEFAULT_KEY_BINDINGS?.[action]?.name || action) before dispatching
savePreferences, so the merge with the default key bindings preserves the
human-readable label; update the logic in persistToPreferences and ensure
DEFAULT_KEY_BINDINGS is imported/available where used.
- Line 501: The current showPencil boolean (const showPencil = isHovered &&
!isEditing && !isDirty) hides the pencil once a row is dirty; update the
condition so the pencil is shown when hovered and not editing even if isDirty
(e.g., const showPencil = isHovered && !isEditing) or alternatively render both
the pencil and the reset/refresh icon when isDirty so users can re-open edit
mode without resetting; change the logic around showPencil and the rendering
that checks isDirty to allow the pencil icon to render for dirty rows.
- Around line 186-201: Remove the dead guard checking DEFAULT_KEY_BINDINGS and
eliminate the stale closure by defining isRowDirty inside the hasDirtyRows
useMemo (so it closes over current keyBindings/os), then iterate
Object.keys(DEFAULT_KEY_BINDINGS) inside that same useMemo and return true if
any getCurrentRowKeysString(action) !== getDefaultRowKeysString(action); keep
useMemo deps [keyBindings, os] (no external closure over isRowDirty) and
reference the symbols isRowDirty, getCurrentRowKeysString,
getDefaultRowKeysString, DEFAULT_KEY_BINDINGS, and hasDirtyRows when making the
change.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js`:
- Around line 696-759: The test sets a custom keybinding for closeAllTabs to
Alt+W+A but never triggers it; after opening and pinning the three request tabs,
simulate the custom shortcut (e.g., emulate the recorded multi-key sequence by
sending Alt down, KeyW down/up, KeyA down/up, then Alt up using page.keyboard
like in the earlier recording block) and then assert the tab count has dropped
(use the same locator '.request-tab' and expect count === 1) to verify the
custom binding invoked closeAllTabs; references: openKeybindingsTab,
getEditBtn('closeAllTabs'), getInput('closeAllTabs'), and the existing
page.keyboard down/up sequence used earlier in the test.
- Around line 1686-1689: The assertion using requestCount >= 1 is too weak;
update the test that uses allRequests/page.locator('.collection-item-name') to
assert requestCount >= 2 to ensure the paste created a second item, and also add
an explicit visibility check for the original request (same check used in the
default test at line ~1580) to confirm both original and pasted items are
present; apply the same change to the folder copy/paste test around the other
failing assertion (near line 1816) so both tests validate paste success.
- Around line 1079-1150: The zoom tests (tests named "FUNCTIONAL: Zoom In" and
the customized variant using getEditBtn/openKeybindingsTab/getInput) currently
only send key events with no verification; update each zoom-in, zoom-out and
reset-zoom test to capture a measurable zoom indicator before and after the
shortcut (e.g., via page.evaluate(() => window.devicePixelRatio) or computing an
element's computedStyle width/height), then assert the value changed in the
expected direction for zoom-in/zoom-out and returned to the base value for
reset-zoom; use the existing test scaffolding (the test blocks, modifier
variable, and helper functions like getEditBtn and getInput) to place the checks
immediately after the key presses and include clear expect assertions comparing
before/after values.
- Around line 1710-1714: There is a duplicate call to openKeybindingsTab()
before interacting with keybinding rows; remove the redundant invocation so the
test only calls openKeybindingsTab() once (the existing call that opens
Preferences and clicks the Keybindings tab) and then proceeds to hover on
resetCopyRow and call getResetBtn(page, 'copyItem').click(); ensure references
to openKeybindingsTab, resetCopyRow, and getResetBtn remain intact.
- Around line 1019-1076: The test sets a custom combo for editEnvironment but
never invokes it — instead it closes tabs and asserts the environment tab is not
visible; change the end of the test to actually trigger the saved shortcut
sequence and assert the environment editor opens: after saving the keybinding
(using getEditBtn and getInput) and closing preferences, simulate the multi-key
sequence (Alt down → KeyE → KeyG → release keys) the same way you recorded it,
then await and expect the envTab locator ('.request-tab' filtered by
'Environments') to be visible; remove or replace the erroneous close-tab +
expect(...).not.toBeVisible() assertions so the test verifies the environment
tab is shown after pressing the custom shortcut.
---
Outside diff comments:
In
`@packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js`:
- Around line 580-601: The useCallback hooks handleCopyItem and handlePasteItem
(which dispatch copyRequest and pasteItem and reference
findParentItemInCollection, dispatch, collection, item, isFolder, collectionUid)
and the related ref assignments must be moved above any conditional early
returns so hooks run unconditionally and in consistent order; relocate those
useCallback declarations and the ref initializations to the top of the component
near the other hooks (keeping their current dependency arrays) and then keep the
existing logic for early returns below them.
---
Duplicate comments:
In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js`:
- Line 223: Replace the smart curly apostrophe in the template literal that
returns `{ code: ERROR.EMPTY, message: \`Shortcut can’t be empty.\` }` with a
straight apostrophe to match the rest of the file (use `Shortcut can't be
empty.`), ensuring the string style is consistent with the other occurrence that
uses `\'`; update the message exactly where that return is generated so it
matches the escaped straight-quote convention used elsewhere.
- Around line 246-275: The subset-conflict check in the loop that iterates
keyBindings (using arr, otherKeys, fromKeysString, ERROR.CONFLICT, binding.name)
is falsely flagging combos like Ctrl+K vs Ctrl+Shift+K; update the logic in the
same function (the block doing "Check for subset conflicts") to treat modifier
keys separately: parse modifiers and the main key(s) from arr and otherKeys
(using fromKeysString), and only consider a subset conflict when the modifier
sets are identical (or when both have no modifiers) and the non-modifier key
sequence is a strict subset; otherwise allow different modifier sets (e.g., Ctrl
vs Ctrl+Shift) as non-conflicting and do not return ERROR.CONFLICT.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js`:
- Line 22: The helper function getError (const getError = (page, action) =>
page.getByTestId(`keybinding-error-${action}`)) is defined but never used;
either delete this unused helper to clean up the test file or replace direct
error selectors in relevant tests with getError(page, '...') to assert
validation messages (e.g., in the keybinding validation/error test cases).
Locate the getError declaration and either remove it or update tests that check
error elements to call getError for clarity and reuse.
- Around line 4-11: The test file overuses waitForTimeout and lacks test.step
blocks; update the suite by replacing arbitrary waits with targeted locator
waits (e.g., replace calls like page.waitForTimeout(...) with await
page.locator('<selector>').waitFor() or await
expect(page.locator('<selector>')).toBeVisible()) and add test.step wrappers
around logical interaction sequences (wrap setup/navigation/assertion groups
inside test.step in the describe block and inside test.beforeEach where setup
occurs); search for symbols like modifier, modifierName, and test.beforeEach to
locate initialization and all test cases in this file and refactor each
occurrence of waitForTimeout to a locator-based wait and group related actions
into test.step blocks for clearer reporting and more reliable timing.
- Around line 603-604: The test uses the wrong keyboard chord to close
Preferences: update the press call that currently uses `${modifier}+KeyT+KeyC`
to either simulate the new binding `Alt+KeyT+KeyC` (replace `${modifier}` with
`Alt`) so it sends the Alt+T+C chord, or replace the keyboard press with a click
on the Preferences close UI button (use the existing selector for the close
button in the preferences modal) to ensure the tab actually closes before
continuing; change the line containing the press call and any nearby wait to
reflect the chosen approach.
---
Nitpick comments:
In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js`:
- Around line 464-469: Remove the unused top-level Tooltip element with
id="kb-editing-error-tooltip" since it has no anchorSelect and is never
referenced; locate the JSX Tooltip component instance (Tooltip
id="kb-editing-error-tooltip") inside the Keybindings component and delete that
element so only the per-row tooltips (the ones around lines where individual
rows create their own Tooltip ids) remain.
In `@packages/bruno-app/src/providers/Hotkeys/index.js`:
- Around line 338-356: The effect currently double-unbinds by using
prevKeyBindingsRef plus the cleanup; remove the prevKeyBindingsRef logic and its
conditional call to unbindAllHotkeys so the effect only calls
bindAllHotkeys(userKeyBindings) and its cleanup returns
unbindAllHotkeys(userKeyBindings). Specifically, delete references to
prevKeyBindingsRef and the block that reads/unbinds prevBindings, keep
bindAllHotkeys(userKeyBindings) and the return cleanup that calls
unbindAllHotkeys(userKeyBindings) inside the useEffect that depends on
userKeyBindings.
- Around line 84-86: The forEach callback in unbindAllHotkeys currently uses a
concise arrow that implicitly returns a value; change the callback to a
block-bodied arrow to make the void intent explicit: update
BOUND_ACTIONS.forEach((action) => unbindHotkey(action, userKeyBindings)) to use
braces and an explicit statement body so it reads like
BOUND_ACTIONS.forEach((action) => { unbindHotkey(action, userKeyBindings); });
this keeps behavior the same while satisfying the static analyzer that the
callback returns void (refer to the unbindAllHotkeys function,
BOUND_ACTIONS.forEach call, and unbindHotkey).
Comment on lines
+186
to
+201
| const isRowDirty = (action) => { | ||
| const current = getCurrentRowKeysString(action); | ||
| const def = getDefaultRowKeysString(action); | ||
| if (!DEFAULT_KEY_BINDINGS) return false; | ||
| return current !== def; | ||
| }; | ||
|
|
||
| // Check if any keybinding is dirty (different from default) | ||
| const hasDirtyRows = useMemo(() => { | ||
| for (const action of Object.keys(DEFAULT_KEY_BINDINGS)) { | ||
| if (isRowDirty(action)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| }, [keyBindings, os]); |
Contributor
There was a problem hiding this comment.
hasDirtyRows has a stale closure and a dead guard.
Two issues:
- Line 189:
if (!DEFAULT_KEY_BINDINGS) return false;—DEFAULT_KEY_BINDINGSis a module-level constant object, so this guard is always falsy and dead code. isRowDirtyis defined outsideuseMemoand closes overkeyBindings(viagetCurrentRowKeysString), butisRowDirtyitself isn't listed as a dependency. The lint rule for exhaustive deps would flag this. SincekeyBindingsis in the dep array it works in practice, but calling a function that implicitly captures state insideuseMemowithout listing it is fragile.
Suggested fix
const isRowDirty = (action) => {
const current = getCurrentRowKeysString(action);
const def = getDefaultRowKeysString(action);
- if (!DEFAULT_KEY_BINDINGS) return false;
return current !== def;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js` around
lines 186 - 201, Remove the dead guard checking DEFAULT_KEY_BINDINGS and
eliminate the stale closure by defining isRowDirty inside the hasDirtyRows
useMemo (so it closes over current keyBindings/os), then iterate
Object.keys(DEFAULT_KEY_BINDINGS) inside that same useMemo and return true if
any getCurrentRowKeysString(action) !== getDefaultRowKeysString(action); keep
useMemo deps [keyBindings, os] (no external closure over isRowDirty) and
reference the symbols isRowDirty, getCurrentRowKeysString,
getDefaultRowKeysString, DEFAULT_KEY_BINDINGS, and hasDirtyRows when making the
change.
Comment on lines
+280
to
+294
| const persistToPreferences = (action, nextKeys) => { | ||
| const updatedPreferences = { | ||
| ...preferences, | ||
| keyBindings: { | ||
| ...(preferences?.keyBindings || {}), | ||
| [action]: { | ||
| ...(preferences?.keyBindings?.[action] || {}), | ||
| name: preferences?.keyBindings?.[action]?.name || action, | ||
| [os]: nextKeys | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| dispatch(savePreferences(updatedPreferences)); | ||
| }; |
Contributor
There was a problem hiding this comment.
persistToPreferences stores wrong name on first edit.
Line 287 falls back to action (e.g., 'globalSearch') as the name when preferences?.keyBindings?.[action]?.name is undefined (i.e., the first time a user edits that binding). This camelCase key then overwrites the human-readable name from DEFAULT_KEY_BINDINGS during the merge at lines 148-155, causing the table and conflict error messages to display "globalSearch" instead of "Global Search".
Suggested fix
const persistToPreferences = (action, nextKeys) => {
const updatedPreferences = {
...preferences,
keyBindings: {
...(preferences?.keyBindings || {}),
[action]: {
...(preferences?.keyBindings?.[action] || {}),
- name: preferences?.keyBindings?.[action]?.name || action,
+ name: preferences?.keyBindings?.[action]?.name || DEFAULT_KEY_BINDINGS[action]?.name || action,
[os]: nextKeys
}
}
};
dispatch(savePreferences(updatedPreferences));
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js` around
lines 280 - 294, persistToPreferences is setting the key-binding "name" to the
camelCase action when no existing preference exists, which overwrites the
human-readable name from DEFAULT_KEY_BINDINGS on first edit; change the fallback
so the name is taken from DEFAULT_KEY_BINDINGS for that action if present (e.g.,
use preferences?.keyBindings?.[action]?.name ||
DEFAULT_KEY_BINDINGS?.[action]?.name || action) before dispatching
savePreferences, so the merge with the default key bindings preserves the
human-readable label; update the logic in persistToPreferences and ensure
DEFAULT_KEY_BINDINGS is imported/available where used.
| const isHovered = hoveredAction === action; | ||
| const isDirty = isRowDirty(action); | ||
|
|
||
| const showPencil = isHovered && !isEditing && !isDirty; |
Contributor
There was a problem hiding this comment.
Users can't re-edit an already-customized keybinding without resetting first.
showPencil is isHovered && !isEditing && !isDirty, meaning once a row is dirty (customized), the pencil icon disappears and only the reset button shows. To change a customized binding again, the user must reset to default first, then re-edit.
Consider showing both the pencil and refresh icons when the row is dirty, so users can directly re-edit.
Suggested fix
-const showPencil = isHovered && !isEditing && !isDirty;
-const showRefresh = isDirty && !isEditing;
+const showPencil = isHovered && !isEditing;
+const showRefresh = isDirty && !isEditing;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/Preferences/Keybindings/index.js` at line
501, The current showPencil boolean (const showPencil = isHovered && !isEditing
&& !isDirty) hides the pencil once a row is dirty; update the condition so the
pencil is shown when hovered and not editing even if isDirty (e.g., const
showPencil = isHovered && !isEditing) or alternatively render both the pencil
and the reset/refresh icon when isDirty so users can re-open edit mode without
resetting; change the logic around showPencil and the rendering that checks
isDirty to allow the pencil icon to render for dirty rows.
Comment on lines
+696
to
+759
| test('should close all tabs using customized-1 Alt+W+A', async ({ page, createTmpDir }) => { | ||
| // Close all collections, tabs, and preferences first | ||
| await closeAllCollections(page); | ||
|
|
||
| // Close any open preference tabs | ||
| const preferenceTabs = page.locator('.request-tab').filter({ hasText: 'Preferences' }); | ||
| const prefTabCount = await preferenceTabs.count(); | ||
| for (let i = 0; i < prefTabCount; i++) { | ||
| await page.keyboard.press(`${modifier}+KeyW`); | ||
| await page.waitForTimeout(200); | ||
| } | ||
|
|
||
| // Open Keybindings preferences and customize closeAllTabs FIRST | ||
| await openKeybindingsTab(page); | ||
|
|
||
| const row = page.getByTestId('keybinding-row-closeAllTabs'); | ||
| await row.hover(); | ||
|
|
||
| // Start recording | ||
| await getEditBtn(page, 'closeAllTabs').click(); | ||
| await page.waitForTimeout(300); | ||
|
|
||
| // Press new combo: Alt+W+A (multi-key sequence) | ||
| // Use down/up to ensure proper key recording | ||
| await page.keyboard.down('Alt'); | ||
| await page.keyboard.down('KeyW'); | ||
| await page.keyboard.down('KeyA'); | ||
| await page.waitForTimeout(200); | ||
| await page.keyboard.up('KeyA'); | ||
| await page.keyboard.up('KeyW'); | ||
| await page.keyboard.up('Alt'); | ||
| await page.waitForTimeout(500); | ||
|
|
||
| // Verify the keybinding was saved | ||
| const input = getInput(page, 'closeAllTabs'); | ||
| const newValue = await input.inputValue(); | ||
| expect(newValue).toContain('alt'); | ||
| expect(newValue).toContain('w'); | ||
| expect(newValue).toContain('a'); | ||
|
|
||
| // Close preferences | ||
| await page.keyboard.press(`${modifier}+KeyW`); | ||
| await page.waitForTimeout(500); | ||
|
|
||
| // Now create collection and requests | ||
| const collectionPath = await createTmpDir('close-all-tabs-customized-1'); | ||
| await createCollection(page, 'test-collection-close-all-customized-1', collectionPath); | ||
|
|
||
| // Create multiple requests | ||
| await createRequest(page, 'request-1', 'test-collection-close-all-customized-1'); | ||
| await createRequest(page, 'request-2', 'test-collection-close-all-customized-1'); | ||
| await createRequest(page, 'request-3', 'test-collection-close-all-customized-1'); | ||
|
|
||
| // Open and pin all requests | ||
| await openRequest(page, 'test-collection-close-all-customized-1', 'request-1', { persist: true }); | ||
| await openRequest(page, 'test-collection-close-all-customized-1', 'request-2', { persist: true }); | ||
| await openRequest(page, 'test-collection-close-all-customized-1', 'request-3', { persist: true }); | ||
|
|
||
| // Wait for tabs to be ready | ||
| await page.waitForTimeout(500); | ||
|
|
||
| // Verify all tabs are closed | ||
| await expect(page.locator('.request-tab')).toHaveCount(3); // Overview / Global Environments / Preferences | ||
| }); |
Contributor
There was a problem hiding this comment.
"Close All Tabs" customized test never invokes the custom shortcut.
The test customizes closeAllTabs to Alt+W+A, opens 3 request tabs, asserts toHaveCount(3), then ends. It never presses Alt+W+A to actually close the tabs and verify the custom binding works. The comment on line 757 even says "Verify all tabs are closed" while asserting count === 3.
Compare with the default test at line 689 which actually presses the shortcut and asserts tabs drop to 1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js` around lines 696 - 759,
The test sets a custom keybinding for closeAllTabs to Alt+W+A but never triggers
it; after opening and pinning the three request tabs, simulate the custom
shortcut (e.g., emulate the recorded multi-key sequence by sending Alt down,
KeyW down/up, KeyA down/up, then Alt up using page.keyboard like in the earlier
recording block) and then assert the tab count has dropped (use the same locator
'.request-tab' and expect count === 1) to verify the custom binding invoked
closeAllTabs; references: openKeybindingsTab, getEditBtn('closeAllTabs'),
getInput('closeAllTabs'), and the existing page.keyboard down/up sequence used
earlier in the test.
Comment on lines
+1019
to
+1076
| test('should open environment editor using customized-1 Alt+E+G', async ({ page, createTmpDir }) => { | ||
| // Close all collections, tabs, and preferences first | ||
| await closeAllCollections(page); | ||
|
|
||
| // Close any open preference tabs | ||
| const preferenceTabs = page.locator('.request-tab').filter({ hasText: 'Preferences' }); | ||
| const prefTabCount = await preferenceTabs.count(); | ||
| for (let i = 0; i < prefTabCount; i++) { | ||
| await page.keyboard.press(`${modifier}+KeyW`); | ||
| await page.waitForTimeout(200); | ||
| } | ||
|
|
||
| // Open Keybindings preferences and customize editEnvironment FIRST | ||
| await openKeybindingsTab(page); | ||
|
|
||
| const row = page.getByTestId('keybinding-row-editEnvironment'); | ||
| await row.hover(); | ||
|
|
||
| // Start recording | ||
| await getEditBtn(page, 'editEnvironment').click(); | ||
| await page.waitForTimeout(300); | ||
|
|
||
| // Press new combo: Alt+E+G (multi-key sequence) | ||
| // Use down/up to ensure proper key recording | ||
| await page.keyboard.down('Alt'); | ||
| await page.keyboard.down('KeyE'); | ||
| await page.keyboard.down('KeyG'); | ||
| await page.waitForTimeout(200); | ||
| await page.keyboard.up('KeyG'); | ||
| await page.keyboard.up('KeyE'); | ||
| await page.keyboard.up('Alt'); | ||
| await page.waitForTimeout(500); | ||
|
|
||
| // Verify the keybinding was saved | ||
| const input = getInput(page, 'editEnvironment'); | ||
| const newValue = await input.inputValue(); | ||
| expect(newValue).toContain('alt'); | ||
| expect(newValue).toContain('e'); | ||
| expect(newValue).toContain('g'); | ||
|
|
||
| // Close preferences | ||
| await page.keyboard.press(`${modifier}+KeyW`); | ||
| await page.waitForTimeout(500); | ||
|
|
||
| // Now create collection | ||
| const collectionPath = await createTmpDir('edit-environment-customized-1'); | ||
| await createCollection(page, 'test-collection-environment-customized-1', collectionPath); | ||
|
|
||
| // Wait for collection to be ready | ||
| await page.waitForTimeout(500); | ||
|
|
||
| // Close the environment tab | ||
| await page.keyboard.press(`${modifier}+KeyW`); | ||
| await page.waitForTimeout(300); | ||
|
|
||
| const envTab = page.locator('.request-tab').filter({ hasText: 'Environments' }); | ||
| await expect(envTab).not.toBeVisible(); | ||
| }); |
Contributor
There was a problem hiding this comment.
"Edit Environment" customized test never invokes the custom shortcut.
The test binds editEnvironment to Alt+E+G and verifies the binding was saved, but never presses Alt+E+G to verify that the environment editor actually opens. Instead, it closes a tab and asserts envTab is not visible — the opposite of what should be tested.
Compare with the default test (line 1003) which presses Cmd/Ctrl+E and asserts the environment tab is visible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js` around lines 1019 - 1076,
The test sets a custom combo for editEnvironment but never invokes it — instead
it closes tabs and asserts the environment tab is not visible; change the end of
the test to actually trigger the saved shortcut sequence and assert the
environment editor opens: after saving the keybinding (using getEditBtn and
getInput) and closing preferences, simulate the multi-key sequence (Alt down →
KeyE → KeyG → release keys) the same way you recorded it, then await and expect
the envTab locator ('.request-tab' filtered by 'Environments') to be visible;
remove or replace the erroneous close-tab + expect(...).not.toBeVisible()
assertions so the test verifies the environment tab is shown after pressing the
custom shortcut.
Comment on lines
+1079
to
+1150
| test.describe('FUNCTIONAL: Zoom In', () => { | ||
| test('should zoom in using default Cmd/Ctrl+=', async ({ page, createTmpDir }) => { | ||
| // Close all collections first for clean state | ||
| await closeAllCollections(page); | ||
|
|
||
| const collectionPath = await createTmpDir('zoom-in-default'); | ||
| await createCollection(page, 'test-collection-zoom-in-default', collectionPath); | ||
|
|
||
| // Wait for collection to be ready | ||
| await page.waitForTimeout(500); | ||
|
|
||
| await page.keyboard.press(`${modifier}+Shift+Equal`); | ||
| await page.waitForTimeout(500); | ||
| }); | ||
|
|
||
| test('should zoom in using customized-1 Alt+Z+ArrowUp', async ({ page, createTmpDir }) => { | ||
| // Close all collections, tabs, and preferences first | ||
| await closeAllCollections(page); | ||
|
|
||
| // Close any open preference tabs | ||
| const preferenceTabs = page.locator('.request-tab').filter({ hasText: 'Preferences' }); | ||
| const prefTabCount = await preferenceTabs.count(); | ||
| for (let i = 0; i < prefTabCount; i++) { | ||
| await page.keyboard.press(`${modifier}+KeyW`); | ||
| await page.waitForTimeout(200); | ||
| } | ||
|
|
||
| // Open Keybindings preferences and customize zoomIn FIRST | ||
| await openKeybindingsTab(page); | ||
|
|
||
| const row = page.getByTestId('keybinding-row-zoomIn'); | ||
| await row.hover(); | ||
|
|
||
| // Start recording | ||
| await getEditBtn(page, 'zoomIn').click(); | ||
| await page.waitForTimeout(300); | ||
|
|
||
| // Press new combo: Alt+Z+ArrowUp | ||
| // Use down/up to ensure proper key recording | ||
| await page.keyboard.down('Alt'); | ||
| await page.keyboard.down('KeyZ'); | ||
| await page.keyboard.down('ArrowUp'); | ||
| await page.waitForTimeout(200); | ||
| await page.keyboard.up('ArrowUp'); | ||
| await page.keyboard.up('KeyZ'); | ||
| await page.keyboard.up('Alt'); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // Verify the keybinding was saved | ||
| const input = getInput(page, 'zoomIn'); | ||
| const newValue = await input.inputValue(); | ||
| expect(newValue).toContain('alt'); | ||
| expect(newValue).toContain('z'); | ||
| expect(newValue).toContain('arrowup'); | ||
|
|
||
| // Close preferences | ||
| await page.keyboard.press(`${modifier}+KeyW`); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // Wait for collection to be ready | ||
| await page.waitForTimeout(500); | ||
|
|
||
| // Press Alt+Z+ArrowUp to zoom in | ||
| await page.keyboard.down('Alt'); | ||
| await page.keyboard.down('KeyZ'); | ||
| await page.keyboard.down('ArrowUp'); | ||
| await page.waitForTimeout(200); | ||
| await page.keyboard.up('ArrowUp'); | ||
| await page.keyboard.up('KeyZ'); | ||
| await page.keyboard.up('Alt'); | ||
| await page.waitForTimeout(500); | ||
| }); |
Contributor
There was a problem hiding this comment.
Zoom In/Out/Reset tests lack any observable assertion.
The default zoom-in test (lines 1080-1092) presses Cmd/Ctrl+Shift+Equal and then... nothing — no assertion on zoom level. Same for zoom-out (lines 1154-1167) and reset-zoom (lines 1233-1250). The customized variants are similarly assertion-free.
Without verifying zoom factor (e.g., via page.evaluate(() => window.devicePixelRatio) or a known element's computed size), these tests only confirm the shortcut doesn't throw, not that it works.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js` around lines 1079 - 1150,
The zoom tests (tests named "FUNCTIONAL: Zoom In" and the customized variant
using getEditBtn/openKeybindingsTab/getInput) currently only send key events
with no verification; update each zoom-in, zoom-out and reset-zoom test to
capture a measurable zoom indicator before and after the shortcut (e.g., via
page.evaluate(() => window.devicePixelRatio) or computing an element's
computedStyle width/height), then assert the value changed in the expected
direction for zoom-in/zoom-out and returned to the base value for reset-zoom;
use the existing test scaffolding (the test blocks, modifier variable, and
helper functions like getEditBtn and getInput) to place the checks immediately
after the key presses and include clear expect assertions comparing before/after
values.
Comment on lines
+1686
to
+1689
| const allRequests = page.locator('.collection-item-name'); | ||
| const requestCount = await allRequests.count(); | ||
| await page.waitForTimeout(500); | ||
| expect(requestCount).toBeGreaterThanOrEqual(1); // Should have at least original and pasted |
Contributor
There was a problem hiding this comment.
Weak assertion: toBeGreaterThanOrEqual(1) doesn't verify paste succeeded.
After copying and pasting, the assertion on line 1689 only checks requestCount >= 1. This passes even if the paste operation failed entirely. Should be >= 2 at minimum, and ideally assert that original-request (1) is visible (the check that existed in the default test at line 1580).
Same issue at line 1816 for the folder copy/paste test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js` around lines 1686 - 1689,
The assertion using requestCount >= 1 is too weak; update the test that uses
allRequests/page.locator('.collection-item-name') to assert requestCount >= 2 to
ensure the paste created a second item, and also add an explicit visibility
check for the original request (same check used in the default test at line
~1580) to confirm both original and pasted items are present; apply the same
change to the folder copy/paste test around the other failing assertion (near
line 1816) so both tests validate paste success.
Comment on lines
+1710
to
+1714
| // Reset first to remove old keybindings | ||
| await openKeybindingsTab(page); | ||
| const resetCopyRow = page.getByTestId('keybinding-reset-copyItem'); | ||
| await resetCopyRow.hover(); | ||
| await getResetBtn(page, 'copyItem').click(); |
Contributor
There was a problem hiding this comment.
openKeybindingsTab called twice in a row.
Lines 1710-1711 call openKeybindingsTab back-to-back. The first call opens Preferences and clicks the Keybindings tab. The second call opens Preferences again (potentially a no-op or creates a duplicate tab) and clicks Keybindings again. This appears to be a copy-paste mistake — the first call on line 1708 already opened the tab.
// Open Keybindings preferences and customize copyItem FIRST
await openKeybindingsTab(page);
- // Reset first to remove old keybindings
- await openKeybindingsTab(page);
const resetCopyRow = page.getByTestId('keybinding-reset-copyItem');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/preference-shortcuts-edit.spec.js` around lines 1710 - 1714,
There is a duplicate call to openKeybindingsTab() before interacting with
keybinding rows; remove the redundant invocation so the test only calls
openKeybindingsTab() once (the existing call that opens Preferences and clicks
the Keybindings tab) and then proceeds to hover on resetCopyRow and call
getResetBtn(page, 'copyItem').click(); ensure references to openKeybindingsTab,
resetCopyRow, and getResetBtn remain intact.
sid-bruno
reviewed
Feb 19, 2026
Collaborator
Screen.Recording.2026-02-19.at.5.37.21.PM.mov |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/collection/moving-tabs/move-tabs.spec.ts (2)
140-141: Drop thewaitForTimeoutcalls after keyboard presses — the followingexpect()assertions auto-retry.Both
await page.waitForTimeout(500)calls at lines 141 and 150 are redundant. Playwright's built-in retry onexpect()already waits for the tab order to settle, so these fixed delays add unnecessary flakiness risk without benefit. As per coding guidelines,page.waitForTimeout()should be avoided unless a locator cannot be found using existingexpect()calls.♻️ Proposed fix
await source.click(); await page.keyboard.press(`${modifier}+BracketLeft`); - await page.waitForTimeout(500); // Verify order of tabs after move await expect(tabs.nth(0)).toHaveText('GETtest-request'); await expect(tabs.nth(1)).toHaveText('test-folder'); // Move the request tab back to its original position using keyboard shortcut await source.click(); await page.keyboard.press(`${modifier}+BracketRight`); - await page.waitForTimeout(500); // Verify order of tabs after move await expect(tabs.nth(0)).toHaveText('test-folder'); await expect(tabs.nth(1)).toHaveText('GETtest-request');Also applies to: 149-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/collection/moving-tabs/move-tabs.spec.ts` around lines 140 - 141, Remove the fixed 500ms delays after keyboard interactions: delete the two await page.waitForTimeout(500) calls that follow page.keyboard.press(`${modifier}+BracketLeft`) and the other press at the later occurrence; rely on the existing expect(...) assertions to auto-retry and wait for the tab order to settle instead of using page.waitForTimeout. Ensure the tests still use the same expect(...) calls (the assertions that check tab order/state) so Playwright's retry mechanism covers the state change after page.keyboard.press.
90-155: Promotetest.stepin the keyboard shortcut test for better report readability.The
'Verify tab move by keyboard shortcut'test has several distinct logical phases (create collection, open tabs, verify initial order, move left, verify, move right, verify) but none are wrapped intest.step. As per coding guidelines,test.stepshould be used as much as possible so generated reports are easier to read.♻️ Illustrative structure with `test.step`
test('Verify tab move by keyboard shortcut', async ({ page, createTmpDir }) => { - // Create a collection - await createCollection(...); - ... - // Open the request tab - ... - // Verify order of tabs before move - ... - // Move the request tab before the folder tab using keyboard shortcut - ... - // Verify order of tabs after move - ... + await test.step('Setup: create collection, folder and request', async () => { + await createCollection(...); + // folder + request creation... + }); + + await test.step('Open tabs and verify initial order', async () => { + // dblclick + initial expect... + }); + + await test.step('Move tab left and verify new order', async () => { + await source.click(); + await page.keyboard.press(`${modifier}+BracketLeft`); + await expect(tabs.nth(0)).toHaveText('GETtest-request'); + await expect(tabs.nth(1)).toHaveText('test-folder'); + }); + + await test.step('Move tab right and verify restored order', async () => { + await source.click(); + await page.keyboard.press(`${modifier}+BracketRight`); + await expect(tabs.nth(0)).toHaveText('test-folder'); + await expect(tabs.nth(1)).toHaveText('GETtest-request'); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/collection/moving-tabs/move-tabs.spec.ts` around lines 90 - 155, Wrap the logical phases inside the 'Verify tab move by keyboard shortcut' test with await test.step(...) blocks to improve report readability: group operations like collection creation (operations using createCollection and the sourceCollection locator), folder creation and opening (locators '#folder-name', '.collection-item-name', '.request-tab .tab-label'), request creation and opening (placeholders 'Request Name', '#new-request-url textarea', '.collection-item-name'), initial order verification (tabs = page.locator('.request-tab .tab-label')), moving left (source locator '.request-tab .tab-label' filter hasText 'test-request' and page.keyboard.press(`${modifier}+BracketLeft`)) and moving right (page.keyboard.press(`${modifier}+BracketRight`)) each into their own test.step with descriptive names and await the async callback to preserve existing awaits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/collection/moving-tabs/move-tabs.spec.ts`:
- Around line 140-141: Remove the fixed 500ms delays after keyboard
interactions: delete the two await page.waitForTimeout(500) calls that follow
page.keyboard.press(`${modifier}+BracketLeft`) and the other press at the later
occurrence; rely on the existing expect(...) assertions to auto-retry and wait
for the tab order to settle instead of using page.waitForTimeout. Ensure the
tests still use the same expect(...) calls (the assertions that check tab
order/state) so Playwright's retry mechanism covers the state change after
page.keyboard.press.
- Around line 90-155: Wrap the logical phases inside the 'Verify tab move by
keyboard shortcut' test with await test.step(...) blocks to improve report
readability: group operations like collection creation (operations using
createCollection and the sourceCollection locator), folder creation and opening
(locators '#folder-name', '.collection-item-name', '.request-tab .tab-label'),
request creation and opening (placeholders 'Request Name', '#new-request-url
textarea', '.collection-item-name'), initial order verification (tabs =
page.locator('.request-tab .tab-label')), moving left (source locator
'.request-tab .tab-label' filter hasText 'test-request' and
page.keyboard.press(`${modifier}+BracketLeft`)) and moving right
(page.keyboard.press(`${modifier}+BracketRight`)) each into their own test.step
with descriptive names and await the async callback to preserve existing awaits.
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
2 participants
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.
Description
In this PR is phase 1 of allowing user to customize keybindings in preferences tab
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Style
Tests
Chores