Repository navigation
Conversation
github-actions
Bot
added
the
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: |
|
Updated the description to follow the PR template (issue link, change type, what/why, verification, checklist). Let me know if anything else is missing. |
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The plumbing works: a named layer command with bind: { key, event: "release" } now reaches the keymap with its event. I checked this with the added test, which fails on the base and passes here. I also fed a Kitty release sequence through the test renderer, and the command ran on release and not on press. keymap.test.tsx and the typecheck scripts for tui and plugin pass.
Public API: the plugin type KeymapCommand.bind changes from false | string to false | string | { key: string; event?: "press" | "release" }. This is backwards compatible: existing plugins are unaffected, and a keybind the user sets in config for the command still replaces the plugin's default.
Concerns:
- Release events never arrive in the real TUI. The renderer is started with
useKittyKeyboard: {}inpackages/tui/src/app.tsx, and OpenTUI'seventsoption (press/repeat/release reporting) defaults tofalse. Terminals therefore only send key presses, and a release binding registered this way never fires. #45423 deliberately restored this default, and earlier opt-in attempts (#41421, #43419) were closed unmerged. As it stands this doesn't make hold-to-talk plugins possible. The change needs either a way to turn release reporting on, or at least a doc comment onbindsaying release bindings only fire once the terminal reports releases. This should be settled with maintainers before relying on it. - A command without an
idthat uses the object form fails with the misleading "Inline keymap commands require bind" (see inline comment).
Related: #35219
| readonly enabled?: boolean | (() => boolean) | ||
| /** Configures automatic binding, or disables it for a named command. */ | ||
| readonly bind?: false | string | ||
| readonly bind?: false | string | { key: string; event?: "press" | "release" } |
There was a problem hiding this comment.
The TUI starts the renderer with useKittyKeyboard: {}, and OpenTUI's events (release reporting) defaults to false. Terminals never send release events, so event: "release" registers fine but never fires in the actual TUI. Either pair this with a way to turn release reporting on (see #45423 for why the default was restored), or document here that release bindings depend on the terminal reporting releases.
There was a problem hiding this comment.
Thanks, I checked 2c5c521: the doc comment on bind now says release bindings depend on the terminal reporting key releases, and that the TUI doesn't turn that on today. Commands without an id that use the object form now fail with a clear "Object keymap bindings require an ID". Both of my comments are covered. One thing is still open: until the TUI turns on release reporting (see #45423), plugins can register release bindings but they never fire. Whether to land the API before that is up to the maintainers.
| if (!command.id) throw new Error("Keymap command IDs cannot be empty") | ||
| if (typeof command.bind === "string" && !command.bind) | ||
| throw new Error("Keymap command bindings cannot be empty") | ||
| if (typeof command.bind === "object" && !command.bind.key) |
There was a problem hiding this comment.
The object form is only handled for named commands. A command without an id that uses bind: { key, event } falls through to the inline path below and throws "Inline keymap commands require bind", which is misleading because bind is set. Either support the object form for inline commands, or throw a clear error saying the object form needs an id.
There was a problem hiding this comment.
Addressed in 0bcff2a: object binds without an id now throw 'Object keymap bindings require an ID' instead of falling through to the misleading inline message. Chose the clear error over supporting the form inline — an anonymous release binding cannot be user-configured or discovered, so failing loudly beats silent magic. Covered by a unit test.
There was a problem hiding this comment.
Thanks, I checked 2c5c521. Requiring an id for the object form makes sense: groupCommands now throws "Object keymap bindings require an ID" instead of the misleading inline message, and the new test covers it. The doc comment on bind now also says release bindings only fire once the terminal reports key releases. One thing is still open: until the TUI turns on release reporting (see #45423), plugins can register release bindings but they never fire. Whether to land the API before that is up to the maintainers.
|
Thanks for the thorough review — I verified both points independently and you're right on each. On release reporting: confirmed On inline object binds: fixed with the clearer error (chose that over supporting it — an anonymous release binding can't be user-configured or discovered, so failing loudly beats silent magic). |
0bcff2a to
7ba28c6
Compare
Widen KeymapCommand.bind to accept { key, event } so TUI plugins can
register key-release bindings. The keybind config schema and the keymap
library already support release events; the plugin API was the only gap.
This unblocks hold-to-talk plugins (see anomalyco#35219).
Address review: release bindings register but only fire when the terminal reports key releases, which the TUI does not enable today. Document that on bind, and reject ID-less object binds with a clear error instead of the misleading inline message.
7ba28c6 to
2c5c521
Compare
|
@thdxr maintainer decision needed: The bot's final review comment states: "One thing is still open: until the TUI turns on release reporting (see #45423), plugins can register release bindings but they never fire. Whether to land the API before that is up to the maintainers." This PR is green and the code is ready. Could a maintainer please decide whether to:
Either decision is fine — we just need a maintainer to pick one so this can merge. |
|
Quick note on the CI emails: the check / test / nix-eval runs that reported failures (commits 598f2e4 and 0bcff2a) are superseded commits from Oct 12. Those runs never started a job — they were the approval-gated fork workflows, declined today, which reports as failure. The current head 2c5c521 is verified locally: bun run check passes 36/36, tui keymap/keybind/config-v2 tests 28/28, plugin tests 9/9. The check / test / nix-eval workflows on the current head are still awaiting approval, so whoever reviews can approve them from the actions run page if they want CI to run. |
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Relates to #35219 (plumbing for it, does not close it — hold-to-talk itself is still to be built on top).
Type of change
What does this PR do?
TUI plugins could only bind commands to key presses (
bind: false | string).The keybind config schema and the keymap library both already support release
events, but a plugin had no way to express one. This widens the plugin
bindtype to accept
{ key, event }and passes the event through when the layercompiles its bindings. Inline (ID-less) commands stay string-only, with a
clear error if the object form is used without an id.
Scope note, per review: this registers release bindings, but they only fire
when the terminal reports key releases, which the TUI does not enable today
(
useKittyKeyboard: {}default, see #45423). Whether/how to enable releasereporting is a maintainer decision this PR deliberately does not make.
How did you verify your code works?
bun run checkclean (34 tasks: lint + typecheck across the monorepo).packages/tui/test/keymap.test.tsx("layer objectbindings carry release events") asserting the release event reaches the
keymap — verified it fails without the fix and passes with it.
keymap,keybind,keymap-scope, andplugin-keymapsuitesall pass unchanged.
Screenshots / recordings
Not a UI change — no screenshots.
Checklist