Skip to content

fix: restore edit-selected-file keyboard shortcut (e key)#402

Merged
benvinegar merged 3 commits into
modem-dev:mainfrom
odjhey:fix/add-back-edit-selected-file
Jun 9, 2026
Merged

fix: restore edit-selected-file keyboard shortcut (e key)#402
benvinegar merged 3 commits into
modem-dev:mainfrom
odjhey:fix/add-back-edit-selected-file

Conversation

@odjhey

@odjhey odjhey commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Re-add support for the 'e' key to open the selected file in $EDITOR. Update help dialog to document the restored shortcut.

prolly from a silent merge conflict of #310 and #303

Re-add support for the 'e' key to open the selected file in $EDITOR.
Update help dialog to document the restored shortcut.
@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR restores the e keyboard shortcut for opening the selected file in $EDITOR, which was previously removed, and documents it in the help dialog. The changes are minimal and consistent with how all other shortcuts in the hook are wired up.

  • useAppKeyboardShortcuts.ts: Adds triggerEditSelectedFile to the hook's parameter destructuring and inserts an e-key handler that follows the same runAndCloseMenu(action) pattern used by every other action shortcut in the file.
  • HelpDialog.tsx: Adds ["e", "open file in $EDITOR"] to the View section of the help dialog, keeping the UI in sync with the restored shortcut.

Confidence Score: 5/5

Safe to merge — the change is a targeted restoration of a previously removed keyboard binding with no side effects on other shortcuts.

The two-line hook change follows the identical pattern used by every other action shortcut in the file, and triggerEditSelectedFile was already declared in the interface, confirming the binding was intentionally removed rather than never implemented. The help dialog entry keeps documentation in sync. No existing shortcuts are affected.

No files require special attention.

Important Files Changed

Filename Overview
src/ui/hooks/useAppKeyboardShortcuts.ts Adds triggerEditSelectedFile to parameter destructuring and wires the 'e' key handler — pattern is identical to existing shortcuts like 'z', 'l', 'w', 'm'.
src/ui/components/chrome/HelpDialog.tsx Adds one entry to the View section items array to document the restored 'e' shortcut; no logic changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    KE[KeyEvent received] --> MT{handleMenuToggleShortcut?}
    MT -->|yes| END[return]
    MT -->|no| PM{pagerMode?}
    PM -->|yes| PS[handlePagerShortcut]
    PM -->|no| HS{handleHelpShortcut?}
    HS -->|yes| END
    HS -->|no| MS{handleMenuShortcut?}
    MS -->|yes| END
    MS -->|no| FI{handleFocusedInputShortcut?}
    FI -->|yes| END
    FI -->|no| AS[handleAppShortcut]
    AS --> EK{"key === 'e'?"}
    EK -->|yes| CM["runAndCloseMenu(triggerEditSelectedFile)"]
    EK -->|no| OTHER[other shortcuts...]
    CM --> END
Loading

Reviews (1): Last reviewed commit: "fix: restore edit-selected-file keyboard..." | Re-trigger Greptile

Display 'e' hint in the app menu for 'Open file in editor' action.
Update help dialog test to reflect the restored shortcut.
@benvinegar

Copy link
Copy Markdown
Member

Oops 😅

Increase test terminal height from 38 to 39 rows. The HelpDialog
content (37 rows) plus modal chrome (6 rows) requires exactly 39 rows
to display without scrolling. At height=38, the modal capped at 36 rows,
triggering scrollbox and hiding the bottom control rows like 'r / q'
from the captured frame, causing the test to fail.

This minimal adjustment ensures all documented control rows render
in the test's viewport without activation of the scrollbox.
@odjhey

odjhey commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Oops 😅

hehe, it happens. i updated the failing tests. ^^

@duarteocarmo

Copy link
Copy Markdown
Contributor

Me would like to have it back :)

@odjhey

odjhey commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

the windows compat test timedout, hmm not sure if its related this PR's change. can we probably retry the test?

@benvinegar benvinegar merged commit 4f0edce into modem-dev:main Jun 9, 2026
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants