Walkthrough

Replaces direct button click handling in the information dialog with a named confirmAction wrapper and adds Enter-key support. A keydown listener triggers confirmAction when Enter is pressed (ignoring inputs/textareas) and is removed after confirmation; existing close/resolve flow is preserved.

Changes

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant UI as InformationDialog
    participant User as User
    participant App as AppState

    Note over UI: Dialog opened with OK button
    User->>UI: Clicks OK
    UI->>UI: confirmAction()
    UI->>App: close dialog / resolve promise
    Note right of UI: remove keydown listener

    par Alternative: press Enter
        User->>UI: Press Enter
        UI->>UI: keydown handler (if not input/textarea)
        UI->>UI: confirmAction()
        UI->>App: close dialog / resolve promise
        Note right of UI: remove keydown listener
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Verify Enter detection across browsers and that event.key/keyCode handling is correct.
• Confirm the listener excludes input and textarea targets reliably (including content-editable or shadow DOM edge cases if applicable).
• Ensure the keydown listener is removed in all code paths (confirm, cancel, dialog close) to avoid leaks or duplicate handlers.

Pre-merge checks and finishing touches

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

It doesn't work

not even with mouse over + enter

@coderabbitai - check implementation - tests are negative

#!/bin/bash
# Read the showInformationDialog function from src/js/gui.js
cat src/js/gui.js | grep -A 100 "showInformationDialog" | head -150

#!/bin/bash
# Find the HTML structure for dialogInformation
rg -n "dialogInformation" --type=html -A 5 -B 2

# Also search in other file types that might contain HTML templates
rg -n "dialogInformation" -g "*.html" -g "*.js" -A 3 | head -50

#!/bin/bash
# Check showYesNoDialog implementation to compare patterns
cat src/js/gui.js | grep -A 80 "showYesNoDialog"

@haslinghuis I found the issue. The keydown handler is attached to the dialog element, but when showModal() is called, the browser automatically focuses the first focusable element inside the dialog (the confirm button), not the dialog itself. Since the button has focus, keydown events on the dialog element won't fire.

The Fix:

Attach the keydown handler to the buttonConfirm element instead of the dialog element:

showInformationDialog(informationDialogSettings) {
    // informationDialogSettings:
    // title, text, buttonConfirmText
    return new Promise((resolve) => {
        const dialog = $(".dialogInformation");
        const title = dialog.find(".dialogInformationTitle");
        const content = dialog.find(".dialogInformationContent");
        const buttonConfirm = dialog.find(".dialogInformation-confirmButton");

        title.html(informationDialogSettings.title);
        content.html(informationDialogSettings.text);
        buttonConfirm.html(informationDialogSettings.buttonConfirmText);

        buttonConfirm.off("click");
        buttonConfirm.off("keydown"); // Remove previous keydown handler from button

        const confirmAction = () => {
            dialog[0].close();
            buttonConfirm.off("keydown");
            resolve();
        };

        buttonConfirm.on("click", confirmAction);

        // Add Enter key support - attach to button which receives focus
        buttonConfirm.on("keydown", (e) => {
            if (e.key === "Enter") {
                e.preventDefault();
                confirmAction();
            }
        });

        dialog[0].showModal();
    });
}

Key Changes:

1. Changed dialog.off("keydown") to buttonConfirm.off("keydown")
2. Changed dialog.on("keydown", ...) to buttonConfirm.on("keydown", ...)
3. Cleanup now targets buttonConfirm.off("keydown") instead of dialog.off("keydown")

The button will naturally have focus when the dialog opens, so the Enter key will now work as expected.

@copilot check previous comment as it does not work

@copilot check previous comment as it does not work

Fixed in commit 50abdb4. The implementation now:

1. Listens on $(document) instead of the dialog element (matches pattern in motors.js)
2. Uses e.which === 13 instead of e.key === "Enter" (jQuery compatibility)
3. Uses namespaced event .informationDialog for clean removal

The Enter key should now work correctly for single-choice information dialogs.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@coderabbitai full review - bot fight competition :)

@copilot checkout latest comments after last commit

I don't see coderabbitai's review comments in the current context. Could you please share the specific feedback or concerns from the review that I should address? The current implementation:

• Listens on $(document) for keydown events
• Uses e.which === 13 for Enter key detection
• Has safety check for input/textarea elements
• Properly cleans up event listeners

If there are specific issues identified in the review, I'm happy to address them.

@copilot check here (#4693 (review))

@coderabbitai your turn

@copilot check here (#4693 (review))

Fixed in commit aa6f589. Added dialog[0].open check to ensure the Enter key only triggers when the information dialog is actually open. This prevents issues if the event listener persists or if the dialog is closed.

The condition now checks:

• e.which === 13 (Enter key)
• dialog[0].open (dialog is currently open)
• !$(e.target).is("input, textarea") (not focused on input fields)

#!/bin/bash
# Read the current showInformationDialog implementation
echo "=== Current showInformationDialog implementation ==="
cat src/js/gui.js | grep -A 120 "showInformationDialog" | head -130

echo -e "\n=== Latest commit message ==="
git log -1 --pretty=format:"%H%n%an%n%ad%n%s%n%b"

echo -e "\n=== Check diff of the last commit ==="
git show HEAD --stat

echo -e "\n=== Full diff of showInformationDialog changes ==="
git show HEAD -- src/js/gui.js | grep -A 80 "showInformationDialog"

#!/bin/bash
# Get the specific review comment details
echo "=== Pull Request Review Details ==="
gh pr view 4693 --json reviews,comments | jq -r '.reviews[] | select(.id) | "Review ID: \(.id)\nState: \(.state)\nBody:\n\(.body)\n---"'

echo -e "\n=== Check if there are review comments on specific lines ==="
gh pr view 4693 --json reviewThreads | jq -r '.reviewThreads[] | "File: \(.path)\nLine: \(.line)\nComments: \(.comments[] | .body)\n---"'

echo -e "\n=== Check the HTML structure of the button ==="
grep -n "dialogInformation-confirmButton" src/index.html -B 2 -A 2

@haslinghuis I've reviewed the latest implementation. I can see the issue now! 🎯

The Problem

The confirmAction function doesn't accept an event parameter and doesn't call preventDefault(). When the anchor button is clicked (or Enter is pressed while the button has focus), the browser navigates to href="#" which interferes with the dialog operation.

Looking at the current code:

const confirmAction = () => {
    dialog[0].close();
    $(document).off("keydown.informationDialog");
    resolve();
};

buttonConfirm.on("click", confirmAction);

The click handler receives an event object from jQuery, but confirmAction doesn't accept it, so preventDefault() can't be called.

The Fix

Make confirmAction accept an event parameter and call preventDefault() when an event is provided:

showInformationDialog(informationDialogSettings) {
    // informationDialogSettings:
    // title, text, buttonConfirmText
    return new Promise((resolve) => {
        const dialog = $(".dialogInformation");
        const title = dialog.find(".dialogInformationTitle");
        const content = dialog.find(".dialogInformationContent");
        const buttonConfirm = dialog.find(".dialogInformation-confirmButton");

        title.html(informationDialogSettings.title);
        content.html(informationDialogSettings.text);
        buttonConfirm.html(informationDialogSettings.buttonConfirmText);

        buttonConfirm.off("click");

        const confirmAction = (e) => {
            if (e) {
                e.preventDefault();
            }
            dialog[0].close();
            $(document).off("keydown.informationDialog");
            resolve();
        };

        buttonConfirm.on("click", confirmAction);

        // Add Enter key support for single-choice dialog
        $(document).off("keydown.informationDialog");
        $(document).on("keydown.informationDialog", (e) => {
            // Only trigger if Enter is pressed, dialog is open, and target is not an input/textarea
            if (e.which === 13 && dialog[0].open && !$(e.target).is("input, textarea")) {
                e.preventDefault();
                confirmAction(e);
            }
        });

        dialog[0].showModal();
    });
}

Key Changes:

1. confirmAction now accepts an optional event parameter: const confirmAction = (e) => {
2. Added if (e) { e.preventDefault(); } to prevent anchor navigation
3. Pass event to confirmAction(e) in the keydown handler

This will prevent the href="#" navigation from interfering with the dialog, allowing both mouse clicks and Enter key presses to work correctly.

Note: The same fix should be applied to showYesNoDialog since it also uses anchor buttons.

Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.

Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.

@copilot this should work #4693 (comment)

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@copilot this should work #4693 (comment)

Refactored in commit 9cc4d0d to follow the codebase pattern. The implementation now:

1. Uses the dialog's native close event for cleanup and promise resolution (matches pattern in initializeModalDialog.js)
2. Named function handleKeydown for proper event listener removal
3. Simpler confirmAction that just closes the dialog
4. { once: true } option for automatic close event cleanup

The cleanup now happens automatically when the dialog closes by any means (button click, Enter key, or ESC key), making it more robust.

@coderabbitai anything left ?

#!/bin/bash
# Check the current implementation of showInformationDialog
echo "=== Current showInformationDialog implementation ==="
cat src/js/gui.js | grep -A 130 "showInformationDialog" | head -140

echo -e "\n=== Check showYesNoDialog for similar issues ==="
cat src/js/gui.js | grep -A 90 "showYesNoDialog" | head -100

echo -e "\n=== Check dialog button HTML structure ==="
grep -n "dialogInformation-confirmButton\|dialogYesNo" src/index.html -B 1 -A 1

@haslinghuis The Enter key support should now work! ✅

Looking at the current implementation, the keydown handler properly calls preventDefault() before triggering the action, so pressing Enter should successfully close the dialog.

One minor robustness improvement (optional):

The confirmAction function could accept an event parameter and call preventDefault() to be extra safe against anchor navigation:

const confirmAction = (e) => {
    if (e) {
        e.preventDefault();
    }
    dialog[0].close();
};

buttonConfirm.on("click", confirmAction);

This would ensure the anchor's href="#" navigation is explicitly prevented on mouse clicks too. However, the current implementation likely works in practice since dialog[0].close() executes before any navigation occurs.

Note about showYesNoDialog:

The Yes/No dialog buttons are also anchor tags with href="#" and don't call preventDefault(). If you encounter similar issues there, the same pattern should be applied.

Otherwise, the implementation looks good! The Enter key support is properly implemented with:

• ✅ Keydown handler on document with namespace
• ✅ Proper cleanup using dialog "close" event
• ✅ Safety check to avoid triggering on input/textarea
• ✅ Check that dialog is still open before triggering

Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.

Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.

@coderabbitai thanks, awaiting testing, in the mean time please approve

@haslinghuis You're welcome! I'll approve the PR now. The implementation looks solid—once testing confirms everything works as expected, this should be ready to merge. 👍

Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.

Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.

Preview URL: https://pr4693.betaflight-app-preview.pages.dev

@haslinghuis test still negative.

unless you can select exact LLM models via API or settings, i would not ask github's integrated copilot to create PR's/code. However, vscode's integrated copilot works much better, Clause Sonnet 4.5 is best, Haiku, Grok, Gemini are okay. GPT worst unless super specific sub-sections or initializing a project. GPT is good for research/proposals then fed into the better "coders". i always tell them "Deep analyze", "Deep research", "fact-check".

Closing for now - as migration from jQuery to Vue is going on - and @copilot did not deliver.