Skip to content

Conversation

@veracioux
Copy link
Contributor

@thdxr I have revamped the keybinds so that (almost) everything is in a central place and it's difficult to reason about the binding precedence and make changes without introducing regressions.

Most notable features:

(There are some rough edges, I'll refactor the api a bit more e.g. I think it's unnecessary to have keybind.keybinds., keybind. should suffice, things like that)

Exceptions:

  • <Prompt>'s onKeyDown passed to <textarea>. I will investigate if this can (or should) be converted to the universal keybind approach
  • The ErrorComponent in app.tsx - useKeybind is a potential point of failure, and our catch-all ErrorBoundary should have minimal features IMO

A few caveats (will address before merge):

  • Please ignore for now the fact that I separated keybind.tsx and keybind_.ts. I did that because debugging breakpoints don't work in tsx files. I will inline all of keybind_.ts into keybind.tsx before merge.
  • I haven't gotten around to fully testing permission keybind handling
  • Haven't tested those dialogs that are a bit tricky to get to display like the openrouter warning and maybe some others
  • It seems there is a bug in opentui where an <input> inserts <letter> upon receiving ctrl+<letter>. I'll investigate a bit more

@thdxr
Copy link
Contributor

thdxr commented Nov 5, 2025

took a quick look let me add some context

  • we generally want to avoid anything that requires centralization of functionality. this is what leads to things being difficult to maintain as local behavior leaks globally

  • this is why we have the whole command system where actions can be registered locally and they get cleaned up as routes change

we should try and rely on opentui's functionality around preventDefault and stopping error propagation - these weren't available when i first started working on this which is why it's messy

so having focus + handling events specifically on the focused element similar to how web works

@veracioux
Copy link
Contributor Author

Here's some additional rationale and why I don't think this PR violates your concerns:

  • Only the keybind-to-command resolution + precedence handling is centralized

  • The logic is localized: each component registers its own callback via useKeybind().keybinds.something.something.setHandler(() => ...)

  • Each handler is tied to the lifecycle (setHandler sets up an onDestroy() to deregister it)

  • The preventDefault() approach would work fine if we didn't have the following tasks to wrestle with, all at once:

    • Keybinds sometimes apply globally, sometimes to the focused component
    • Users can override keybinds, which are sometimes unconditional, sometimes global, sometimes state-specific, can conflict with builtin bindings etc
    • Satisfying people's muscle memory and adding workarounds for backwards-compat with 0.15.x

    Because of these, I think relying on preventDefault() and stopPropagation() would might prove difficult to maintain.

  • I think having a centralized overview of all actions and their keybinds improves maintainability

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.

2 participants