Fix intermittent invalid state error in loadfile (cli)

[haslinghuis]

Summary by CodeRabbit

• New Features

  • None

• Bug Fixes

  • Improved reliability of the snippet preview dialog in the CLI tab: opens only when available, avoids duplicate openings, ensures the confirm action applies to the current file, and closes consistently.

• Refactor

  • Streamlined dialog handling to use native browser dialog behavior for better stability and responsiveness.

[coderabbitai]

Walkthrough

Refactors CLI tab snippet preview dialog management to use a direct DOM <dialog id="snippetpreviewdialog"> reference. Updates open/close logic, event binding for confirm action with current fileName, and guards to only show the modal if not already open. Removes reliance on initializeModalDialog and self.GUI.snippetPreviewWindow.

Changes

Cohort / File(s)	Summary
CLI tab dialog handling src/js/tabs/cli.js	Replace initializeModalDialog/self.GUI.snippetPreviewWindow with direct document.getElementById('snippetpreviewdialog'). Update open/close via showModal() and .close(). Add existence checks, rebind confirm handler per preview invocation with current fileName, and avoid reopening if already open.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant CLI as CLI Tab
  participant DOM as Snippet Dialog (#snippetpreviewdialog)

  User->>CLI: Click "Preview Commands"
  CLI->>CLI: Lookup dialog element
  alt Dialog found
    CLI->>DOM: Attach confirm handler (captures fileName)
    opt If not already open
      CLI->>DOM: showModal()
    end
    Note over DOM,CLI: User reviews commands
    User->>DOM: Click Confirm
    DOM->>CLI: Confirm event
    CLI->>DOM: close()
    CLI->>CLI: Execute commands
  else Dialog missing
    CLI->>CLI: Abort / no-op
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Title Check	⚠️ Warning	The pull request title indicates a fix for an intermittent invalid state error in loadfile, but the actual changes rework the snippet preview dialog handling in the CLI tab rather than addressing loadfile state errors, making the title misleading and not reflective of the main modifications.	Update the title to clearly summarize the primary change, for example “Refactor CLI snippet preview dialog handling in cli.js,” so that it accurately describes the modifications being made.
Description Check	⚠️ Warning	The pull request description is completely missing and does not follow the repository’s required template, omitting all sections such as important considerations, coding style guidelines, testing instructions, and issue references.	Provide a full description using the provided template by adding sections for pull request guidelines, coding style compliance, CI and test instructions, and any related issue numbers to ensure reviewers have all necessary context.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[sonarqubecloud]

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

[github-actions]

Preview URL: https://pr4642.betaflight-app.pages.dev

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/js/tabs/cli.js (1)

169-171: Consider removing redundant initialization flag.

The self.GUI.snippetPreviewWindow property is now primarily used as an initialization flag, but since the dialog element is retrieved directly each time via $("#snippetpreviewdialog")[0], this tracking may be unnecessary. The current implementation works correctly, but simplifying this could reduce cognitive overhead.

Potential cleanup:

-            // Mark as initialized after first use
-            if (!self.GUI.snippetPreviewWindow) {
-                self.GUI.snippetPreviewWindow = snippetDialog;
-            }

You could also remove the property declaration from line 23 if it's not used elsewhere.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 105aa26 and b110ff3.

📒 Files selected for processing (1)

• src/js/tabs/cli.js (1 hunks)

🔇 Additional comments (2)

src/js/tabs/cli.js (2)

148-154: LGTM! Clean refactoring with proper null guard.

The direct DOM reference and null check prevent potential errors when closing the dialog.

---

156-179: Excellent fix for the invalid state error!

The refactoring correctly addresses the intermittent issue by:

• Adding early validation of the dialog element
• Always updating the click handler to capture the current fileName
• Guarding showModal() with !snippetDialog.hasAttribute("open") to prevent calling showModal on an already-open dialog (which throws InvalidStateError)

The direct DOM manipulation is more reliable than the previous modal wrapper pattern.

[nerdCopter]

• approving, shallow test: flashed and pasted.