Add modern UI enhancements for multiple views #39
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughLarge cross-cutting update: modern UI enhancements (Intune/Winget tooling, Packaging Wizard, Notification Center), startup/crash resiliency and splash handling, AddonService instance/static API adjustments, extensive i18n keys, Intune/Winget search robustness, PyInstaller spec changes, CI workflow permission tweak, and many new unit tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ModernApp
participant AddonService
participant IntuneAPI as "Microsoft Graph / Intune"
participant PackagingWizard
User->>ModernApp: Open Intune Store, perform search
ModernApp->>AddonService: instance.import_addon_module("winget"/"ai")
AddonService-->>ModernApp: addon module (or None)
ModernApp->>IntuneAPI: search apps (multi-stage query)
IntuneAPI-->>ModernApp: app list
User->>ModernApp: select app
ModernApp->>ModernApp: store selected app in switchcraft_session
User->>ModernApp: navigate to Packaging Wizard
ModernApp->>PackagingWizard: open (reads switchcraft_session.pending_packaging_app)
PackagingWizard-->>User: show pre-filled packaging form
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
github-actions
bot
added
documentation
FaserF
changed the title
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @FaserF. * #39 (comment) The following files were modified: * `src/switchcraft/controllers/analysis_controller.py` * `src/switchcraft/gui/app.py` * `src/switchcraft/gui/views/analyzer_view.py` * `src/switchcraft/gui/views/intune_view.py` * `src/switchcraft/gui/views/settings_view.py` * `src/switchcraft/gui_modern/app.py` * `src/switchcraft/gui_modern/controls/sidebar.py` * `src/switchcraft/gui_modern/views/analyzer_view.py` * `src/switchcraft/gui_modern/views/crash_view.py` * `src/switchcraft/gui_modern/views/dashboard_view.py` * `src/switchcraft/gui_modern/views/helper_view.py` * `src/switchcraft/gui_modern/views/intune_store_view.py` * `src/switchcraft/gui_modern/views/intune_view.py` * `src/switchcraft/gui_modern/views/packaging_wizard_view.py` * `src/switchcraft/gui_modern/views/settings_view.py` * `src/switchcraft/gui_modern/views/winget_view.py` * `src/switchcraft/modern_main.py` * `src/switchcraft/services/addon_service.py` * `src/switchcraft/services/ai_service.py` * `src/switchcraft/services/intune_service.py` * `src/switchcraft/utils/logging_handler.py` * `src/switchcraft_ai/service.py` * `src/switchcraft_winget/utils/winget.py` * `tests/test_app_startup_critical.py` * `tests/test_crash_view.py` * `tests/test_i18n_integrity.py` * `tests/test_intune_service_core.py` * `tests/test_navigation_map.py` * `tests/test_notification_service.py` * `tests/test_packaging_wizard.py` * `tests/test_session_storage.py` * `tests/test_settings_language.py` * `tests/test_template_company.py`
📝 Add docstrings to `general-improvements-and-fixes`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/test_phase2.py (1)
28-29: Potential inconsistency: setUp skips tests in stub mode, but tests have stub-mode branches.The
setUpmethod skips the entire test class when stub mode is detected (lines 28-29), yet individual test methods (lines 36-41, 47-52, etc.) contain conditional branches for stub mode responses. IfsetUpskips on stub detection, these stub-mode branches will never execute.Either:
- Remove the
skipTestinsetUpto allow stub-mode testing, or- Remove the stub-mode branches in individual tests if they're intentionally unreachable.
#!/bin/bash # Check if there are other test files with similar patterns rg -n "skipTest.*stub" tests/src/switchcraft/assets/lang/en.json (1)
252-252: Duplicate keybtn_canceldetected.The key
btn_cancelappears twice in this file (lines 252 and 855). JSON parsers will silently use the last occurrence, which can lead to confusion and maintenance issues. Remove one of the duplicate entries.🔧 Proposed fix
Remove the duplicate entry at line 855:
"btn_later": "Later", -~ - "btn_cancel": "Cancel", -~ "setting_changed": "Setting Changed",Also applies to: 855-855
src/switchcraft/assets/lang/de.json (1)
275-275: Duplicate keybtn_canceldetected.Same issue as en.json - the key
btn_cancelappears twice (lines 275 and 855). Remove one of the duplicate entries.🔧 Proposed fix
Remove the duplicate entry at line 855:
"btn_later": "Später", -~ - "btn_cancel": "Abbrechen", -~ "setting_changed": "Einstellung geändert",Also applies to: 855-855
🤖 Fix all issues with AI agents
In `@docs/.vitepress/config.mts`:
- Line 28: The logo path in the VitePress config (the logo property in
docs/.vitepress/config.mts) incorrectly includes the public/ prefix and will
404; update the logo value to remove /public so it points to the served root
(e.g., change '/public/screenshots/switchcraft_logo_with_Text.png' to
'/screenshots/switchcraft_logo_with_Text.png') so VitePress can find and serve
the asset.
In `@docs/index.md`:
- Line 9: Replace the GitHub raw URL used for the hero image in docs/index.md
with a site-relative path so VitePress can optimize the asset; locate the image
reference containing "switchcraft_logo_with_Text.png" and change its src to use
the relative path /public/screenshots/switchcraft_logo_with_Text.png (or simply
/screenshots/switchcraft_logo_with_Text.png depending on your VitePress static
folder setup) so the image is bundled and served by the static site generator.
In `@src/switchcraft_winget/utils/winget.py`:
- Around line 130-134: The ps_script builds a PowerShell command by
interpolating query directly (variable names: query, ps_script) which allows
command injection; fix by escaping single quotes in query before interpolation
(e.g., replace each ' with '' for PowerShell single-quoted strings) and use that
escaped value when constructing ps_script, or better, avoid string interpolation
entirely by passing the query as a safe argument to PowerShell (use a
param/ArgumentList approach) and invoke via subprocess.run (cmd) so ps_script
never contains raw unescaped user input.
In `@src/switchcraft/gui_modern/app.py`:
- Around line 1608-1611: The code checks self._view_cache using
NavIndex.ANALYZER (an enum/int) but the cache keys are stored as the string
'analyzer', so the lookup never succeeds; change the lookup to use the string
key 'analyzer' (e.g., if 'analyzer' in self._view_cache) and then retrieve
analyzer_view = self._view_cache['analyzer'] before calling
hasattr(analyzer_view, 'start_analysis') and self.page.run_task(lambda:
analyzer_view.start_analysis(tmp.name)); this ensures the cached view is found
and start_analysis is invoked.
- Around line 1586-1597: A temporary .msi file is created with
tempfile.NamedTemporaryFile(delete=False) and never removed; keep using
NamedTemporaryFile(delete=False) and pass the temp path (tmp.name) into the
existing start_analysis cleanup_path parameter so the analysis routine will
unlink the file when finished (ensure tmp.close() remains before download and
supply cleanup_path=tmp.name when calling start_analysis).
In `@src/switchcraft/gui_modern/views/dashboard_view.py`:
- Around line 88-99: The docstring for _refresh_ui is placed after a comment so
Python treats it as a no-op string literal; move the triple-quoted string
immediately below the def _refresh_ui(self): line so it becomes the method's
docstring, and delete or convert the original string literal (currently after
the "# Stats" comment) into a normal comment if needed; ensure the docstring
text remains unchanged and still describes rebuilding stats, chart, recent
activity and the swallowed update() exceptions.
In `@src/switchcraft/gui_modern/views/helper_view.py`:
- Around line 257-267: Replace the hardcoded tooltip "Send message" on the
ft.IconButton with the internationalized string used elsewhere in this file
(e.g., call the module's i18n/gettext helper) so tooltips are consistent; locate
the ft.IconButton instance rendering the send button (the one with
ft.Icons.SEND_ROUNDED and on_click=send_message) and swap the literal tooltip
value for the i18n lookup (use the same key/helper style as the copy button
nearby).
- Around line 161-167: Replace the hardcoded tooltip string on the ft.IconButton
(the COPY icon created near the ft.IconButton call that references copy_handler)
with the i18n lookup used elsewhere; specifically, call i18n.get with the same
key/fallback pattern used in this file (e.g., i18n.get("copy_message", "Copy
message")) and pass that result to the tooltip parameter so the button’s tooltip
is localized consistently.
- Around line 107-135: The copy_handler's try/except currently treats
ImportError and other pyperclip exceptions differently, skipping the Windows
'clip' fallback when pyperclip is present but pyperclip.copy() fails; change the
control flow so you first attempt to import pyperclip (catch ImportError and set
a flag), then regardless of import success attempt pyperclip.copy() inside its
own try/except and if that copy fails fall through to try the subprocess.clip
fallback (subprocess.run(['clip'], ...)) and finally the Flet fallback
(page.set_clipboard) if clip fails; ensure you log the exception from
pyperclip.copy() (e.g., via logger.warning) and only treat ImportError as a
signal that pyperclip isn't available, not as the only reason to try other
fallbacks.
In `@src/switchcraft/modern_main.py`:
- Around line 99-112: The fallback SimpleI18n.get signature must match the real
i18n.get; change SimpleI18n.get to accept arbitrary keyword args (def get(self,
key, default=None, **kwargs)) and handle formatting: if default is a string and
kwargs are provided, return default.format(**kwargs), otherwise return default,
ensuring callers that pass i18n.get(key, kw=value) won't raise.
- Around line 212-257: The CLI argument handling (help/version/factory-reset) is
currently inside main(page) which is invoked by ft.app(target=main), so calling
sys.exit() and using blocking input() will interfere with Flet's event loop;
move all that logic out of main and into the script's startup path (the if
__name__ == "__main__": block) so it runs before ft.app() is called.
Specifically, remove the block that checks sys.argv (the
help/version/factory-reset handling that uses input(),
SwitchCraftConfig.delete_all_application_data(), and sys.exit()) from main and
place equivalent pre-checks in the __main__ section, preserving behavior for
printing help/version and performing the factory reset confirmation, then call
ft.app(target=main) only after those checks complete.
🧹 Nitpick comments (39)
src/switchcraft_winget/utils/winget.py (1)
68-79: Consider distinguishing "no results" from "search errors" in fallback logic.The log messages state "returned no results," but the underlying methods return an empty list for both actual empty results and errors (e.g., timeouts, PowerShell not installed, parsing failures). This could make debugging harder when the fallback chain activates due to errors rather than genuine absence of matches.
Consider having the search methods return a sentinel or tuple to differentiate between "searched successfully but found nothing" and "search failed," so the fallback logging can be more accurate.
src/switchcraft/services/addon_service.py (1)
222-235: Minor docstring typo: use Python boolean literals.The docstring says "Returns:
trueif...falseotherwise" but Python usesTrue/False.📝 Proposed fix
Returns: - `true` if an addon with the given id is installed, `false` otherwise. + True if an addon with the given id is installed, False otherwise.src/switchcraft_ai/service.py (2)
228-234: Greeting responses use hardcoded strings instead of i18n.The greeting responses are hardcoded in German and English, while all other responses in this method use
i18n.get()for localization. For consistency and easier maintenance, consider adding i18n keys likeai_greeting_response.♻️ Proposed refactor
# Check for greetings first if self._is_greeting(query): - if lang == "de": - return "Hallo! 👋 Ich bin SwitchCraft AI, dein Paketierungs-Assistent. Wie kann ich dir helfen?" - else: - return "Hello! 👋 I'm SwitchCraft AI, your packaging assistant. How can I help you?" + return i18n.get("ai_greeting_response", lang=lang)
196-208: Fix docstring and consider extracting duplicate_is_greetingmethod.The identical
_is_greetingmethod exists in bothsrc/switchcraft_ai/service.pyandsrc/switchcraft/services/ai_service.py(lines 23-27). Extract to a shared utility module to eliminate duplication and maintain a single source of truth.Also, fix the docstring typo: use Python's
True/False(not backtick-enclosedtrue/false).docs/index.md (1)
63-77: Improve alt text for better accessibility.The alt attributes ("SwitchCraft Screenshot 2", "SwitchCraft Screenshot 3", etc.) are generic and don't describe what each screenshot displays. Replace with descriptive content that explains the actual UI shown (e.g., "SwitchCraft Intune packaging interface", "SwitchCraft application settings panel") to help users relying on screen readers understand the purpose of each screenshot.
src/switchcraft/controllers/analysis_controller.py (1)
170-172: LGTM! Instance-based AddonService usage is correct.The change from a static call to an instance-based approach aligns with the
import_addon_modulemethod signature inaddon_service.py, which is an instance method requiringself.Consider caching the
AddonServiceinstance at the controller level (e.g., in__init__) if this method is called frequently, to avoid repeated instantiation overhead.src/switchcraft/gui_modern/views/crash_view.py (1)
210-214: Inconsistent snack bar API usage.The
_copy_errormethod (lines 72-81) assigns directly topage.snack_barand sets.open = True, whereas here you usepage.show_snack_bar(...). This may work if Flet supports both patterns, but consider unifying the approach for consistency.tests/test_notification_service.py (1)
42-53: Test relies on internal implementation details.Directly mutating
self.service.notifications[0]["read"] = Truebypasses any service API. If the service provides amark_as_read(notification_id)method, prefer using that to make tests more resilient to internal changes.tests/test_crash_view.py (1)
81-93: Consider verifyingsys.exitwas called.The test patches
sys.exitbut doesn't assert it was called. Adding this assertion would verify the complete reload flow executed successfully.Suggested improvement
def test_reload_app_calls_subprocess(self, mock_exit, mock_popen): """Test that reload app calls subprocess.Popen.""" error = ValueError("Test error") view = CrashDumpView(self.page, error) # Test that reload_app calls subprocess.Popen # We don't need to mock sys.frozen since getattr handles it gracefully view._reload_app(self.page) mock_popen.assert_called_once() self.page.clean.assert_called_once() self.page.add.assert_called_once() + mock_exit.assert_called_once_with(0)src/switchcraft/services/ai_service.py (2)
45-61: Consider extracting duplicated response generation logic.The response generation logic (title, msg, tips_header, and formatted string) is duplicated between
update_contextandask. Extracting this to a private helper method would improve maintainability.Suggested refactor
+ def _build_addon_required_response(self, query: str) -> str: + """Build the standard addon-required guidance response.""" + title = i18n.get("ai_addon_required_title") or "🤖 **AI Addon Required**" + msg = i18n.get("ai_addon_required_msg") or ( + "The AI Assistant addon is not installed. This feature requires the AI Addon " + "to be installed via the Addon Manager to get intelligent responses." + ) + tips_header = i18n.get("ai_tips_header") or "**In the meantime, here are some tips:**" + return ( + f"{title}\n\n" + f"{msg}\n\n" + f"{tips_header}\n" + "• For MSI files: Use `/qn /norestart` for silent install\n" + "• For NSIS: Use `/S` (case sensitive)\n" + "• For Inno Setup: Use `/VERYSILENT /SUPPRESSMSGBOXES`\n" + "• For InstallShield: Use `/s /v\"/qn\"`\n\n" + f"Your question: *{query}*" + ) + def update_context(self, data: dict): # ... if self._is_greeting(query): return i18n.get("ai_stub_greeting") or "Hello! ..." - title = i18n.get("ai_addon_required_title") or ... - # ... duplicated logic ... + return self._build_addon_required_response(query) def ask(self, query): # ... if self._is_greeting(query): return i18n.get("ai_stub_welcome") or "Hello! ..." - title = i18n.get("ai_addon_required_title") or ... - # ... duplicated logic ... + return self._build_addon_required_response(query)Also applies to: 78-95
56-59: Hardcoded English tips may be intentional but worth noting.The installer tips are hardcoded in English while other strings use
i18n. If localization is desired for these tips, they should use i18n keys. However, since these are technical command-line switches, keeping them in English may be intentional.src/switchcraft/gui/views/settings_view.py (1)
1054-1071: Consider caching addon status for the loop.The static method
is_addon_installed_staticis called once per addon in the loop. Per the relevant snippet inaddon_service.py, this method internally iterates through all addons each time. For better efficiency with larger addon lists, consider fetching installed addon IDs once before the loop.However, with only 3 addons currently, this is a minor optimization opportunity rather than a blocking issue.
tests/test_intune_service_core.py (3)
7-8: Consider using pytest with proper package configuration instead of sys.path manipulation.While this pattern works, it's fragile and can cause issues if tests are run from different directories. Consider configuring
pyproject.tomlorsetup.pywith proper package discovery, or use pytest'sconftest.pywithpythonpathconfiguration.
28-32: Test is environment-dependent and non-deterministic.This test's result depends on whether
IntuneWinAppUtil.exeexists on the test machine. Consider using mocking consistently (like intest_is_tool_available_mock) or removing this test in favor of the mocked version.
34-39: Consider verifying the mock was called on the expected path.The test patches
Path.existsglobally but doesn't verify it was called with the expected path. Adding an assertion would make the test more robust.Suggested improvement
`@patch`('pathlib.Path.exists') def test_is_tool_available_mock(self, mock_exists): """Test is_tool_available with mocked path.""" mock_exists.return_value = True result = self.service.is_tool_available() self.assertTrue(result) + mock_exists.assert_called()src/switchcraft/modern_main.py (1)
350-358: Redundantpage.update()calls and blockingtime.sleep()in UI thread.Calling
page.update()three times consecutively is redundant—only the final call has any effect. Thetime.sleep(0.3)blocks the Flet event loop, which can cause UI jank. Consider usingpage.run_task()withasyncio.sleep()for non-blocking delays if timing is essential.Suggested simplification
# Add loading screen FIRST - before any other operations page.add(loading_container) -# Force immediate rendering - multiple updates to ensure visibility -page.update() -page.update() # Second update to force render -page.update() # Third update to really ensure it's visible - -# Give Flet time to actually render the loading screen before heavy operations -import time -time.sleep(0.3) # 300ms delay to ensure loading screen is rendered and visible -page.update() # Final update after delay +# Single update is sufficient +page.update()src/switchcraft/gui/app.py (1)
148-193: RedundantAddonServiceimports within the same method.
AddonServiceis imported from the same module multiple times (lines 148-149, 160-161, 178-179, 192). Consider importing once at the top of the method.Suggested consolidation
def _run_background_init(self): try: self.after(0, lambda: self._update_loading("Registering Addons...")) from switchcraft.services.addon_service import AddonService AddonService.register_addons() # ... later uses can reference AddonService directly without re-importing ... - from switchcraft.services.addon_service import AddonService addon_service = AddonService() ai_mod = addon_service.import_addon_module("ai", "service")src/switchcraft/gui_modern/views/intune_view.py (2)
386-398: Redundant conditional pattern for field assignment.The pattern
if app_data.get("key"): field.value = app_data.get("key", "")makes two dict lookups. The default""is never used since the condition already ensures the value is truthy.Suggested simplification
-if app_data.get("displayName"): - self.up_display_name.value = app_data.get("displayName", "") -if app_data.get("description"): - self.up_description.value = app_data.get("description", "") +if val := app_data.get("displayName"): + self.up_display_name.value = val +if val := app_data.get("description"): + self.up_description.value = val
428-435: Dead code:detection.xmlcontent is read but never used.The code reads
detection_contentbut then does nothing with it (the comment says "for now"). Either implement the XML parsing or remove this block to avoid confusion.Suggested cleanup
for file_name in file_list: - if 'detection.xml' in file_name.lower(): - try: - detection_content = zip_ref.read(file_name).decode('utf-8') - # Parse XML to extract info if needed - # For now, we'll focus on the manifest if available - except Exception: - pass - # Look for manifest or metadata JSON if 'manifest' in file_name.lower() or 'metadata' in file_name.lower():src/switchcraft/gui_modern/views/settings_view.py (1)
1184-1184: Inconsistent use of instance method vs static method for addon checks.This line uses
AddonService().is_addon_installed(addon["id"])(instance method), but other places in this file andapp.pyuseAddonService.is_addon_installed_static(). Consider using the static method consistently for simple availability checks.Suggested fix
-is_installed = AddonService().is_addon_installed(addon["id"]) +is_installed = AddonService.is_addon_installed_static(addon["id"])tests/test_i18n_integrity.py (1)
134-157: Potential false positives with possessive pronoun patterns.The patterns include
Ihre,Ihren,Ihrem,Ihres(lines 137-140), but line 156 only skips exact match"Ihr". These possessive forms can also mean "their" (3rd person plural possessive) in valid informal German text, not just formal "your".Consider either:
- Adding these forms to the skip list, or
- Documenting that these may produce false positives requiring manual review
💡 Suggested enhancement
# Skip "Ihr" as it can be ambiguous (could be "their" in some contexts) # But flag all other capitalized forms - if matched_text != "Ihr": # "Ihr" is too ambiguous + # Skip possessive forms that are ambiguous (could be "their") + ambiguous_forms = {"Ihr", "Ihre", "Ihren", "Ihrem", "Ihres"} + if matched_text not in ambiguous_forms: violations.append(f"{key}: Contains Sie-Form: '{matched_text}'")Alternatively, keep the current behavior but add a comment noting that these patterns prioritize catching formal language at the cost of potential false positives.
tests/test_settings_language.py (2)
57-67: Unused mock parameter in test.The
mock_get_valueparameter from the@patchdecorator is not used in the test body. Either remove the unnecessary patch or add assertions that verify the expected config interactions.♻️ Proposed fix - remove unused patch
- `@patch`('switchcraft.utils.config.SwitchCraftConfig.get_value') - def test_build_date_display(self, mock_get_value): + def test_build_date_display(self): """Test that build date is displayed in settings.""" from switchcraft.gui_modern.views.settings_view import ModernSettingsView view = ModernSettingsView(self.page) build_date = view._get_build_date() # Build date should be a string self.assertIsInstance(build_date, str) self.assertGreater(len(build_date), 0)
45-55: Missing assertion forgoto_tabcall.Unlike
test_language_change_immediate, this test does not verify thatgoto_tabwas called after the language change. Consider adding this assertion for consistency, or document why it's intentionally omitted.💡 Suggested enhancement
`@patch`('switchcraft.utils.config.SwitchCraftConfig.set_user_preference') `@patch`('switchcraft.utils.i18n.i18n.set_language') def test_language_change_german(self, mock_set_language, mock_set_pref): """Test changing language to German.""" from switchcraft.gui_modern.views.settings_view import ModernSettingsView view = ModernSettingsView(self.page) view._on_lang_change("de") mock_set_pref.assert_called_once_with("Language", "de") mock_set_language.assert_called_once_with("de") + + # Verify view reload was triggered + self.page.switchcraft_app.goto_tab.assert_called_once_with(0)src/switchcraft/gui_modern/views/winget_view.py (3)
306-310: Unnecessary try/except around string formatting.The f-string formatting
f"https://cdn.winstall.app/packages/{pkg_id.replace('.', '/')}/icon.png"cannot raise an exception sincepkg_idis already verified as truthy. The try/except adds no value here.♻️ Proposed simplification
if pkg_id: - # Try winstall.app logo API - try: - logo_url = f"https://cdn.winstall.app/packages/{pkg_id.replace('.', '/')}/icon.png" - except Exception: - pass + # winstall.app logo API + logo_url = f"https://cdn.winstall.app/packages/{pkg_id.replace('.', '/')}/icon.png"Same applies to lines 376-379.
650-652: Redundant import.
webbrowseris already imported at the top of the file (line 4). Remove this local import.♻️ Proposed fix
def _deploy_wau(self, info): - import webbrowser webbrowser.open("https://github.com/Romanitho/Winget-AutoUpdate") self._show_snack("WAU info opened in browser.")
720-761: Elevation restart logic should handle cleanup errors gracefully.The restart logic includes cleanup steps (logging.shutdown, gc.collect, time.sleep), but if any of these fail unexpectedly, the restart process might be incomplete. Consider wrapping the entire cleanup sequence in a try block.
💡 Suggested enhancement
try: import sys import time import gc import logging - # 1. Close all file handles and release resources + # Cleanup (best effort - don't let failures block restart) try: logging.shutdown() - except Exception: - pass - - # 2. Force garbage collection - gc.collect() - - # 3. Small delay to allow file handles to be released - time.sleep(0.2) + gc.collect() + time.sleep(0.2) + except Exception: + pass # Continue with restart even if cleanup fails executable = sys.executablesrc/switchcraft/gui_modern/app.py (3)
119-123: Duplicate splash termination logic.The splash process is terminated twice: once at line 119-123 (after
build_ui()in__init__) and again at lines 983-987 (at the end ofbuild_ui()). The first termination attempt will succeed, making the second redundant. Consider removing one of them to avoid confusion.Suggested fix: Remove duplicate termination
Remove the termination block at lines 119-123 since
build_ui()already handles it at the end:self.build_ui() - - # Now that UI is built, shutdown splash screen - if self.splash_proc: - try: - self.splash_proc.terminate() - except Exception: - passAlso applies to: 983-987
977-980: Redundant consecutivepage.update()calls.Calling
page.update()three times in succession has no additional effect—Flet batches updates, so only the first call is meaningful. The comments suggest this is for "ensuring rendering," but multiple synchronous calls don't guarantee that.Suggested fix
- # Force multiple updates to ensure UI is visible and rendered - self.page.update() - self.page.update() # Second update to ensure rendering - self.page.update() # Third update for good measure + self.page.update()
927-948: Navigation delay viatime.sleep()on background thread is fragile.Using fixed
time.sleep(0.5)delays to wait for UI readiness is unreliable—different machines or load conditions may require more or less time. Consider using Flet'spage.run_task()or event-based signaling instead.src/switchcraft/gui_modern/views/intune_store_view.py (2)
187-194: Misleading comment: "Create a copy reference" doesn't create a copy.The comment says "Create a copy reference for closure" but
app_copy = appjust creates another reference to the same object—not a copy. The code works correctly because the lambda's default argumenta=app_copycaptures the value at definition time, but the comment is misleading.Suggested fix: Clarify the comment
- # Create ListTile with direct lambda (like winget_view does) - # Capture app in lambda default argument to avoid closure issues - app_copy = app # Create a copy reference for closure tile = ft.ListTile( leading=leading_widget, title=ft.Text(app.get("displayName", i18n.get("unknown") or "Unknown"), selectable=True), subtitle=ft.Text(app.get("publisher", ""), selectable=True), - on_click=lambda e, a=app_copy: self._handle_app_click(a) + on_click=lambda e, a=app: self._handle_app_click(a) # Default arg captures value )The intermediate variable isn't needed since the default argument already handles the closure capture.
168-177: Duplicate logo extraction logic.The same logo extraction pattern (checking
largeIcondict, converting base64 to data URL, falling back toiconUrl/logoUrl) appears in both_update_list(lines 168-177) and_show_details(lines 246-254). Consider extracting this to a helper method.Suggested helper method
def _extract_logo_url(self, app: dict) -> str | None: """Extract logo URL from app metadata, handling base64 and various key names.""" logo_url = None large_icon = app.get("largeIcon") if isinstance(large_icon, dict): logo_url = large_icon.get("value") if logo_url and not logo_url.startswith(("http://", "https://", "data:")): logo_url = f"data:image/png;base64,{logo_url}" if not logo_url: logo_url = app.get("iconUrl") or app.get("logoUrl") return logo_urlAlso applies to: 246-254
tests/test_session_storage.py (1)
19-22:hasattrcheck on MagicMock is always True for non-spec attributes.
MagicMockauto-creates attributes on access, sohasattr(self.page, 'switchcraft_session')would returnTrueeven before assignment. However, sincespec=ft.Pageis used andswitchcraft_sessionisn't part offt.Page, the check works correctly here. The explicit assignment on line 22 ensures the dict exists regardless.Consider simplifying to always assign:
Suggested simplification
def setUp(self): self.page = MagicMock(spec=ft.Page) - # Initialize session storage like ModernApp does - if not hasattr(self.page, 'switchcraft_session'): - self.page.switchcraft_session = {} + # Initialize session storage like ModernApp does + self.page.switchcraft_session = {}Since this is test setup, unconditional assignment is clearer and matches what
test_packaging_wizard.pydoes.tests/test_legacy_startup_critical.py (2)
14-20: Simplify redundant assertion.
self.assertTrue(True)on line 18 is a no-op. If the import succeeds without exception, the test passes implicitly.Suggested simplification
def test_legacy_app_imports(self): """Test that gui.app can be imported without errors.""" - try: - import switchcraft.gui.app - self.assertTrue(True) - except Exception as e: - self.fail(f"Failed to import gui.app: {e}") + # Import will raise if there's an error; no assertion needed + import switchcraft.gui.app
55-73: Static method detection logic may produce false negatives.The check on line 68 looks for
AddonService.is_addon_installed(but excludes lines containingis_addon_installed_static. However, a valid call likeAddonService().is_addon_installed(...)(with instantiation) would still trigger the inner check on line 70, which correctly allows it. The logic is sound but somewhat convoluted.Consider adding a comment explaining the intent or simplifying the check to only flag the pattern
AddonService.is_addon_installed(without an instance().tests/test_packaging_wizard.py (2)
47-54: Remove unnecessary hasattr guards.
did_mountis a standard Flet lifecycle method andupload_infois initialized in__init__. Thehasattrchecks add unnecessary conditional logic that could mask test failures if these attributes were accidentally removed.Suggested simplification
- # Simulate did_mount (which would normally be called by Flet) - if hasattr(view, 'did_mount'): - view.did_mount() + # Simulate did_mount (which would normally be called by Flet) + view.did_mount() - # Verify that upload_info was populated - if hasattr(view, 'upload_info'): - self.assertEqual(view.upload_info["displayName"], "Test App") - self.assertEqual(view.upload_info["publisher"], "Test Publisher") + # Verify that upload_info was populated + self.assertEqual(view.upload_info["displayName"], "Test App") + self.assertEqual(view.upload_info["publisher"], "Test Publisher")
56-73: Same hasattr pattern - consider removing guards.Similar to the previous test, the
hasattr(view, 'did_mount')check on line 69 is unnecessary sincedid_mountis a known method.Suggested simplification
- # After did_mount, session should be cleared - if hasattr(view, 'did_mount'): - view.did_mount() + # After did_mount, session should be cleared + view.did_mount()tests/test_app_startup_critical.py (3)
1-9: Remove unused imports.
subprocessandtimeare imported but never used in this test file.Suggested cleanup
import unittest import sys import os -import subprocess -import time from pathlib import Path
127-133: Redundant exception type check.Lines 130-131 check if
"UnboundLocalError"or"NameError"appears instr(type(e)), but this code path is unreachable—these exception types are already caught by theexcept (UnboundLocalError, NameError)clause on line 125.Suggested simplification
try: main(page) except (UnboundLocalError, NameError) as e: self.fail(f"Startup error detected: {e}. This indicates a variable scope issue (e.g., splash_proc not declared as global).") except Exception as e: # Other exceptions might be expected (e.g., if ModernApp fails to initialize) - # But UnboundLocalError/NameError should never happen - if "UnboundLocalError" in str(type(e)) or "NameError" in str(type(e)): - self.fail(f"Variable scope error detected: {e}") - # Other exceptions are acceptable for this test pass
137-166: Duplicate test logic for splash_proc validation.
test_splash_proc_global_declarationandtest_no_unbound_local_errorsboth check for the samesplash_procglobal declaration issue using nearly identical string analysis. Consider consolidating into a single test or having one call the other's logic.Also applies to: 194-226
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
docs/public/screenshots/switchcraft_logo_with_Text.pngis excluded by!**/*.pngdocs/public/screenshots/switchcraft_ui.pngis excluded by!**/*.pngdocs/public/screenshots/switchcraft_ui_2.pngis excluded by!**/*.pngdocs/public/screenshots/switchcraft_ui_3.pngis excluded by!**/*.pngdocs/public/screenshots/switchcraft_ui_4.pngis excluded by!**/*.pngdocs/public/screenshots/switchcraft_ui_5.pngis excluded by!**/*.png
📒 Files selected for processing (52)
.github/workflows/weekly.yml.gitignoreREADME.mddocs/.vitepress/config.mtsdocs/FEATURES.mddocs/INTUNE.mddocs/INTUNE_SETUP.mddocs/WINGET.mddocs/faq.mddocs/index.mddocs/upgrade.mdsrc/entry.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/views/intune_view.pysrc/switchcraft/gui/views/settings_view.pysrc/switchcraft/gui_modern/app.pysrc/switchcraft/gui_modern/controls/sidebar.pysrc/switchcraft/gui_modern/views/analyzer_view.pysrc/switchcraft/gui_modern/views/crash_view.pysrc/switchcraft/gui_modern/views/dashboard_view.pysrc/switchcraft/gui_modern/views/helper_view.pysrc/switchcraft/gui_modern/views/intune_store_view.pysrc/switchcraft/gui_modern/views/intune_view.pysrc/switchcraft/gui_modern/views/packaging_wizard_view.pysrc/switchcraft/gui_modern/views/settings_view.pysrc/switchcraft/gui_modern/views/winget_view.pysrc/switchcraft/modern_main.pysrc/switchcraft/services/addon_service.pysrc/switchcraft/services/ai_service.pysrc/switchcraft/services/intune_service.pysrc/switchcraft/utils/logging_handler.pysrc/switchcraft_ai/service.pysrc/switchcraft_winget/utils/winget.pyswitchcraft_legacy.spectests/test_app_startup_critical.pytests/test_crash_view.pytests/test_i18n_integrity.pytests/test_intune_service_core.pytests/test_legacy_startup_critical.pytests/test_navigation_map.pytests/test_notification_service.pytests/test_packaging_wizard.pytests/test_phase2.pytests/test_session_storage.pytests/test_settings_language.pytests/test_template_company.pytests/test_ui_regressions.pytests/verify_build.ps1
🧰 Additional context used
🧬 Code graph analysis (21)
src/switchcraft/gui_modern/controls/sidebar.py (1)
src/switchcraft/gui_modern/nav_constants.py (1)
NavIndex(13-47)
tests/test_ui_regressions.py (1)
src/switchcraft/gui_modern/app.py (1)
ModernApp(25-1633)
src/switchcraft/controllers/analysis_controller.py (1)
src/switchcraft/services/addon_service.py (1)
import_addon_module(110-147)
tests/test_crash_view.py (1)
src/switchcraft/gui_modern/views/crash_view.py (4)
CrashDumpView(4-214)_copy_error(52-83)_close_app(85-129)_reload_app(131-214)
tests/test_packaging_wizard.py (1)
src/switchcraft/gui_modern/views/packaging_wizard_view.py (1)
did_mount(60-79)
src/switchcraft/gui_modern/views/intune_view.py (3)
src/switchcraft/gui_modern/utils/view_utils.py (1)
_show_snack(9-20)src/switchcraft/services/intune_service.py (2)
search_apps(396-446)get_app_details(448-468)src/switchcraft/gui_modern/utils/file_picker_helper.py (2)
FilePickerHelper(5-85)pick_file(12-38)
tests/test_intune_service_core.py (1)
src/switchcraft/services/intune_service.py (3)
is_tool_available(33-34)download_tool(36-57)create_intunewin(59-143)
src/switchcraft/gui_modern/views/helper_view.py (2)
src/switchcraft/gui_modern/app.py (1)
goto_tab(1061-1069)src/switchcraft/utils/i18n.py (1)
get(179-206)
tests/test_session_storage.py (1)
tests/test_packaging_wizard.py (1)
setUp(12-24)
src/switchcraft_ai/service.py (2)
src/switchcraft/services/ai_service.py (1)
_is_greeting(24-27)src/switchcraft/utils/i18n.py (1)
get(179-206)
src/switchcraft/gui/views/analyzer_view.py (1)
src/switchcraft/services/addon_service.py (1)
is_addon_installed_static(238-248)
src/switchcraft/gui_modern/views/packaging_wizard_view.py (1)
src/switchcraft/gui_modern/utils/view_utils.py (2)
_show_snack(9-20)_close_dialog(50-75)
src/switchcraft/gui_modern/views/intune_store_view.py (3)
src/switchcraft/utils/i18n.py (1)
get(179-206)src/switchcraft/services/intune_service.py (1)
list_app_assignments(470-484)src/switchcraft/gui_modern/utils/view_utils.py (1)
_show_snack(9-20)
tests/test_settings_language.py (2)
src/switchcraft/gui_modern/app.py (1)
goto_tab(1061-1069)src/switchcraft/gui_modern/views/settings_view.py (3)
ModernSettingsView(17-1599)_on_lang_change(886-1038)_get_build_date(1570-1599)
tests/test_phase2.py (2)
src/switchcraft/services/ai_service.py (2)
ask(63-96)update_context(29-61)src/switchcraft_ai/service.py (2)
ask(91-105)update_context(86-89)
src/switchcraft/gui/views/settings_view.py (1)
src/switchcraft/services/addon_service.py (2)
AddonService(21-397)is_addon_installed_static(238-248)
src/switchcraft/gui_modern/app.py (5)
src/switchcraft/services/notification_service.py (3)
get_notifications(115-116)mark_all_read(101-105)clear_all(107-110)src/switchcraft/utils/i18n.py (1)
get(179-206)src/switchcraft/services/addon_service.py (1)
install_from_github(282-397)src/switchcraft/gui_modern/nav_constants.py (1)
NavIndex(13-47)src/switchcraft/gui_modern/utils/view_utils.py (1)
_show_snack(9-20)
src/switchcraft/gui_modern/views/analyzer_view.py (2)
src/switchcraft/debug_views.py (2)
open(22-22)update(19-19)src/switchcraft/gui_modern/utils/view_utils.py (1)
_show_snack(9-20)
tests/test_app_startup_critical.py (1)
src/switchcraft/modern_main.py (1)
main(198-506)
src/switchcraft/gui_modern/views/crash_view.py (5)
src/switchcraft/debug_views.py (2)
update(19-19)clean(21-21)src/switchcraft/gui_modern/views/winget_view.py (1)
target(210-214)tests/test_all_views.py (1)
page(12-16)tests/test_gui_views.py (1)
page(19-20)src/switchcraft/gui/views/analyzer_view.py (1)
copy(569-572)
src/switchcraft/modern_main.py (3)
src/switchcraft/utils/i18n.py (1)
get(179-206)src/switchcraft/gui/app.py (1)
main(1119-1204)src/switchcraft/main.py (1)
main(8-72)
🪛 LanguageTool
docs/upgrade.md
[grammar] ~90-~90: Ensure spelling is correct
Context: ...ically update addons: - Automatic: Addons are checked and updated to match your S...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~91-~91: Ensure spelling is correct
Context: ... Help → Addon Manager to update addons manually ### Configuration Migration ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~91-~91: Ensure spelling is correct
Context: ...ddon Manager** to update addons manually ### Configuration Migration SwitchCraft aut...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/FEATURES.md
94-94: Headings must start at the beginning of the line
(MD023, heading-start-left)
108-108: Headings must start at the beginning of the line
(MD023, heading-start-left)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| themeConfig: { | ||
| logo: 'https://github.com/FaserF/SwitchCraft/raw/main/images/switchcraft_logo_with_Text.png', | ||
| logo: '/public/screenshots/switchcraft_logo_with_Text.png', |
There was a problem hiding this comment.
Incorrect logo path—VitePress serves public/ contents at root.
The path /public/screenshots/switchcraft_logo_with_Text.png will result in a 404. VitePress serves files from the public/ directory at the site root, so the path should not include /public/.
🐛 Proposed fix
- logo: '/public/screenshots/switchcraft_logo_with_Text.png',
+ logo: '/screenshots/switchcraft_logo_with_Text.png',📝 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.
| logo: '/public/screenshots/switchcraft_logo_with_Text.png', | |
| logo: '/screenshots/switchcraft_logo_with_Text.png', |
🤖 Prompt for AI Agents
In `@docs/.vitepress/config.mts` at line 28, The logo path in the VitePress config
(the logo property in docs/.vitepress/config.mts) incorrectly includes the
public/ prefix and will 404; update the logo value to remove /public so it
points to the served root (e.g., change
'/public/screenshots/switchcraft_logo_with_Text.png' to
'/screenshots/switchcraft_logo_with_Text.png') so VitePress can find and serve
the asset.
| tagline: "Analyze installers, create Intune packages, and streamline your IT workflow — all in one powerful tool." | ||
| image: | ||
| src: https://github.com/FaserF/SwitchCraft/raw/main/src/switchcraft/assets/switchcraft_logo_with_Text.png | ||
| src: https://github.com/FaserF/SwitchCraft/raw/main/docs/public/screenshots/switchcraft_logo_with_Text.png |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the hero image file exists and check the docs structure
fd -t f "switchcraft_logo_with_Text.png" docs/
echo "---"
echo "Checking docs directory structure:"
ls -la docs/
echo "---"
echo "Checking docs/public structure if it exists:"
if [ -d "docs/public" ]; then
find docs/public -type f -name "*.png" 2>/dev/null | head -20
fiRepository: FaserF/SwitchCraft
Length of output: 2074
🏁 Script executed:
#!/bin/bash
# Check the exact image reference in docs/index.md
grep -n "switchcraft_logo_with_Text" docs/index.mdRepository: FaserF/SwitchCraft
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Search for VitePress config to understand how assets are served
fd -t f "config\.(ts|js|mts)" . | grep -E "docs|vitepress" | head -10Repository: FaserF/SwitchCraft
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Get context around line 9 in docs/index.md
head -20 docs/index.mdRepository: FaserF/SwitchCraft
Length of output: 691
🏁 Script executed:
#!/bin/bash
# Check how other images are referenced in the file
grep -n "src:" docs/index.md | head -10Repository: FaserF/SwitchCraft
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Check if other images in the docs use relative paths or GitHub URLs
rg "!\[.*\]\(.*\)" docs/ -A 0 -B 0 | head -20Repository: FaserF/SwitchCraft
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if there are any other image references using YAML syntax in docs
grep -r "src:" docs/ --include="*.md" | head -10Repository: FaserF/SwitchCraft
Length of output: 186
Use a relative path for the hero image instead of the GitHub raw URL.
The image file exists at docs/public/screenshots/switchcraft_logo_with_Text.png, but referencing it via the GitHub raw URL bypasses VitePress asset optimization and bundling. Use a relative path like /public/screenshots/switchcraft_logo_with_Text.png for proper static site generation and better performance.
🤖 Prompt for AI Agents
In `@docs/index.md` at line 9, Replace the GitHub raw URL used for the hero image
in docs/index.md with a site-relative path so VitePress can optimize the asset;
locate the image reference containing "switchcraft_logo_with_Text.png" and
change its src to use the relative path
/public/screenshots/switchcraft_logo_with_Text.png (or simply
/screenshots/switchcraft_logo_with_Text.png depending on your VitePress static
folder setup) so the image is bundled and served by the static site generator.
| ps_script = f"Find-WinGetPackage -Query '{query}' | Select-Object Name, Id, Version, Source | ConvertTo-Json -Depth 1" | ||
| cmd = ["powershell", "-NoProfile", "-NonInteractive", "-Command", ps_script] | ||
| startupinfo = self._get_startup_info() | ||
|
|
||
| proc = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="ignore", startupinfo=startupinfo, timeout=30) | ||
| proc = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="ignore", startupinfo=startupinfo, timeout=45) |
There was a problem hiding this comment.
Command injection vulnerability in PowerShell query.
The query parameter is directly interpolated into the PowerShell script string without sanitization. A malicious or malformed query containing single quotes could break out of the string context and execute arbitrary PowerShell commands.
Example: query = "test'; Remove-Item -Recurse C:\\Windows -Force; echo '" would escape the string and execute destructive commands.
🔒 Proposed fix: Escape single quotes in the query
def _search_via_powershell(self, query: str) -> List[Dict[str, str]]:
"""
Perform a Winget package search via PowerShell and return normalized results.
Parameters:
query (str): Search query string used to find matching packages.
Returns:
List[Dict[str, str]]: A list of result dictionaries with keys 'Name', 'Id', 'Version', and 'Source'. Returns an empty list when no results are found or on error.
"""
try:
- ps_script = f"Find-WinGetPackage -Query '{query}' | Select-Object Name, Id, Version, Source | ConvertTo-Json -Depth 1"
+ # Escape single quotes by doubling them (PowerShell escape sequence)
+ safe_query = query.replace("'", "''")
+ ps_script = f"Find-WinGetPackage -Query '{safe_query}' | Select-Object Name, Id, Version, Source | ConvertTo-Json -Depth 1"
cmd = ["powershell", "-NoProfile", "-NonInteractive", "-Command", ps_script]🤖 Prompt for AI Agents
In `@src/switchcraft_winget/utils/winget.py` around lines 130 - 134, The ps_script
builds a PowerShell command by interpolating query directly (variable names:
query, ps_script) which allows command injection; fix by escaping single quotes
in query before interpolation (e.g., replace each ' with '' for PowerShell
single-quoted strings) and use that escaped value when constructing ps_script,
or better, avoid string interpolation entirely by passing the query as a safe
argument to PowerShell (use a param/ArgumentList approach) and invoke via
subprocess.run (cmd) so ps_script never contains raw unescaped user input.
| url = "https://www.7-zip.org/a/7z2409-x64.msi" | ||
|
|
||
| tmp = tempfile.NamedTemporaryFile(delete=False, suffix=".msi") | ||
| tmp.close() | ||
|
|
||
| # Download | ||
| response = requests.get(url, stream=True, timeout=30) | ||
| response.raise_for_status() | ||
|
|
||
| with open(tmp.name, 'wb') as f: | ||
| for chunk in response.iter_content(chunk_size=8192): | ||
| f.write(chunk) |
There was a problem hiding this comment.
Temporary file left on disk after demo analysis.
The temporary .msi file is created with delete=False and is never cleaned up. This will accumulate temporary files in the system temp directory over time.
Suggested fix: Pass cleanup_path to start_analysis
The start_analysis method already supports a cleanup_path parameter. Use it:
with open(tmp.name, 'wb') as f:
for chunk in response.iter_content(chunk_size=8192):
f.write(chunk)
# Navigate to analyzer if not already there
if hasattr(self, '_current_tab_index') and self._current_tab_index != NavIndex.ANALYZER:
self.goto_tab(NavIndex.ANALYZER)
# Wait a bit for view to load
import time
time.sleep(0.5)
# Trigger analysis
- if NavIndex.ANALYZER in self._view_cache:
- analyzer_view = self._view_cache[NavIndex.ANALYZER]
+ if 'analyzer' in self._view_cache:
+ analyzer_view = self._view_cache['analyzer']
if hasattr(analyzer_view, 'start_analysis'):
- self.page.run_task(lambda: analyzer_view.start_analysis(tmp.name))
+ self.page.run_task(lambda: analyzer_view.start_analysis(tmp.name, cleanup_path=tmp.name))🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/app.py` around lines 1586 - 1597, A temporary .msi
file is created with tempfile.NamedTemporaryFile(delete=False) and never
removed; keep using NamedTemporaryFile(delete=False) and pass the temp path
(tmp.name) into the existing start_analysis cleanup_path parameter so the
analysis routine will unlink the file when finished (ensure tmp.close() remains
before download and supply cleanup_path=tmp.name when calling start_analysis).
| if NavIndex.ANALYZER in self._view_cache: | ||
| analyzer_view = self._view_cache[NavIndex.ANALYZER] | ||
| if hasattr(analyzer_view, 'start_analysis'): | ||
| self.page.run_task(lambda: analyzer_view.start_analysis(tmp.name)) |
There was a problem hiding this comment.
View cache key mismatch: using NavIndex.ANALYZER instead of string 'analyzer'.
The view cache elsewhere uses string keys like 'analyzer' (e.g., line 1249-1252), but here NavIndex.ANALYZER (an integer) is used as the key. This will never find the cached view, so the analyzer won't receive the start_analysis call.
Proposed fix
- if NavIndex.ANALYZER in self._view_cache:
- analyzer_view = self._view_cache[NavIndex.ANALYZER]
+ if 'analyzer' in self._view_cache:
+ analyzer_view = self._view_cache['analyzer']📝 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.
| if NavIndex.ANALYZER in self._view_cache: | |
| analyzer_view = self._view_cache[NavIndex.ANALYZER] | |
| if hasattr(analyzer_view, 'start_analysis'): | |
| self.page.run_task(lambda: analyzer_view.start_analysis(tmp.name)) | |
| if 'analyzer' in self._view_cache: | |
| analyzer_view = self._view_cache['analyzer'] | |
| if hasattr(analyzer_view, 'start_analysis'): | |
| self.page.run_task(lambda: analyzer_view.start_analysis(tmp.name)) |
🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/app.py` around lines 1608 - 1611, The code checks
self._view_cache using NavIndex.ANALYZER (an enum/int) but the cache keys are
stored as the string 'analyzer', so the lookup never succeeds; change the lookup
to use the string key 'analyzer' (e.g., if 'analyzer' in self._view_cache) and
then retrieve analyzer_view = self._view_cache['analyzer'] before calling
hasattr(analyzer_view, 'start_analysis') and self.page.run_task(lambda:
analyzer_view.start_analysis(tmp.name)); this ensures the cached view is found
and start_analysis is invoked.
| ft.IconButton( | ||
| ft.Icons.COPY, | ||
| icon_size=12, | ||
| icon_size=16, | ||
| icon_color="GREY_400", | ||
| tooltip="Copy", | ||
| on_click=lambda _: page.set_clipboard(text) | ||
| tooltip="Copy message", | ||
| on_click=copy_handler | ||
| ) |
There was a problem hiding this comment.
Hardcoded tooltip string should use i18n for consistency.
The tooltip "Copy message" on line 165 is hardcoded in English, while other user-facing strings in this file use i18n.get() with fallbacks. Consider using a localized string for consistency.
💡 Suggested fix
ft.IconButton(
ft.Icons.COPY,
icon_size=16,
icon_color="GREY_400",
- tooltip="Copy message",
+ tooltip=i18n.get("copy_message_tooltip") or "Copy message",
on_click=copy_handler
)📝 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.
| ft.IconButton( | |
| ft.Icons.COPY, | |
| icon_size=12, | |
| icon_size=16, | |
| icon_color="GREY_400", | |
| tooltip="Copy", | |
| on_click=lambda _: page.set_clipboard(text) | |
| tooltip="Copy message", | |
| on_click=copy_handler | |
| ) | |
| ft.IconButton( | |
| ft.Icons.COPY, | |
| icon_size=16, | |
| icon_color="GREY_400", | |
| tooltip=i18n.get("copy_message_tooltip") or "Copy message", | |
| on_click=copy_handler | |
| ) |
🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/views/helper_view.py` around lines 161 - 167,
Replace the hardcoded tooltip string on the ft.IconButton (the COPY icon created
near the ft.IconButton call that references copy_handler) with the i18n lookup
used elsewhere; specifically, call i18n.get with the same key/fallback pattern
used in this file (e.g., i18n.get("copy_message", "Copy message")) and pass that
result to the tooltip parameter so the button’s tooltip is localized
consistently.
| ft.IconButton( | ||
| ft.Icons.SEND, | ||
| ft.Icons.SEND_ROUNDED, | ||
| on_click=send_message, | ||
| tooltip="Send", | ||
| icon_color="BLUE_400", | ||
| bgcolor="BLUE_900", | ||
| style=ft.ButtonStyle(shape=ft.RoundedRectangleBorder(radius=10)) | ||
| tooltip="Send message", | ||
| icon_color="WHITE", | ||
| bgcolor="BLUE_600", | ||
| style=ft.ButtonStyle( | ||
| shape=ft.RoundedRectangleBorder(radius=10), | ||
| ), | ||
| icon_size=24 | ||
| ) |
There was a problem hiding this comment.
Hardcoded tooltip string should use i18n for consistency.
Similar to the copy button tooltip, the "Send message" tooltip on line 260 is hardcoded. Consider using i18n for consistency with the rest of the file.
💡 Suggested fix
ft.IconButton(
ft.Icons.SEND_ROUNDED,
on_click=send_message,
- tooltip="Send message",
+ tooltip=i18n.get("send_message_tooltip") or "Send message",
icon_color="WHITE",
bgcolor="BLUE_600",📝 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.
| ft.IconButton( | |
| ft.Icons.SEND, | |
| ft.Icons.SEND_ROUNDED, | |
| on_click=send_message, | |
| tooltip="Send", | |
| icon_color="BLUE_400", | |
| bgcolor="BLUE_900", | |
| style=ft.ButtonStyle(shape=ft.RoundedRectangleBorder(radius=10)) | |
| tooltip="Send message", | |
| icon_color="WHITE", | |
| bgcolor="BLUE_600", | |
| style=ft.ButtonStyle( | |
| shape=ft.RoundedRectangleBorder(radius=10), | |
| ), | |
| icon_size=24 | |
| ) | |
| ft.IconButton( | |
| ft.Icons.SEND_ROUNDED, | |
| on_click=send_message, | |
| tooltip=i18n.get("send_message_tooltip") or "Send message", | |
| icon_color="WHITE", | |
| bgcolor="BLUE_600", | |
| style=ft.ButtonStyle( | |
| shape=ft.RoundedRectangleBorder(radius=10), | |
| ), | |
| icon_size=24 | |
| ) |
🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/views/helper_view.py` around lines 257 - 267,
Replace the hardcoded tooltip "Send message" on the ft.IconButton with the
internationalized string used elsewhere in this file (e.g., call the module's
i18n/gettext helper) so tooltips are consistent; locate the ft.IconButton
instance rendering the send button (the one with ft.Icons.SEND_ROUNDED and
on_click=send_message) and swap the literal tooltip value for the i18n lookup
(use the same key/helper style as the copy button nearby).
| class SimpleI18n: | ||
| def get(self, key, default=None): | ||
| """ | ||
| Always return the provided default value. | ||
| Parameters: | ||
| key: The lookup key (ignored by this implementation). | ||
| default: The value to return. | ||
| Returns: | ||
| The provided `default` value. | ||
| """ | ||
| return default | ||
| i18n = SimpleI18n() |
There was a problem hiding this comment.
Fallback SimpleI18n.get() signature doesn't match the real i18n.get() signature.
The real i18n.get() accepts **kwargs for string formatting (per src/switchcraft/utils/i18n.py:178-205), but the fallback only accepts key and default. If any code in the crash path calls i18n.get(key, kwarg=value), it will fail.
Suggested fix
class SimpleI18n:
- def get(self, key, default=None):
+ def get(self, key, default=None, **kwargs):
"""
Always return the provided default value.
Parameters:
key: The lookup key (ignored by this implementation).
default: The value to return.
+ **kwargs: Ignored formatting arguments.
Returns:
The provided `default` value.
"""
return default📝 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.
| class SimpleI18n: | |
| def get(self, key, default=None): | |
| """ | |
| Always return the provided default value. | |
| Parameters: | |
| key: The lookup key (ignored by this implementation). | |
| default: The value to return. | |
| Returns: | |
| The provided `default` value. | |
| """ | |
| return default | |
| i18n = SimpleI18n() | |
| class SimpleI18n: | |
| def get(self, key, default=None, **kwargs): | |
| """ | |
| Always return the provided default value. | |
| Parameters: | |
| key: The lookup key (ignored by this implementation). | |
| default: The value to return. | |
| **kwargs: Ignored formatting arguments. | |
| Returns: | |
| The provided `default` value. | |
| """ | |
| return default | |
| i18n = SimpleI18n() |
🤖 Prompt for AI Agents
In `@src/switchcraft/modern_main.py` around lines 99 - 112, The fallback
SimpleI18n.get signature must match the real i18n.get; change SimpleI18n.get to
accept arbitrary keyword args (def get(self, key, default=None, **kwargs)) and
handle formatting: if default is a string and kwargs are provided, return
default.format(**kwargs), otherwise return default, ensuring callers that pass
i18n.get(key, kw=value) won't raise.
| # --- Handle Command Line Arguments FIRST --- | ||
| import sys | ||
|
|
||
| # Check for help/version flags (before UI initialization) | ||
| if "--help" in sys.argv or "-h" in sys.argv or "/?" in sys.argv: | ||
| print("SwitchCraft - Packaging Assistant for IT Professionals") | ||
| print("\nUsage: SwitchCraft.exe [OPTIONS]") | ||
| print("\nOptions:") | ||
| print(" --help, -h, /? Show this help message") | ||
| print(" --version, -v Show version information") | ||
| print(" --wizard Open Packaging Wizard on startup") | ||
| print(" --analyzer, --all-in-one Open Installer Analyzer on startup") | ||
| print(" --factory-reset Delete all user data and settings (requires confirmation)") | ||
| print(" --protocol <URL> Handle protocol URL (switchcraft://...)") | ||
| print(" --silent Silent mode (minimize UI, auto-accept prompts)") | ||
| print("\nExamples:") | ||
| print(" SwitchCraft.exe --wizard") | ||
| print(" SwitchCraft.exe --analyzer") | ||
| print(" SwitchCraft.exe --factory-reset") | ||
| print(" SwitchCraft.exe switchcraft://analyzer") | ||
| sys.exit(0) | ||
|
|
||
| if "--version" in sys.argv or "-v" in sys.argv: | ||
| try: | ||
| from switchcraft import __version__ | ||
| print(f"SwitchCraft v{__version__}") | ||
| except ImportError: | ||
| print("SwitchCraft (version unknown)") | ||
| sys.exit(0) | ||
|
|
||
| # Handle factory reset (before UI initialization) | ||
| if "--factory-reset" in sys.argv: | ||
| try: | ||
| from switchcraft.utils.config import SwitchCraftConfig | ||
| print("WARNING: This will delete ALL user data, settings, and secrets.") | ||
| print("Are you sure? (Type 'yes' to confirm)") | ||
| confirmation = input("> ") | ||
| if confirmation.strip().lower() == "yes": | ||
| SwitchCraftConfig.delete_all_application_data() | ||
| print("Factory reset complete.") | ||
| else: | ||
| print("Aborted.") | ||
| sys.exit(0) | ||
| except Exception as e: | ||
| print(f"Factory reset failed: {e}") | ||
| sys.exit(1) |
There was a problem hiding this comment.
CLI argument handling inside Flet callback is problematic.
The main(page) function is called by ft.app(target=main), so by this point Flet is already running. Calling sys.exit(0) here may not cleanly exit, and input() on line 248 will block the Flet event loop, potentially freezing the application or causing undefined behavior.
These CLI flags should be handled in the if __name__ == "__main__": block before calling ft.app().
Suggested restructuring
if __name__ == "__main__":
+ # Handle CLI args BEFORE starting Flet
+ if "--help" in sys.argv or "-h" in sys.argv or "/?" in sys.argv:
+ print("SwitchCraft - Packaging Assistant for IT Professionals")
+ print("\nUsage: SwitchCraft.exe [OPTIONS]")
+ # ... rest of help text ...
+ sys.exit(0)
+
+ if "--version" in sys.argv or "-v" in sys.argv:
+ try:
+ from switchcraft import __version__
+ print(f"SwitchCraft v{__version__}")
+ except ImportError:
+ print("SwitchCraft (version unknown)")
+ sys.exit(0)
+
+ if "--factory-reset" in sys.argv:
+ # Handle factory reset here
+ sys.exit(0)
+
start_splash()
+ ft.app(target=main, assets_dir="assets")Then remove lines 212-257 from inside main().
🤖 Prompt for AI Agents
In `@src/switchcraft/modern_main.py` around lines 212 - 257, The CLI argument
handling (help/version/factory-reset) is currently inside main(page) which is
invoked by ft.app(target=main), so calling sys.exit() and using blocking input()
will interfere with Flet's event loop; move all that logic out of main and into
the script's startup path (the if __name__ == "__main__": block) so it runs
before ft.app() is called. Specifically, remove the block that checks sys.argv
(the help/version/factory-reset handling that uses input(),
SwitchCraftConfig.delete_all_application_data(), and sys.exit()) from main and
place equivalent pre-checks in the __main__ section, preserving behavior for
printing help/version and performing the factory reset confirmation, then call
ft.app(target=main) only after those checks complete.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.