Skip to content

Commit 0176e8b

Browse files
clementb49claude
andauthored
refactor: apply MVP design pattern (#1062)
* feat(convDatabase): add sqlalchemy and alembic * feat(convDatabase): implement a first version of the conversation database * chore: ignore claude code settings * refactor(convDatabase): optimize subsequent save query * feat(convDatabase): add settings and private mode * refactor(convDatabase): manage database singleton inside wx app singleton * refactor(convDatabase): optimize database manager code * fix(convDatabase): code review comment * fix(convDatabase): use correct method name * fix(convDatabase): disable btn when no conversation history is selected * fix(build_exe): add missing package for sqlalchemy and alembic * feat: move alembic version in resource dir to simplify fhrozen version * fix(frozenAPP): move alembic dir in resource * refactor(gui): rename gui/ to views/ and update imports Rename the basilisk/gui/ package to basilisk/views/ as the first step of the MVP refactoring. Update all 5 external absolute imports that reference basilisk.gui to basilisk.views. Internal relative imports within the package are unaffected by the folder rename. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(services): extract ConversationService from ConversationTab Create basilisk/services/conversation_service.py containing all database persistence and business logic extracted from ConversationTab: auto_save_to_db, update_db_title, set_private, save_conversation, save_draft_to_db, and generate_title. ConversationTab now delegates to self.service for these operations. db_conv_id and private are properties proxying to the service. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(presenters): extract ConversationPresenter from ConversationTab Create basilisk/presenters/conversation_presenter.py containing all orchestration logic: completion flow, recording coordination, error recovery, submission handling, and draft management. ConversationTab is now a thin view layer (~640 lines, down from ~1170) that creates widgets, handles pure-UI events, and delegates to the presenter via one-line methods. RecordingThread parameter renamed from conversation_tab to callbacks, now pointing to the presenter which provides the same 5 callback methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(mvp): add unit tests for ConversationService and ConversationPresenter Add tests for the service and presenter layers extracted in previous commits. Install translation builtins as passthrough in conftest.py for test environment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(MVP): add a presenter for the main frame * refactor(MVP): split base conversation and conversation pofile * refactor(MVP): split accout dialog * refactor(MVP): extract SearchService, AttachmentService, EditBlockPresenter - Add SearchService with compile_pattern/find_all_matches static methods; move SearchMode/SearchDirection enums and adjust_utf16_position from SearchDialog into the service (bug in reverse preserved + documented) - Add AttachmentService with async download interface (thread + wx.CallAfter callbacks) and static validation helpers (is_format_supported, build_attachment_from_path, validate_attachments, check_model_vision_compatible, resize_attachments) - Add EditBlockPresenter owning CompletionHandler; extract all completion callbacks and save/regenerate orchestration from EditBlockDialog - Update SearchDialog, PromptAttachmentsPanel, EditBlockDialog to delegate to their respective service/presenter - Add tests for all three new components (68 new tests) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style: fix docstring * fix: address review findings across database, GUI, and test layers - gitignore: remove personal .claude/settings.local.json entry (advise contributors to add it to their global gitignore instead) - models: use timezone-aware datetime.now(timezone.utc) for all created_at/updated_at column defaults and onupdate callables - models: add ondelete="SET NULL" to conversation_system_prompt_id FK to avoid constraint violations during cascaded deletes - conversation_model: add SystemMessage.__eq__ that compares only role and content (mirrors __hash__), fixing lookup in OrderedSet when db_id differs - manager: add ConversationDatabase.from_engine() factory classmethod for test-friendly initialisation without running Alembic migrations - main_app: fix docstring typo ("started the background" → "started in the background"); wrap init_conversation_db in try/except; capitalise log messages in close_conversation_db - main_frame: guard on_close against empty tabs_panels / invalid index before calling current_tab - conversation_tab: remove stale _conv_db class-level cache; always return wx.GetApp().conv_db directly; add DRAFT_SAVE_DELAY_MS constant; extract _pop_draft_if_present helper; fix set_private to only clear db_conv_id on successful delete; make _build_draft_block accept optional prompt_text/attachments args; pass pre-fetched values from _save_draft_to_db to avoid double-read - conversation_history_dialog: replace hardcoded limit=200 with PAGE_SIZE=100 constant; add offset/reset pagination with Load More button; add count_label showing "Showing N of M"; fix _on_item_deselected to use wx.CallAfter(_update_buttons_state) - preferences_dialog: disable auto_save_draft when auto_save_to_db is unchecked; add on_auto_save_to_db_changed EVT_CHECKBOX handler - pyproject.toml: sort alembic and sqlalchemy into correct alphabetical position in dependencies - tests/conftest: use ConversationDatabase.from_engine() instead of __new__ + manual attribute injection - tests/test_manager: fix flaky test_list_ordered_by_updated with deterministic updated_at values via direct DB update - tests/test_models: add db_session.rollback() after every pytest.raises(IntegrityError) block - tests/test_roundtrip: verify restored attachment content matches original in test_attachment_dedup_roundtrip Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(database): add cleanup_orphan_attachments to reclaim orphaned blobs DBAttachment uses content-hash deduplication and has no cascade delete from DBMessageAttachment, so blob rows can accumulate as orphans when conversations or draft blocks are deleted. Add ConversationDatabase.cleanup_orphan_attachments() that identifies unreferenced rows via a LEFT JOIN on DBMessageAttachment and removes them in a single transaction. Wire it to: - __init__ (once at startup, to sweep orphans from previous sessions) - delete_conversation (after each cascade delete commits) Add TestCleanupOrphanAttachments covering: no-orphan path, full cleanup after a conversation delete, and shared-attachment survival when only one of two referencing conversations is removed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(conversation): reduce setup duplication across database and conversation tests - Add db_message_block fixture to database conftest; drop redundant created_at/updated_at kwargs from all DBConversation/DBMessageBlock constructions in test_models.py - Replace _make_block, _make_message_and_attachment, _make_message class helpers with the shared db_message_block fixture - Lift local imports and add _count_attachments helper in test_manager.py - Add tests/conversation/conftest.py with shared bskc_path and storage_path fixtures; remove duplicate definitions from 3 test classes - Replace 6 numbered fixtures with make_user_block/make_system factories in TestConversationWithMultipleBlocks - Move import io and from PIL import Image to module level in test_migration.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: save draft prompt after model and account selection * chore: remove claude file * refactor: add presenter for conversation history and preference * refactor(MVP): add SearchPresenter, OCRPresenter, UpdatePresenter Extract business logic from SearchDialog, OCRHandler, UpdateDialog and DownloadUpdateDialog into dedicated presenters (Phase 4). - SearchPresenter owns search state and navigation; SearchTargetAdapter wraps TextCtrl; SearchDialog is now a pure view with a defined getter/ setter interface; HistoryMsgTextCtrl constructs presenter lazily - OCRPresenter owns subprocess, queue and cancel flag; scheduler is injectable for test isolation; OCRHandler becomes a thin IOCRView wrapper - UpdatePresenter / DownloadPresenter extract threading; fix bug where wx.Destroy() was called directly from a worker thread (now routed via wx.CallAfter); UpdateDialog / DownloadUpdateDialog are thin view wrappers - main_frame.py wires presenter before Show() / ShowModal() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: add unit tests for Phase 4 presenters 91 new tests covering SearchPresenter, OCRPresenter, UpdatePresenter and DownloadPresenter introduced in the previous commit. - test_search_presenter: target adapter delegation, initial state, property setters, on_find() navigation (forward/backward/not-found), history deduplication, case-insensitive and regex modes, search_next / search_previous, on_mode_changed - test_ocr_presenter: is_running property, on_ocr() validation paths (no capability, no attachments, invalid attachments, no client) and successful start (button disabled, dialog created, process spawned, scheduler called), check_task_progress reschedule / cleanup / cancellation, _handle_ocr_message dispatch (all message types), _process_ocr_queue draining - test_update_presenter: UpdatePresenter.start() with/without updater, disabled-updates path, _do_check() success / no-update / exception, on_update_clicked / on_close / on_release_notes_clicked; DownloadPresenter.start() already-downloaded / thread-start, _do_download() success / cancelled / exception, _on_progress() percentage calculation, on_update_clicked / on_cancel / on_release_notes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(MVP): DRY auto-save draft guard and timer access Extract the 4-condition auto-save guard into ConversationService.should_auto_save_draft() and add start/stop_draft_timer() methods to ConversationTab so the presenter no longer accesses the internal wx.Timer directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(MVP): extract PromptAttachmentPresenter from PromptAttachmentsPanel Move all business logic (attachment state, engine validation, URL download orchestration, clipboard routing, file/URL dialog handling) into a new PromptAttachmentPresenter. The panel becomes a thin view that implements a plain interface (show_error, show_file_dialog, show_url_dialog, refresh_attachments_display, write_prompt_text, focus_attachments) and delegates every public API call to its presenter. Adds 32 unit tests in tests/test_attachment_panel_presenter.py covering add_attachments, check_attachments_valid, ensure_model_compatibility, on_paste_text, on_add_url, on_add_files, download callbacks, and resize_all_attachments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style: move all presenters test in the same folder * refactor(MVP): add HistoryPresenter and BaseConversationPresenter Extract business logic from HistoryMsgTextCtrl into HistoryPresenter (segment_manager, speak_response, a_output, navigation, citations, streaming, search lifecycle) and from BaseConversation into BaseConversationPresenter (account/model resolution, display helpers). Views become thin: proxy properties delegate to the presenter; HistoryMsgTextCtrl gains a bell() method so the presenter stays wx-free. Adds 56 unit tests (674 total passing). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: flush the draft before closing tab * refactor(MVP): Phase 7 — extract EnhancedErrorPresenter and fix NameConversationDialog coupling - Add EnhancedErrorPresenter with URL detection, clipboard, and browser logic - Strip business logic from EnhancedErrorDialog; add set_copy_state/set_open_url_state/bell view interface - Replace self.parent.current_tab coupling in NameConversationDialog with generate_fn callable - Remove auto param from NameConversationDialog; MainFramePresenter pre-generates title when auto=True - Fix wx.MessageBox argument order bug in NameConversationDialog - Add 13 unit tests for EnhancedErrorPresenter (no wx display required) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(MVP): Phase 7 audit — fix SyntaxError, style issues, and expand tests - Fix Python 3 except syntax in history_presenter (2 sites) - Move show_enhanced_error_dialog import to module level in conversation_presenter - Add from __future__ import annotations and use list[re.Match] in search_service - Replace Optional[...] with | None syntax in base_conversation_presenter - Add view._is_destroying = False to mock_view in test_edit_block_presenter - Add 10 tests to test_conversation_presenter: TestBuildDraftBlock, TestCleanup, TestIsDestroyingGuard Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: use local var to avoid setting read-only account_model_service property `BaseConversation` exposes `account_model_service` as a read-only property proxying `base_conv_presenter.account_model_service`. Assigning to it in `ConversationTab.__init__` raised an AttributeError on startup. Use a local variable and pass it directly to `BaseConversation.__init__` instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(presenters): address code review findings from mvc branch audit * fix(conversation_presenter): guard against None current_engine start_recording(), transcribe_audio_file(), and on_transcribe_audio_file() all dereferenced self.view.current_engine without checking for None first, which would raise AttributeError when no account is selected. Add early-return guards before any attribute access or use of the engine value. Add a test covering the None-engine path in start_recording(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(conversation_profile_presenter): honor nullable contract and sync menu_update validate_and_build_profile() documented returning None on invalid input but let model_validate() raise ValidationError instead. Wrap the call in try/except, log the error, and return None so callers get the documented behaviour. set_default() was the only mutating method that did not set menu_update = True after saving, leaving the menu stale after a default-profile change. Add the flag to match add_profile(), edit_profile(), and remove_profile(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(attachment_panel_presenter): use Windows-safe timestamp in clipboard filename isoformat() produces colons in the timestamp (e.g. 14:30:00) which are invalid in Windows file paths. Replace it with strftime using underscores as separators. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(conversation_profile_dialog): translate missing hardcoded dialog title "Edit Conversation Profile" was not wrapped in _(), leaving it untranslated regardless of the active locale. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(search_presenter): catch re.error from invalid regex and refactor on_find Wrap SearchService.find_all_matches in try/except re.error so that an invalid pattern in REGEX mode calls view.show_error() instead of propagating an unhandled exception. Refactor on_find() into three private helpers to reduce its length: - _sync_state_from_view(): snapshot widget values into presenter state - _find_matches(): run the search, absorb re.error, return None on error - _navigate_to_match(): select nearest match or report not found Remove duplication in search_next() and search_previous(): both were manually replicating the search_direction property setter logic; they now delegate to the property setter directly. Add TestSearchPresenterInvalidRegex with 4 tests covering the new re.error handling path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(main_frame_presenter): correct exc_info arg and guard post_screen_capture exc_info= expects a bool (or exception) understood by logging; passing the exception object directly as exc_info=e worked by accident but the conventional form is exc_info=True. post_screen_capture() dereferenced self.view.current_tab without checking for None; add an early-return guard so a stale capture callback does not crash when no tab is open. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(enhanced_error_presenter): extract fail_open_browser helper and check return value Moves the browser-failure notification into a reusable `fail_open_browser()` method and checks `webbrowser.open()` return value so a silent failure (returns False) is also surfaced to the user. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style(preferences_presenter): rename module-level vars to UPPER_SNAKE_CASE `release_channels` and `auto_update_modes` are module-level constants; rename to `RELEASE_CHANNELS` and `AUTO_UPDATE_MODES` per project convention. Update the import in `preferences_dialog.py` accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: relocate enhanced_error_presenter tests under tests/presenters/ Moves test_enhanced_error_presenter.py from tests/ into the tests/presenters/ sub-package to match the presenter module's location, consistent with the layout used by the other presenter test files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(conversation_service): retain db_conv_id and notify user on failed privacy switch `set_private()` previously cleared `self.db_conv_id` unconditionally, even when `delete_conversation()` raised. Move the assignment inside the `try` block so a failed deletion leaves the id intact for retries, and revert the `private` flag so the conversation is not silently marked private while its record still exists in the database. The return type changes from `bool` to `tuple[bool, bool]` (success, should_stop_timer). `ConversationTab.set_private()` unpacks the tuple and shows a localized error dialog when success is False. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(main_frame): show (private) label in tab and frame title when toggling privacy toggle_privacy() never called a title refresh, so converting an existing conversation to private left the tab label and window title unchanged. Two fixes: - refresh_tab_title() now appends " (private)" to the notebook tab text when the current tab is private, matching the existing behaviour of refresh_frame_title() for the window title bar. - toggle_privacy() calls refresh_tab_title(include_frame=True) after set_private() so both the tab label and the frame title update immediately (whether the toggle succeeded or was reverted on DB error). - add_conversation_tab() now calls refresh_tab_title(include_frame=True) instead of refresh_frame_title() so newly created private conversations also display the label on the tab. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(update_presenter): fix typo in log message and guard against zero total_length Corrects "Update are disabled" → "Updates are disabled" in the log call and adds an early-return guard in on_download_progress when total_length is <= 0 to prevent a ZeroDivisionError before the percentage calculation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style(attachment_service): add UPath | str type annotation to conv_storage_path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(main_frame_presenter): eliminate duplication between default and private conversation creation on_new_default_conversation and on_new_private_conversation shared the same three steps (handle_no_account_configured, default-profile lookup, log, new_conversation call). Extract them into a single private helper _new_conversation_from_default_profile(private=False) and reduce each public method to a one-line delegation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(search_service): correct reverse UTF-16 position adjustment adjust_utf16_position() with reverse=True was computing count -= count (always 0) instead of negating the counters, so the reverse direction never subtracted the surrogate-pair and newline offsets. This caused cursor_pos to remain in UTF-16 space when navigating backwards, making backward search skip or land on the wrong match in text containing non-BMP characters or newlines. Fix: negate count_high_surrogates and count_line_breaks when reverse=True so the final addition becomes a subtraction. Update the docstring to remove the bug note and update the test test_reverse_zeroes_adjustment_bug → test_reverse_subtracts_adjustment to assert the corrected return value (0 instead of 1 for a UTF-16 position inside an emoji surrogate pair). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(account_dialog): guard against -1 selection in on_edit and on_remove GetFirstSelected() returns -1 when no item is selected. Both handlers indexed into self.presenter.organizations with that value before the guard was added, causing an IndexError. The UI already disables the buttons via update_ui() on EVT_LIST_ITEM_SELECTED, but two paths bypass that: - EVT_LIST_ITEM_DESELECTED is not bound, so clicking empty space can leave the buttons enabled. - on_org_list_key_down forwards Enter/Delete directly to the handlers, completely bypassing the button enabled state. Add an early return when selected_item == -1 in both on_edit and on_remove so neither path can reach the index operation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(account_dialog): guard against -1 selection in AccountDialog.update_ui GetFirstSelected() returns -1 when no account is selected. update_ui() indexed self.account_manager[-1] unconditionally, returning the last account and passing an invalid index to presenter.is_editable / presenter.is_default. Add an early-return branch: when index == -1 disable edit_btn, remove_btn, manage_organizations, and default_account_btn, then return without touching the presenter or account_manager. The other handlers (on_edit, on_remove, on_default_account) already carried this guard; update_ui was the only remaining gap. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(conversation_history_dialog): add Translators comments to deletion error MessageBox The two translatable strings in the failed-delete MessageBox lacked # Translators: context comments, unlike every other translatable string in the file. Add them so gettext tooling and translators receive the intended context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(conversation_history): surface DB errors instead of silently returning empty results load_conversations() and get_conversation_count() swallowed all exceptions and returned []/0, making a DB failure indistinguishable from a legitimately empty history. Remove the try/except from both presenter methods so exceptions propagate to the caller. Wrap both calls together in _refresh_list() with a single try/except that logs the error and shows a wx.MessageBox so the user sees a failure notification rather than a confusingly empty list. Update tests: - test_returns_empty_list_on_error → test_propagates_db_error (raises) - Add TestGetConversationCount (was entirely untested) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(edit_block_dialog): guard against out-of-range block_index in __init__ Indexing self.conversation.messages[self.block_index] without a bounds check raised an unhandled IndexError when the index was stale (e.g. a block was deleted between the dialog being triggered and __init__ running). Add a guard: if block_index is outside [0, len(messages)), log a warning, set self.block = None, show a localized wx.MessageBox, then schedule EndModal(ID_CANCEL) via wx.CallAfter so the dialog dismisses itself as soon as ShowModal enters its event loop, and return early to skip init_ui / load_message_block_data. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(update_dialog): destroy MessageDialog and DownloadUpdateDialog after ShowModal Three dialog leaks fixed: 1. DownloadUpdateDialog.show_error: wx.MessageDialog was constructed inline and ShowModal() called without any reference, so Destroy() was never called. Store in dlg and use try/finally to always Destroy(). 2. UpdateDialog.show_error: same inline wx.MessageDialog pattern. Apply the same try/finally fix. 3. UpdateDialog.start_download: DownloadUpdateDialog.close() was calling self.Destroy() to end the modal loop, which made it unsafe for the caller to Destroy() again. Change close() to use EndModal(ID_CANCEL) instead — the proper way to end a modal loop — and wrap start_download's ShowModal() in try/finally so the caller always owns and destroys the dialog. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style(test_history_presenter): move SearchDirection import to module level Two tests imported SearchDirection inline (after the assertion setup), which is unconventional and hides the dependency. Move the import to the module-level imports block and remove the two inline occurrences. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(account_presenter): tighten remove and set_default assertions to check arguments Both tests were using assert_called_once() which only verifies call count, not arguments. Replacing with assert_called_once_with() revealed that the presenter passes the account object (not the integer index) to account_manager.remove() and account_manager.set_default_account(). Capture the expected account via mock_account_manager[0] before calling the presenter so the assertion matches the actual argument. Also tighten save() to assert_called_once_with() for consistency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(mvp): introduce presenter and view mixins to reduce duplication Add DestroyGuardMixin/_guard_destroying decorator (replaces 12 inline `if self.view._is_destroying: return` guards in ConversationPresenter), ManagerCrudMixin (consolidates add/edit/remove/save in AccountPresenter and ConversationProfilePresenter), ErrorDisplayMixin (unifies show_error and show_enhanced_error across 7 views, moving wx.MessageBox calls out of presenters), and require_list_selection decorator (replaces 10 manual `if index == -1: return` guards in account, history, and profile dialogs). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(attachment_panel_presenter): guard against None engine in ensure_model_compatibility Add a None check for self.current_engine before passing it to AttachmentService.check_model_vision_compatible, which dereferences engine.models and would raise AttributeError if called with None. Also replace .format() with %-style formatting on the translatable error string to follow project translation guidelines. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(conversation): remove MessageBlock model_id/provider_id shim The custom __init__ on MessageBlock that accepted model_id and provider_id as convenience kwargs was only used in three call sites. Update all three (conversation_presenter, edit_block_presenter, conversation_service) to pass model=AIModelInfo(...) directly, then remove the now-dead shim so MessageBlock relies on standard Pydantic initialisation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(conversation_tab): remove dead extract_text_from_message method The method was never called anywhere in the codebase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(edit_block_presenter): persist edited block to database on save save_block() mutated the in-memory MessageBlock but never called service.auto_save_to_db(), so edits were lost from the conversation database on restart. Pass ConversationService from ConversationTab into EditBlockPresenter and call auto_save_to_db (plus refresh updated_at) at the end of a successful save. Add three new tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: fix ruff PT006/PT011/PT012/D104 lint errors in test suite - PT006: use tuple instead of string for parametrize first arg (test_attachment.py, 6 occurrences) - PT011: add match= to pytest.raises(ValueError/Exception) calls (test_manager, test_conversation_profile, test_message_position_manager) - PT012: remove dead assert statements from inside pytest.raises blocks (test_model.py, 4 occurrences) - D104: add missing docstring to tests/views/__init__.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(deps): add pytest mock for modern testing * refactor: rework tests * fix: correct translation string and comment * refactor: improve i18n string formatting and translator context - Consolidate OCR success message into a single translatable template with %s placeholder; add Translators comments for both OCR dialogs - Add remove_attachment() to PromptAttachmentPresenter and call it from the view instead of mutating attachment_files directly; add test - Merge extended search mode label suffix into the translatable string so translators can position escape-sequence examples freely - Replace .format() with % formatting in search not-found message - Add Translators comments for DownloadUpdateDialog title and label - Add Translators comment for default "Error" dialog title in view mixin Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: remove .claude directory * fix: correct test assertions and i18n/consistency issues - Add Translators comments to both no-engine guards in attachment presenter - Set MessageBlock.stream=False for title generation to match completion_kw - Remove locale-specific words from toggle_speak_response test; assert non-empty - Fix test_case_insensitive_by_default: use text with match after cursor and assert set_selection called instead of show_not_found Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: update CLAUDE.md, coderabbit and copilot instructions for MVP architecture Reflect the completed MVP refactoring: views in basilisk/views/, presenters in basilisk/presenters/, services in basilisk/services/. Document presenter rules (no wx, DestroyGuardMixin, ErrorDisplayMixin), resource cleanup pattern, and updated test infrastructure (pytest-mock, mocker fixture, PT006, subtests, _is_destroying requirement). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: make remove_attachment idempotent on missing items Guard against ValueError when the attachment is not in the list, logging a debug message instead of raising. Add a test to cover the no-op path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 474d96b commit 0176e8b

File tree

93 files changed

+14358
-5408
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

93 files changed

+14358
-5408
lines changed

.coderabbit.yaml

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,19 @@ reviews:
2626
- Use **type hints** for all public classes and functions
2727
- Use **google style** for docstrings
2828
## Strings
29-
- Always use **double quotes** for strings
30-
- Use **percent formatting** for string
29+
- Preserve existing quote style; prefer double quotes for new code
3130
- Use **\_("string")** for translatable strings
32-
- Use \*\*# translator: \*\* with context for translatable strings
31+
- Use **# Translators:** comment (capital T) before translatable strings to provide context for translators
32+
## Imports
33+
- Use **from __future__ import annotations** in all presenter and service modules
34+
- Group imports: standard library → third-party → local (basilisk), alphabetically sorted
35+
## MVP Architecture
36+
- **Views** live in `basilisk/views/` (wxPython UI, no business logic)
37+
- **Presenters** live in `basilisk/presenters/` (business logic, no wx imports where possible)
38+
- **Services** live in `basilisk/services/` (reusable logic, thread-safe async operations)
39+
- Presenters receive a view via injection; do not access wx directly in presenters
40+
- Use `DestroyGuardMixin` + `@_guard_destroying` for presenter callbacks that may fire after view destruction
41+
- Use `ErrorDisplayMixin` on views; call `view.show_error()` / `view.show_enhanced_error()` from presenters
3342
3443
tools:
3544
github-checks:

.github/copilot-instructions.md

Lines changed: 96 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,71 @@
22

33
## Architecture Overview
44

5-
BasiliskLLM is a **wxPython desktop GUI** application that provides a unified interface for multiple LLM providers (OpenAI, Anthropic, Gemini, Mistral, Ollama, etc.).
5+
BasiliskLLM is a **wxPython desktop GUI** application that provides a unified interface for multiple LLM providers (OpenAI, Anthropic, Gemini, Mistral, Ollama, etc.). The codebase follows an **MVP (Model-View-Presenter)** pattern.
66

77
### Key Components
88

99
- **Main Application**: `basilisk/main_app.py` - wxPython app initialization, IPC, auto-updates
10-
- **GUI Framework**: `basilisk/gui/` - wxPython-based interface with conversation tabs, dialogs, preferences
10+
- **Views**: `basilisk/views/` - wxPython-based UI components (no business logic)
11+
- **Presenters**: `basilisk/presenters/` - Business logic, decoupled from wx
12+
- **Services**: `basilisk/services/` - Reusable async/thread-safe logic
1113
- **Provider Engine**: `basilisk/provider_engine/` - Abstracted LLM provider implementations using `BaseEngine`
1214
- **Configuration**: `basilisk/config/` - Pydantic-based settings with YAML persistence
1315
- **Conversation System**: `basilisk/conversation/` - Pydantic models for messages, attachments, and persistence
1416

17+
### MVP Pattern
18+
19+
#### Presenters (`basilisk/presenters/`)
20+
21+
- `MainFramePresenter` - Root window logic, tab management, file operations
22+
- `ConversationPresenter` - Chat logic, completion handling, streaming
23+
- `BaseConversationPresenter` - Shared account/model resolution
24+
- `HistoryPresenter` - Message history display and navigation
25+
- `AccountPresenter` - Account CRUD management
26+
- `ConversationProfilePresenter` - Profile CRUD management
27+
- `EditBlockPresenter` - Message block editing and regeneration
28+
- `ConversationHistoryPresenter` - Conversation history dialog
29+
- `PreferencesPresenter` - Preferences dialog logic
30+
- `PromptAttachmentPresenter` - File attachment handling
31+
- `SearchPresenter` - In-conversation search
32+
- `OCRPresenter` - Screen capture / OCR logic
33+
- `UpdatePresenter` / `DownloadPresenter` - Auto-update flow
34+
- `EnhancedErrorPresenter` - Error dialog with URL detection
35+
36+
Presenter rules:
37+
38+
- Receive view via constructor injection
39+
- Avoid `import wx` where possible; delegate all wx calls to the view
40+
- Use `from __future__ import annotations` at the top of every presenter module
41+
- Inherit `DestroyGuardMixin` + decorate callbacks with `@_guard_destroying` to guard against post-destroy calls
42+
- Call `view.show_error()` / `view.show_enhanced_error()` (from `ErrorDisplayMixin`) instead of `wx.MessageBox`
43+
44+
#### Views (`basilisk/views/`)
45+
46+
- Thin UI layer — layout, event binding, widget access
47+
- Inherit `ErrorDisplayMixin` from `basilisk/views/view_mixins.py` for standardised error display
48+
- Expose widget state through properties; delegate logic calls to the presenter
49+
50+
#### Services (`basilisk/services/`)
51+
52+
- `ConversationService` - LLM completion pipeline
53+
- `AccountModelService` - Account/model lookup and filtering
54+
- `SearchService` - Text search across conversation history
55+
- `AttachmentService` - File attachment processing and resizing
56+
- Instance-based for async operations (thread + `wx.CallAfter` callbacks); static methods for pure logic
57+
58+
### Presenter Mixins & Decorators
59+
60+
- `basilisk/presenters/presenter_mixins.py`:
61+
- `DestroyGuardMixin` + `_guard_destroying` decorator — skip callbacks after view is being destroyed
62+
- `ManagerCrudMixin` — standard add/edit/delete/move operations for list-based managers
63+
- `basilisk/views/view_mixins.py`:
64+
- `ErrorDisplayMixin``show_error(message, title)` and `show_enhanced_error(message, title, is_completion_error)`
65+
- `basilisk/decorators.py`:
66+
- `ensure_no_task_running` — guards thread-starting methods (requires `self.task`)
67+
- `require_list_selection(widget_attr)` — guards list-action handlers when nothing is selected
68+
- `measure_time` — performance logging
69+
1570
### Provider Engine Pattern
1671

1772
Each LLM provider inherits from `BaseEngine` in `basilisk/provider_engine/base_engine.py`:
@@ -34,41 +89,24 @@ class AnthropicEngine(BaseEngine):
3489
- **YAML-based**: Configuration stored in user config directory using `platformdirs`
3590
- **Pydantic Models**: Type-safe config with validation (`basilisk/config/main_config.py`)
3691
- **Account Management**: Secure API key storage with Windows Credential Manager support
37-
- **Main Config file**: Contains the main configuration parameters for the app (langage, log level, update settings, network settings)
38-
- **Conversation Profile**: a config file to define a profile for new conversation (account, system prompt, model, ETC)
92+
- **Main Config file**: Contains the main configuration parameters for the app (language, log level, update settings, network settings)
93+
- **Conversation Profile**: A config file to define a profile for new conversations (account, system prompt, model, etc.)
3994
- **Loading Order**: YAML files → Environment variables → Default values
40-
- **Hot Reload**: Settings changes applied immediately without restart where possible
41-
42-
### UI Architecture & Component Division
4395

44-
#### Main Frame Structure (`basilisk/gui/main_frame.py`)
96+
### Main Frame Structure (`basilisk/views/main_frame.py`)
4597

46-
- **MainFrame**: Root window with menu bar, notebook tabs, and status bar
47-
- **Notebook Tabs**: Each conversation runs in separate `ConversationTab`
98+
- **MainFrame**: Root window with menu bar, notebook tabs, and status bar; delegates logic to `MainFramePresenter`
99+
- **Notebook Tabs**: Each conversation runs in a separate `ConversationTab`
48100
- **System Tray**: `TaskBarIcon` for minimize-to-tray functionality
49101
- **Global Hotkeys**: Windows-specific hotkey registration for screen capture
50102

51-
#### Dialog System
103+
### Dialog System
52104

53105
- **Modal Dialogs**: Account management, preferences, about dialog
54106
- **Base Pattern**: Always call `dialog.Destroy()` after `ShowModal()`
55107
- **Preferences**: `PreferencesDialog` with nested configuration groups
56108
- **Account Management**: `AccountDialog` for provider API key setup
57109

58-
#### Conversation UI Components
59-
60-
- **ConversationTab**: Main chat interface inheriting from `BaseConversation`
61-
- **PromptAttachmentsPanel**: Input area with file attachment support
62-
- **Message History**: Scrollable display with accessibility support
63-
- **Model Selection**: Provider/model picker with capability indicators
64-
65-
#### Component Communication
66-
67-
- **Event System**: wxPython event binding (`self.Bind(wx.EVT_*, self.on_*)`)
68-
- **Configuration Updates**: Dialogs directly modify config objects
69-
- **Tab Management**: MainFrame manages notebook and tab lifecycle
70-
- **Cross-Component**: Use parent references and method calls for coordination
71-
72110
## Development Workflows
73111

74112
### Building & Distribution
@@ -99,17 +137,27 @@ iscc win_installer.iss
99137

100138
### Testing & Debugging
101139

102-
- **Entry point**: `uv run -m basilisk` or `uv run basilisk/__main__.py`
140+
- **Entry point**: `uv run -m basilisk`
103141
- **Unit Tests**: `uv run -m pytest` for test suite in `tests/`
104142
- **Debugging**: Use VS Code debugger with `launch.json` configuration
105143
- **Command line args**: `--log-level DEBUG`, `--minimize`, `--language`, `--no-env-account`
106144
- **Logs**: Written to user config directory, accessible via Help → View Log
107145
- **Config location**: Uses `platformdirs.user_config_path()` for cross-platform config storage
108146

147+
### Test Infrastructure
148+
149+
- **Fixtures**: `tests/conftest.py` (global), `tests/presenters/conftest.py` (presenter-specific)
150+
- **pytest-mock**: Use `mocker.patch()` / `mocker.patch.object()` — no `@patch` decorators or `with patch()` context managers
151+
- **Shared presenter fixtures**: `base_mock_view`, `mock_account`, `mock_model`, `mock_engine`
152+
- **Parametrize syntax**: Use tuple `("a", "b")` not string `"a,b"` for multi-parameter marks (PT006)
153+
- **Subtests**: Use native pytest 9 `subtests` fixture with `with subtests.test(label=x):`
154+
- **Mock views** for `ConversationPresenter` must set `view._is_destroying = False` explicitly
155+
- Do **not** use bare `_` for tuple unpacking in tests — it shadows the translation builtin and breaks `_("Error")` calls
156+
109157
### Translation System
110158

111-
- **Babel-based**: Uses `_("string")` to mark translatable strings
112-
- **context comment**: Use `# Translators:` or `# translators:` before the string to provide helpful context for translators
159+
- **Babel-based**: Uses `_("string")` to mark translatable strings; builtin, no import needed
160+
- **Context comment**: Use `# Translators:` (capital T) before the string to provide context for translators
113161
- **Compilation**: `uv run setup.py compile_catalog` before building
114162
- **Supported**: Multiple languages with automatic locale detection
115163

@@ -118,13 +166,15 @@ iscc win_installer.iss
118166
### Code Style
119167

120168
- **Indentation**: Tabs, not spaces
169+
- **Line length**: 80 characters
121170
- **Naming**: `snake_case` for variables/functions, `PascalCase` for classes, `UPPER_SNAKE_CASE` for constants
122171
- **Docstrings**: Google style with type hints for all public APIs
123-
- **Strings**: Double quotes, use `_("string")` for translatable text
124-
- **Custtom builtin functons**: the translation function is defined as a builtin and you don't need to import it explicitly.
125-
- **Imports**: Grouped as standard library, third-party, local imports; sorted alphabetically
172+
- **Quote style**: Preserve existing quotes; prefer double quotes for new code
173+
- **Translation**: `_("string")` is a builtin — no import needed
174+
- **Imports**: Grouped as standard library → third-party → local; sorted alphabetically
175+
- **Future annotations**: Add `from __future__ import annotations` in all presenter and service modules
126176

127-
#### format
177+
#### Format
128178

129179
```bash
130180
uv run -m ruff format
@@ -142,6 +192,7 @@ uv run -m ruff check --fix
142192
- **Dialog Management**: Always call `dialog.Destroy()` after `ShowModal()`
143193
- **Sizer Layout**: Use BoxSizer for responsive layouts, avoid fixed positioning
144194
- **Accessibility**: Support screen readers with proper labels and NVDA integration
195+
- **No wx in presenters**: Wx imports belong in views; use view proxy methods from presenters
145196

146197
### Conversation & Message Handling
147198

@@ -162,21 +213,27 @@ uv run -m ruff check --fix
162213
- **Settings Classes**: Inherit from `BasiliskBaseSettings` for automatic YAML loading
163214
- **Account Security**: Use `SecretStr` for API keys, support credential manager storage
164215
- **Validation**: Leverage Pydantic validators for robust configuration validation
165-
- **Hot Reload**: Settings changes applied immediately without restart where possible
166216

167217
## Project-Specific Patterns
168218

219+
### Resource Cleanup
220+
221+
- `ConversationTab._is_destroying` flag — set to `True` in `cleanup_resources()`
222+
- `cleanup_resources()` calls `presenter.cleanup()` and `ocr_handler.cleanup()`
223+
- `ConversationPresenter.cleanup()`: stops completion (`skip_callbacks=True`), aborts recording, stops sound, flushes draft
224+
- `HistoryPresenter.cleanup()`: destroys `_search_dialog`, resets presenter reference
225+
- `MainFramePresenter.flush_and_save_on_quit()` and `close_conversation()` call `tab.cleanup_resources()`
226+
169227
### Singleton & IPC
170228

171-
- **Single Instance**: Use `SingletonInstance` class to prevent multiple app instances
172-
- **IPC Communication**: Windows named pipes for opening files and focus management
173-
- **Signal Handling**: Cross-instance communication for file associations
229+
- **Single Instance**: Mutex-based (Windows) / file-lock (POSIX) enforcement
230+
- **IPC Communication**: Windows named pipes for file open and focus signals
174231

175232
### Accessibility Focus
176233

177-
- **Screen Reader Support**: Extensive NVDA integration with custom add-on
178-
- **Keyboard Navigation**: Full keyboard accessibility with standard shortcuts
179-
- **Audio Feedback**: Optional accessible output for status updates
234+
- **Screen Reader Support**: Extensive NVDA integration
235+
- **Keyboard Navigation**: Full keyboard accessibility
236+
- **Audio Feedback**: Optional accessible output for status updates via `sound_manager`
180237

181238
### Windows Integration
182239

0 commit comments

Comments
 (0)