ammachado commented on PR #24669:
URL: https://github.com/apache/camel/pull/24669#issuecomment-4962259300
Thanks for this. I traced the full dispatch chain and the menu structures.
The core fix is correct and nicely minimal, but I have a few asks before merge.
**What I verified holds:**
- Typing in the AI input is preserved: `PopupManager.handleKeyEvent` returns
`false` when no popup is visible, so hoisting it above `aiPanel.isOpen()` is
safe. Non-function keys still hit `AiPanel`'s greedy `return true` and never
leak to global handlers.
- The fall-through surface is tightly bounded to function keys only, and F8
is correctly excluded from `isFunctionKey` and still closes the panel.
- The three parallel structures in `ActionsPopup` (`buildMcpSubmenuList`,
`getActionLabels`, `renderMcpSubmenu`) stay in lockstep under the same
`mcpEnabled` conditional, so the index->action mapping is preserved.
**Asks:**
1. **Add unit tests.** `AiPanelTest` already drives `handleKeyEvent`
directly, so pinning this contract is cheap and would guard against a
regression that reorders the F8 handling:
```java
assertFalse(panel.handleKeyEvent(KeyEvent.ofKey(KeyCode.F1,
KeyModifiers.NONE))); // passes through
assertFalse(panel.handleKeyEvent(KeyEvent.ofKey(KeyCode.F2,
KeyModifiers.NONE)));
assertTrue(panel.handleKeyEvent(KeyEvent.ofKey(KeyCode.F8,
KeyModifiers.NONE))); // still consumed (close)
```
The test plan in the description is currently all-manual.
2. **Comment the F8 exclusion in `isFunctionKey`.** Its omission from the
list is load-bearing (F8 is the panel's own toggle) but invisible to a reader.
Something like `// F8 is intentionally excluded - it closes the panel (handled
above)` would prevent a maintainer from "completing" the sequence. The explicit
enumeration itself is fine as-is.
**Two things to confirm (not blockers):**
3. The `isFunctionKey` check sits *before* the `thinking.get()` block, so
F-keys now pass through even while a request is in flight (e.g. F6 shell / F2
actions mid-request). This seems reasonable, but it is a behavior change from
the previous "swallow everything except Ctrl+C/Esc while thinking." Intended?
4. The `ActionsPopup` menu restructuring is logically separate from the
F-key dispatch fix and isn't reflected in the title. Could you expand the
description (or split it) so the changeset is self-describing?
_Claude Code on behalf of @ammachado_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
