docs/CI_Architecture.md-8-8 (1)

8-8: Fix the operating system name capitalization.

Apple's operating system should be written as "macOS" not "MacOS".

🔎 Proposed fix

-  - **Standard Build**: Windows (GUI+CLI), Linux, MacOS.
+  - **Standard Build**: Windows (GUI+CLI), Linux, macOS.

switchcraft_modern.spec-49-49 (1)

49-49: Update the executable name from test placeholder.

The executable name SwitchCraft-new-Test appears to be a temporary/test name. Consider using a production-ready name like SwitchCraft-modern or SwitchCraft-Modern-windows to align with the other build variants.

🔎 Proposed fix

-    name='SwitchCraft-new-Test', # Modern (Flet) test build
+    name='SwitchCraft-Modern', # Modern (Flet) GUI build

switchcraft_modern.spec-61-61 (1)

61-61: Convert the icon file from PNG to ICO format.

PyInstaller on Windows requires .ico format for application icons. The .png file specified will not work as an executable icon on Windows. Convert images/switchcraft_logo.png to .ico format before building.

src/switchcraft/gui_modern/app.py-122-124 (1)

122-124: Address the unused variable flagged by the linter.

The variable app is assigned but never used, as flagged by the CI pipeline. Determine if:

1. The reference must be retained to prevent garbage collection of the ModernApp instance, or
2. The variable can be removed since ModernApp.__init__ modifies the page in place

If the reference is necessary, add a comment explaining why. Otherwise, remove the assignment or use _ = to indicate intentional discard.

🔎 Potential fixes

If the reference is needed:

     # Initialize the app
     # ModernApp.__init__ calls page.clean()
-    app = ModernApp(page)
+    app = ModernApp(page)  # Keep reference to prevent garbage collection

If the reference is not needed:

     # Initialize the app
     # ModernApp.__init__ calls page.clean()
-    app = ModernApp(page)
+    ModernApp(page)

src/switchcraft/gui_modern/app.py-117-120 (1)

117-120: Replace time.sleep() with asyncio.sleep() for async-compatible delays.

In async Flet apps, asyncio.sleep() should be used instead of time.sleep(). In Flet 1.0+, blocking calls like time.sleep() freeze the UI and should be replaced with await asyncio.sleep().

README.md-16-17 (1)

16-17: Fix duplicate heading.

Line 17 contains a duplicate "## 📚 Documentation" heading. This appears to be a copy-paste error and will cause rendering issues.

🔎 Proposed fix

-## 📚 Documentation
 ## 📚 Documentation

src/switchcraft/gui/splash.py-3-5 (1)

3-5: Remove unused imports.

As flagged by the pipeline, sys, os, and Path are imported but never used.

🔎 Proposed fix

 import tkinter as tk
 from tkinter import ttk
-import sys
-import os
-from pathlib import Path

Committable suggestion skipped: line range outside the PR's diff.

src/switchcraft/gui/views/analyzer_view.py-969-979 (1)

969-979: Remove unused messagebox import and dead code.

Pipeline failure indicates the messagebox import on line 973 is unused. The entire conditional block (lines 971-975) appears to be a no-op placeholder.

🔎 Proposed fix

     def _open_manifest_dialog(self, info):
-        # Check Config
-        repo_path = SwitchCraftConfig.get_value("WingetRepoPath")
-        if not repo_path:
-             from tkinter import messagebox
-             # Optional: Ask user if they want to configure it, or just rely on default logic in Service
-             pass
-
         from switchcraft.gui.views.manifest_dialog import ManifestDialog
         dlg = ManifestDialog(self.winfo_toplevel(), info)
         dlg.grab_set()

src/switchcraft/gui/views/manifest_dialog.py-4-4 (1)

4-4: Remove unused import webbrowser.

Pipeline failure indicates this import is unused.

🔎 Proposed fix

-import webbrowser

scripts/build_release.ps1-11-12 (1)

11-12: Documentation mismatch: -Gui parameter documented but code uses -Modern and -Legacy.

The .PARAMETER Gui documentation doesn't match the actual parameter names in the param block (lines 37-44).

🔎 Proposed fix for documentation

-.PARAMETER Gui
-    Builds the Standard GUI application (SwitchCraft.exe). Default if no other flags used.
+.PARAMETER Modern
+    Builds the Modern GUI application using Flet (SwitchCraft-new-Test.exe). Default if no other flags used.
+
+.PARAMETER Legacy
+    Builds the Legacy GUI application using Tkinter (SwitchCraft-windows.exe).

Also update the examples:

 .EXAMPLE
-    .\build_release.ps1 -Gui -Cli
+    .\build_release.ps1 -Modern -Cli
     Builds both versions.

src/switchcraft_winget/utils/winget.py-3-3 (1)

3-3: Remove unused import shutil.

Pipeline failure indicates this import is unused.

🔎 Proposed fix

-import shutil

src/switchcraft/gui/views/manifest_dialog.py-8-8 (1)

8-8: Remove unused import SwitchCraftConfig.

Pipeline failure indicates this import is unused.

🔎 Proposed fix

-from switchcraft.utils.config import SwitchCraftConfig

src/switchcraft/services/winget_manifest_service.py-1-7 (1)

1-7: Remove unused imports flagged by pipeline.

Pipeline failures indicate os, shutil, and datetime are imported but never used.

🔎 Proposed fix

-import os
 import yaml
 import logging
 import subprocess
-import shutil
 from pathlib import Path
-from datetime import datetime
 from switchcraft.utils.config import SwitchCraftConfig

src/switchcraft/cli/commands.py-178-185 (1)

178-185: Remove or use the unused output_log variable.

The variable is assigned but never used, causing the pipeline failure (F841).

Proposed fix

     try:
-        output_log = svc.create_intunewin(
+        svc.create_intunewin(
             source_folder=source,
             setup_file=setup_file,
             output_folder=output,
             quiet=quiet,
             progress_callback=lambda x: print(x.strip()) if not quiet else None
         )
         print("[green]Package created successfully![/green]")

Or if you want to log the output in verbose mode:

     try:
         output_log = svc.create_intunewin(
             source_folder=source,
             setup_file=setup_file,
             output_folder=output,
             quiet=quiet,
             progress_callback=lambda x: print(x.strip()) if not quiet else None
         )
+        if not quiet and output_log:
+            logger.debug(f"Tool output: {output_log}")
         print("[green]Package created successfully![/green]")

src/switchcraft/services/notification_service.py (1)

79-83: Update the docstring to reflect current behavior.

The docstring mentions conditional behavior based on foreground state ("If app is in background... If app is in foreground..."), but the code now always sends system notifications regardless of foreground state. Update the docstring to accurately describe the current implementation.

🔎 Proposed docstring update

     @staticmethod
     def send_notification(title: str, message: str, timeout: int = 10):
         """
         Sends a desktop notification.
-        If app is in background, uses system notification center.
-        If app is in foreground, notification still shows but app stays visible.
+        Always sends system notifications through the notification center.
+        On Windows, uses winotify for toast notifications with click support,
+        falling back to plyer or PowerShell if unavailable.
         """

pyproject.toml (1)

17-31: Consider pinning dependency versions for stability and security.

All dependencies are unpinned, which can lead to:

• Unexpected breaking changes from major version updates
• Potential security vulnerabilities
• Non-reproducible builds

Consider adding version constraints (at minimum, upper bounds) for production stability, especially for security-sensitive packages like requests, PyJWT, openai, and google-generativeai.

Example approach:

dependencies = [
    "requests>=2.31.0,<3.0",
    "click>=8.1.0,<9.0",
    "PyJWT>=2.8.0,<3.0",
    # ... etc
]

src/switchcraft/gui_modern/app.py (1)

1-3: Consider using the imported i18n module for UI strings.

The i18n module is imported but never used. All UI strings are hardcoded in English (e.g., "Home", "Analyzer", "Settings", etc.). Consider either:

1. Using i18n to internationalize the UI strings, or
2. Removing the unused import if internationalization is planned for a later phase

docs/CLI_Reference.md (1)

46-46: Consider fixing list indentation for consistency.

The nested list item uses 4 spaces instead of the Markdown standard 2 spaces, flagged by markdownlint.

🔎 Proposed fix

 *   `switchcraft intune package <setup_file> -o <out_folder> -s <source_folder>`
 *   `switchcraft intune upload <intunewin> --name "App Name" --publisher "Pub" ...`
-    *   Requires `IntuneTenantId`, `IntuneClientId`, `IntuneClientSecret` in config.
+  *   Requires `IntuneTenantId`, `IntuneClientId`, `IntuneClientSecret` in config.

.github/workflows/test.yml (1)

30-30: Consider declaring PYTHONPATH separately.

Shellcheck flags the combined export PYTHONPATH=$PYTHONPATH:$(pwd)/src pattern as it can mask return values from the command substitution.

🔎 Proposed fix

-           export PYTHONPATH=$PYTHONPATH:$(pwd)/src
+           REPO_SRC="$(pwd)/src"
+           export PYTHONPATH="$PYTHONPATH:$REPO_SRC"

src/switchcraft/main.py (1)

27-28: Consider enhancing the GUI error message.

The error message for missing GUI dependencies could guide users toward installing GUI extras or using the CLI.

🔎 Proposed enhancement

         except ImportError as e:
-             print(f"GUI dependencies not found. Error: {e}")
+             print(f"GUI dependencies not found. Error: {e}")
+             print("Hint: Install GUI support with 'pip install switchcraft[gui]' or use CLI mode.")
              sys.exit(1)

switchcraft_legacy.spec (1)

31-42: Minor style issues in module enumeration logic.

1. Line 33: Prefer file != '__init__.py' over not file == '__init__.py' for readability.
2. Line 39: Inconsistent indentation (extra space before full_path).

🔎 Suggested style improvements

 for root, dirs, files in os.walk(pkg_path):
     for file in files:
-        if file.endswith('.py') and not file == '__init__.py':
+        if file.endswith('.py') and file != '__init__.py':
             full_path = os.path.join(root, file)
             rel_path = os.path.relpath(full_path, src_root)
             module_name = rel_path.replace(os.sep, '.').replace('.py', '')
             hidden_imports.append(module_name)
         elif file == '__init__.py':
-             full_path = os.path.join(root, file)
-             rel_path = os.path.relpath(root, src_root)
-             module_name = rel_path.replace(os.sep, '.')
-             hidden_imports.append(module_name)
+            full_path = os.path.join(root, file)
+            rel_path = os.path.relpath(root, src_root)
+            module_name = rel_path.replace(os.sep, '.')
+            hidden_imports.append(module_name)

src/switchcraft/cli_main.py (1)

1-1: Remove leading blank line.

PEP 8 recommends no blank lines at the start of a file.

tests/test_cli_dynamic.py (2)

6-12: Unused ctx parameter in get_all_commands.

The ctx parameter is passed but never used in the function body. Consider removing it.

🔎 Suggested fix

-def get_all_commands(cli_obj, ctx):
+def get_all_commands(cli_obj):
     """Recursively yield all command paths and command objects."""
     yield [], cli_obj
     if isinstance(cli_obj, click.Group):
         for name, cmd in cli_obj.commands.items():
-            for sub_path, sub_cmd in get_all_commands(cmd, ctx):
+            for sub_path, sub_cmd in get_all_commands(cmd):
                 yield [name] + sub_path, sub_cmd

And update the call site at line 30:

-            commands = list(get_all_commands(cli, ctx))
+            commands = list(get_all_commands(cli))

---

48-65: Test is functional but comments are verbose.

The test correctly validates that --json flag is accepted. Consider trimming the inline comments - the test name and docstring already convey the purpose.

🔎 Suggested simplification

 def test_cli_json_flag():
     """Verify the --json flag on the main entry point."""
     runner = CliRunner()
-    # We pass no arguments, which should trigger help or specific behavior
-    # But here we want to test if --json flag is accepted even without file (might fail validation but check it exists)
-
-    # Actually, main cli requires filepath argument?
-    # Let's check commands.py: @click.argument('filepath', required=False)
-
     result = runner.invoke(cli, ['--json'])
     assert result.exit_code == 0
-    # Expected: empty JSON output with maybe default values or handled gracefully
-    # Based on commands.py: if not filepath: click.echo(ctx.get_help()) -> output_json not reached.
-    # Wait, commands.py logic:
-    # if not filepath: click.echo(ctx.get_help())
-
-    # So --json without file just shows help.
+    # Without required arguments, CLI shows help even with --json
     assert "Usage:" in result.output

src/switchcraft/gui/app.py (1)

242-273: Consider the implications of accessing _tab_dict and re-initializing HistoryView.

Two observations:

1. self.tabview._tab_dict is a private/internal attribute of CTkTabview. This could break with future customtkinter updates.

2. When the History tab is re-added (line 264-266), setup_history_tab() creates a new HistoryView instance. If the old self.history_view held state or references, this could cause orphaned objects or lost state.

🔎 Consider storing History tab state before deletion

 if history_exists:
     # Delete History, add Winget Store, re-add History
+    # Note: Any unsaved state in history_view will be lost
     self.tabview.delete("History")
     logger.debug("Temporarily removed History tab for repositioning")

Consider whether HistoryView maintains any state that should be preserved across this tab repositioning operation.

tests/test_full_coverage.py (1)

95-100: Unused mock parameter and test doesn't exercise registry code.

The @patch("winreg.OpenKey") decorator provides mock_open_key, but it's never used. More importantly, checking is_addon_installed("fake_addon") returns False because "fake_addon" isn't in AddonService.ADDONS, so the registry code path is never reached.

🔎 Consider testing an actual addon ID

-@patch("winreg.OpenKey")
-def test_addon_detection(mock_open_key):
+def test_addon_detection():
     """Test registry detection for advanced addon."""
-    # This is tricky to test on non-windows or without registry,
-    # but we can verify it doesn't crash on import/usage
-    assert not AddonService.is_addon_installed("fake_addon")
+    # Verify known addon ID resolution and that method doesn't crash
+    # "advanced" is a valid addon ID that will check filesystem
+    result = AddonService.is_addon_installed("advanced")
+    assert isinstance(result, bool)
+    
+    # Unknown addon should always return False
+    assert not AddonService.is_addon_installed("fake_addon")

src/switchcraft/gui/views/manifest_dialog.py (1)

154-157: os.startfile is Windows-only.

Consider using a cross-platform approach or adding a platform check, though this appears to be a Windows-focused application.

🔎 Proposed cross-platform alternative

     def _show_next_steps(self, manifest_dir):
         # Open folder
         import os
-        os.startfile(manifest_dir)
+        import sys
+        if sys.platform == 'win32':
+            os.startfile(manifest_dir)
+        elif sys.platform == 'darwin':
+            subprocess.run(['open', manifest_dir])
+        else:
+            subprocess.run(['xdg-open', manifest_dir])

scripts/build_release.ps1 (1)

56-62: Variable $AutoLaunch should be initialized at script scope.

$AutoLaunch is set conditionally inside the default behavior block but referenced later (line 170). If no default path is taken, it will be $null. Consider initializing it at the script scope.

🔎 Proposed fix

+$AutoLaunch = $false
+
 # Default behavior: If no specific target provided, build Modern (Standard)
 if (-not $Modern -and -not $Legacy -and -not $Cli -and -not $Pip -and -not $Installer) {
     Write-Host "No specific target variables provided. Defaulting to Modern GUI." -ForegroundColor Cyan
     Write-Host "TIP: Use '.\scripts\build_release.ps1 -All' to build Modern, Legacy, CLI, and Installer at once." -ForegroundColor Green
     $Modern = $true
     $AutoLaunch = $true
 }

src/switchcraft/services/winget_manifest_service.py (1)

132-136: Remove dead code block.

This conditional block does nothing (just contains pass).

🔎 Proposed fix

         filename = f"{meta['PackageIdentifier']}.installer.yaml"
-
-        # Global scope/type if consistent across installers
-        if len(installers) > 0:
-            first = installers[0]
-            if "InstallerType" in first: # If all have same type, can move to root, but keeping explicit is safer
-                 pass
-
         self._dump_yaml(folder / filename, data)

src/switchcraft/cli/commands.py (2)

77-90: Consider type coercion for configuration values.

The value argument is always passed as a string from the CLI. If users need to set numeric or boolean preferences, consider adding type inference or an explicit --type option. Per the SwitchCraftConfig context, set_user_preference accepts an optional value_type parameter.

---

276-283: Consider logging unhandled prompt types.

The cli_prompt callback only handles 'ask_browser' and returns False for all other types. Consider logging unhandled types for debugging purposes.

Proposed enhancement

     def cli_prompt(type, **kwargs):
         # CLI Handler for addon prompts
         if type == 'ask_browser':
              # Maybe skip browser for CLI or print URL
              url = kwargs.get('url')
              print(f"Please check: {url}")
              return False # Don't rely on browser flow in CLI
+        logger.debug(f"Unhandled prompt type in CLI: {type}")
         return False

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/switchcraft/services/winget_manifest_service.py (2)

53-57: Guard against empty publisher string.

If publisher is an empty string, publisher[0] will raise an IndexError. This issue was previously identified but remains unresolved.

🔎 Proposed fix

         # Folder structure: manifests/{first_char_lower}/{Publisher}/{PackageName}/{Version}
-        p_char = publisher[0].lower()
+        p_char = publisher[0].lower() if publisher else "_"
         package_name = meta.get("PackageName", pkg_id.split('.')[-1])

Alternatively, strengthen the validation at line 39:

-        if not (pkg_id and version and publisher):
-            raise ValueError("Missing required fields: PackageIdentifier, PackageVersion, Publisher")
+        if not (pkg_id and version and publisher and len(publisher) > 0):
+            raise ValueError("Missing or empty required fields: PackageIdentifier, PackageVersion, Publisher")

Based on past review comments.

---

80-81: subprocess.STARTUPINFO() is Windows-only.

This code will raise an AttributeError on non-Windows platforms. The winget.py file has a _get_startup_info() helper that handles this correctly—consider using a similar pattern here.

🔎 Proposed fix

+    def _get_startup_info(self):
+        if hasattr(subprocess, 'STARTUPINFO'):
+            si = subprocess.STARTUPINFO()
+            si.dwFlags |= subprocess.STARTF_USESHOWWINDOW
+            return si
+        return None
+
     def validate_manifest(self, manifest_dir: str) -> dict:
         """
         Validates the generated manifests using 'winget validate'.
         Returns dict with keys: 'valid' (bool), 'output' (str), 'errors' (list)
         """
         try:
             # winget validate --manifest <path_to_directory>
             cmd = [self.winget_exe, "validate", "--manifest", str(manifest_dir)]

             # Start process without window
-            startupinfo = subprocess.STARTUPINFO()
-            startupinfo.dwFlags |= subprocess.STARTF_USESHOWWINDOW
+            startupinfo = self._get_startup_info()

             result = subprocess.run(
                 cmd,
                 capture_output=True,
                 text=True,
                 startupinfo=startupinfo,
                 check=False
             )

Based on past review comments.

src/switchcraft_winget/utils/winget.py (1)

101-112: Command injection vulnerability with shell=True.

Using shell=True with string interpolation of package_id and scope parameters allows command injection. An attacker-controlled package ID like foo; rm -rf / could execute arbitrary commands.

🔎 Proposed fix using argument list

     def install_package(self, package_id: str, scope: str = "machine") -> bool:
         """Install a package via Winget CLI."""
-        cmd = f"winget install --id {package_id} --scope {scope} --accept-package-agreements --accept-source-agreements"
+        if scope not in ("machine", "user"):
+            logger.error(f"Invalid scope: {scope}")
+            return False
+        cmd = [
+            "winget", "install",
+            "--id", package_id,
+            "--scope", scope,
+            "--accept-package-agreements",
+            "--accept-source-agreements"
+        ]
         try:
-            proc = subprocess.run(cmd, shell=True, capture_output=True, text=True)
+            proc = subprocess.run(cmd, capture_output=True, text=True)
             if proc.returncode != 0:
                 logger.error(f"Winget install failed: {proc.stderr}")
                 return False
             return True
         except Exception as e:
             logger.error(f"Winget install exception: {e}")
             return False

Based on past review comments.

src/switchcraft/gui/splash.py (2)

37-67: Consider font fallback for cross-platform compatibility.

The hard-coded "Segoe UI" font (lines 43, 50, 59) is Windows-specific. On Linux and macOS, this font may not be available, causing tkinter to fall back to a default font, which could affect the visual consistency of the splash screen.

🔎 Suggested improvement

Consider using a font fallback list or a cross-platform font:

+# At the top of __init__, define a cross-platform font
+font_family = "Segoe UI" if self.root.tk.call("tk", "windowingsystem") == "win32" else "Helvetica"
+
 # Then use it in the labels:
 tk.Label(
     main_frame,
     text="SwitchCraft",
-    font=("Segoe UI", 32, "bold"),
+    font=(font_family, 32, "bold"),
     bg="#2c3e50",
     fg="#ecf0f1"
 ).pack(pady=(40, 10))

Or use tkinter's default font families like "TkDefaultFont" or "Helvetica" which are available across platforms.

Optional: Store progress bar reference for explicit cleanup.

The progress bar is started but the reference is not stored (line 65). While destroy() will clean it up, storing it as self.progress = progress would allow you to explicitly stop it in the close() method with self.progress.stop() if needed.

---

75-76: Optional: Explicitly stop the progress bar before destroying.

While destroy() will clean up all widgets, explicitly stopping the progress bar first is a minor best practice improvement:

def close(self):
    if hasattr(self, 'progress'):
        self.progress.stop()
    self.root.destroy()

This requires storing the progress bar reference as self.progress during initialization (see previous comment).

src/switchcraft/gui_modern/app.py (1)

77-92: Consider dictionary-based navigation for cleaner code.

The if-elif chain works correctly, but a dictionary mapping index to content would be more maintainable as the number of navigation items grows.

🔎 Optional refactor using dictionary mapping

    def nav_change(self, e):
        idx = e.control.selected_index
        self.content_area.controls.clear()

-        if idx == 0:
-            self.content_area.controls.append(ft.Text("Home", size=30))
-        elif idx == 1:
-            self.content_area.controls.append(ft.Text("Analyzer (Coming Soon)", size=30))
-        elif idx == 2:
-            self.content_area.controls.append(ft.Text("Winget (Coming Soon)", size=30))
-        elif idx == 3:
-            self.content_area.controls.append(ft.Text("Intune (Coming Soon)", size=30))
-        elif idx == 4:
-             self.content_area.controls.append(ft.Text("Settings (Coming Soon)", size=30))
+        nav_content = {
+            0: "Home",
+            1: "Analyzer (Coming Soon)",
+            2: "Winget (Coming Soon)",
+            3: "Intune (Coming Soon)",
+            4: "Settings (Coming Soon)",
+        }
+        
+        if idx in nav_content:
+            self.content_area.controls.append(ft.Text(nav_content[idx], size=30))

        self.page.update()

src/switchcraft/gui/views/analyzer_view.py (1)

969-978: Consider adding error handling for dialog instantiation.

The lazy import and dialog creation could fail if the ManifestDialog module has issues. While unlikely, wrapping the dialog instantiation in a try-except would improve robustness.

🔎 Optional error handling enhancement

 def _open_manifest_dialog(self, info):
     # Check Config
     repo_path = SwitchCraftConfig.get_value("WingetRepoPath")
     if not repo_path:
          # Optional: Ask user if they want to configure it, or just rely on default logic in Service
          pass

-    from switchcraft.gui.views.manifest_dialog import ManifestDialog
-    dlg = ManifestDialog(self.winfo_toplevel(), info)
-    dlg.grab_set()
+    try:
+        from switchcraft.gui.views.manifest_dialog import ManifestDialog
+        dlg = ManifestDialog(self.winfo_toplevel(), info)
+        dlg.grab_set()
+    except Exception as e:
+        logger.error(f"Failed to open manifest dialog: {e}")
+        messagebox.showerror("Error", f"Could not open manifest dialog: {e}")

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/switchcraft/gui_modern/app.py (1)

121-127: Verify entry point in modern_main.py matches this file's pattern.

This file correctly passes the main function to ft.app(target=main) (line 127). However, the relevant code snippet shows that src/switchcraft/modern_main.py passes ModernApp class directly: ft.app(target=ModernApp). This will fail at runtime since Flet expects a callable that accepts a page parameter.

Verify that modern_main.py has been updated to match this pattern:

#!/bin/bash
# Check if modern_main.py passes the correct callable to ft.app
cat src/switchcraft/modern_main.py | grep -A 2 "def main"

src/switchcraft/gui/splash.py (1)

10-87: Consider using _owns_root flag in close() method.

The _owns_root flag is set in __init__ but never checked in close(). If the splash owns the root (standalone run), destroying just the splash window may leave the Tk root running. Consider conditionally destroying the root when _owns_root is True.

🔎 Proposed fix

     def close(self):
         if hasattr(self, 'progress'):
             self.progress.stop()
-        self.root.destroy()
+        if self._owns_root:
+            self.root.quit()  # Stop mainloop if we own it
+        self.root.destroy()

src/switchcraft_winget/utils/winget.py (1)

124-142: Consider filtering installer files by modification time.

Lines 134-137 search for any .exe or .msi file in dest_dir. If the directory contains pre-existing installers, this could return the wrong file. Consider filtering by files modified after the winget download command started, or use a unique temporary directory for each download.

🔎 Possible improvement

     def download_package(self, package_id: str, dest_dir: Path) -> Optional[Path]:
         """Download a package installer to dest_dir. Returns path to installer if found."""
+        import time
+        start_time = time.time()
         cmd = ["winget", "download", "--id", package_id, "--dir", str(dest_dir), "--accept-source-agreements", "--accept-package-agreements"]
         try:
             proc = subprocess.run(cmd, capture_output=True, text=True)
             if proc.returncode != 0:
                 logger.error(f"Winget download failed: {proc.stderr}")
                 return None

             # Find the installer file
             files = list(dest_dir.glob("*.*"))
             for f in files:
-                if f.suffix.lower() in [".exe", ".msi"]:
+                if f.suffix.lower() in [".exe", ".msi"] and f.stat().st_mtime >= start_time:
                     return f
             return None

src/switchcraft/services/winget_manifest_service.py (1)

70-103: Windows-specific subprocess handling fixed.

Lines 80-83 correctly use hasattr(subprocess, 'STARTUPINFO') to check for Windows-specific functionality before using it. This resolves the cross-platform compatibility issue from the previous review.

For consistency with winget.py, consider extracting this pattern into a _get_startup_info() helper method:

def _get_startup_info(self):
    if hasattr(subprocess, 'STARTUPINFO'):
        si = subprocess.STARTUPINFO()
        si.dwFlags |= subprocess.STARTF_USESHOWWINDOW
        return si
    return None

src/switchcraft/gui/views/analyzer_view.py (1)

969-981: Consider explicit modal dialog handling.

Line 979 calls dlg.grab_set() to make the dialog modal, but doesn't wait for it to close or handle its result. In CTk, dialogs are typically non-blocking unless explicitly managed. Consider whether the dialog should block until closed or if the current fire-and-forget pattern is intentional.

If blocking behavior is desired:

dlg = ManifestDialog(self.winfo_toplevel(), info)
dlg.transient(self.winfo_toplevel())
dlg.grab_set()
self.wait_window(dlg)  # Wait for dialog to close

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/switchcraft/gui/app.py (1)

294-327: Tab reordering still loses History view state (as noted in previous review).

The added debug logging (lines 296-297, 314, 320, 325, 327) is helpful for troubleshooting. However, the fundamental issue identified in the previous review remains: recreating the History tab (line 319) creates a new HistoryView instance, which loses any runtime state (selected items, scroll position, filters, etc.).

The previous review comment suggested state preservation or documenting this limitation. Consider addressing this in a future iteration if user experience is impacted.

src/switchcraft_winget/utils/winget.py (1)

130-147: Consider documenting behavior when multiple installers exist.

The method returns the first installer file found, but if Winget downloads multiple installers (e.g., x86 and x64 versions), the choice is non-deterministic since glob iteration order is not guaranteed. Consider documenting this behavior or filtering for a specific architecture if determinism is required.

Also consider adding a timeout parameter to the subprocess.run call to prevent indefinite hangs.

switchcraft_legacy.spec (1)

17-40: Redundant module collection with collect_submodules.

Line 19 already uses collect_submodules('switchcraft') to automatically discover all switchcraft submodules recursively. The manual directory walking (lines 21-40) duplicates this effort. While PyInstaller deduplicates hidden imports internally, the manual walk adds unnecessary complexity and maintenance burden.

🔎 Proposed simplification

Remove the manual walking logic and rely solely on collect_submodules:

 # MANUAL COLLECTION to ensure robustness
 hidden_imports = ['PIL._tkinter_finder', 'tkinterdnd2', 'plyer.platforms.win.notification', 'defusedxml', 'winotify', 'switchcraft.services.addon_service']
 hidden_imports += collect_submodules('switchcraft')
-
-# Manually walk src/switchcraft to find all modules
-src_root = os.path.abspath('src')
-if src_root not in sys.path:
-    sys.path.insert(0, src_root)
-
-import switchcraft
-pkg_path = os.path.dirname(switchcraft.__file__)
-
-for root, dirs, files in os.walk(pkg_path):
-    for file in files:
-        if file.endswith('.py') and not file == '__init__.py':
-            full_path = os.path.join(root, file)
-            rel_path = os.path.relpath(full_path, src_root)
-            module_name = rel_path.replace(os.sep, '.').replace('.py', '')
-            hidden_imports.append(module_name)
-        elif file == '__init__.py':
-             full_path = os.path.join(root, file)
-             rel_path = os.path.relpath(root, src_root)
-             module_name = rel_path.replace(os.sep, '.')
-             hidden_imports.append(module_name)

src/switchcraft/gui/views/settings_view.py (2)

972-975: Consider making _show_restart_countdown a public API.

The code defensively checks for the existence of self.app._show_restart_countdown() and falls back gracefully. However, accessing a private method (indicated by the leading underscore) from outside the class is generally discouraged. Consider either:

1. Making this a public method by removing the underscore prefix
2. Providing a formal public API for triggering restart countdowns

This improves code maintainability and signals the intended usage pattern.

---

972-975: Consider extracting the restart UX pattern to a helper method.

The same pattern (check for app._show_restart_countdown() with fallback) is repeated at lines 972-975 and 991-995. Consider extracting this to a helper method to reduce duplication:

🔎 Possible refactor

def _trigger_restart_ui(self, success_msg=None):
    """Trigger app restart UI with fallback."""
    if hasattr(self.app, '_show_restart_countdown'):
        self.app._show_restart_countdown()
    else:
        msg = success_msg or i18n.get("status_installed_restart") or "Addon installed successfully! Please restart."
        messagebox.showinfo(i18n.get("restart_required"), msg)

Then use it as:

if AddonService.install_addon(addon_id, prompt_callback=prompt_handler):
    self._trigger_restart_ui(f"Addon {addon_id} installed! Please restart.")

Also applies to: 991-995

src/switchcraft/gui/app.py (1)

250-291: Solid implementation of weekly cloud backup check.

The logic correctly:

• Checks authentication and config before proceeding
• Uses a 7-day interval (604800 seconds)
• Detects changes via MD5 hash comparison (appropriate for non-cryptographic use)
• Updates the timestamp even when no changes are detected to avoid repeated checks
• Runs in a background thread to avoid blocking the UI

Consider adding error notification for users if the auto-backup fails:

🔎 Optional enhancement

 if SyncService.sync_up():
     logger.info("Auto-backup successful.")
     SwitchCraftConfig.set_user_preference("LastCloudBackup", now)
     SwitchCraftConfig.set_user_preference("LastBackupHash", current_hash)
+else:
+    logger.warning("Auto-backup failed. Will retry next week.")
+    # Optionally notify user via NotificationService

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/switchcraft/gui/app.py (1)

312-346: Note: History view state is recreated during Winget tab toggle.

The tab reordering logic (lines 326-338) correctly repositions tabs but recreates the History view, which will lose any runtime state (scroll position, filters, selections). This is a known limitation from the previous review.

The added debug logging is helpful for troubleshooting tab management issues.

tests/test_config.py (2)

98-118: Consider adding edge case tests for float conversion.

The test verifies that float values are converted to int for REG_DWORD, but doesn't cover edge cases:

1. Negative floats: Test how negative values are handled (e.g., -123.45)
2. Large floats: Test floats that might exceed DWORD range (e.g., 5e9)
3. Precision loss: Verify truncation behavior (e.g., 123.99 becomes 123, not 124)
4. Round-trip: Test that writing and reading back produces expected results

These edge cases would help ensure the conversion logic handles all scenarios correctly.

Example edge case tests

@patch('sys.platform', 'win32')
def test_set_user_preference_float_negative(self):
    """Test that negative float values are handled."""
    with patch.object(self.winreg_mock, 'CreateKey'):
        mock_key = MagicMock()
        self.winreg_mock.OpenKey = MagicMock(return_value=mock_key)
        mock_key.__enter__ = MagicMock(return_value=mock_key)
        mock_key.__exit__ = MagicMock(return_value=False)
        
        SwitchCraftConfig.set_user_preference("TestNegFloat", -123.45)
        call_args = self.winreg_mock.SetValueEx.call_args[0]
        self.assertEqual(call_args[4], -123)

@patch('sys.platform', 'win32')
def test_set_user_preference_float_truncation(self):
    """Test that float truncation works as expected."""
    with patch.object(self.winreg_mock, 'CreateKey'):
        mock_key = MagicMock()
        self.winreg_mock.OpenKey = MagicMock(return_value=mock_key)
        mock_key.__enter__ = MagicMock(return_value=mock_key)
        mock_key.__exit__ = MagicMock(return_value=False)
        
        SwitchCraftConfig.set_user_preference("TestTrunc", 123.99)
        call_args = self.winreg_mock.SetValueEx.call_args[0]
        self.assertEqual(call_args[4], 123)  # Truncated, not rounded

---

120-131: Add test coverage for False and verify REG_DWORD type.

The test verifies that True is converted to 1, but doesn't test:

1. False value: Verify that False is converted to 0
2. Type verification: Assert that REG_DWORD type is used (similar to the float test at line 117)

Enhanced test

 @patch('sys.platform', 'win32')
 def test_set_user_preference_bool(self):
     """Test that bool values are converted to 0/1 for REG_DWORD."""
     with patch.object(self.winreg_mock, 'CreateKey'):
         mock_key = MagicMock()
         self.winreg_mock.OpenKey = MagicMock(return_value=mock_key)
         mock_key.__enter__ = MagicMock(return_value=mock_key)
         mock_key.__exit__ = MagicMock(return_value=False)

         SwitchCraftConfig.set_user_preference("TestBool", True)
         call_args = self.winreg_mock.SetValueEx.call_args[0]
+        self.assertEqual(call_args[3], self.winreg_mock.REG_DWORD)
         self.assertEqual(call_args[4], 1)
+
+        # Reset mock for False test
+        self.winreg_mock.SetValueEx.reset_mock()
+        SwitchCraftConfig.set_user_preference("TestBool", False)
+        call_args = self.winreg_mock.SetValueEx.call_args[0]
+        self.assertEqual(call_args[3], self.winreg_mock.REG_DWORD)
+        self.assertEqual(call_args[4], 0)

tests/test_winget.py (2)

6-29: Test name misleading and unused mock present.

The test name test_search_by_name_cli_found suggests it's testing the CLI fallback path, but it's actually mocking a successful PowerShell JSON response, which exercises the primary PowerShell module path. Additionally, the @patch('shutil.which', ...) decorator appears unused since the WingetHelper implementation doesn't call shutil.which.

Consider:

1. Renaming to test_search_by_name_powershell_success or similar
2. Removing the unused shutil.which patch
3. Adding a separate test for the CLI fallback by mocking a PowerShell failure (returncode != 0 with "Find-WinGetPackage" in stderr)

🔎 Suggested improvements

-    @patch('shutil.which', return_value="C:\\winget.exe")
     @patch('subprocess.run')
-    def test_search_by_name_cli_found(self, mock_run, mock_which):
+    def test_search_by_name_powershell_success(self, mock_run):
         # Mock successful winget search output

Additionally, consider verifying that the correct PowerShell command is called:

# After helper.search_by_name("7zip")
mock_run.assert_called()
call_args = mock_run.call_args
cmd = call_args[0][0]
self.assertIn("Find-WinGetPackage", cmd[4])
self.assertIn("-Query '7zip'", cmd[4])

---

31-54: Remove unused shutil.which patch for consistency.

Similar to the previous test, the @patch('shutil.which', ...) decorator is not used by the implementation. Removing it will make the test cleaner and more accurate.

🔎 Suggested diff

-    @patch('shutil.which', return_value="C:\\winget.exe")
     @patch('subprocess.run')
-    def test_search_packages_found(self, mock_run, mock_which):
+    def test_search_packages_found(self, mock_run):

switchcraft.spec (2)

43-50: Consider moving the importlib import to module level.

The can_import() helper imports importlib on every call. While this runs only at build time (not performance-critical), moving the import to the top of the file would be more idiomatic.

🔎 Proposed refactor

Move the import to the top of the file alongside other imports:

 from pathlib import Path
 from PyInstaller.utils.hooks import collect_submodules
+import importlib

Then simplify the function:

 def can_import(module_name):
     """Test if a module can actually be imported."""
     try:
-        import importlib
         importlib.import_module(module_name)
         return True
     except (ImportError, ModuleNotFoundError):
         return False

---

59-61: Optional: Simplify duplicate checking logic.

The inline module_name not in hidden_imports checks are O(n) operations repeated for each discovered module. Since line 70 performs final deduplication anyway, these inline checks add complexity without much benefit.

Consider either:

1. Removing the inline duplicate checks and relying solely on the final set() deduplication at line 70, or
2. Using a set for hidden_imports from the start and converting to list only when passing to PyInstaller.

Also applies to: 66-67

src/switchcraft/gui/app.py (1)

846-876: Consider simplifying the comment block.

The logic is correct: for dev/nightly builds, the debug console is force-enabled at startup (line 875). However, the comment block (lines 846-874) is quite verbose.

🔎 Suggested simplification

-        # --- Auto-Enable Debug Console for Dev/Nightly Builds ---
-        from switchcraft import __version__
-        if "dev" in __version__.lower() or "nightly" in __version__.lower():
-            # If preference is missing (None) or True, ensure it is set to True.
-            # If user explicitly disabled it (False), we respect it (unless first run logic overrides, but simple is better).
-            # User request: "Start also if disabled in user settings... IF it is dev/nightly. If user manually turns off, keep off."
-            # To track "manually turned off", we rely on the config value being explicitly False.
-            # If it is None (default), we force True.
-            # Wait, user said: "Das soll auch hochkommen, selbst wenn in den User/Systemeinstellungen debugging aus ist." (Even if debug is off).
-            # "Wenn der User es dann nach dem Start manuell ausschaltet, soll es auch aus bleiben" (If user turns off AFTER start, keep off).
-            # This implies a runtime override: Always ON at startup for Dev, unless... "until man die anwendung neustartet"?
-            # "bis man die anwendung neustartet" -> "until one restarts application".
-            # Logic: Force ON at startup always for Dev?
-            # "Wenn der User es dann nach dem Start manuell ausschaltet, soll es auch aus bleiben, bis man die anwendung neustartet"
-            # --> If I turn it off, it stays off UNTIL restart. So restart -> ON again.
-            # So: ALWAYS ON at startup for Dev.
-            # But "bei beta und stable ... wie bisher".
-
-            # Simple Logic: For Dev/Nightly, Runtime Debug = True.
-            # switchcraft.services.addon_service or config needs to know.
-            # We can just set the config in memory? Or set the preference?
-            # If we set preference, it saves to file.
-            # If we just open the window...
-            # The Debug Console is likely opened by SettingsView checking config or App checking config.
-            # Let's override the in-memory config to True on startup. (But not save it if we don't want to mess up user pref?)
-            # SwitchCraftConfig is a singleton handling file I/O?
-            # Let's just set the user preference to True. Ideally we wouldn't persist it if user wants it "off by default" but we are forcing it "on by default" for dev.
-            # But "Always ON at startup" means we overwrite "Off".
-
+        # Auto-enable debug console for dev/nightly builds
+        # User can disable during session, but it will be re-enabled on next startup
+        from switchcraft import __version__
+        if "dev" in __version__.lower() or "nightly" in __version__.lower():
             SwitchCraftConfig.set_user_preference("ShowDebugConsole", True)

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/switchcraft/utils/config.py (1)

128-129: Consider validating integer range as well.

Currently, only float values are validated for the REG_DWORD range. If a caller passes a large integer directly (e.g., 2**40), it bypasses validation and may cause SetValueEx to fail with an unclear error.

For consistency and robustness, consider applying the same range check:

🔎 Suggested validation for integers

 elif isinstance(value, int):
     value_type = winreg.REG_DWORD
+    # Validate range for REG_DWORD (unsigned 32-bit: 0 to 4294967295)
+    if value < 0 or value > 0xFFFFFFFF:
+        raise ValueError(f"Registry value '{value_name}' out of range for REG_DWORD: {value}")

tests/test_config.py (1)

98-118: Test coverage looks good.

The test verifies that float values are converted to int and stored as REG_DWORD. The mock setup is correct.

Optional enhancement: Consider also verifying the actual converted value to ensure rounding works as expected:

🔎 Optional verification of converted value

 # Verify SetValueEx was called with int, not float
 self.winreg_mock.SetValueEx.assert_called_once()
 call_args = self.winreg_mock.SetValueEx.call_args[0]
 # SetValueEx(key, name, reserved, type, value)
 # Index 3 is type, Index 4 is value
 self.assertEqual(call_args[3], self.winreg_mock.REG_DWORD)
 self.assertIsInstance(call_args[4], int)
+# Verify the value is close to the original (within 1 due to rounding)
+self.assertAlmostEqual(call_args[4], now, delta=1)

src/switchcraft/services/addon_service.py (1)

263-270: Remove incomplete/debugging comments.

Lines 263-270 contain comments that suggest concerns about the path normalization approach but don't lead to any implementation. These appear to be leftover debugging notes or incomplete thoughts from development. Since the logic on line 262 correctly calculates the prefix using normalized paths, these comments can be removed for clarity.

🔎 Proposed cleanup

                                 suffix = f"{part}/__init__.py"
                                 source_prefix = f_norm[:-len(suffix)]
-                                # Maintain original slash style for the zip member lookup or just assume normalized?
-                                # z.open(member) handles the object, but we need the prefix valid for matching string
-                                # Ideally we map back to original filename, but zipfile might just handle /
-                                # Actually, member.filename is the source of truth.
-                                # Let's find the ORIGINAL prefix
-
-                                # If separators differ, len might match but content differs.
-                                # Safe bet: Use index from original 'f' if we can find the pattern
                                 break

switchcraft.spec (1)

20-27: Consider clarifying the addon dependency strategy.

The comment on Lines 20–21 states that addon modules are not bundled and are downloaded at runtime. However, Line 26 includes py7zr (a dependency of the switchcraft_advanced addon) in the main executable's hidden_imports. This approach increases the base executable size and may cause confusion for future maintainers.

Options to consider:

1. Keep as-is but clarify: Update the comment to explicitly state that common or critical addon dependencies (like py7zr) are pre-bundled for reliability:

   # Note: Addon modules (switchcraft_winget, switchcraft_ai, etc.) are NOT bundled.
   # They are downloaded separately at runtime.
   # However, commonly-needed addon dependencies (py7zr) are pre-bundled
   # to reduce runtime issues and ensure 7z extraction works out-of-the-box.

2. Move dependency to addon: Let each addon manage its own dependencies (including py7zr) so the main executable remains lean. This requires addons to bundle or install their dependencies on first run.

3. Lazy-load py7zr: Only import and use py7zr when the addon is actually loaded, with a fallback or user prompt if it's missing.

The current approach is valid if pre-bundling common dependencies is intentional, but clarifying the rationale will help maintainability.

src/switchcraft/gui/app.py (1)

87-98: Well-designed error tracking for addon load failures.

The distinction between addon not installed vs. installed-but-failed is handled correctly, and the exception logging provides good diagnostics. The error is appropriately stored for user notification later.

💡 Optional: Consider enhancing the user-facing error message

The message on line 94 could be more actionable for users:

-                self.winget_load_error = "Import failed (returned None). Check log for details."
+                self.winget_load_error = "Addon failed to load. This may indicate a corrupted installation or missing dependencies. Try reinstalling from the Addon Manager."

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/switchcraft/services/addon_service.py (1)

260-260: Fix typo in comment (previously flagged but not addressed).

"Calulcate" should be "Calculate".

🔎 Proposed fix

-                                # Calulcate prefix
+                                # Calculate prefix

src/switchcraft/services/addon_service.py (1)

74-80: Consider whether catching all exceptions is necessary.

While catching Exception provides robustness for addon loading, it may mask unexpected errors like SyntaxError or AttributeError in addon code, making debugging harder. If the primary concern is missing dependencies (per the comment), consider catching (ImportError, ModuleNotFoundError) specifically and letting other exceptions propagate with a clear traceback.

🔎 Alternative: More targeted exception handling

-        # Gracefully handle import failures (e.g., missing dependencies like py7zr)
         try:
             import importlib
             return importlib.import_module(full_name)
-        except Exception as e:
-            logger.error(f"Failed to import addon module {full_name}: {e}")
+        except (ImportError, ModuleNotFoundError) as e:
+            logger.error(f"Failed to import addon module {full_name} (likely missing dependency): {e}")
             return None
+        except Exception as e:
+            logger.exception(f"Unexpected error importing addon module {full_name}: {e}")
+            raise

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/switchcraft/gui/app.py (2)

317-351: Tab reordering still loses History view state.

The implementation continues to delete and recreate the History tab (lines 331, 341-342) when enabling Winget, which loses any runtime state in the History view as previously noted.

---

268-315: Handle the False return case from sync_up().

The exception handling is good (lines 309-312), but if SyncService.sync_up() returns False (line 301), the timestamps won't update. This means failed syncs will retry every 7 days, which may be undesirable for persistent failures.

Consider handling the False case explicitly as suggested in the previous review:

🔎 Proposed enhancement

                     if current_hash != last_hash:
                         logger.info("Changes detected since last backup. Performing auto-backup...")
                         if SyncService.sync_up():
                             logger.info("Auto-backup successful.")
                             SwitchCraftConfig.set_user_preference("LastCloudBackup", now)
                             SwitchCraftConfig.set_user_preference("LastBackupHash", current_hash)
+                        else:
+                            logger.warning("Auto-backup failed (returned False).")
+                            # Still update timestamp to avoid continuous retries
+                            SwitchCraftConfig.set_user_preference("LastCloudBackup", now)

Based on past review comments suggesting similar handling.

switchcraft.spec (1)

19-27: Maintainability concern: Fixed list may become stale.

The switch from dynamic collection to a fixed list addresses the redundancy issue from past reviews. However, this approach requires manual updates whenever new modules are added to the codebase. If a developer adds a new module but forgets to update this list, the PyInstaller build may silently exclude it, leading to runtime import errors.

Consider keeping a hybrid approach: maintain the explicit list for critical dependencies like py7zr, but continue to use the dynamic collection (lines 29-33) as a safety net to catch newly added switchcraft modules.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/switchcraft/gui/app.py (1)

321-354: Tab reordering still loses History view state.

The tab reordering logic continues to delete and recreate the History tab (lines 335, 345-347), which will lose any runtime state as previously noted. The past review comment's concern remains unaddressed.

src/switchcraft/gui/app.py (1)

855-861: Consider documenting the debug console auto-enable behavior.

The auto-enable of debug console for dev/nightly builds overrides user preference, which is reasonable for troubleshooting but may surprise users. Consider adding a log message or comment explaining this behavior.

🔎 Suggested documentation addition

         if "dev" in __version__.lower() or "nightly" in __version__.lower():
             # Force Debug Console ON for Dev/Nightly builds at startup for better troubleshooting.
             # This overrides user preference for the session but respects if user manually closes/disables it later.
+            logger.info("Dev/Nightly build detected: Auto-enabling debug console for troubleshooting.")
             SwitchCraftConfig.set_user_preference("ShowDebugConsole", True)