Repository navigation
Finalize modern Flet design - #27
Conversation
github-actions
Bot
added
documentation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughBumps project to 2026.01.0-dev, adds a unified AnalysisController and session logging, introduces a Modern Flet UI (multiple views) with deferred loading, expands Winget/Intune features and localization, updates packaging/build specs and installer paths, and adds CLI log export and factory-reset flows. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as AnalyzerView / ModernAnalyzerView
participant Controller as AnalysisController
participant Analyzers as Analyzer Modules
participant Addons as AddonService (winget/ai)
participant Progress as Progress Callback
UI->>Controller: analyze_file(path, progress_callback)
activate Controller
Controller->>Controller: validate file
loop phases
Controller->>Progress: update(progress, label, eta)
alt Standard analyzers
Controller->>Analyzers: run per-format analyzer
Analyzers-->>Controller: InstallerInfo
else Nested / brute-force
Controller->>Analyzers: extract / brute-force
Analyzers-->>Controller: nested/brute data
else Winget lookup
Controller->>Addons: query winget addon
Addons-->>Controller: winget_url or error
else AI context
Controller->>Addons: update AI context
Addons-->>Controller: ack
end
end
Controller->>UI: return AnalysisResult (info, winget_url, nested, brute, error)
deactivate Controller
UI->>UI: render results or show error
sequenceDiagram
participant User as User
participant Main as main()/ModernMain
participant App as App / ModernApp
participant Loading as Loading Frame
participant InitThread as Background Init
participant Services as Addons & Core Services
User->>Main: launch app
Main->>App: instantiate
App->>Loading: show loading UI
App->>InitThread: spawn background initialization
activate InitThread
InitThread->>Services: register addons, init services
InitThread->>App: update loading text
InitThread->>Services: build tabs, initialize addons
InitThread-->>App: init complete
deactivate InitThread
App->>Loading: remove loading frame
App->>User: display main UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (30)
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: 11
🤖 Fix all issues with AI agents
In @src/switchcraft/controllers/analysis_controller.py:
- Around line 16-23: The AnalysisResult dataclass declares info: InstallerInfo
but callers sometimes construct AnalysisResult(info=None, ...); change the type
annotation to info: Optional[InstallerInfo] and keep the default as None, i.e.
update the field to Optional[InstallerInfo] (ensure typing.Optional is
imported), and run a quick search for constructions of AnalysisResult to confirm
all None usages remain valid and type-consistent.
In @src/switchcraft/gui_modern/views/analyzer_view.py:
- Around line 52-56: The view is referencing non-existent attributes: replace
uses of result.installer_info with result.info, and use the InstallerInfo field
install_switches instead of silent_args; update the ft.Text lines in
analyzer_view.py that read result.installer_info and info.silent_args to use
result.info and info.install_switches (and adjust the displayed label from
"Silent Args" to something like "Install Switches" or "Install Options") so the
code accesses the correct AnalysisResult.info and InstallerInfo.install_switches
attributes.
In @src/switchcraft/gui_modern/views/helper_view.py:
- Around line 62-74: Replace the raw background thread with Flet's
page.run_thread: move the work currently in _get_response (calling
ai_service.ask and catching exceptions) into the function you pass to
page.run_thread, and ensure any UI mutations (removing typing_indicator from
chat_history.controls, calling add_message, and calling page.update()) are
executed back on the UI thread via page.run_task or by scheduling a callback
inside page.run_thread that uses page.callable/run_task; keep references to
ai_service.ask, typing_indicator, chat_history.controls and add_message intact
but ensure only non-UI blocking work runs off-thread and all UI updates are
dispatched through page.run_task/page.run_thread callbacks instead of direct
calls from the worker thread.
In @src/switchcraft/gui_modern/views/history_view.py:
- Around line 21-28: Replace the bare except that swallows errors around
history_service.get_history() with proper logging (import logging and create
logger = logging.getLogger(__name__), then use logger.exception or logger.error
with the exception) so failures are visible; also remove the items.reverse()
call (or document explicitly if you truly want oldest-first) because
HistoryService.get_history() already returns newest-first, so removing
items.reverse() keeps the intended newest-first order before calling
show_items(items).
In @src/switchcraft/gui_modern/views/settings_view.py:
- Around line 30-40: The save_all function uses outdated SwitchCraftConfig
method names; replace calls to SwitchCraftConfig.set_secure_value(key, value)
with SwitchCraftConfig.set_secret(key, value) and replace
SwitchCraftConfig.set_value(key, value) with
SwitchCraftConfig.set_user_preference(key, value) in the save_all handler so it
matches the API defined in SwitchCraftConfig; keep the same arguments and
behavior, only update the method names referenced in save_all.
- Around line 14-16: The handler on_change calls a non-existent
SwitchCraftConfig.set_value which will raise AttributeError; change it to call
the real method SwitchCraftConfig.set_user_preference and pass the same key and
value (use key and e.control.value), then keep the existing
page.show_snack_bar(...) behavior to confirm save; update any related tests or
usages of on_change to expect set_user_preference rather than set_value.
In @src/switchcraft/gui_modern/views/winget_view.py:
- Around line 97-105: The code in _fetch reads short_info['Id'] which can raise
KeyError; change to id_val = short_info.get('Id') and if id_val is falsy show a
user-facing error in details_area (e.g., clear controls, append ft.Text("Error:
missing Id", color="red"), page.update()) and return early; otherwise call
winget.get_package_details(id_val) as before, merge into merged, and keep the
existing try/except around the external call to show_details_ui on success and
handle exceptions the same way.
In @src/switchcraft/gui/app.py:
- Around line 287-298: There is a duplicate except Exception handler causing a
syntax error; remove the second identical block (the one that calls
logging.getLogger(__name__).error) so only a single except Exception block
remains after the try, and ensure that the remaining block reinitializes logging
(logging.basicConfig(level=logging.INFO)), logs the error using the existing
logger variable (logger.error(f"Restart failed: {e}")) and shows the messagebox
("Could not restart automatically. Please restart manually.").
- Around line 68-69: The AttributeError occurs because self.logo_image is
checked before load_assets() runs; fix by ensuring load_assets() is invoked
earlier in initialization or by initialising the attribute first: either call
self.load_assets() before the code that references self.logo_image (the CTkLabel
creation in the initialization that uses self.loading_frame), or add
self.logo_image = None (and similarly any other asset attributes) at the start
of __init__ so the conditional if self.logo_image: is safe even if load_assets()
runs later.
In @switchcraft.spec:
- Around line 13-15: The build spec collects sc_binaries but does not pass them
into the PyInstaller Analysis call; change the Analysis invocation to use
binaries=sc_binaries (i.e., replace binaries=[] with binaries=sc_binaries) so
the shared libraries collected by collect_all('switchcraft') are included at
build/runtime; ensure the symbol sc_binaries (from the earlier sc_datas,
sc_binaries, sc_hidden_imports = collect_all('switchcraft')) is referenced
exactly in Analysis(..., binaries=sc_binaries).
🧹 Nitpick comments (6)
src/switchcraft/assets/lang/de.json (1)
3-12: Nice i18n coverage; consider a more natural DE welcome title.
Maybe “Willkommen bei Modern SwitchCraft” / “Willkommen zu SwitchCraft (Modern)”.Also applies to: 184-185
src/switchcraft/gui_modern/views/analyzer_view.py (1)
32-40: Progress callback not utilized.The
AnalysisController.analyze_fileaccepts an optionalprogress_callbackparameter for real-time progress updates, but it's not being passed. The progress bar remains static during analysis.♻️ Proposed enhancement
def _run(): try: - result = controller.analyze_file(filepath) + def update_progress(pct, msg, eta=None): + progress_bar.value = pct + status_text.value = msg + page.update() + + result = controller.analyze_file(filepath, progress_callback=update_progress) show_results(result) except Exception as ex: status_text.value = f"Error: {ex}"src/switchcraft/gui_modern/views/history_view.py (1)
59-59: Load button is non-functional.The Load button only shows a snack bar message but doesn't actually load or re-analyze the history item. Consider either implementing the functionality or clarifying the button's purpose.
Would you like me to suggest an implementation that triggers re-analysis of the selected history item?
src/switchcraft/gui_modern/views/settings_view.py (1)
48-55: Switches save immediately but Tenant/Client IDs require explicit save.The UX is inconsistent: switches auto-save on toggle (line 15-16), but text fields require clicking "Save Settings". Consider either making all fields auto-save or requiring explicit save for everything.
src/switchcraft/gui_modern/views/intune_view.py (2)
62-76: Thread-safety concern: UI updates from background thread.The
_runfunction callslog()andpage.show_snack_bar()directly from a background thread. Whilelog()internally callspage.update(), Flet's UI should generally be updated from the main thread. Consider usingpage.run_task_asyncorpage.invokepatterns if available, or schedule updates via a thread-safe mechanism.Additionally, the error case on line 74 only logs the error but doesn't show a snack bar to notify the user of failure.
Suggested improvement for error notification
except Exception as ex: log(f"ERROR: {ex}") + page.show_snack_bar(ft.SnackBar(ft.Text(f"Error: {ex}"), bgcolor="red"))
27-36: Move import to module level.The
import oson line 30 is inside the function. While functional, it's more conventional and slightly more efficient to import at the module level.Move import to top of file
import flet as ft import threading import logging +import os from switchcraft.services.intune_service import IntuneServiceThen remove line 30.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
README.mdfile_version_info.txthooks/hook-switchcraft.pypyproject.tomlsrc/entry.pysrc/switchcraft/__init__.pysrc/switchcraft/assets/lang/de.jsonsrc/switchcraft/assets/lang/en.jsonsrc/switchcraft/controllers/analysis_controller.pysrc/switchcraft/gui/app.pysrc/switchcraft/gui/views/analyzer_view.pysrc/switchcraft/gui_modern/app.pysrc/switchcraft/gui_modern/views/analyzer_view.pysrc/switchcraft/gui_modern/views/helper_view.pysrc/switchcraft/gui_modern/views/history_view.pysrc/switchcraft/gui_modern/views/intune_view.pysrc/switchcraft/gui_modern/views/settings_view.pysrc/switchcraft/gui_modern/views/winget_view.pysrc/switchcraft/services/addon_service.pysrc/switchcraft_winget/utils/winget.pyswitchcraft.spec
💤 Files with no reviewable changes (1)
- hooks/hook-switchcraft.py
🧰 Additional context used
🧬 Code graph analysis (10)
src/switchcraft/controllers/analysis_controller.py (2)
src/switchcraft/models.py (1)
InstallerInfo(5-32)src/switchcraft/utils/config.py (2)
SwitchCraftConfig(10-312)get_value(29-66)
src/switchcraft/gui_modern/views/intune_view.py (1)
tests/test_full_coverage.py (1)
intune_service(54-55)
src/switchcraft/gui_modern/views/analyzer_view.py (1)
src/switchcraft/controllers/analysis_controller.py (2)
AnalysisResult(17-23)analyze_file(34-166)
src/switchcraft/gui_modern/views/settings_view.py (1)
src/switchcraft/utils/config.py (3)
SwitchCraftConfig(10-312)get_value(29-66)get_secure_value(178-232)
src/switchcraft/gui_modern/app.py (3)
src/switchcraft/utils/i18n.py (1)
get(116-143)src/switchcraft/gui_modern/views/winget_view.py (1)
ModernWingetView(10-148)src/switchcraft/modern_main.py (1)
main(4-6)
src/switchcraft/gui_modern/views/history_view.py (1)
src/switchcraft/services/history_service.py (3)
HistoryService(9-63)clear(55-56)get_history(22-35)
src/switchcraft/gui_modern/views/helper_view.py (1)
src/switchcraft/services/addon_service.py (1)
import_addon_module(62-82)
src/switchcraft/gui/views/analyzer_view.py (1)
src/switchcraft/controllers/analysis_controller.py (2)
AnalysisController(25-166)analyze_file(34-166)
src/entry.py (5)
src/switchcraft/gui/app.py (1)
main(910-944)src/switchcraft/gui_modern/app.py (1)
main(141-143)src/switchcraft/modern_main.py (1)
main(4-6)src/switchcraft/main.py (1)
main(8-28)src/switchcraft/cli_main.py (1)
main(9-19)
src/switchcraft/gui/app.py (3)
src/switchcraft/utils/i18n.py (1)
get(116-143)src/switchcraft/services/addon_service.py (3)
register_addons(53-59)import_addon_module(62-82)is_addon_installed(41-50)src/switchcraft/services/notification_service.py (2)
NotificationService(29-162)set_app_window(41-43)
🪛 RuboCop (1.82.1)
switchcraft.spec
[fatal] 8-8: unexpected token kIN
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
[fatal] 8-8: unexpected token tCOLON
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
🔇 Additional comments (25)
pyproject.toml (1)
7-7: Version bump coordinated with optional Flet dependency.The version update to 2026.01.0-dev aligns with the modern Flet GUI feature (line 41), and the optional dependency structure properly isolates Flet to keep the core lightweight.
src/switchcraft/__init__.py (1)
1-1: Version bump consistent with pyproject.toml.The version string is properly synchronized across project metadata.
README.md (1)
40-43: Good user documentation for design versions.The new comparison table clearly communicates the Classic vs. Modern design options and appropriately marks Flet as Preview/Beta to set user expectations.
file_version_info.txt (2)
9-10: Version tuple bump looks consistent.
34-40: String metadata bump looks consistent (FileVersion/ProductVersion/Copyright).src/switchcraft/assets/lang/en.json (1)
3-12: Added nav/welcome/search guidance strings look consistent.Also applies to: 165-166
switchcraft.spec (1)
7-9: RuboCop is not used in the CI pipeline. The lint workflow only runs Ruff (a Python linter), not RuboCop. No changes to RuboCop configuration are needed forswitchcraft.spec.Likely an incorrect or invalid review comment.
src/switchcraft_winget/utils/winget.py (1)
149-156: Verify if positionalqueryargument represents a behavioral change or is intentional.The current syntax using a positional query argument (
winget search <query>) is valid and fully supported by winget;--accept-source-agreementsis also confirmed to work with the search command. However, positional queries do search across all fields (Name, Id, etc.) whereas--namerestricts matching to package names only. If this code was changed from using--name, verify that the broader search semantics are intentional and documented. If backward compatibility with name-only search is required, consider adding--nameback.src/entry.py (2)
10-13: LGTM!The guarded import with a bare
except ImportError: passis appropriate here since this is only a PyInstaller bundling hint, not a runtime dependency. The actual GUI import happens inswitchcraft.main.
22-40: LGTM!Solid error handling improvements:
- Properly filters out clean
SystemExit(0)to avoid noisy output- Prints traceback and critical failure message for actual errors
- The frozen-mode pause with
EOFError/RuntimeErrorhandling is robust for environments without stdinsrc/switchcraft/gui_modern/views/helper_view.py (1)
84-90: Resilient fallback for missing color constant.Good defensive check using
hasattr(ft.colors, "SURFACE_CONTAINER_HIGHEST")with a fallback toGREY_900. This ensures compatibility across Flet versions.src/switchcraft/services/addon_service.py (1)
62-82: LGTM!The new
raise_errorparameter is well-designed:
- Backward compatible with
default=False- Logs the error regardless of the flag
- Re-raises only when explicitly requested
This enables callers like the GUI to choose between graceful degradation (default) or strict error propagation.
src/switchcraft/gui_modern/views/winget_view.py (2)
10-26: LGTM!Good graceful degradation pattern: attempts to load the addon, catches initialization failures, and shows a user-friendly fallback UI when unavailable.
115-118: The case-sensitive key lookup is not an issue here.The
get_package_details()method insrc/switchcraft_winget/utils/winget.pyexplicitly returns a dictionary with lowercase keys ("publisher","description", etc.). The code correctly uses.lower()to match these lowercase keys, so the lookup will work as intended.Note: The
'License'key is not returned by the winget API, so it will never be displayed. However, this is handled gracefully by theif val:check and won't cause failures.Likely an incorrect or invalid review comment.
src/switchcraft/gui_modern/views/analyzer_view.py (1)
62-74: Update UI text: "Drag & Drop" cannot be supported without native OS file-drop handling.The
on_clickhandler usingFilePickeris correct for Flet, but the "Drag & Drop Installer Here" text is misleading. Flet'sDragTargethandles in-app dragging only, not external OS file drops. Implement actual drag-and-drop by either: (1) remove the "Drag & Drop" text and clarify it as "Click to browse", or (2) add a custom JavaScript overlay to capture native file drops and pass them to the FilePicker—native support is not available in Flet's standard API.Likely an incorrect or invalid review comment.
src/switchcraft/gui_modern/views/intune_view.py (1)
79-107: LGTM!Good defensive programming with the tool availability check and early return. The fallback for
SURFACE_CONTAINER_HIGHESTcolor constant provides backward compatibility with older Flet versions.src/switchcraft/gui_modern/app.py (3)
89-138: Verifypage.update()call timing.The
nav_changemethod modifiesself.content.controlsand then callsself.page.update()at line 138. This is correct. However, for each view loaded (lines 102, 108, 114, 120, 126, 132), if the view constructor itself modifies the page or needs an update, ensure consistency.The error handling pattern with try/except for each view is well-implemented.
14-35: LGTM!Good UX pattern: showing a loading indicator immediately and then building the full UI. The
page.clean()at line 77 properly removes the loading indicator before adding the main layout.
41-45: LGTM!Defensive handling for older Flet versions that may not support
window.min_width/min_heightproperties.src/switchcraft/gui/views/analyzer_view.py (2)
212-260: LGTM - Clean refactoring to use AnalysisController.The migration from multi-analyzer inline logic to the unified
AnalysisControlleris well-executed. The progress callback adapter correctly maps the controller's progress format to the UI. Error handling with notifications and early return is appropriate.Note:
NotificationService.send_notificationon line 235 and 259 is called from a background thread. Verify thatNotificationServiceis thread-safe or uses internal thread-safe mechanisms.
262-268: Clean API design.Changing
_show_resultsto accept a singleAnalysisResultobject instead of multiple parameters improves maintainability and aligns with the controller's return type.src/switchcraft/controllers/analysis_controller.py (2)
54-166: LGTM - Well-structured analysis pipeline.The 5-phase analysis pipeline with individual error handling for each phase is robust. Failures in optional phases (Winget search, AI context) don't abort the analysis. The controller successfully consolidates logic previously spread across the view.
112-123: No issue found. Thenested_progress_handlercorrectly assumespctis in the 0-100 range, which is whatuni.extract_and_analyze_nestedprovides (calling the callback with values like 10, 20, and 30-90). The math maps this correctly to the 0.5-0.9 global progress range.src/switchcraft/gui/app.py (2)
254-286: LGTM - Robust restart implementation.The restart logic properly:
- Shuts down logging to release file locks
- Cleans PyInstaller environment variables (
_MEI*,LD_LIBRARY_PATH)- Uses
DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUPon Windows for the child to survive parent death- Handles both frozen and script execution modes
89-170: LGTM - Well-structured deferred initialization.The staged initialization with
_update_loading()provides good UX feedback. Usingafter()for delayed tasks (demo init, error checks, addon status, cloud backup) keeps the UI responsive during startup.
| - **User Scope**: Installs to `%LOCALAPPDATA%\FaserF\SwitchCraft` (Default). | ||
| - **Machine Scope**: Run as Admin to install to `%ProgramFiles(x86)%\FaserF\SwitchCraft`. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find installer-related files
echo "=== Searching for Inno Setup installer files (.iss) ==="
fd -e iss . 2>/dev/null
echo -e "\n=== Searching for PowerShell scripts (.ps1) ==="
fd -e ps1 . 2>/dev/null
echo -e "\n=== Searching for build/config files (pyproject.toml, setup.py) ==="
fd -e toml -e cfg . 2>/dev/null | head -10
echo -e "\n=== Searching for files containing 'SwitchCraft' ==="
rg "SwitchCraft" --type-list | head -20
fd . -type f 2>/dev/null | xargs rg "SwitchCraft" -l 2>/dev/null | head -20Repository: FaserF/SwitchCraft
Length of output: 1313
🏁 Script executed:
#!/bin/bash
# Examine the main installer scripts for path structure and upgrade handling
echo "=== Contents of setup.iss ==="
cat -n setup.iss
echo -e "\n=== Contents of switchcraft.iss ==="
cat -n switchcraft.iss
echo -e "\n=== Checking install.ps1 for path handling ==="
cat -n install.ps1Repository: FaserF/SwitchCraft
Length of output: 14771
Fix AppId mismatch and add upgrade detection to prevent dual installations.
The installer paths correctly implement the FaserF vendor folder structure and switchcraft.iss supports both user and machine scopes. However, setup.iss (AppId: C6E72169-E342-4363-9D67-37A4928A613F) and switchcraft.iss (AppId: F4A53RF0-5W1T-CH3R-AFTF-ASE3RF453RF0) use different AppIds. Inno Setup identifies applications by AppId to detect upgrades; mismatched AppIds prevent upgrade detection and allow dual installations. Ensure both installers share the same AppId, or migrate users explicitly by adding uninstall logic to detect and remove older installations from previous paths.
| @dataclass | ||
| class AnalysisResult: | ||
| info: InstallerInfo | ||
| winget_url: Optional[str] = None | ||
| brute_force_data: Optional[str] = None | ||
| nested_data: Optional[Dict] = None | ||
| silent_disabled_info: Optional[Dict] = None | ||
| error: Optional[str] = None |
There was a problem hiding this comment.
Type hint inconsistency: info field should be Optional[InstallerInfo].
The AnalysisResult dataclass defines info: InstallerInfo as required, but line 47 and line 166 return AnalysisResult(info=None, error=...). This type mismatch could cause issues with type checkers and IDE warnings.
Fix type hint
@dataclass
class AnalysisResult:
- info: InstallerInfo
+ info: Optional[InstallerInfo]
winget_url: Optional[str] = None
brute_force_data: Optional[str] = None
nested_data: Optional[Dict] = None
silent_disabled_info: Optional[Dict] = None
error: Optional[str] = None📝 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.
| @dataclass | |
| class AnalysisResult: | |
| info: InstallerInfo | |
| winget_url: Optional[str] = None | |
| brute_force_data: Optional[str] = None | |
| nested_data: Optional[Dict] = None | |
| silent_disabled_info: Optional[Dict] = None | |
| error: Optional[str] = None | |
| @dataclass | |
| class AnalysisResult: | |
| info: Optional[InstallerInfo] | |
| winget_url: Optional[str] = None | |
| brute_force_data: Optional[str] = None | |
| nested_data: Optional[Dict] = None | |
| silent_disabled_info: Optional[Dict] = None | |
| error: Optional[str] = None |
🤖 Prompt for AI Agents
In @src/switchcraft/controllers/analysis_controller.py around lines 16 - 23, The
AnalysisResult dataclass declares info: InstallerInfo but callers sometimes
construct AnalysisResult(info=None, ...); change the type annotation to info:
Optional[InstallerInfo] and keep the default as None, i.e. update the field to
Optional[InstallerInfo] (ensure typing.Optional is imported), and run a quick
search for constructions of AnalysisResult to confirm all None usages remain
valid and type-consistent.
| def _get_response(): | ||
| try: | ||
| response = ai_service.ask(user_msg) | ||
| if typing_indicator in chat_history.controls: | ||
| chat_history.controls.remove(typing_indicator) | ||
| add_message("AI", response, is_user=False) | ||
| except Exception as ex: | ||
| if typing_indicator in chat_history.controls: | ||
| chat_history.controls.remove(typing_indicator) | ||
| add_message("AI Error", str(ex), is_user=False, is_error=True) | ||
| page.update() | ||
|
|
||
| threading.Thread(target=_get_response, daemon=True).start() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/switchcraft/gui_modern/views/helper_view.pyRepository: FaserF/SwitchCraft
Length of output: 4185
🌐 Web query:
Flet Python thread safe page update background thread
💡 Result:
Short answer: don't call UI methods directly from arbitrary threads — run background work with page.run_task (async) or page.run_thread and schedule UI updates on the page/event loop (call page.update() from a page task/coroutine or use the page's scheduling helpers). See Flet docs for run_task/run_thread and async examples.
Examples:
-
Prefer async background task (safe to call update() inside coroutine):
import asyncio
import flet as ftasync def main(page: ft.Page):
text = ft.Text("...")
page.add(text)async def bg(): while True: await asyncio.sleep(1) text.value = "tick" page.update() # safe inside run_task coroutine page.run_task(bg) # runs coroutine in page event loopft.app(target=main)
-
If you must use a thread, run it via page.run_thread and dispatch updates to the page (do not call page.update() directly from worker thread; schedule the update on the page's event loop). For example, run background work in run_thread and then call an async helper with run_task or set state and let the main loop update:
def worker():
# do blocking work...
page.run_task(lambda: do_ui_update()) # schedules coroutine to run on page looppage.run_thread(worker)
References: Flet async/threading guide and Page API (run_task, run_thread, schedule/update). [1][2]
Sources:
[1] Flet — Async apps / threading examples. [2] Flet — Page API (run_task, run_thread, update, schedule_update).
Use Flet's page.run_thread() for background work instead of raw threading.Thread.
The code calls page.update() directly from a background thread (line 72, and via add_message() on lines 67 and 71). Flet's threading model requires that UI updates be dispatched through the page's event loop, not called directly from worker threads. Replace the raw threading.Thread with page.run_thread(), and schedule UI updates using a callback that calls page.run_task() or delegates to the main event loop. This prevents race conditions and UI state corruption.
🤖 Prompt for AI Agents
In @src/switchcraft/gui_modern/views/helper_view.py around lines 62 - 74,
Replace the raw background thread with Flet's page.run_thread: move the work
currently in _get_response (calling ai_service.ask and catching exceptions) into
the function you pass to page.run_thread, and ensure any UI mutations (removing
typing_indicator from chat_history.controls, calling add_message, and calling
page.update()) are executed back on the UI thread via page.run_task or by
scheduling a callback inside page.run_thread that uses page.callable/run_task;
keep references to ai_service.ask, typing_indicator, chat_history.controls and
add_message intact but ensure only non-UI blocking work runs off-thread and all
UI updates are dispatched through page.run_task/page.run_thread callbacks
instead of direct calls from the worker thread.
| items = history_service.get_history() | ||
| except Exception: | ||
| pass | ||
|
|
||
| if items: | ||
| items.reverse() | ||
|
|
||
| show_items(items) |
There was a problem hiding this comment.
Silent exception swallowing and confusing sort order.
- The
except Exception: passsilently discards errors - consider at least logging them. HistoryService.get_history()already returns items sorted newest-first, butitems.reverse()flips this to oldest-first. Is this intentional?
♻️ Suggested fix
def _fetch():
items = []
try:
items = history_service.get_history()
except Exception:
- pass
+ logger.exception("Failed to load history")
- if items:
- items.reverse()
-
show_items(items)Note: Add import logging and logger = logging.getLogger(__name__) at the top if keeping newest-first order is intended.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/switchcraft/gui_modern/views/history_view.py around lines 21 - 28,
Replace the bare except that swallows errors around
history_service.get_history() with proper logging (import logging and create
logger = logging.getLogger(__name__), then use logger.exception or logger.error
with the exception) so failures are visible; also remove the items.reverse()
call (or document explicitly if you truly want oldest-first) because
HistoryService.get_history() already returns newest-first, so removing
items.reverse() keeps the intended newest-first order before calling
show_items(items).
| def save_all(e): | ||
| count = 0 | ||
| for key, field, is_secure in text_fields: | ||
| value = field.value | ||
| if is_secure: | ||
| SwitchCraftConfig.set_secure_value(key, value) | ||
| else: | ||
| SwitchCraftConfig.set_value(key, value) | ||
| count += 1 | ||
|
|
||
| page.show_snack_bar(ft.SnackBar(ft.Text("Settings Saved Successfully!"), bgcolor=ft.colors.GREEN)) |
There was a problem hiding this comment.
Method names don't match SwitchCraftConfig API.
Based on src/switchcraft/utils/config.py:
- Use
set_secret()instead ofset_secure_value() - Use
set_user_preference()instead ofset_value()
🐛 Proposed fix
def save_all(e):
count = 0
for key, field, is_secure in text_fields:
value = field.value
if is_secure:
- SwitchCraftConfig.set_secure_value(key, value)
+ SwitchCraftConfig.set_secret(key, value)
else:
- SwitchCraftConfig.set_value(key, value)
+ SwitchCraftConfig.set_user_preference(key, value)
count += 1
page.show_snack_bar(ft.SnackBar(ft.Text("Settings Saved Successfully!"), bgcolor=ft.colors.GREEN))🤖 Prompt for AI Agents
In @src/switchcraft/gui_modern/views/settings_view.py around lines 30 - 40, The
save_all function uses outdated SwitchCraftConfig method names; replace calls to
SwitchCraftConfig.set_secure_value(key, value) with
SwitchCraftConfig.set_secret(key, value) and replace
SwitchCraftConfig.set_value(key, value) with
SwitchCraftConfig.set_user_preference(key, value) in the save_all handler so it
matches the API defined in SwitchCraftConfig; keep the same arguments and
behavior, only update the method names referenced in save_all.
| def _fetch(): | ||
| try: | ||
| full = winget.get_package_details(short_info['Id']) | ||
| merged = {**short_info, **full} | ||
| show_details_ui(merged) | ||
| except Exception as ex: | ||
| details_area.controls.clear() | ||
| details_area.controls.append(ft.Text(f"Error: {ex}", color="red")) | ||
| page.update() |
There was a problem hiding this comment.
Potential KeyError on missing 'Id' field.
Line 99 accesses short_info['Id'] directly, which will raise KeyError if the field is missing. Use .get() for safer access.
🐛 Proposed fix
def _fetch():
try:
- full = winget.get_package_details(short_info['Id'])
+ pkg_id = short_info.get('Id')
+ if not pkg_id:
+ raise ValueError("Package ID not found")
+ full = winget.get_package_details(pkg_id)
merged = {**short_info, **full}
show_details_ui(merged)📝 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 _fetch(): | |
| try: | |
| full = winget.get_package_details(short_info['Id']) | |
| merged = {**short_info, **full} | |
| show_details_ui(merged) | |
| except Exception as ex: | |
| details_area.controls.clear() | |
| details_area.controls.append(ft.Text(f"Error: {ex}", color="red")) | |
| page.update() | |
| def _fetch(): | |
| try: | |
| pkg_id = short_info.get('Id') | |
| if not pkg_id: | |
| raise ValueError("Package ID not found") | |
| full = winget.get_package_details(pkg_id) | |
| merged = {**short_info, **full} | |
| show_details_ui(merged) | |
| except Exception as ex: | |
| details_area.controls.clear() | |
| details_area.controls.append(ft.Text(f"Error: {ex}", color="red")) | |
| page.update() |
🤖 Prompt for AI Agents
In @src/switchcraft/gui_modern/views/winget_view.py around lines 97 - 105, The
code in _fetch reads short_info['Id'] which can raise KeyError; change to id_val
= short_info.get('Id') and if id_val is falsy show a user-facing error in
details_area (e.g., clear controls, append ft.Text("Error: missing Id",
color="red"), page.update()) and return early; otherwise call
winget.get_package_details(id_val) as before, merge into merged, and keep the
existing try/except around the external call to show_details_ui on success and
handle exceptions the same way.
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.