Open
Conversation
Contributor
WalkthroughAdds a Mako-based TemplateService, configuration and UI for HTML templates, rendering of system prompts and conversation exports to HTML, template-path management in preferences, and tests exercising rendering and export/error paths. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainFrame
participant ConversationTab
participant ConversationPresenter
participant TemplateService
participant FileSystem
User->>MainFrame: Click "Export conversation as HTML"
MainFrame->>ConversationTab: ask_export_html()
ConversationTab->>MainFrame: return path
MainFrame->>ConversationPresenter: export_to_html(path)
ConversationPresenter->>TemplateService: render_conversation_export(conversation, profile, template_path)
TemplateService->>FileSystem: load custom template (if provided)
FileSystem-->>TemplateService: template content / error
TemplateService->>TemplateService: render Mako template with context
TemplateService-->>ConversationPresenter: rendered HTML
ConversationPresenter->>FileSystem: write HTML file
FileSystem-->>ConversationPresenter: success / OSError
ConversationPresenter-->>MainFrame: success / show error
sequenceDiagram
participant User
participant View
participant BaseConversationPresenter
participant TemplateService
User->>View: Apply conversation profile
View->>BaseConversationPresenter: render_system_prompt(profile, account, model)
BaseConversationPresenter->>TemplateService: render_system_prompt(template_str, profile, account, model)
TemplateService->>TemplateService: build context (now, profile, account, model)
TemplateService->>TemplateService: render Mako template
TemplateService-->>BaseConversationPresenter: rendered prompt
BaseConversationPresenter-->>View: rendered prompt string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@basilisk/presenters/base_conversation_presenter.py`:
- Around line 88-93: The public method render_system_prompt has an untyped
parameter model; add an explicit type annotation (e.g., model: config.Model) to
the signature and update its Google-style docstring to include the model
parameter type and description so the presenter API remains fully typed and
analyzable; ensure you modify the function signature in
base_conversation_presenter.RenderSystemPrompt and update the docstring for the
model param accordingly.
In `@basilisk/presenters/conversation_presenter.py`:
- Around line 153-167: The TemplateService.render_conversation_export call can
raise (e.g., on a broken template) before the file open, so move the render and
file write into a single try block and catch OSError and ValueError together;
specifically, wrap the
TemplateService.render_conversation_export(self.conversation, profile,
template_path) and the with open(path, "w", encoding="utf-8") as f:
f.write(html) sequence in one try/except and on exception call
self.view.show_error(...) as currently done to surface the error instead of
crashing the menu handler.
In `@basilisk/presenters/conversation_profile_presenter.py`:
- Around line 51-58: The preview is using self.profile (possibly None or stale)
when calling TemplateService.render_system_prompt; instead, gather the current
widget values from the view and construct a temporary profile object (matching
the shape used by validate_and_build_profile) before calling
render_system_prompt so the preview reflects the current form state; replace the
direct use of self.profile in the TemplateService.render_system_prompt call with
this temporary profile built from the view's fields (e.g., values used by
validate_and_build_profile) and keep validate_and_build_profile unchanged.
In `@basilisk/presenters/main_frame_presenter.py`:
- Around line 280-284: The wildcard string passed to the save dialog is not
marked for translation; wrap the file-type filter value "HTML files
(*.html)|*.html" with _(...) and add a translator comment above it (matching the
existing pattern used for the dialog title) so translators see context; update
the call where wildcard=... (alongside defaultFile and style in the same
FileDialog invocation in main_frame_presenter.py) to use _("HTML files
(*.html)|*.html") and add a "# Translators:" line before it.
- Around line 272-288: The presenter method export_conversation currently
constructs a wx.FileDialog and uses a non-localized wildcard; move the
FileDialog logic into the view and have the view return the chosen path to the
presenter: in export_conversation (presenter) get the current tab and, if
present, call a new view method (e.g., view.prompt_export_path(...) or reuse an
existing view API) to obtain the file path, then call
tab.presenter.export_to_html(path) only when a path is returned; remove any
direct wx usage/import from main_frame_presenter.export_conversation. Also
update the wildcard to be localized as _("HTML files") + " (*.html)|*.html" and
add a "# Translators:" comment above that localized wildcard in the view code
where the FileDialog is created.
In `@basilisk/services/template_service.py`:
- Around line 43-65: The _render_inline function currently uses Mako (Template)
which executes arbitrary Python and must not be used to render untrusted
profile.system_prompt content; replace usage by implementing a non-executable
interpolator (e.g., a simple safe string interpolator) and call that for system
prompts instead of _render_inline, keep Mako-only for trusted file-based
templates, and update all callers that render profile.system_prompt (including
the code paths referenced around lines 43-65 and 92-118) to use the new safe
renderer; ensure _render_inline remains for trusted template files or is renamed
to make its executable nature explicit.
- Around line 167-173: The template uses ngettext and pgettext but they are not
guaranteed to be installed into builtins; either import ngettext and pgettext
explicitly in the module that defines ctx (so add imports for ngettext and
pgettext next to _), or update the localization module where
translation.install(...) is called to pass
names=['ngettext','pgettext','npgettext'] so those functions are installed into
builtins; locate the call to translation.install and add the names parameter or
add the explicit imports in the template_service module and ensure ctx includes
the imported names.
In `@basilisk/views/base_conversation.py`:
- Around line 462-468: The system prompt is rendered using self.current_account
and self.current_model before applying the profile's resolved account/model, so
templates referencing account/model get stale context; move the call to
base_conv_presenter.render_system_prompt (and the subsequent
self.system_prompt_txt.SetValue(...)) to after
set_account_and_model_from_profile(profile, fall_back_default_account) or re-run
the render immediately after that call so render_system_prompt uses the updated
current_account/current_model from the profile.
- Around line 462-465: The current code calls
base_conv_presenter.render_system_prompt(profile, self.current_account,
self.current_model) and sets system_prompt_txt before updating the
profile-backed context, and it uses Mako-style rendering which allows arbitrary
Python execution; fix by moving the render call to after the code that updates
self.current_account and self.current_model from profile so the template sees
the new profile context, and replace or constrain Mako rendering in
base_conv_presenter.render_system_prompt to a safe templating approach (e.g., a
sandboxed/limited template engine or explicit variable substitution that forbids
execution of expressions) or sanitize profile.system_prompt before rendering to
strip/escape executable tags; ensure system_prompt_txt.SetValue receives only
the sanitized/safer-rendered output.
In `@basilisk/views/html_view_window.py`:
- Around line 61-64: The HtmlViewWindow constructor calls
TemplateService.render_html_message with
config.conf().templates.html_message_template_path and does not handle template
render errors; wrap the render_html_message call in a try/except around
TemplateService.render_html_message (or wherever render is invoked), catch
exceptions from Mako/runtime, log the error, and then fall back to the embedded
default template (or a bundled default template path) to produce content so a
bad html_message_template_path does not prevent the viewer from opening; ensure
the fallback path or default template is referenced explicitly and that the
exception is surfaced as a controlled log/error message rather than letting it
abort construction.
In `@basilisk/views/preferences_dialog.py`:
- Around line 83-98: The _on_write_default event handler must catch filesystem
errors from shutil.copy and show them via the view error UI instead of letting
exceptions escape; wrap the copy of global_vars.templates_path /
self._default_template_filename to dest in a try/except OSError block inside
_on_write_default, call the standardized error display method provided by
ErrorDisplayMixin (ensure the view class inherits ErrorDisplayMixin from
basilisk/views/view_mixins.py) to report the exception message to the user, and
only call self._path_ctrl.SetValue(str(dest)) on successful copy.
In `@tests/presenters/test_conversation_export.py`:
- Around line 15-18: The mock_view fixture does not set the view lifecycle flag,
so explicitly set conversation_view_base._is_destroying = False inside the
mock_view function before returning it; keep the existing
conversation_view_base.current_profile = None assignment and return
conversation_view_base so tests that instantiate ConversationPresenter have a
deterministic non-destroying view.
In `@tests/services/test_template_service.py`:
- Around line 28-40: The tests permit arbitrary Python execution via
TemplateService.render_prompt by supporting <% ... %> blocks and imports (see
tests test_python_block_execution and test_import_in_block); to fix, remove or
disable interpretation of <% %> execution in TemplateService.render_prompt (or
add a strict non-executable mode by default) so templates are treated as
data-only interpolation, and update/remove tests that assert code
execution/import behavior; if retaining blocks for trusted-local-only use, gate
that behavior behind an explicit opt-in flag (e.g.,
render_prompt(trust_templates=True)) and ensure imports are blocked or sandboxed
unless the opt-in is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5162d41-156f-4eee-b083-32d82e962fc4
⛔ Files ignored due to path filters (3)
basilisk/res/templates/conversation_export.makois excluded by!basilisk/res/**basilisk/res/templates/html_message.makois excluded by!basilisk/res/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.gitignorebasilisk/config/__init__.pybasilisk/config/main_config.pybasilisk/global_vars.pybasilisk/presenters/base_conversation_presenter.pybasilisk/presenters/conversation_presenter.pybasilisk/presenters/conversation_profile_presenter.pybasilisk/presenters/main_frame_presenter.pybasilisk/presenters/preferences_presenter.pybasilisk/services/template_service.pybasilisk/views/base_conversation.pybasilisk/views/conversation_profile_dialog.pybasilisk/views/html_view_window.pybasilisk/views/main_frame.pybasilisk/views/preferences_dialog.pypyproject.tomltests/presenters/test_base_conversation_presenter.pytests/presenters/test_conversation_export.pytests/services/test_template_service.py
- Add _ask_save_path() helper to ConversationTab to factorize wx.FileDialog for save operations (wildcard, defaultFile, FD_SAVE | FD_OVERWRITE_PROMPT) - Add ask_save_as() and ask_export_html() to ConversationTab; MainFrame delegates to these instead of owning the dialogs - Remove save_conversation_as() and export_conversation() from MainFramePresenter (wx.FileDialog no longer in presenter layer) - Simplify save_current_conversation() and on_save_as_conversation() now that current_tab is always non-None Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- base_conversation_presenter: annotate model param as ProviderAIModel | None - conversation_presenter: wrap render + file write in single try/except (OSError, ValueError) so broken templates surface via show_error - conversation_profile_presenter: preview_system_prompt builds profile from current form state via validate_and_build_profile() instead of using the possibly-None/stale self.profile - base_conversation.py: render system prompt after set_account_and_model_from_profile so template context reflects the profile's resolved account/model - preferences_dialog: TemplatePathWidget becomes wx.Panel + ErrorDisplayMixin; PreferencesDialog inherits ErrorDisplayMixin; _on_write_default catches OSError and calls show_error() instead of crashing - preferences_presenter: wrap conf.save() in try/except (OSError, ValidationError) and show error without closing dialog on failure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
basilisk/presenters/conversation_profile_presenter.py (1)
57-64:⚠️ Potential issue | 🟠 MajorKeep preview rendering side-effect free.
validate_and_build_profile()writes back intoself.profileon Lines 82-125, so clicking Preview can mutate the live profile before Save/Cancel. Theor self.profilefallback also reuses stale data when Line 79 returns early on a blank name. Please render against a temporary/copy-on-write profile built from the current widget state instead of reusingself.profile.Suggested direction
- preview_profile = self.validate_and_build_profile() or self.profile + preview_profile = self._build_preview_profile()def _build_preview_profile(self) -> ConversationProfile: preview_profile = ( self.profile.model_copy(deep=True) if self.profile is not None else ConversationProfile.model_construct() ) preview_profile.name = self.view.profile_name_txt.GetValue() preview_profile.system_prompt = self.view.system_prompt_txt.GetValue() # copy the remaining fields from the current widget state here return preview_profile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@basilisk/presenters/conversation_profile_presenter.py` around lines 57 - 64, The preview flow mutates self.profile because validate_and_build_profile() writes back to it and the current code falls back to self.profile; instead build a temporary preview profile from the widget state and pass that into TemplateService.render_system_prompt so Preview is side-effect free. Implement a helper like _build_preview_profile() that creates a deep copy (or new model_construct) of ConversationProfile, populate fields from the view widgets (e.g. view.profile_name_txt.GetValue(), view.system_prompt_txt.GetValue(), and other widget values), then replace the call site in render path (where validate_and_build_profile() and self.profile are used) to use this temporary profile; keep TemplateService.render_system_prompt(template_str, preview_profile, self.view.current_account, self.view.current_model) unchanged.tests/presenters/test_conversation_export.py (1)
15-18:⚠️ Potential issue | 🟡 MinorSet
_is_destroying = Falseon the mock view fixture.These tests instantiate
ConversationPresenter, so the fixture should define the lifecycle flag explicitly instead of relying on whateverconversation_view_basehappens to provide.Proposed fix
`@pytest.fixture` def mock_view(conversation_view_base): """Return a mock view with current_profile set to None.""" conversation_view_base.current_profile = None + conversation_view_base._is_destroying = False return conversation_view_baseBased on learnings: Mock views for
ConversationPresentermust setview._is_destroying = Falseexplicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/presenters/test_conversation_export.py` around lines 15 - 18, The mock view fixture function mock_view should explicitly set the lifecycle flag on the provided conversation_view_base by assigning conversation_view_base._is_destroying = False before returning it so that tests instantiating ConversationPresenter get a deterministic non-destroying view; update the mock_view function to set current_profile = None and also set _is_destroying = False on conversation_view_base before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@basilisk/presenters/preferences_presenter.py`:
- Around line 122-138: The code mutates the live config (conf) before calling
conf.save(), so on save failure the runtime config is left changed; fix by
staging edits on a copy or restoring the previous state: capture the existing
values from config.conf() (or deep-copy the conf object) before assigning the
new paths from self.view.html_message_template_path.get_path() and
self.view.html_export_template_path.get_path(), then call conf.save() on the
modified copy (or apply assignments to the real conf only after successful
save); if save raises in the except block, restore the original values back into
the live config and then call self.view.show_error(...) so runtime state remains
unchanged on failure.
In `@basilisk/views/base_conversation.py`:
- Around line 465-468: The code currently overwrites the editor control with the
rendered template by calling self.system_prompt_txt.SetValue(rendered) after
base_conv_presenter.render_system_prompt(...); instead, leave the editor showing
the raw template (profile.system_prompt) so previews can re-render; remove or
replace the SetValue(rendered) line and ensure rendering is only done in
preview/send/export code paths (where base_conv_presenter.render_system_prompt
is invoked), and if needed set the control to profile.system_prompt (not
rendered) so system_prompt_txt continues to expose the original Mako template
used by conversation_profile_presenter.
In `@basilisk/views/conversation_tab.py`:
- Around line 630-638: The default export filename in ask_export_html uses
self.GetLabel() (panel label) which often yields "conversation.html"; change it
to use the conversation's title (prefer self.conversation.title, falling back to
self.title or a localized "conversation") when building default, i.e. update the
default variable assignment in ask_export_html to derive the base name from
self.conversation.title/self.title and then append ".html", ensuring proper
fallback and localization.
In `@basilisk/views/preferences_dialog.py`:
- Around line 75-80: The file dialog wildcard contains hard-coded English
labels; wrap the file-filter labels inside the wildcard in _(...) and add
translator context comments so they are localizable: update the wx.FileDialog
wildcard entries used in the preferences dialog (the call to wx.FileDialog) to
build the wildcard string with _("Mako files (*.mako)|*.mako") and _("All files
(*.*)|*.*") (or equivalent localized pieces) and place preceding "#
Translators:" comments describing each label so translators have context; do the
same for the second wildcard usage later in the same function/block.
In `@tests/presenters/test_preferences_presenter.py`:
- Around line 141-178: The tests
test_oserror_on_save_shows_error_and_keeps_dialog_open and
test_validation_error_on_save_shows_error_and_keeps_dialog_open should
explicitly set mock_view.html_message_template_path and
mock_view.html_export_template_path to mocks that have get_path.return_value
(e.g., MagicMock(get_path=MagicMock(return_value="..."))) so presenter.on_ok()
calls to html_message_template_path.get_path() and
html_export_template_path.get_path() are exercised before mock_conf.save() is
invoked; update both tests to assign those attributes on mock_view prior to
calling presenter.on_ok() so autovivification does not hide interface
regressions.
---
Duplicate comments:
In `@basilisk/presenters/conversation_profile_presenter.py`:
- Around line 57-64: The preview flow mutates self.profile because
validate_and_build_profile() writes back to it and the current code falls back
to self.profile; instead build a temporary preview profile from the widget state
and pass that into TemplateService.render_system_prompt so Preview is
side-effect free. Implement a helper like _build_preview_profile() that creates
a deep copy (or new model_construct) of ConversationProfile, populate fields
from the view widgets (e.g. view.profile_name_txt.GetValue(),
view.system_prompt_txt.GetValue(), and other widget values), then replace the
call site in render path (where validate_and_build_profile() and self.profile
are used) to use this temporary profile; keep
TemplateService.render_system_prompt(template_str, preview_profile,
self.view.current_account, self.view.current_model) unchanged.
In `@tests/presenters/test_conversation_export.py`:
- Around line 15-18: The mock view fixture function mock_view should explicitly
set the lifecycle flag on the provided conversation_view_base by assigning
conversation_view_base._is_destroying = False before returning it so that tests
instantiating ConversationPresenter get a deterministic non-destroying view;
update the mock_view function to set current_profile = None and also set
_is_destroying = False on conversation_view_base before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4dc7814e-8080-4d54-a68a-9ca9425a9787
📒 Files selected for processing (12)
basilisk/presenters/base_conversation_presenter.pybasilisk/presenters/conversation_presenter.pybasilisk/presenters/conversation_profile_presenter.pybasilisk/presenters/main_frame_presenter.pybasilisk/presenters/preferences_presenter.pybasilisk/views/base_conversation.pybasilisk/views/conversation_tab.pybasilisk/views/main_frame.pybasilisk/views/preferences_dialog.pytests/presenters/test_conversation_export.pytests/presenters/test_main_frame_presenter.pytests/presenters/test_preferences_presenter.py
💤 Files with no reviewable changes (2)
- tests/presenters/test_main_frame_presenter.py
- basilisk/presenters/main_frame_presenter.py
Comment on lines
+122
to
+138
| conf.templates.html_message_template_path = ( | ||
| self.view.html_message_template_path.get_path() | ||
| ) | ||
| conf.templates.html_export_template_path = ( | ||
| self.view.html_export_template_path.get_path() | ||
| ) | ||
|
|
||
| conf.save() | ||
| try: | ||
| conf.save() | ||
| except (OSError, ValidationError) as exc: | ||
| self.view.show_error( | ||
| # Translators: Error shown when saving preferences fails | ||
| _("Failed to save preferences: {error}").format(error=exc), | ||
| # Translators: Title for the preferences save error dialog | ||
| _("Save error"), | ||
| ) | ||
| return |
Contributor
There was a problem hiding this comment.
Roll back staged config changes when saving fails.
These assignments mutate the live config.conf() object before conf.save() runs. If save() raises, the dialog stays open but the process is still using the unsaved values, so a failed save can silently change runtime behavior for the rest of the session. Stage the edits on a copy or restore the previous config state in the except path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@basilisk/presenters/preferences_presenter.py` around lines 122 - 138, The
code mutates the live config (conf) before calling conf.save(), so on save
failure the runtime config is left changed; fix by staging edits on a copy or
restoring the previous state: capture the existing values from config.conf() (or
deep-copy the conf object) before assigning the new paths from
self.view.html_message_template_path.get_path() and
self.view.html_export_template_path.get_path(), then call conf.save() on the
modified copy (or apply assignments to the real conf only after successful
save); if save raises in the except block, restore the original values back into
the live config and then call self.view.show_error(...) so runtime state remains
unchanged on failure.
Comment on lines
+465
to
+468
| rendered = self.base_conv_presenter.render_system_prompt( | ||
| profile, self.current_account, self.current_model | ||
| ) | ||
| self.system_prompt_txt.SetValue(rendered) |
Contributor
There was a problem hiding this comment.
Keep system_prompt_txt on the raw template.
basilisk/presenters/conversation_profile_presenter.py:50-75 reads self.view.system_prompt_txt.GetValue() as the template source for preview rendering. Writing rendered back into this control here strips the Mako syntax as soon as apply_profile() runs, so subsequent previews cannot re-render against the current profile/account/model state. Render only in preview/send/export paths, and leave the editor bound to profile.system_prompt.
Suggested fix
- rendered = self.base_conv_presenter.render_system_prompt(
- profile, self.current_account, self.current_model
- )
- self.system_prompt_txt.SetValue(rendered)
+ self.system_prompt_txt.SetValue(profile.system_prompt or "")📝 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.
Suggested change
| rendered = self.base_conv_presenter.render_system_prompt( | |
| profile, self.current_account, self.current_model | |
| ) | |
| self.system_prompt_txt.SetValue(rendered) | |
| self.system_prompt_txt.SetValue(profile.system_prompt or "") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@basilisk/views/base_conversation.py` around lines 465 - 468, The code
currently overwrites the editor control with the rendered template by calling
self.system_prompt_txt.SetValue(rendered) after
base_conv_presenter.render_system_prompt(...); instead, leave the editor showing
the raw template (profile.system_prompt) so previews can re-render; remove or
replace the SetValue(rendered) line and ensure rendering is only done in
preview/send/export code paths (where base_conv_presenter.render_system_prompt
is invoked), and if needed set the control to profile.system_prompt (not
rendered) so system_prompt_txt continues to expose the original Mako template
used by conversation_profile_presenter.
Comment on lines
+630
to
+638
| def ask_export_html(self) -> None: | ||
| """Show a save dialog for .html and export the conversation.""" | ||
| default = f"{self.GetLabel() or _('conversation')}.html" | ||
| path = self._ask_save_path( | ||
| # Translators: Dialog title when exporting a conversation to HTML | ||
| _("Export conversation as HTML"), | ||
| "HTML files (*.html)|*.html", | ||
| default_file=default, | ||
| ) |
Contributor
There was a problem hiding this comment.
Use the conversation title for the default export filename.
self.GetLabel() is the panel label, not the notebook tab title, so this will usually fall back to "conversation.html" even when the tab already has a meaningful title. Derive the suggestion from self.conversation.title or self.title instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@basilisk/views/conversation_tab.py` around lines 630 - 638, The default
export filename in ask_export_html uses self.GetLabel() (panel label) which
often yields "conversation.html"; change it to use the conversation's title
(prefer self.conversation.title, falling back to self.title or a localized
"conversation") when building default, i.e. update the default variable
assignment in ask_export_html to derive the base name from
self.conversation.title/self.title and then append ".html", ensuring proper
fallback and localization.
Comment on lines
+75
to
+80
| with wx.FileDialog( | ||
| self, | ||
| # Translators: Dialog title when browsing for a template file | ||
| _("Select template file"), | ||
| wildcard="Mako files (*.mako)|*.mako|All files (*.*)|*.*", | ||
| style=wx.FD_OPEN | wx.FD_FILE_MUST_EXIST, |
Contributor
There was a problem hiding this comment.
Localize the file-filter labels in these dialogs.
"Mako files" and "All files" are user-visible here, but they are hard-coded English strings inside the wildcard. Please wrap those labels in _() and add # Translators: comments so the new template dialogs stay consistent with the rest of the localized UI.
As per coding guidelines, "Use _(\"string\") for all user-facing text to enable translation" and "Use # Translators: comment before translatable strings to provide context for translators".
Also applies to: 87-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@basilisk/views/preferences_dialog.py` around lines 75 - 80, The file dialog
wildcard contains hard-coded English labels; wrap the file-filter labels inside
the wildcard in _(...) and add translator context comments so they are
localizable: update the wx.FileDialog wildcard entries used in the preferences
dialog (the call to wx.FileDialog) to build the wildcard string with _("Mako
files (*.mako)|*.mako") and _("All files (*.*)|*.*") (or equivalent localized
pieces) and place preceding "# Translators:" comments describing each label so
translators have context; do the same for the second wildcard usage later in the
same function/block.
Comment on lines
+141
to
+178
| def test_oserror_on_save_shows_error_and_keeps_dialog_open( | ||
| self, mock_view, make_presenter, mocker | ||
| ): | ||
| """OSError from conf.save() shows error and does not close the dialog.""" | ||
| mock_wx = MagicMock() | ||
| mocker.patch.dict(sys.modules, {"wx": mock_wx}) | ||
| mocker.patch("basilisk.presenters.preferences_presenter.set_log_level") | ||
| presenter, mock_conf = make_presenter(view=mock_view) | ||
| mock_conf.save.side_effect = OSError("disk full") | ||
|
|
||
| presenter.on_ok() | ||
|
|
||
| mock_view.show_error.assert_called_once() | ||
| mock_view.EndModal.assert_not_called() | ||
|
|
||
| def test_validation_error_on_save_shows_error_and_keeps_dialog_open( | ||
| self, mock_view, make_presenter, mocker | ||
| ): | ||
| """ValidationError from conf.save() shows error and does not close dialog.""" | ||
| from pydantic import BaseModel, ValidationError | ||
|
|
||
| mock_wx = MagicMock() | ||
| mocker.patch.dict(sys.modules, {"wx": mock_wx}) | ||
| mocker.patch("basilisk.presenters.preferences_presenter.set_log_level") | ||
| presenter, mock_conf = make_presenter(view=mock_view) | ||
|
|
||
| class _M(BaseModel): | ||
| x: int | ||
|
|
||
| try: | ||
| _M(x="bad") | ||
| except ValidationError as exc: | ||
| mock_conf.save.side_effect = exc | ||
|
|
||
| presenter.on_ok() | ||
|
|
||
| mock_view.show_error.assert_called_once() | ||
| mock_view.EndModal.assert_not_called() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make these error-path tests configure the new template-path widgets explicitly.
on_ok() now reads html_message_template_path.get_path() and html_export_template_path.get_path() before save(), but these tests currently succeed via MagicMock autovivification. That means interface regressions around the new widgets will not fail the suite. Set both attributes on mock_view to explicit mocks with get_path.return_value so this path is exercised realistically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/presenters/test_preferences_presenter.py` around lines 141 - 178, The
tests test_oserror_on_save_shows_error_and_keeps_dialog_open and
test_validation_error_on_save_shows_error_and_keeps_dialog_open should
explicitly set mock_view.html_message_template_path and
mock_view.html_export_template_path to mocks that have get_path.return_value
(e.g., MagicMock(get_path=MagicMock(return_value="..."))) so presenter.on_ok()
calls to html_message_template_path.get_path() and
html_export_template_path.get_path() are exercised before mock_conf.save() is
invoked; update both tests to assign those attributes on mock_view prior to
calling presenter.on_ok() so autovivification does not hide interface
regressions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Chores
Tests