feat: enhance error dialogs with URL detection and copy functionality #900
Conversation
- Add enhanced error dialog that detects URLs in error messages - Provide "Open in Browser" button for URLs in error messages - Add "Copy Error" button to copy error messages to clipboard - Replace standard wx.MessageBox with enhanced dialog for all errors - Apply enhanced dialogs to completion, transcription, OCR, and save errors
There was a problem hiding this comment.
Pull Request Overview
This PR enhances error handling throughout the application by replacing standard wx.MessageBox error dialogs with a new enhanced error dialog that detects URLs in error messages and provides additional functionality.
- Introduces
EnhancedErrorDialogwith URL detection, "Open in Browser" button, and "Copy Error" functionality - Replaces all
wx.MessageBoxerror displays with the enhanced dialog across multiple components - Provides context-aware messaging for completion errors vs. other error types
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| basilisk/gui/enhanced_error_dialog.py | New module implementing the enhanced error dialog with URL detection and copy functionality |
| basilisk/gui/ocr_handler.py | Replaces wx.MessageBox with enhanced error dialog for OCR errors |
| basilisk/gui/conversation_tab.py | Updates transcription, title generation, save, and completion error handling to use enhanced dialog |
| basilisk/completion_handler.py | Replaces fallback error dialog with enhanced version |
| URL_PATTERN = re.compile( | ||
| r'https?://(?:[-\w.])+(?:\:[0-9]+)?(?:/(?:[\w/_.])*(?:\?(?:[\w&=%.])*)?(?:\#(?:[\w.])*)?)?', | ||
| re.IGNORECASE, |
There was a problem hiding this comment.
The URL regex pattern has several issues: it uses (?:\:[0-9]+)? which requires escaping the colon, and the character classes are incomplete. The pattern may fail to match valid URLs or match invalid ones. Consider using a more robust URL detection library or fixing the regex to properly escape special characters and include complete character sets for URL components.
| URL_PATTERN = re.compile( | |
| r'https?://(?:[-\w.])+(?:\:[0-9]+)?(?:/(?:[\w/_.])*(?:\?(?:[\w&=%.])*)?(?:\#(?:[\w.])*)?)?', | |
| re.IGNORECASE, | |
| # Improved URL detection pattern that matches most http/https URLs, including ports, paths, queries, and fragments | |
| URL_PATTERN = re.compile( | |
| r'https?://[^\s/$.?#].[^\s]*', | |
| re.IGNORECASE, |
| def show_enhanced_error_dialog( | ||
| parent: Optional[wx.Window], | ||
| message: str, | ||
| title: str = None, |
There was a problem hiding this comment.
Using None as a default argument should be Optional[str] = None for clarity, but the type hint on line 41 shows str = None which is incorrect. The parameter should be typed as Optional[str] and defaulted to None.
| title: str = None, | |
| title: Optional[str] = None, |
| wx.MessageBox( | ||
| _("Failed to access clipboard"), | ||
| _("Error"), | ||
| wx.OK | wx.ICON_ERROR, | ||
| self, | ||
| ) |
There was a problem hiding this comment.
Using wx.MessageBox within the enhanced error dialog defeats the purpose of having an enhanced dialog system. This creates inconsistent error handling and could lead to recursive issues. Consider using a simpler notification method or logging the error instead of showing another dialog.
| wx.MessageBox( | ||
| _("Failed to copy to clipboard: ") + str(e), | ||
| _("Error"), | ||
| wx.OK | wx.ICON_ERROR, | ||
| self, | ||
| ) |
There was a problem hiding this comment.
Same issue as above - using wx.MessageBox within the enhanced error dialog creates inconsistent error handling. This should use a simpler notification method or just log the error.
| wx.MessageBox( | ||
| _("Failed to open URL in browser: ") + str(e), | ||
| _("Error"), | ||
| wx.OK | wx.ICON_ERROR, | ||
| self, | ||
| ) |
There was a problem hiding this comment.
Another instance of using wx.MessageBox within the enhanced error dialog. This breaks the consistency of the enhanced error handling system and should be replaced with logging or a simpler notification method.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new enhanced error dialog with URL detection, copy and open-in-browser actions, and replaces several wx.MessageBox error calls in completion_handler, conversation_tab, and ocr_handler to use the new dialog while preserving existing control flow and callbacks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Action
participant C as Component (Completion / Conversation / OCR)
participant D as EnhancedErrorDialog
participant B as System Browser
participant CL as Clipboard
U->>C: Trigger operation
C->>C: Error occurs
C->>D: show_enhanced_error_dialog(parent, message, title, is_completion_error)
activate D
Note over D: Render message, extract URLs, provide Copy/Open actions
D-->>C: Modal result (OK/closed)
deactivate D
opt User clicks "Copy Error"
D->>CL: Copy error text
CL-->>D: Success/Failure
end
opt User selects URL and clicks "Open in Browser"
D->>B: Open selected URL
B-->>D: Success/Failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
basilisk/completion_handler.py(2 hunks)basilisk/gui/conversation_tab.py(5 hunks)basilisk/gui/enhanced_error_dialog.py(1 hunks)basilisk/gui/ocr_handler.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use tabs (not spaces) for indentation in Python code
Use snake_case for variables/functions, PascalCase for classes, and UPPER_SNAKE_CASE for constants
Write Google-style docstrings with type hints for all public APIs
Use double quotes for strings; wrap translatable text with _("...")
Do not import the translation function _; it is provided as a builtin
Group imports as standard library, third-party, then local; sort each group alphabetically
Mark translatable strings with _("...") and provide preceding translator context comments using '# Translators:' or '# translators:' when helpful
Use provider-specific streaming APIs to display messages in real time
Files:
basilisk/completion_handler.pybasilisk/gui/ocr_handler.pybasilisk/gui/enhanced_error_dialog.pybasilisk/gui/conversation_tab.py
⚙️ CodeRabbit configuration file
**/*.py: ## Indentation
We use tabs, not spaces.naming conventions
- snake_case for variables and functions
- PascalCase for classes
- UPPER_SNAKE_CASE for constants
- snake_case for files and directories
- Use whole words in names when possible
documentation
- Use docstrings for all public classes and functions
- Use type hints for all public classes and functions
- Use google style for docstrings
Strings
- Always use double quotes for strings
- Use percent formatting for string
- Use _("string") for translatable strings
- Use **# translator: ** with context for translatable strings
Files:
basilisk/completion_handler.pybasilisk/gui/ocr_handler.pybasilisk/gui/enhanced_error_dialog.pybasilisk/gui/conversation_tab.py
basilisk/gui/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
basilisk/gui/**/*.py: In wxPython UI code, bind events using self.Bind(wx.EVT_, self.on_) consistently
After any dialog.ShowModal(), always call dialog.Destroy()
Use BoxSizer-based layouts; avoid fixed positioning for responsiveness
Ensure accessibility: provide labels/roles for screen readers and maintain NVDA integration hooks
Files:
basilisk/gui/ocr_handler.pybasilisk/gui/enhanced_error_dialog.pybasilisk/gui/conversation_tab.py
🧬 Code graph analysis (4)
basilisk/completion_handler.py (2)
basilisk/gui/enhanced_error_dialog.py (1)
show_enhanced_error_dialog(38-65)__builtins__.pyi (1)
_(1-1)
basilisk/gui/ocr_handler.py (2)
basilisk/gui/enhanced_error_dialog.py (1)
show_enhanced_error_dialog(38-65)__builtins__.pyi (1)
_(1-1)
basilisk/gui/enhanced_error_dialog.py (1)
__builtins__.pyi (1)
_(1-1)
basilisk/gui/conversation_tab.py (2)
basilisk/gui/enhanced_error_dialog.py (1)
show_enhanced_error_dialog(38-65)__builtins__.pyi (1)
_(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_app / build_windows (x86)
- GitHub Check: build_app / build_windows (x86_64)
| def show_enhanced_error_dialog( | ||
| parent: Optional[wx.Window], | ||
| message: str, | ||
| title: str = None, | ||
| is_completion_error: bool = False, | ||
| ) -> int: | ||
| """Show an enhanced error dialog that can handle URLs. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Type hint fix: title should be Optional[str]
Matches how parent is annotated and avoids mypy warnings.
Apply this diff:
-def show_enhanced_error_dialog(
- parent: Optional[wx.Window],
- message: str,
- title: str = None,
- is_completion_error: bool = False,
-) -> int:
+def show_enhanced_error_dialog(
+ parent: Optional[wx.Window],
+ message: str,
+ title: Optional[str] = None,
+ is_completion_error: bool = False,
+) -> int:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def show_enhanced_error_dialog( | |
| parent: Optional[wx.Window], | |
| message: str, | |
| title: str = None, | |
| is_completion_error: bool = False, | |
| ) -> int: | |
| """Show an enhanced error dialog that can handle URLs. | |
| def show_enhanced_error_dialog( | |
| parent: Optional[wx.Window], | |
| message: str, | |
| title: Optional[str] = None, | |
| is_completion_error: bool = False, | |
| ) -> int: | |
| """Show an enhanced error dialog that can handle URLs. |
🤖 Prompt for AI Agents
In basilisk/gui/enhanced_error_dialog.py around lines 38 to 44, update the
function signature to annotate title as Optional[str] instead of str to match
parent and satisfy mypy; ensure Optional is imported from typing (it already is
if parent uses Optional) and run type checks to confirm no other annotations
need adjustment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…le error messages
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
basilisk/gui/conversation_tab.py (4)
463-468: Replace remaining wx.MessageBox error dialogs with enhanced dialogThese instances violate the PR objective to standardize error dialogs.
@@ - wx.MessageBox( - _("The selected provider does not support speech-to-text"), - _("Error"), - wx.OK | wx.ICON_ERROR, - ) + show_enhanced_error_dialog( + parent=self, + message=_("The selected provider does not support speech-to-text"), + title=_("Error"), + is_completion_error=False, + ) return @@ - wx.MessageBox( - _("The selected provider does not support speech-to-text"), - _("Error"), - wx.OK | wx.ICON_ERROR, - ) + show_enhanced_error_dialog( + parent=self, + message=_("The selected provider does not support speech-to-text"), + title=_("Error"), + is_completion_error=False, + ) return @@ - wx.MessageBox( - _( - "A completion is already in progress. Please wait until it finishes." - ), - _("Error"), - wx.OK | wx.ICON_ERROR, - ) + show_enhanced_error_dialog( + parent=self, + message=_( + "A completion is already in progress. Please wait until it finishes." + ), + title=_("Error"), + is_completion_error=False, + ) returnAs per coding guidelines.
Also applies to: 547-552, 694-700
308-311: Fix default key handling: wx.KeyEvent.Skip is an instance methodCurrent code attempts to call a class method reference; it won’t skip properly.
- shortcut = (event.GetModifiers(), event.GetKeyCode()) - actions = {(wx.MOD_CONTROL, ord('P')): self.on_choose_profile} - action = actions.get(shortcut, wx.KeyEvent.Skip) - action(event) + shortcut = (event.GetModifiers(), event.GetKeyCode()) + actions = {(wx.MOD_CONTROL, ord("P")): self.on_choose_profile} + action = actions.get(shortcut) + if action: + action(event) + else: + event.Skip()
379-388: Do not destroy a menu you didn’t createadd_standard_context_menu_items() destroys the passed-in menu, which is unexpected and can crash callers.
- menu.Destroy() + # Caller is responsible for menu lifetime.
235-267: House style: prefer self.Bind(...) for event hookupsMigrate per-control .Bind calls to self.Bind with source for consistency across basilisk/gui.
- self.submit_btn.Bind(wx.EVT_BUTTON, self.on_submit) + self.Bind(wx.EVT_BUTTON, self.on_submit, self.submit_btn) @@ - self.stop_completion_btn.Bind(wx.EVT_BUTTON, self.on_stop_completion) + self.Bind(wx.EVT_BUTTON, self.on_stop_completion, self.stop_completion_btn) @@ - self.toggle_record_btn.Bind(wx.EVT_BUTTON, self.toggle_recording) + self.Bind(wx.EVT_BUTTON, self.toggle_recording, self.toggle_record_btn) @@ - self.apply_profile_btn.Bind(wx.EVT_BUTTON, self.on_choose_profile) + self.Bind(wx.EVT_BUTTON, self.on_choose_profile, self.apply_profile_btn)Based on learnings.
Also applies to: 240-241, 249-251, 259-260, 266-267
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
basilisk/gui/conversation_tab.py(5 hunks)basilisk/gui/enhanced_error_dialog.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
basilisk/gui/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
basilisk/gui/**/*.py: In wxPython UI code, bind events using self.Bind(wx.EVT_, self.on_) consistently
After any dialog.ShowModal(), always call dialog.Destroy()
Use BoxSizer-based layouts; avoid fixed positioning for responsiveness
Ensure accessibility: provide labels/roles for screen readers and maintain NVDA integration hooks
Files:
basilisk/gui/conversation_tab.pybasilisk/gui/enhanced_error_dialog.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use tabs (not spaces) for indentation in Python code
Use snake_case for variables/functions, PascalCase for classes, and UPPER_SNAKE_CASE for constants
Write Google-style docstrings with type hints for all public APIs
Use double quotes for strings; wrap translatable text with _("...")
Do not import the translation function _; it is provided as a builtin
Group imports as standard library, third-party, then local; sort each group alphabetically
Mark translatable strings with _("...") and provide preceding translator context comments using '# Translators:' or '# translators:' when helpful
Use provider-specific streaming APIs to display messages in real time
Files:
basilisk/gui/conversation_tab.pybasilisk/gui/enhanced_error_dialog.py
⚙️ CodeRabbit configuration file
**/*.py: ## Indentation
We use tabs, not spaces.naming conventions
- snake_case for variables and functions
- PascalCase for classes
- UPPER_SNAKE_CASE for constants
- snake_case for files and directories
- Use whole words in names when possible
documentation
- Use docstrings for all public classes and functions
- Use type hints for all public classes and functions
- Use google style for docstrings
Strings
- Always use double quotes for strings
- Use percent formatting for string
- Use _("string") for translatable strings
- Use **# translator: ** with context for translatable strings
Files:
basilisk/gui/conversation_tab.pybasilisk/gui/enhanced_error_dialog.py
🧠 Learnings (2)
📚 Learning: 2025-09-28T08:26:32.431Z
Learnt from: CR
PR: SigmaNight/basiliskLLM#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-28T08:26:32.431Z
Learning: Applies to basilisk/gui/**/*.py : In wxPython UI code, bind events using self.Bind(wx.EVT_*, self.on_*) consistently
Applied to files:
basilisk/gui/enhanced_error_dialog.py
📚 Learning: 2025-09-28T08:26:32.432Z
Learnt from: CR
PR: SigmaNight/basiliskLLM#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-28T08:26:32.432Z
Learning: Applies to **/*.py : Use double quotes for strings; wrap translatable text with _("...")
Applied to files:
basilisk/gui/enhanced_error_dialog.py
🧬 Code graph analysis (2)
basilisk/gui/conversation_tab.py (1)
basilisk/gui/enhanced_error_dialog.py (1)
show_enhanced_error_dialog(38-65)
basilisk/gui/enhanced_error_dialog.py (2)
__builtins__.pyi (1)
_(1-1)basilisk/conversation/attached_file.py (1)
url(466-475)
🔇 Additional comments (10)
basilisk/gui/enhanced_error_dialog.py (7)
58-65: Good: dialog lifecycle handled correctlyShowModal followed by Destroy satisfies our wx dialog lifecycle rule.
Based on learnings.
102-124: Layout follows sizers; accessible text presentBoxSizer usage and wrapped StaticText look good. Keep it consistent elsewhere.
As per coding guidelines.
20-23: Use double quotes for regex literal; consider a simpler, more robust URL patternSwitch to double quotes to match project string rules. Also consider a simpler, battle‑tested pattern to reduce false positives.
Apply this minimal style fix:
URL_PATTERN = re.compile( - r'https?://(?:[-\w.])+(?:\:[0-9]+)?(?:/(?:[\w/_.])*(?:\?(?:[\w&=%.])*)?(?:\#(?:[\w.])*)?)?', + r"https?://(?:[-\w.])+(?::[0-9]+)?(?:/(?:[\w/_.])*(?:\?(?:[\w&=%.])*)?(?:#(?:[\w.])*)?)?", re.IGNORECASE, )Optional: replace with a simpler pattern if acceptable:
- r"https?://[^\s/$.?#].[^\s]*"
As per coding guidelines.
38-44: Type hint fix: title should be Optional[str]Current annotation allows None but types say str. Align with typing and mypy.
def show_enhanced_error_dialog( parent: Optional[wx.Window], message: str, - title: str = None, + title: Optional[str] = None, is_completion_error: bool = False, ) -> int:As per coding guidelines.
186-189: Bind on the dialog, not per‑control; fix quotingUse self.Bind(...) consistently and double quotes.
- if hasattr(self, 'open_url_btn'): - self.open_url_btn.Bind(wx.EVT_BUTTON, self.on_open_url) - self.copy_btn.Bind(wx.EVT_BUTTON, self.on_copy_error) + if hasattr(self, "open_url_btn"): + self.Bind(wx.EVT_BUTTON, self.on_open_url, self.open_url_btn) + self.Bind(wx.EVT_BUTTON, self.on_copy_error, self.copy_btn)Based on learnings.
196-215: Avoid wx.MessageBox inside the enhanced dialog; ensure clipboard is always closed
- Don’t invoke wx.MessageBox from within this dialog; it undermines the unified error UX and can recurse.
- Guarantee wx.TheClipboard.Close() via finally to avoid locking the clipboard on exceptions.
def on_copy_error(self, event: wx.CommandEvent): @@ - try: - if wx.TheClipboard.Open(): - wx.TheClipboard.SetData(wx.TextDataObject(self.message)) - wx.TheClipboard.Close() - - # Change button label to show confirmation - original_label = self.copy_btn.GetLabel() - self.copy_btn.SetLabel(_("Copied!")) - self.copy_btn.Disable() - - # Reset button label after 2 seconds - wx.CallLater(2000, self._reset_copy_button, original_label) - else: - wx.MessageBox( - _("Failed to access clipboard"), - _("Error"), - wx.OK | wx.ICON_ERROR, - self, - ) - except Exception as e: - logger.error("Failed to copy to clipboard: %s", e) - wx.MessageBox( - _("Failed to copy to clipboard: %s") % str(e), - _("Error"), - wx.OK | wx.ICON_ERROR, - self, - ) + try: + opened = wx.TheClipboard.Open() + try: + if opened: + if not wx.TheClipboard.SetData(wx.TextDataObject(self.message)): + raise RuntimeError("SetData returned False") + else: + # Inline, non-modal feedback instead of MessageBox + self._flash_copy_button(_("Failed to access clipboard")) + return + finally: + if opened: + wx.TheClipboard.Close() + + # Success feedback + original_label = self.copy_btn.GetLabel() + self.copy_btn.SetLabel(_("Copied!")) + self.copy_btn.Disable() + wx.CallLater(2000, self._reset_copy_button, original_label) + except Exception as e: + logger.error("Failed to copy to clipboard: %s", e) + self._flash_copy_button(_("Failed to copy to clipboard: %s") % e)Add this helper (place near other privates):
+ def _flash_copy_button(self, text: str): + original_label = self.copy_btn.GetLabel() + self.copy_btn.SetLabel(text) + self.copy_btn.Enable() + wx.CallLater(2000, self._reset_copy_button, original_label)As per coding guidelines.
240-258: Avoid MessageBox on URL open failure; provide inline feedback and fix quoting
- Do not open MessageBox from inside the enhanced dialog.
- Provide inline button feedback and ensure double quotes.
- if not hasattr(self, 'url_choice'): + if not hasattr(self, "url_choice"): return @@ try: logger.info("Opening URL in browser: %s", url) webbrowser.open(url) except Exception as e: logger.error("Failed to open URL %s: %s", url, e) - wx.MessageBox( - _("Failed to open URL in browser: %s") % str(e), - _("Error"), - wx.OK | wx.ICON_ERROR, - self, - ) + # Inline, non-modal feedback on the button + if hasattr(self, "open_url_btn"): + orig = self.open_url_btn.GetLabel() + self.open_url_btn.SetLabel(_("Open failed")) + wx.CallLater(2000, self.open_url_btn.SetLabel, orig)As per coding guidelines.
basilisk/gui/conversation_tab.py (3)
42-42: LGTM: using enhanced error dialogThe import is correct and matches project direction.
733-738: LGTM: title generation errors use enhanced dialog and percent formattingConforms to translation and formatting rules.
887-893: LGTM: completion errors migrated to enhanced dialogMessage and title are localized; flag set appropriately.
… error dialog call
AAClause
force-pushed
the
errorMsgURL
branch
from
9ff4cfb to
c8d56fc
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
AAClause
force-pushed
the
errorMsgURL
branch
from
ef65e59 to
2a051e8
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
basilisk/gui/ocr_handler.py (7)
93-101: Replace MessageBox with enhanced dialog for consistencyAligns with PR objective to unify error dialogs.
Apply:
- wx.MessageBox( - _( - "An error occurred while processing OCR results. Details: \n%s" - ) - % e, - _("Error"), - wx.OK | wx.ICON_ERROR, - ) + show_enhanced_error_dialog( + parent=self.parent, + message=_("An error occurred while processing OCR results. Details: \n%s") % e, + title=_("OCR Error"), + is_completion_error=False, + )
277-282: Unify provider‑capability error to enhanced dialogKeep error UX consistent.
Apply:
- wx.MessageBox( - # Translators: This message is displayed when the current provider does not support OCR. - _("The selected provider does not support OCR."), - _("Error"), - wx.OK | wx.ICON_ERROR, - ) + show_enhanced_error_dialog( + parent=self.parent, + # Translators: This message is displayed when the current provider does not support OCR. + message=_("The selected provider does not support OCR."), + title=_("Error"), + is_completion_error=False, + )
286-291: Unify “no attachments” error to enhanced dialogSame migration as above.
Apply:
- wx.MessageBox( - # Translators: This message is displayed when there are no attachments to perform OCR on. - _("No attachments to perform OCR on."), - _("Error"), - wx.OK | wx.ICON_ERROR, - ) + show_enhanced_error_dialog( + parent=self.parent, + # Translators: This message is displayed when there are no attachments to perform OCR on. + message=_("No attachments to perform OCR on."), + title=_("Error"), + is_completion_error=False, + )
299-304: Unify “no client” error to enhanced dialogComplete the migration.
Apply:
- wx.MessageBox( - # Translators: This message is displayed when the current provider does not have a client. - _("The selected provider does not have a client."), - _("Error"), - wx.OK | wx.ICON_ERROR, - ) + show_enhanced_error_dialog( + parent=self.parent, + # Translators: This message is displayed when the current provider does not have a client. + message=_("The selected provider does not have a client."), + title=_("Error"), + is_completion_error=False, + )
226-226: Logging format bug: missing %s placeholderThis will raise during logging formatting.
Apply:
- log.error("Error terminating process: %", e, exc_info=True) + log.error("Error terminating process: %s", e, exc_info=True)
167-175: Optional: migrate info popups to non‑blocking UXThe YES/NO MessageBox here is informational and not an error; optional to keep. If you want full consistency, consider a non‑modal notice or in‑panel banner.
If you decide to migrate, confirm whether EnhancedErrorDialog should support YES/NO; if not, keep as is. As per coding guidelines.
267-345: Centralize error MessageBoxes across GUI
Multiple standalonewx.MessageBox(..., wx.OK | wx.ICON_ERROR)calls remain in basilisk/gui:
- prompt_attachments_panel.py (lines 423, 692, 741)
- html_view_window.py (line 113)
- main_frame.py (lines 341, 554, 650, 670, 847, 1039, 1061)
- account_dialog.py (lines 139, 144, 149, 1071, 1121)
Extract these into a shared error-display helper and replace direct calls with that centralized method.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
basilisk/completion_handler.py(2 hunks)basilisk/gui/conversation_tab.py(5 hunks)basilisk/gui/enhanced_error_dialog.py(1 hunks)basilisk/gui/ocr_handler.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use tabs (not spaces) for indentation in Python code
Use snake_case for variables/functions, PascalCase for classes, and UPPER_SNAKE_CASE for constants
Write Google-style docstrings with type hints for all public APIs
Use double quotes for strings; wrap translatable text with _("...")
Do not import the translation function _; it is provided as a builtin
Group imports as standard library, third-party, then local; sort each group alphabetically
Mark translatable strings with _("...") and provide preceding translator context comments using '# Translators:' or '# translators:' when helpful
Use provider-specific streaming APIs to display messages in real time
Files:
basilisk/completion_handler.pybasilisk/gui/conversation_tab.pybasilisk/gui/ocr_handler.pybasilisk/gui/enhanced_error_dialog.py
⚙️ CodeRabbit configuration file
**/*.py: ## Indentation
We use tabs, not spaces.naming conventions
- snake_case for variables and functions
- PascalCase for classes
- UPPER_SNAKE_CASE for constants
- snake_case for files and directories
- Use whole words in names when possible
documentation
- Use docstrings for all public classes and functions
- Use type hints for all public classes and functions
- Use google style for docstrings
Strings
- Always use double quotes for strings
- Use percent formatting for string
- Use _("string") for translatable strings
- Use **# translator: ** with context for translatable strings
Files:
basilisk/completion_handler.pybasilisk/gui/conversation_tab.pybasilisk/gui/ocr_handler.pybasilisk/gui/enhanced_error_dialog.py
basilisk/gui/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
basilisk/gui/**/*.py: In wxPython UI code, bind events using self.Bind(wx.EVT_, self.on_) consistently
After any dialog.ShowModal(), always call dialog.Destroy()
Use BoxSizer-based layouts; avoid fixed positioning for responsiveness
Ensure accessibility: provide labels/roles for screen readers and maintain NVDA integration hooks
Files:
basilisk/gui/conversation_tab.pybasilisk/gui/ocr_handler.pybasilisk/gui/enhanced_error_dialog.py
🧠 Learnings (2)
📚 Learning: 2025-09-28T08:26:32.432Z
Learnt from: CR
PR: SigmaNight/basiliskLLM#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-28T08:26:32.432Z
Learning: Applies to **/*.py : Use double quotes for strings; wrap translatable text with _("...")
Applied to files:
basilisk/completion_handler.pybasilisk/gui/enhanced_error_dialog.py
📚 Learning: 2025-09-28T08:26:32.431Z
Learnt from: CR
PR: SigmaNight/basiliskLLM#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-28T08:26:32.431Z
Learning: Applies to basilisk/gui/**/*.py : In wxPython UI code, bind events using self.Bind(wx.EVT_*, self.on_*) consistently
Applied to files:
basilisk/gui/enhanced_error_dialog.py
🧬 Code graph analysis (4)
basilisk/completion_handler.py (2)
basilisk/gui/enhanced_error_dialog.py (1)
show_enhanced_error_dialog(38-65)__builtins__.pyi (1)
_(1-1)
basilisk/gui/conversation_tab.py (1)
basilisk/gui/enhanced_error_dialog.py (1)
show_enhanced_error_dialog(38-65)
basilisk/gui/ocr_handler.py (2)
basilisk/gui/enhanced_error_dialog.py (1)
show_enhanced_error_dialog(38-65)__builtins__.pyi (1)
_(1-1)
basilisk/gui/enhanced_error_dialog.py (1)
__builtins__.pyi (1)
_(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_app / build_windows (x86_64)
- GitHub Check: build_app / build_windows (x86)
🔇 Additional comments (11)
basilisk/completion_handler.py (1)
27-27: Import looks goodCentralizing error UI via show_enhanced_error_dialog aligns with the PR intent.
basilisk/gui/ocr_handler.py (2)
21-21: Import of enhanced dialog – OKMatches usage below; consistent with other modules.
147-152: Good switch to enhanced dialog for OCR errorsCorrect parent and is_completion_error=False.
basilisk/gui/enhanced_error_dialog.py (7)
41-43: Type hint: title should be Optional[str]Annotate the nullable title correctly to avoid mypy warnings and clarify API.
Apply:
- title: str = None, + title: Optional[str] = None,As per coding guidelines.
20-23: URL regex is brittle; broaden allowed charactersCurrent pattern misses many valid URLs (queries/fragments, UTF‑8, parentheses) and over‑constrains allowed chars. Prefer a simpler, more permissive detector.
Apply:
-URL_PATTERN = re.compile( - r"https?://(?:[-\w.])+(?:\:[0-9]+)?(?:/(?:[\w/_.])*(?:\?(?:[\w&=%.])*)?(?:\#(?:[\w.])*)?)?", - re.IGNORECASE, -) +URL_PATTERN = re.compile( + r"https?://[^\s<>()\"']+", + re.IGNORECASE, +)This matches typical http/https URLs (incl. ports/paths/queries/fragments) while avoiding trailing punctuation. As per coding guidelines.
184-189: Use dialog-level event binding (self.Bind) and double quotesBind on the dialog per house style; also fix quoting.
Apply:
- if hasattr(self, 'open_url_btn'): - self.open_url_btn.Bind(wx.EVT_BUTTON, self.on_open_url) - self.copy_btn.Bind(wx.EVT_BUTTON, self.on_copy_error) + if hasattr(self, "open_url_btn"): + self.Bind(wx.EVT_BUTTON, self.on_open_url, self.open_url_btn) + self.Bind(wx.EVT_BUTTON, self.on_copy_error, self.copy_btn)Based on learnings.
253-258: Avoid MessageBox on URL open failure; log and non-intrusive feedbackDon’t pop a second dialog from within the error dialog. Beep and consider adding a status label if desired.
Apply:
- except Exception as e: - logger.error("Failed to open URL %s: %s", url, e) - wx.MessageBox( - _("Failed to open URL in browser: %s") % str(e), - _("Error"), - wx.OK | wx.ICON_ERROR, - self, - ) + except Exception as e: + logger.error("Failed to open URL %s: %s", url, e) + wx.Bell()
215-222: Same: replace MessageBox on copy exception with inline feedbackKeep UX consistent; avoid recursive dialogs.
Apply:
- except Exception as e: - logger.error("Failed to copy to clipboard: %s", e) - wx.MessageBox( - _("Failed to copy to clipboard: %s") % str(e), - _("Error"), - wx.OK | wx.ICON_ERROR, - self, - ) + except Exception as e: + logger.error("Failed to copy to clipboard: %s", e) + wx.Bell() + original_label = self.copy_btn.GetLabel() + self.copy_btn.SetLabel(_("Copy failed: %s") % e) + self.copy_btn.Disable() + wx.CallLater(2000, self._reset_copy_button, original_label)
206-215: Avoid MessageBox inside the enhanced dialog; give inline feedback insteadShowing wx.MessageBox here creates nested error popups from within the error dialog. Provide subtle inline feedback and log/beep.
Apply:
- else: - wx.MessageBox( - _("Failed to access clipboard"), - _("Error"), - wx.OK | wx.ICON_ERROR, - self, - ) + else: + logger.warning("Failed to access clipboard") + wx.Bell() + original_label = self.copy_btn.GetLabel() + self.copy_btn.SetLabel(_("Copy failed")) + self.copy_btn.Disable() + wx.CallLater(2000, self._reset_copy_button, original_label)As per coding guidelines.
240-241: Consistent quotingUse double quotes per project standard.
Apply:
- if not hasattr(self, 'url_choice'): + if not hasattr(self, "url_choice"):As per coding guidelines.
basilisk/gui/conversation_tab.py (1)
524-528: Drop redundant str() when using %s formatting
%already callsstr()on its operand, so the explicit wrap is unnecessary. Removing it keeps the formatting idiomatic.- message=_("An error occurred during transcription: %s") - % str(error), + message=_("An error occurred during transcription: %s") % error,As per coding guidelines.
| show_enhanced_error_dialog( | ||
| parent=None, | ||
| message=_("An error occurred during completion: %s") | ||
| % error_message, | ||
| title=_("Completion Error"), | ||
| is_completion_error=True, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Good: errors now use enhanced dialog on UI thread
This path runs on the main thread (invoked via wx.CallAfter earlier), uses i18n with percent formatting, and sets the correct title/flag.
Run to find any remaining error MessageBoxes to migrate:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'wx\.MessageBox\(' --type=py | rg -nP 'ICON_ERROR|wx\.(OK|YES_NO)\s*\|\s*wx\.ICON_ERROR'Length of output: 1653
Migrate remaining error dialogs to show_enhanced_error_dialog
Found multiple wx.MessageBox calls still using wx.ICON_ERROR; replace each with show_enhanced_error_dialog for UI-thread safety, consistent i18n, and proper title/flag. Affected locations:
• basilisk/gui/html_view_window.py (line 113)
• basilisk/gui/prompt_attachments_panel.py (lines 423, 692, 741)
• basilisk/gui/account_dialog.py (lines 139, 144, 149, 1071, 1121)
• basilisk/gui/main_frame.py (lines 341, 554, 650, 670, 847, 1039, 1061)
🤖 Prompt for AI Agents
In basilisk/completion_handler.py around lines 299 to 305, and for all listed
files (basilisk/gui/html_view_window.py line 113;
basilisk/gui/prompt_attachments_panel.py lines 423, 692, 741;
basilisk/gui/account_dialog.py lines 139, 144, 149, 1071, 1121;
basilisk/gui/main_frame.py lines 341, 554, 650, 670, 847, 1039, 1061), replace
any wx.MessageBox(...) uses that show errors with
show_enhanced_error_dialog(...); pass parent=None (or appropriate parent if
already used), localize the message with _("...") and format it with the error
text, provide a descriptive title (e.g. _("Error") or _("Completion Error")
where relevant), and set is_completion_error=True only for completion-related
dialogs; ensure calls run on the UI thread if necessary.
| from .base_conversation import BaseConversation | ||
| from .enhanced_error_dialog import show_enhanced_error_dialog | ||
| from .history_msg_text_ctrl import HistoryMsgTextCtrl | ||
| from .ocr_handler import OCRHandler |
There was a problem hiding this comment.
Replace remaining wx.MessageBox error paths
Even with the enhanced dialog imported here, on_transcribe_audio_file() and start_recording() still call wx.MessageBox, so those error flows miss URL detection and copy/open actions. Please route those code paths through show_enhanced_error_dialog for consistency with the new UX.
- wx.MessageBox(
- _("The selected provider does not support speech-to-text"),
- _("Error"),
- wx.OK | wx.ICON_ERROR,
- )
+ show_enhanced_error_dialog(
+ parent=self,
+ message=_("The selected provider does not support speech-to-text"),
+ title=_("Error"),
+ is_completion_error=False,
+ )- if ProviderCapability.STT not in cur_provider.capabilities:
- wx.MessageBox(
- _("The selected provider does not support speech-to-text"),
- _("Error"),
- wx.OK | wx.ICON_ERROR,
- )
+ if ProviderCapability.STT not in cur_provider.capabilities:
+ show_enhanced_error_dialog(
+ parent=self,
+ message=_("The selected provider does not support speech-to-text"),
+ title=_("Error"),
+ is_completion_error=False,
+ )As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from .base_conversation import BaseConversation | |
| from .enhanced_error_dialog import show_enhanced_error_dialog | |
| from .history_msg_text_ctrl import HistoryMsgTextCtrl | |
| from .ocr_handler import OCRHandler | |
| def on_transcribe_audio_file(self, event): | |
| ... | |
| show_enhanced_error_dialog( | |
| parent=self, | |
| message=_("The selected provider does not support speech-to-text"), | |
| title=_("Error"), | |
| is_completion_error=False, | |
| ) | |
| ... | |
| def start_recording(self): | |
| ... | |
| if ProviderCapability.STT not in cur_provider.capabilities: | |
| show_enhanced_error_dialog( | |
| parent=self, | |
| message=_("The selected provider does not support speech-to-text"), | |
| title=_("Error"), | |
| is_completion_error=False, | |
| ) | |
| ... |
🤖 Prompt for AI Agents
In basilisk/gui/conversation_tab.py around lines 41 to 44, remaining error flows
in on_transcribe_audio_file() and start_recording() still use wx.MessageBox
which bypasses the new URL-detection/copy/open UX; replace those wx.MessageBox
calls with show_enhanced_error_dialog(...) calls, passing the same title/message
and any exception details (or the caught error) so the enhanced dialog can
detect links and offer copy/open actions, and keep any modality/parent window
arguments consistent with other usages of show_enhanced_error_dialog in the
codebase.
Summary by CodeRabbit
New Features
Refactor