Many new features and UX improvements - #30

src/switchcraft/gui_modern/views/detection_tester_view.py-6-7 (1)

6-7: Guard Windows-specific imports for cross-platform compatibility.

Top-level imports of winreg and win32api will cause ImportError on non-Windows platforms, preventing the entire module from loading. While this is a Windows-specific tool, graceful handling would improve maintainability.

🐛 Proposed fix with lazy imports or platform check

 import flet as ft
 import logging
 import subprocess
 import os
+import sys
 from pathlib import Path
-import winreg
-import win32api
+
+if sys.platform == "win32":
+    import winreg
+    import win32api
+else:
+    winreg = None
+    win32api = None

 logger = logging.getLogger(__name__)

Then add a check at the start of methods using these modules:

def _check_registry(self, key_path, value_name, expected_value):
    if winreg is None:
        return False, "Registry checks only available on Windows"
    # ... rest of method

src/switchcraft/gui_modern/views/detection_tester_view.py-260-298 (1)

260-298: Ensure temp file cleanup on all exit paths.

If subprocess.run raises an exception, the temp file created at line 261 won't be deleted. Use try/finally to guarantee cleanup.

🐛 Proposed fix

     def _check_script(self, script_content):
         # Run script in temp file
         import tempfile
+        temp_path = None
         try:
             with tempfile.NamedTemporaryFile(mode='w', delete=False, suffix=".ps1") as f:
                 f.write(script_content)
                 temp_path = f.name

             cmd = ["powershell.exe", "-ExecutionPolicy", "Bypass", "-File", temp_path]

             startupinfo = subprocess.STARTUPINFO()
             startupinfo.dwFlags |= subprocess.STARTF_USESHOWWINDOW

             res = subprocess.run(cmd, capture_output=True, text=True, startupinfo=startupinfo)

-            os.remove(temp_path)
-
             stdout = res.stdout.strip()
             stderr = res.stderr.strip()
             exit_code = res.returncode

             if exit_code == 0:
                 if stdout:
                      return True, f"Detected (Exit 0 + Stdout): {stdout[:100]}..."
                 else:
                      return False, "Not Detected (Exit 0 but Empty Stdout)"
             else:
                 return False, f"Not Detected (Exit Code {exit_code}). Err: {stderr[:100]}"

         except Exception as e:
             return False, f"Script Execution Error: {e}"
+        finally:
+            if temp_path and os.path.exists(temp_path):
+                os.remove(temp_path)

scripts/process_issue_ops.py-66-72 (1)

66-72: Exit code 0 on duplicate will create an empty PR.

When a duplicate entry is found, the script exits with code 0, which signals success to GitHub Actions. The workflow will then proceed to create a PR with no changes. Exit with a non-zero code to fail the workflow gracefully, or use GitHub Actions outputs to conditionally skip PR creation.

🐛 Proposed fix

     # Check for duplicate (naive check)
     for entry in db:
         if (entry.get("app_name") == new_entry.get("app_name") and
             entry.get("version") == new_entry.get("version")):
             print("Entry already exists for this version.")
-            # We might want to update it instead? For now, skip.
-            sys.exit(0)
+            sys.exit(1)

scripts/process_issue_ops.py-14-16 (1)

14-16: Regex incorrectly captures subsequent headers when a field is empty.

The pattern (.+?) requires at least one character, causing it to match across empty fields. When a field is truly empty (e.g., ### Application Name\n\n### Version), the regex captures the next header and its value instead—e.g., "Application Name": "### Version\n\n1.0.0" instead of an empty string. This corrupts the parsed data.

The pattern needs to handle optional values. Consider using (.*) with a modified lookahead or restructure the regex to explicitly match empty lines followed by the next ###.

src/switchcraft/gui_modern/app.py-168-194 (1)

168-194: Navigation index comments are inconsistent with actual positions.

The inline comments indicate index numbers that don't match the actual list positions. For example:

• Line 169: "History" is at index 8, but comment says "# 8 History"
• Line 173: "Settings" comment says "# 7 Settings" but it's at index 9
• Lines 175-188: Comments show indices 8, 9, 10, 11, 14, 15, 16 but actual indices are 10-16

This inconsistency will cause navigation bugs when goto_tab or card clicks use hardcoded indices.

src/switchcraft/services/addon_service.py-69-81 (1)

69-81: Dynamic code execution from untrusted addons allows arbitrary code execution.

The load_addon_view method uses importlib to execute arbitrary Python code from addon files. The addon manager UI explicitly allows users to upload custom ZIP files via file picker, and the install_addon method performs no signature verification or integrity checks. While ADDONS.md documents a warning to use only trusted sources, this is insufficient—there are no technical safeguards preventing execution of malicious addon code.

Implement addon signing with signature verification, or add sandboxing to restrict addon capabilities in future iterations. For now, the upload feature should prominently warn users that addons execute with full app privileges.

src/switchcraft/gui_modern/views/group_manager_view.py-10-23 (1)

10-23: Initialize and gate self.token before create/delete (avoid race/AttributeError).
Create/delete assume self.token exists and is valid; users can trigger dialogs before _load_data() finishes or succeeds.

Proposed fix

 class GroupManagerView(ft.Column):
     def __init__(self, page: ft.Page):
@@
         self.filtered_groups = []
+        self.token = None
@@
     def _load_data(self):
@@
         def _bg():
             try:
                 self.token = self.intune_service.authenticate(tenant, client, secret)
@@
             finally:
@@

     def _show_create_dialog(self, e):
+        if not self.token:
+            self._show_snack("Load groups / authenticate first", ft.Colors.RED)
+            return
@@
     def _confirm_delete(self, e):
         if not self.selected_group: return
+        if not self.token:
+            self._show_snack("Load groups / authenticate first", ft.Colors.RED)
+            return

Also applies to: 83-109, 152-184, 185-216

src/switchcraft/gui_modern/views/stack_manager_view.py-69-82 (1)

69-82: Handle JSON read/write failures explicitly (avoid crashing UI callbacks).
Right now _save_stacks() can raise and break the UI, and _load_stacks() silently hides corruption without leaving diagnostics.

Proposed fix

 def _load_stacks(self):
     if not self.stacks_file.exists():
         return {}
     try:
-        with open(self.stacks_file, "r") as f:
-            return json.load(f)
-    except Exception:
-        return {}
+        with open(self.stacks_file, "r", encoding="utf-8") as f:
+            data = json.load(f)
+        # minimal schema validation
+        if not isinstance(data, dict):
+            raise ValueError("stacks.json must be a JSON object")
+        for k, v in data.items():
+            if not isinstance(k, str) or not isinstance(v, list):
+                raise ValueError("stacks.json must map string -> list")
+        return data
+    except Exception as ex:
+        logger.warning("Failed to load stacks DB; starting empty: %s", ex)
+        return {}

 def _save_stacks(self):
     self.stacks_file.parent.mkdir(parents=True, exist_ok=True)
-    with open(self.stacks_file, "w") as f:
-        json.dump(self.stacks, f, indent=4)
+    try:
+        with open(self.stacks_file, "w", encoding="utf-8") as f:
+            json.dump(self.stacks, f, indent=4)
+    except Exception as ex:
+        logger.error("Failed to save stacks DB: %s", ex)
+        raise

Also applies to: 83-86

src/switchcraft/services/notification_service.py-8-70 (1)

8-70: Add synchronization + make timestamp serializable (threaded UI elsewhere makes this risky).
This service is very likely to be called across threads in this PR, but it has no locking; also datetime.now() is timezone-naive and not JSON-serializable.

Proposed fix

 import logging
 import uuid
-from datetime import datetime
+from datetime import datetime, timezone
+import threading
 from typing import List, Dict, Callable
@@
 class NotificationService:
     _instance = None
     _listeners: List[Callable] = []
@@
     def __new__(cls):
         if cls._instance is None:
             cls._instance = super(NotificationService, cls).__new__(cls)
             cls._instance.notifications = []
+            cls._instance._lock = threading.RLock()
         return cls._instance

-    def add_notification(self, title: str, message: str, type: str = "info"):
+    def add_notification(self, title: str, message: str, type: str = "info"):
@@
-        notif = {
+        notif = {
             "id": str(uuid.uuid4()),
             "title": title,
             "message": message,
             "type": type,
-            "timestamp": datetime.now(),
+            "timestamp": datetime.now(timezone.utc).isoformat(),
             "read": False
         }
-        self.notifications.insert(0, notif)
-        self._notify_listeners()
+        with self._lock:
+            self.notifications.insert(0, notif)
+        self._notify_listeners()
         return notif
@@
     def _notify_listeners(self):
-        for callback in self._listeners:
+        # Copy to avoid issues if listeners are added/removed during callbacks
+        for callback in list(self._listeners):
             try:
                 callback()
             except Exception as e:
                 logger.error(f"Error in notification listener: {e}")

src/switchcraft/services/community_db_service.py-39-46 (1)

39-46: Add error handling to _get_hash() to make hash lookup best-effort (graceful fallback on unreadable files).

The method can raise exceptions on file access errors, preventing the fallback to name-based lookup in analysis_controller.py. Return None on hash calculation failure so the caller can proceed to alternative strategies.

Proposed fix

 def _get_hash(self, filepath):
-    h = hashlib.sha256()
-    with open(filepath, "rb") as f:
-        while chunk := f.read(8192):
-            h.update(chunk)
-    return h.hexdigest()
+    try:
+        h = hashlib.sha256()
+        with open(filepath, "rb") as f:
+            while chunk := f.read(8192):
+                h.update(chunk)
+        return h.hexdigest()
+    except Exception as ex:
+        logger.debug("Failed to hash %s: %s", filepath, ex)
+        return None

 def get_switches_by_hash(self, file_path):
     """Calculate hash and lookup."""
     if not Path(file_path).exists():
         return None
 
     sha256 = self._get_hash(file_path)
+    if not sha256:
+        return None
     return self.db.get("hash_map", {}).get(sha256)

Also applies to: 56-61

src/switchcraft/gui_modern/views/script_upload_view.py-95-117 (1)

95-117: Refactor to use Flet's thread-safe UI update pattern.

Both upload flows (lines 95-117 and 187-212) directly mutate controls and call self.update() from a raw threading.Thread, which violates Flet's threading guidelines. Flet documentation explicitly recommends either:

1. Async tasks (preferred): Use async/await with page.run_task() for background I/O
2. Thread-safe execution: Use page.run_thread() and marshal UI updates back via page.run_task() to avoid concurrent page.update() calls

Calling update() directly from arbitrary threads can cause intermittent errors. Refactor both locations to use one of the documented patterns.

src/switchcraft/gui_modern/views/addon_manager_view.py-63-78 (1)

63-78: Use page.run_task() or page.run_thread() instead of plain threading.Thread() for UI updates.

Flet enforces a single-threaded async UI model—mutating control state (e.g., DataTable.rows) and calling update() from arbitrary worker threads is unsafe and can fail intermittently. Both _load_data() and _update_table() violate this constraint.

Move background work to a task scheduled via the page's event loop so that all control mutations and update() calls happen on the UI thread. Alternatively, collect results in the worker, then use page.run_task() to perform the mutation and update on the main context.

This pattern also appears in _install() (lines 109–118).

src/switchcraft/gui_modern/views/group_manager_view.py-95-108 (1)

95-108: Refactor to marshal UI updates from background thread to page event loop.

The code calls _update_table() and self.update() from a background thread spawned with threading.Thread(). Flet requires all UI mutations (including DataTable.rows modifications and Control.update() calls) to occur on the page event loop. Direct thread-based updates can cause race conditions and intermittent errors.

Use page.run_task() or page.run_thread() with proper result marshalling instead. Pattern: background work updates the data model; schedule the UI update on the page via page.run_task() so that _update_table() and update() execute in the page context.

Also applies to: 110-126

src/switchcraft/gui_modern/views/macos_wizard_view.py-147-181 (1)

147-181: Replace daemon thread with Flet's page.run_thread() and marshal UI updates back to the page event loop.

The code directly mutates control properties and calls self.update() from a background thread, which is unsafe in Flet. Flet's UI state is managed on the page event loop; arbitrary thread access causes intermittent errors and crashes.

Use page.run_thread() for the blocking Intune operation and schedule UI updates back to the page via page.run_task():

async def _upload_bg():
    try:
        token = await page.run_thread(
            self.intune_service.authenticate,
            tenant, client, secret
        )
        await page.run_thread(
            self.intune_service.upload_macos_shell_script,
            token,
            f"Install {self.app_name.value}",
            f"Auto-generated installer for {self.app_name.value}",
            self.preview_field.value
        )
        self.status_txt.value = "Success! Script Uploaded to MacOS > Shell Scripts."
        self.status_txt.color = ft.Colors.GREEN
    except Exception as ex:
        self.status_txt.value = f"Error: {ex}"
        self.status_txt.color = ft.Colors.RED
    finally:
        self.upload_btn.disabled = False
        self.update()

# In _upload_to_intune:
self.status_txt.value = "Uploading..."
self.upload_btn.disabled = True
self.update()
page.run_task(_upload_bg)

src/switchcraft/services/intune_service.py-605-629 (1)

605-629: Payload has undocumented field and is missing required property; fix before shipping.

The endpoint and @odata.type are correct per Microsoft Graph API, but the payload has issues:

• "isSupersedence": True is not a documented field in the Graph API spec and will likely be rejected or ignored.
• "targetType" field is missing; the API expects this to be set to "parent" or "child".

Remove isSupersedence and add "targetType": "parent" (or "child" depending on your relationship model).

Also improve exception handling by using raise instead of raise e to preserve the original traceback:

Fix traceback preservation

         except Exception as e:
             logger.error(f"Failed to add supersedence: {e}")
-            raise e
+            raise

src/switchcraft/services/intune_service.py-663-688 (1)

663-688: Major: Mutable default arg + payload configuration breaks M365 group creation.

1. Mutable default argument (group_types=[]): While the current code doesn't mutate it, this is a Python anti-pattern; use group_types=None with a default assignment instead.

2. API payload inconsistency for M365 groups: The docstring documents group_types=["Unified"] for M365 groups, but the code always sets mailEnabled=False and securityEnabled=True regardless of the group type. Per Microsoft Graph API requirements, M365 (Unified) groups require mailEnabled=true and securityEnabled=false. The current payload will fail or produce incorrect results when called with ["Unified"].

Fix: Conditionally set mailEnabled and securityEnabled based on whether "Unified" is in group_types:

Proposed fix

-    def create_group(self, token, name, description, group_types=[]):
+    def create_group(self, token, name, description, group_types=None):
         """
         Creates a new group.
         group_types: ["Unified"] for M365, [] for Security.
         """
+        group_types = group_types or []
+        is_m365 = "Unified" in group_types
+
         headers = {"Authorization": f"Bearer {token}", "Content-Type": "application/json"}
         url = "https://graph.microsoft.com/v1.0/groups"
 
         payload = {
             "displayName": name,
             "description": description,
-            "mailEnabled": False,
-            "securityEnabled": True,
+            "mailEnabled": True if is_m365 else False,
+            "securityEnabled": False if is_m365 else True,
             "mailNickname": name.replace(" ", "").lower(),
             "groupTypes": group_types
         }

src/switchcraft/gui_modern/views/detection_tester_view.py-236-247 (1)

236-247: Version comparison may behave unexpectedly with different segment counts.

List comparison treats [1, 0] < [1, 0, 0], so "1.0" >= "1.0.0.0" would return False. Consider normalizing version lists to equal length by padding with zeros.

💡 Suggested improvement

             def parse_ver(v_str):
-                return [int(x) for x in v_str.split('.')]
+                parts = [int(x) for x in v_str.split('.')]
+                # Normalize to 4 segments for consistent comparison
+                while len(parts) < 4:
+                    parts.append(0)
+                return parts

src/switchcraft/controllers/analysis_controller.py-137-140 (1)

137-140: Type mismatch: get_switches_by_name expects a filename string, not a Path.

Looking at CommunityDBService.get_switches_by_name(), it expects a filename string and uses Path(filename).stem.lower() internally. Passing a Path object works due to implicit string conversion, but get_switches_by_hash already handles the path, so passing path (a Path object) to get_switches_by_name will use the full path as the stem, not just the filename.

🐛 Proposed fix

                 db_switches = db.get_switches_by_hash(path)
                 if not db_switches:
                     # Fallback to name
-                    db_switches = db.get_switches_by_name(path)
+                    db_switches = db.get_switches_by_name(path.name)

src/switchcraft/gui_modern/views/library_view.py-36-45 (1)

36-45: Status filter dropdown is wired but filtering logic is disabled.

The filter_dd dropdown is visible and wired to _on_filter_change, which updates self.status_filter, but the actual filtering logic in _refresh_grid (lines 84-88) is commented out. This creates a confusing UX where the dropdown appears functional but does nothing.

Consider either implementing the status filtering or hiding the dropdown until it's ready.

Also applies to: 84-88

src/switchcraft/gui_modern/views/packaging_wizard_view.py-204-204 (1)

204-204: Typo: "Download form Web" should be "Download from Web".

📝 Proposed fix

-                ft.Text("Download form Web", size=24, weight=ft.FontWeight.BOLD),
+                ft.Text("Download from Web", size=24, weight=ft.FontWeight.BOLD),

src/switchcraft/gui_modern/views/packaging_wizard_view.py-510-515 (1)

510-515: Duplicate self.txt_desc in UI layout.

self.txt_desc is added twice to the Column controls (lines 513-514), causing the description field to render twice.

🧹 Proposed fix

             self.txt_app_name,
             self.txt_publisher,
             self.txt_desc,
-            self.txt_desc,
             ft.Divider(),

src/switchcraft/gui_modern/views/packaging_wizard_view.py-28-33 (1)

28-33: Remove duplicate assignments.

self.package_path (lines 28-29) and self.signing_cert (lines 31-32) are each assigned twice consecutively. These appear to be copy-paste errors.

🧹 Proposed fix

         self.generated_script_path = None
         self.package_path = None
-        self.package_path = None
         self.upload_info = {}
         self.signing_cert = SwitchCraftConfig.get_value("SigningCertThumbprint")
-        self.signing_cert = SwitchCraftConfig.get_value("SigningCertThumbprint")
         self.packaging_mode = "win32" # win32 or lob

src/switchcraft/gui_modern/views/packaging_wizard_view.py-609-612 (1)

609-612: Remove duplicate stub method definition.

_run_upload is defined twice. This stub definition (lines 609-611) is immediately shadowed by the full implementation below. Remove the stub to avoid confusion.

🧹 Proposed fix

-    def _run_upload(self, e):
-        # Override to include supersedence logic at the end
-        pass # Replaced by below logic via tool
-
     def _run_upload(self, e):

src/switchcraft/gui_modern/views/addon_manager_view.py-120-138 (1)

120-138: Handle “delete returned False” as a user-visible failure.
AddonService.delete_addon() can return False; the UI always shows success.

Proposed fix

 def delete(e):
     aid = self.selected_addon['id']
     def _bg():
         try:
-            self.addon_service.delete_addon(aid)
-            self._show_snack("Addon deleted.", ft.Colors.GREEN)
+            ok = self.addon_service.delete_addon(aid)
+            if not ok:
+                self._show_snack("Addon not found (nothing deleted).", ft.Colors.RED)
+                return
+            self._show_snack("Addon deleted.", ft.Colors.GREEN)
             self.selected_addon = None
             self.app_page.close_dialog()
             self._load_data()

src/switchcraft/gui_modern/views/stack_manager_view.py-124-139 (1)

124-139: Guard _select_stack() against stale/invalid names.
If the stack name is missing (e.g., race between delete/select or corrupted JSON), self.stacks[name] will throw.

Proposed fix

 def _select_stack(self, name):
     self.current_stack = name
-    items = self.stacks[name]
+    items = self.stacks.get(name)
+    if items is None:
+        self.current_stack = None
+        self.stack_content_list.controls.clear()
+        self._show_snack("Stack not found", ft.Colors.RED)
+        self.update()
+        return

src/switchcraft/services/intune_service.py-689-700 (1)

689-700: Change raise e to bare raise for cleaner exception re-raising. In Python 3, using bare raise in an except block re-raises the exception while preserving the original traceback. This is the preferred style over raise e.

Proposed fix

         except Exception as e:
             logger.error(f"Failed to delete group: {e}")
-            raise e
+            raise

README.md (1)

66-66: Consider using an absolute GitHub URL.

The relative link ../../issues may not render correctly in all contexts where the README is displayed (e.g., documentation sites, package registries, or mirrors).

🔗 Proposed fix

-1. Go to the [Issues](../../issues) tab.
+1. Go to the [Issues](https://github.com/FaserF/SwitchCraft/issues) tab.

src/switchcraft/gui_modern/controls/skeleton.py (1)

46-51: Consider using a single persistent thread instead of recursive spawning.

The current pattern spawns a new daemon thread every 0.8 seconds. While functional, this creates many short-lived threads over the component's lifetime. A single persistent loop thread would be more efficient.

♻️ Proposed refactor using a single thread

     def did_mount(self):
-        self._animate()
+        threading.Thread(target=self._animation_loop, daemon=True).start()

     def will_unmount(self):
         self.aborted = True

-    def _animate(self):
+    def _animation_loop(self):
+        while not self.aborted:
+            self._toggle_opacity()
+            time.sleep(0.8)
+
+    def _toggle_opacity(self):
         if self.aborted:
             return
-
-        # Simple pulsing opacity animation
         self.opacity = 0.5 if self.opacity == 1.0 else 1.0
         self.update()
-
-        # Schedule next frame
-        if not self.aborted:
-            # ... comments ...
-
-            def _loop():
-                time.sleep(0.8)
-                if not self.aborted:
-                    self._animate()
-
-            threading.Thread(target=_loop, daemon=True).start()

tests/test_modern_gui.py (1)

42-61: The dynamic import approach is cleaner; consider improving error handling.

The refactoring to use importlib is a good improvement for maintainability. However, importlib.import_module() raises ImportError on failure rather than returning None, so the assertion assert module is not None provides no additional value.

♻️ Suggested improvement with better error reporting

 def test_view_imports():
     """Ensure all view modules can be imported."""
     view_names = [
         "home_view",
         "analyzer_view",
         "helper_view",
         "winget_view",
         "intune_view",
         "intune_store_view",
         "history_view",
         "settings_view",
         "packaging_wizard_view",
         "detection_tester_view",
         "stack_manager_view",
         "dashboard_view",
         "library_view",
     ]
     for view_name in view_names:
-        module = importlib.import_module(f"switchcraft.gui_modern.views.{view_name}")
-        assert module is not None
+        try:
+            module = importlib.import_module(f"switchcraft.gui_modern.views.{view_name}")
+        except ImportError as e:
+            pytest.fail(f"Failed to import {view_name}: {e}")

src/switchcraft/controllers/analysis_controller.py (2)

142-151: Unfinished merge logic leaves potential enhancements undocumented.

The else branch contains comments about potential merge strategies but does nothing. Consider either:

1. Implementing a merge/comparison strategy
2. Removing the else block entirely
3. Adding a TODO comment or logging that alternatives were found

♻️ Option: Log when community alternatives exist

                 if db_switches:
                     if not info.install_switches:
                         info.install_switches = db_switches
                         community_match = True
                     else:
-                        # Merge? Or just flag that we found alternatives?
-                        # For now, let's append if completely different?
-                        # Simpler: If analyzer found nothing, use DB.
-                        # If analyzer found something, maybe trust analyzer?
-                        pass
+                        # Analyzer found switches; log community alternatives for reference
+                        logger.debug(f"Community DB has alternative switches: {db_switches}")

---

131-135: Consider caching CommunityDBService instance.

CommunityDBService is instantiated on every analysis call. If this involves file I/O (loading JSON), it could be cached as an instance variable in __init__ or as a module-level singleton.

♻️ Proposed refactor

 class AnalysisController:
     """
     Shared controller for handling the analysis workflow.
     Used by both Classic (Tkinter) and Modern (Flet) UIs.
     """

     def __init__(self, ai_service=None):
         self.ai_service = ai_service
+        self._community_db = CommunityDBService()

Then in analyze_file:

-            db = CommunityDBService()
+            db = self._community_db

scripts/process_issue_ops.py (1)

63-64: Use actual timestamp and add contributor field.

The added_at field uses a placeholder string. Additionally, the example entry in switches.json includes a contributor field that this script doesn't populate.

♻️ Proposed fix

+from datetime import datetime, timezone
+
 def update_db(new_entry):
     # ... existing code ...

     # Add metadata
-    new_entry["added_at"] = "pending-merge" # In real action, could use current time
+    new_entry["added_at"] = datetime.now(timezone.utc).isoformat()
+    new_entry["contributor"] = os.environ.get("GITHUB_ACTOR", "unknown")

In the workflow, ensure GITHUB_ACTOR is available (it's set by default in GitHub Actions).

src/switchcraft/gui_modern/views/dashboard_view.py (1)

1-29: LGTM - Clean dashboard view initialization.

The class structure and mock data setup are appropriate for a prototype dashboard. The layout composition with stats row and a two-column chart/activity section is well-organized.

Note: logger is imported (line 4) but not used anywhere in this file.

.github/ISSUE_TEMPLATE/add_switch.yml (1)

1-76: Issue template is well-structured.

The template covers all necessary fields for switch submissions and aligns well with the automation workflow that processes these issues. The dropdown for installer types provides good coverage of common frameworks.

Minor observation: The notes textarea (lines 72-76) is missing an explicit validations block. While it defaults to required: false, adding it explicitly would maintain consistency with other fields.

Optional: Add explicit validation for notes field

   - type: textarea
     id: notes
     attributes:
       label: Additional Notes
       description: Any other details or caveats?
+    validations:
+      required: false

src/switchcraft/gui_modern/views/library_view.py (2)

103-106: Avoid bare except: clause.

Using a bare except: catches all exceptions including KeyboardInterrupt and SystemExit, which can hide bugs and make debugging difficult. Catch specific exceptions instead.

Suggested fix

         try:
             dt = datetime.fromisoformat(ts).strftime("%Y-%m-%d")
-        except:
+        except (ValueError, TypeError):
             dt = ts

---

119-124: Complex inline lambda with side effects is hard to read.

The hover handler uses a chained setattr(...) or e.control.update() pattern which relies on setattr returning None to execute update(). While functional, this is non-idiomatic and harder to maintain.

Suggested refactor

-            on_hover=lambda e: setattr(e.control, "bgcolor", ft.Colors.with_opacity(0.2, ft.Colors.WHITE) if e.data == "true" else ft.Colors.with_opacity(0.1, ft.Colors.WHITE)) or e.control.update()
+            on_hover=self._on_tile_hover
         )
+
+    def _on_tile_hover(self, e):
+        e.control.bgcolor = (
+            ft.Colors.with_opacity(0.2, ft.Colors.WHITE)
+            if e.data == "true"
+            else ft.Colors.with_opacity(0.1, ft.Colors.WHITE)
+        )
+        e.control.update()

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

92-93: Duplicate import of time module.

The time module is already imported at line 10. This redundant import should be removed.

Suggested fix

         # Perform deferred UI building.
         # User requested visible loading screen. Since Flet execution here is linear during init,
         # we add a small sleep to ensure the "Loading..." ring is perceived.
-        import time
         time.sleep(1.5)

---

511-514: Lambda captures drawer variable correctly but clear_all() and close() in list may have unintended behavior.

The expression lambda _: [self.notification_service.clear_all(), self.page.close(drawer)] uses a list to execute multiple statements. While this works, it's unconventional and the return value (a list) is discarded. Consider using a proper handler method.

Suggested refactor

+        def clear_and_close(_):
+            self.notification_service.clear_all()
+            self.page.close(drawer)
+
         drawer = ft.NavigationDrawer(
             controls=[
                 ft.Container(height=12),
                 ft.Row([
                     ft.Text("Notifications", size=20, weight=ft.FontWeight.BOLD),
-                    ft.TextButton("Clear All", on_click=lambda _: [self.notification_service.clear_all(), self.page.close(drawer)])
+                    ft.TextButton("Clear All", on_click=clear_and_close)
                 ], alignment=ft.MainAxisAlignment.SPACE_BETWEEN, run_spacing=10),

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

57-57: Bare except: pass silently swallows all errors.

Lines 57 and 134 use bare except: pass which hides all exceptions including critical ones. This makes debugging difficult and can mask serious issues.

Suggested fix for line 57

                  try:
                      with open(d / "manifest.json") as f:
                          data = json.load(f)
                          if data.get("id") == addon_id:
                              addon_path = d
                              manifest_data = data
                              break
-                 except: pass
+                 except (OSError, json.JSONDecodeError) as e:
+                     logger.warning(f"Skipping invalid addon at {d}: {e}")

Also applies to: 134-134

src/switchcraft/gui_modern/views/packaging_wizard_view.py (1)

270-271: Add timeout to prevent indefinite hangs.

requests.get() has no timeout, which can block the thread indefinitely if the server doesn't respond.

♻️ Proposed fix

-                with requests.get(url, stream=True) as r:
+                with requests.get(url, stream=True, timeout=60) as r:

src/switchcraft/services/intune_service.py (3)

404-433: Make script base64 encoding robust to bytes input + validate run_as_account values.
If UI passes bytes (or already-base64), .encode('utf-8') will throw / double-encode.

Proposed hardening

-        # Script content must be base64 encoded
-        encoded_script = base64.b64encode(script_content.encode('utf-8')).decode('utf-8')
+        # Script content must be base64 encoded (Graph expects base64 string)
+        if isinstance(script_content, bytes):
+            raw = script_content
+        else:
+            raw = str(script_content).encode("utf-8")
+        encoded_script = base64.b64encode(raw).decode("ascii")
+
+        if run_as_account not in {"system", "user"}:
+            raise ValueError("run_as_account must be 'system' or 'user'")

---

434-466: Same base64 robustness needed for remediation scripts; avoid raise e.
Prefer raise to preserve original traceback context.

Proposed tweak

-        enc_detection = base64.b64encode(detection_content.encode('utf-8')).decode('utf-8')
-        enc_remediation = base64.b64encode(remediation_content.encode('utf-8')).decode('utf-8')
+        det_raw = detection_content if isinstance(detection_content, bytes) else str(detection_content).encode("utf-8")
+        rem_raw = remediation_content if isinstance(remediation_content, bytes) else str(remediation_content).encode("utf-8")
+        enc_detection = base64.b64encode(det_raw).decode("ascii")
+        enc_remediation = base64.b64encode(rem_raw).decode("ascii")
@@
-        except Exception as e:
+        except Exception as e:
             # Check for license errors common with Remediations
             logger.error(f"Failed to upload Remediation: {e}")
-            raise e
+            raise

---

649-662: list_groups: build filters via params and handle pagination.
String-concatenating $filter is brittle (escaping) and Graph often paginates with @odata.nextLink.

Configuration used: defaults

Review profile: CHILL

Plan: Pro