More bug fixes for modern UI - #35

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

114-128: Use label instead of text for ft.Tab parameter.

This issue was previously identified and causes the CI pipeline failure. The ft.Tab component uses label as the parameter name, not text.

Proposed fix

         source_tabs = ft.Tabs(
             selected_index=0,
             animation_duration=300,
             on_change=on_source_change,
             tabs=[
                 ft.Tab(
-                    text=i18n.get("download_url") or "Download URL",
+                    label=i18n.get("download_url") or "Download URL",
                     icon=ft.Icons.LINK
                 ),
                 ft.Tab(
-                    text=i18n.get("local_file") or "Local File",
+                    label=i18n.get("local_file") or "Local File",
                     icon=ft.Icons.COMPUTER
                 )
             ]
         )

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

61-80: Replace text with label parameter in ft.Tab() constructor.

This issue was previously flagged: Flet 0.80.1+ uses label instead of text for the Tab display name. The current code will cause: Tab.__init__() got an unexpected keyword argument 'text'.

🐛 Proposed fix

         t = ft.Tabs(
             selected_index=0,
             animation_duration=300,
             expand=True,
             on_change=on_change,
             tabs=[
                 ft.Tab(
-                    text=i18n.get("tab_platform_scripts") or "Platform Scripts",
+                    label=i18n.get("tab_platform_scripts") or "Platform Scripts",
                     icon=ft.Icons.TERMINAL
                 ),
                 ft.Tab(
-                    text=i18n.get("tab_remediations") or "Remediations",
+                    label=i18n.get("tab_remediations") or "Remediations",
                     icon=ft.Icons.HEALING
                 ),
                 ft.Tab(
-                    text=i18n.get("tab_github_import") or "GitHub Import",
+                    label=i18n.get("tab_github_import") or "GitHub Import",
                     icon=ft.Icons.CODE
                 )
             ]
         )

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

286-292: os.startfile is Windows-only and will raise AttributeError on macOS/Linux.

This issue was previously flagged. The code will crash on non-Windows platforms.

🐛 Proposed cross-platform fix

     def _open_folder(self, path):
         """Open the folder containing the file."""
         try:
+            import sys
+            import subprocess
             folder = os.path.dirname(path)
-            os.startfile(folder)
+            if sys.platform == 'win32':
+                os.startfile(folder)
+            elif sys.platform == 'darwin':
+                subprocess.run(['open', folder])
+            else:
+                subprocess.run(['xdg-open', folder])
         except Exception as ex:
             logger.error(f"Failed to open folder: {ex}")

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

496-505: Windows-only subprocess.STARTUPINFO lacks platform guard.

This issue was previously flagged. subprocess.STARTUPINFO() and STARTF_USESHOWWINDOW are Windows-only and will raise AttributeError on macOS/Linux.

The same issue exists at lines 585-586 (_update_manifest) and lines 635-636 (_validate_manifest).

🛠️ Suggested fix with platform guard

+                import sys
+                if sys.platform == "win32":
+                    startupinfo = subprocess.STARTUPINFO()
+                    startupinfo.dwFlags |= subprocess.STARTF_USESHOWWINDOW
+                else:
+                    startupinfo = None
-                startupinfo = subprocess.STARTUPINFO()
-                startupinfo.dwFlags |= subprocess.STARTF_USESHOWWINDOW

                 result = subprocess.run(
                     cmd,
                     capture_output=True,
                     text=True,
                     startupinfo=startupinfo,
                     timeout=120
                 )

Apply this pattern to all three locations: _generate_new_manifest, _update_manifest, and _validate_manifest.

---

664-669: os.startfile is Windows-only.

This will raise AttributeError on macOS/Linux. Apply the same cross-platform fix pattern suggested for library_view.py.

🐛 Proposed cross-platform fix

     def _open_manifest_dir(self, e):
         """Open the manifest directory in file explorer."""
         try:
-            os.startfile(str(self.manifest_dir))
+            import sys
+            import subprocess as sp
+            if sys.platform == 'win32':
+                os.startfile(str(self.manifest_dir))
+            elif sys.platform == 'darwin':
+                sp.run(['open', str(self.manifest_dir)])
+            else:
+                sp.run(['xdg-open', str(self.manifest_dir)])
         except Exception as ex:
             logger.error(f"Failed to open manifest dir: {ex}")

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

170-188: Tab construction uses non-standard nested structure.

The ft.Tabs construction here uses content=ft.TabBar(tabs=[...], on_change=...) with a length parameter, which is an unusual pattern. Additionally, a past review flagged that label= should be used instead of text= for ft.Tab. The current code uses label= which is correct, but the overall Tabs structure with nested TabBar may cause issues.

Flet Tabs component API ft.Tabs with TabBar content parameter

src/switchcraft/gui_modern/views/intune_view.py (3)

36-53: Pipeline failure: Tab.__init__() got unexpected keyword argument text.

The past review flagged this issue and it appears to still be present. The ft.Tab constructor in recent Flet versions doesn't accept text as a keyword argument - use positional argument or label= instead.

🐛 Proposed fix

         self.tabs = ft.Tabs(
             selected_index=0,
             animation_duration=300,
             tabs=[
                 ft.Tab(
-                    text=i18n.get("tab_packager") or "Packager",
+                    i18n.get("tab_packager") or "Packager",
                     icon=ft.Icons.INVENTORY_2,
                     content=self._build_packager_tab()
                 ),
                 ft.Tab(
-                    text=i18n.get("tab_uploader") or "Uploader & Update",
+                    i18n.get("tab_uploader") or "Uploader & Update",
                     icon=ft.Icons.CLOUD_UPLOAD,
                     content=self._build_uploader_tab()
                 ),
             ],
             expand=True
         )

---

330-334: subprocess.run with shell=True poses command injection risk.

This issue was flagged in a past review and is still present. If output_file contains shell metacharacters, this could lead to command injection.

🔒️ Proposed secure fix

                 def open_folder(e):
                     import subprocess
-                    if os.name == 'nt': subprocess.run(f'explorer /select,"{output_file}"', shell=True)
+                    if os.name == 'nt':
+                        subprocess.run(['explorer', f'/select,{output_file}'])
                     dlg.open = False
                     self.app_page.update()

---

413-418: Bare except: pass swallows all exceptions silently.

This issue was flagged in a past review. At minimum, log the exception for debugging purposes.

♻️ Proposed fix

     def _show_snack(self, msg, color="GREEN"):
         try:
             self.app_page.snack_bar = ft.SnackBar(ft.Text(msg), bgcolor=color)
             self.app_page.snack_bar.open = True
             self.app_page.update()
-        except: pass
+        except Exception as ex:
+            logger.debug(f"Failed to show snack: {ex}")

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

709-715: os.execv restart pattern will fail on Windows with PyInstaller builds.

This issue was flagged in a past review. The os.execv approach doesn't work properly on Windows, especially with PyInstaller. The codebase has a working pattern in crash_view.py that uses subprocess.Popen instead.

🔒️ Proposed fix using subprocess pattern

         def do_restart(e):
             dlg.open = False
             self.app_page.update()
-            # Request restart
             import sys
-            import os
-            os.execv(sys.executable, [sys.executable] + sys.argv)
+            import subprocess
+            
+            try:
+                if getattr(sys, 'frozen', False):
+                    # PyInstaller build
+                    subprocess.Popen([sys.executable] + sys.argv[1:])
+                else:
+                    subprocess.Popen([sys.executable] + sys.argv)
+                
+                # Close current instance
+                if hasattr(self.app_page, 'window'):
+                    self.app_page.window.destroy()
+                else:
+                    sys.exit(0)
+            except Exception as ex:
+                self._show_snack(f"Restart failed: {ex}", "RED")

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

455-476: Potential race condition: UI updates from background thread without safety checks.

This issue was flagged in a past review. The _base_install background thread directly modifies dialog content (content.controls, dlg.actions) and calls dlg.update(). If the user closes the dialog before the thread completes, this could cause errors.

♻️ Suggested improvement

             def _base_install():
                 # Install Advanced
                 success, msg = self.addon_service.install_from_github("advanced")
 
-                # UI Update needs to happen on loop? Flet is thread-safe for simple updates usually
-                if success:
+                try:
+                    if not dlg.open:
+                        return  # Dialog was closed
+                    if success:
                          # Update Dialog to ask for Optional
                          content.controls.clear()
                          content.controls.append(ft.Icon(ft.Icons.CHECK_CIRCLE, color="GREEN", size=48))
                          # ... rest of success handling
-                else:
+                    else:
                          content.controls.append(ft.Text(f"Failed to install base: {msg}", color="RED"))
                          dlg.actions.clear()
                          dlg.actions.append(ft.TextButton("Close", on_click=close_wizard))
                          dlg.update()
+                except Exception as ex:
+                    logger.warning(f"Could not update first-run dialog: {ex}")

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

266-271: Consider sanitizing name to prevent shell script issues.

The app name is directly interpolated into the shell script. If a user enters a name containing shell metacharacters (e.g., "; rm -rf /;), it could produce malformed or dangerous scripts. While this is an internal tool, sanitizing input improves robustness.

Suggested sanitization

# Before using in script template
import re
safe_name = re.sub(r'[^a-zA-Z0-9 _.-]', '', name)

src/switchcraft/modern_main.py (2)

261-279: Sub-action parameter from protocol URL is not utilized.

The parse_protocol_url function returns a sub key for URLs like switchcraft://settings/updates, but the action handling only checks the top-level action. For example, switchcraft://settings/updates would navigate to settings (not updates) since _INITIAL_ACTION.get("sub") is never used.

Consider whether this is intentional or if sub-action routing should be added:

♻️ Suggested enhancement for sub-action handling

         if _INITIAL_ACTION:
             action = _INITIAL_ACTION.get("action", "home")
+            sub_action = _INITIAL_ACTION.get("sub")
             from switchcraft.gui_modern.nav_constants import NavIndex

             action_map = {
                 "notifications": lambda: app._toggle_notification_drawer(None),
                 "settings": lambda: app.goto_tab(NavIndex.SETTINGS),
                 "updates": lambda: app.goto_tab(NavIndex.SETTINGS_UPDATES),
                 "analyzer": lambda: app.goto_tab(NavIndex.ANALYZER),
                 "home": lambda: app.goto_tab(NavIndex.HOME),
             }

+            # Handle settings sub-actions
+            if action == "settings" and sub_action:
+                sub_map = {
+                    "updates": lambda: app.goto_tab(NavIndex.SETTINGS_UPDATES),
+                }
+                handler = sub_map.get(sub_action, action_map.get(action))
+            else:
+                handler = action_map.get(action)
-            handler = action_map.get(action)
             if handler:

---

267-267: Add a public API method for notification drawer to match other action handlers.

The _toggle_notification_drawer method is private, while other similar protocol actions use the public app.goto_tab(NavIndex.XXX) API (lines 268–271). Consider creating a public method (e.g., open_notifications()) to maintain consistency and avoid coupling to internal implementation details.

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

290-296: PowerShell script execution lacks timeout.

The subprocess.run call has no timeout parameter. A malicious or poorly written script could hang indefinitely, blocking the UI thread callback.

Proposed fix

-            res = subprocess.run(cmd, capture_output=True, text=True, startupinfo=startupinfo)
+            res = subprocess.run(cmd, capture_output=True, text=True, startupinfo=startupinfo, timeout=60)

You may also want to catch subprocess.TimeoutExpired in the exception handler.

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

131-155: Consider using i18n's built-in format support.

The error handling improvements are valuable—differentiating permission (403) and authentication (401) errors provides better UX. However, lines 136 and 148 use manual str.replace("{permissions}", ...) instead of leveraging i18n.get()'s native **kwargs formatting support (as shown in the relevant snippet from i18n.py).

Proposed improvement

-                error_msg = i18n.get("graph_permission_error") or "Missing Graph API permissions: {permissions}"
-                error_msg = error_msg.replace("{permissions}", missing_perms)
+                error_msg = i18n.get("graph_permission_error", permissions=missing_perms) or f"Missing Graph API permissions: {missing_perms}"

This approach is cleaner and consistent with the i18n module's design.

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

292-335: Deployment is a placeholder - consider adding a TODO or logging.

The _execute_deploy method only shows a snack message but doesn't perform actual deployment. While this is acceptable for scaffolding, consider adding a # TODO comment or logging to indicate this needs implementation.

Would you like me to open an issue to track the implementation of the actual deployment logic?

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

508-518: Placeholder implementations are acceptable for scaffolding.

These placeholder methods correctly show feedback to users. Consider adding TODO comments to track the implementation.

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

240-245: Pluralization uses positional format but i18n.get supports kwargs.

The format string uses positional {0} but i18n.get supports named kwargs for formatting. This works, but consider using .format(count=count) with a {count} placeholder for better readability and consistency with the i18n API.

♻️ Suggested improvement

-        self.results_count.value = (i18n.get("apps_found") or "Found {0} apps").format(count) if count != 1 else (i18n.get("app_found") or "Found 1 app")
+        self.results_count.value = (i18n.get("apps_found", count=count) or f"Found {count} apps") if count != 1 else (i18n.get("app_found") or "Found 1 app")

---

576-612: Admin elevation flow duplicates pattern from analyzer_view.py - consider extracting to shared utility.

The admin check and elevation logic (IsUserAnAdmin, ShellExecuteW with runas, sys.exit) is nearly identical to the pattern in analyzer_view.py (lines 803-837). This duplication increases maintenance burden.

Additionally, ctypes is imported inside the function but used later at line 598 without re-importing after the except block scope ends - this works because ctypes is already imported at module level in some views but not explicitly here.

♻️ Suggested improvement - extract to utility

Consider creating a shared utility function:

# In a new file like utils/admin_helper.py
def check_admin() -> bool:
    """Check if running with admin privileges."""
    try:
        import ctypes
        return ctypes.windll.shell32.IsUserAnAdmin() != 0
    except Exception:
        return False

def restart_as_admin() -> bool:
    """Restart the application with admin privileges. Returns False on failure."""
    try:
        import ctypes
        import sys
        executable = sys.executable
        params = f'"{sys.argv[0]}"'
        if len(sys.argv) > 1:
            params += " " + " ".join(f'"{a}"' for a in sys.argv[1:])
        ctypes.windll.shell32.ShellExecuteW(None, "runas", executable, params, None, 1)
        sys.exit(0)
    except Exception:
        return False

src/switchcraft/gui_modern/views/analyzer_view.py (3)

219-275: URL download lacks SSL certificate verification control and has potential security considerations.

The URL download implementation:

1. Accepts any HTTP/HTTPS URL without domain validation
2. Downloads executable files directly
3. The filename derivation from URL (line 243) could be exploited with crafted URLs

Consider adding warnings for HTTP (non-HTTPS) URLs and potentially limiting to known safe domains or showing a security warning.

🔒️ Suggested security improvements

         # Validate URL
         if not url.startswith(("http://", "https://")):
             self._show_snack("Invalid URL format", "RED")
             return
+        
+        # Warn about non-HTTPS
+        if url.startswith("http://") and not url.startswith("https://"):
+            self._show_snack(
+                i18n.get("warning_http_insecure") or "Warning: HTTP is insecure. Consider using HTTPS.",
+                "ORANGE"
+            )

---

1006-1013: Debug print statements should be removed or converted to logger calls.

The print(f"DEBUG: ...") statements at lines 1007 and 1013 should use the existing logger instead. Debug prints can clutter stdout and are not controllable via logging configuration.

♻️ Proposed fix

     def _show_snack(self, msg, color="GREEN"):
-        print(f"DEBUG: Showing Snack: {msg}")
+        logger.debug(f"Showing Snack: {msg}")
         try:
             self.app_page.snack_bar = ft.SnackBar(ft.Text(msg), bgcolor=color)
             self.app_page.snack_bar.open = True
             self.app_page.update()
         except Exception as e:
-             print(f"DEBUG: Failed to show snack: {e}")
+             logger.debug(f"Failed to show snack: {e}")

---

803-837: Admin elevation flow is duplicated from winget_view.py.

This is the same admin check and elevation pattern as in winget_view.py (lines 576-612). Consider extracting to a shared utility to reduce duplication and ensure consistent behavior across views.

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

5-5: Unused import: time is imported but never used.

The time module is imported at line 5 but there's no usage of it in the file.

♻️ Proposed fix

-import time

---

420-422: Empty method _open_explorer_select should be removed or implemented.

The method body is just pass with a comment that it's "kept for methods that might call it". If it's not being called, it should be removed. If it is being called, it should be implemented.

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

1065-1113: Windows toast notification logic is comprehensive but complex.

The toast notification handling is well-implemented with:

• Proper gating on WINOTIFY_AVAILABLE
• Deduplication via _last_notif_id
• Different actions for update vs. regular notifications
• Appropriate audio settings

However, the nested conditionals (lines 1091-1106) add complexity. Consider extracting toast creation to a helper method.

---

1007-1009: Dynamic addon index calculation uses magic number 21.

The calculation dynamic_idx = idx - 21 uses a hardcoded value. If NavIndex is updated or new navigation items are added, this offset could become incorrect. Consider deriving this from NavIndex constants.

♻️ Suggested improvement

         else:
-            # Dynamic Addons (start at idx 21)
-            dynamic_idx = idx - 21
+            # Dynamic Addons (start after last NavIndex constant)
+            DYNAMIC_ADDON_START = 21  # Should match NavIndex.DYNAMIC_ADDONS_START or similar
+            dynamic_idx = idx - DYNAMIC_ADDON_START

Or better, add a constant to nav_constants.py:

class NavIndex:
    # ... existing constants ...
    DYNAMIC_ADDONS_START = 21

tests/test_navigation_map.py (4)

43-50: Consider removing or condensing these design notes.

These inline comments explain the rationale for patching in the test rather than the fixture. While useful during development, they add noise. Consider moving to a brief docstring or removing entirely since the test implementation already demonstrates the correct approach.

---

140-146: Dead code: tuple handling branch is unreachable.

The expected_views dictionary contains only class types, never tuples. This isinstance(expected_type, tuple) branch will never execute.

♻️ Remove dead code

                 expected_type = expected_views.get(idx)
 
-                if isinstance(expected_type, tuple):
-                     # Handle placeholders (Type, Value)
-                     t, val = expected_type
-                     assert isinstance(view_instance, t), f"Index {idx} expected {t}, got {type(view_instance)}"
-                     if isinstance(view_instance, ft.Text):
-                          # Allow "Unknown Tab" if expected?
-                          pass
-                else:
-                     # Real View
+                # Real View

---

116-118: Consider using pytest -v or capsys instead of print statements.

Print statements work but can clutter test output. Use pytest's verbose mode (-v) or capsys fixture for cleaner debugging output that integrates with pytest's reporting.

---

160-163: Consider using elif or a dictionary for tab index assertions.

Multiple independent if statements all execute even when only one can match. Using elif or a mapping would be cleaner.

♻️ Cleaner pattern with dictionary lookup

settings_tab_map = {2: 1, 3: 2, 4: 3, 13: 0}
if idx in settings_tab_map:
    expected_tab = settings_tab_map[idx]
    assert view_instance.initial_tab_index == expected_tab, f"Index {idx} should be Settings Tab {expected_tab}"

tests/test_ui_interactions.py (6)

20-28: Remove duplicate page.open assignment.

page.open is assigned twice (lines 20 and 28). The second assignment overwrites the first.

♻️ Remove redundant line

     page.open = MagicMock() # Fix for Flet 0.21+ dialogs
 
     # Mock window object
     page.window = MagicMock()
     page.theme_mode = ft.ThemeMode.DARK
     page.padding = 10
-
-    # Mock open for dialogs (newer Flet API)
-    page.open = MagicMock()
     return page

---

51-58: Remove or implement the TabBar handling block.

This code block iterates over tabs but does nothing (pass). Either remove the dead code or implement actual tab content traversal if needed.

♻️ Remove dead code

     elif hasattr(control, "content") and control.content:
         buttons.extend(find_buttons(control.content))
 
-    # Handle TabBar tabs
-    if isinstance(control, ft.TabBar) and control.tabs:
-        for tab in control.tabs:
-             # Tabs usually have content if they are TabBarView, but Tab control has content/label
-             # We just traverse Tab children? Tab doesn't have children usually unless it stores content
-             # Wait, Tab has 'content' or 'icon' etc.
-             pass
-
     return buttons

---

64-71: File-based logging is non-standard for pytest.

Writing to test_result.log creates side effects and may cause issues in CI/parallel test runs. Consider using pytest's capfd fixture or Python's logging module configured with pytest's caplog.

♻️ Alternative using capsys

def test_settings_view_buttons(mock_page, capsys):
    """Test all buttons in SettingsView."""
    
    def log(msg):
        print(msg)  # Will be captured by capsys
    
    # ... rest of test ...
    
    # At end, if needed:
    captured = capsys.readouterr()
    # captured.out contains all printed output

---

269-270: Comment contradicts actual behavior.

The comment says "Don't fail immediately, try others?" but the code immediately calls pytest.fail. Remove the misleading comment.

♻️ Remove contradictory comment

                 except Exception as ex:
                     log(f"CRITICAL: Button '{label}' crashed: {ex}")
                     import traceback
                     log(traceback.format_exc())
-                    # Don't fail immediately, try others? No, fail is better to fix one by one.
                     pytest.fail(f"Button '{label}' crashed: {ex}")

---

290-292: Consider importing LibraryView at module level.

Importing inside the test function works but is non-standard. If this is intentional to ensure patches are applied before import, document the reason.

---

408-435: Consider consolidating with module-level find_buttons.

This local helper is similar to the module-level find_buttons but adds TextField support. Consider extending find_buttons with a parameter for additional control types, or create a second module-level helper.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

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

290-303: LGTM - Cross-platform folder opening implemented.

The _open_folder method now correctly handles Windows (os.startfile), macOS (open), and Linux (xdg-open), addressing the previous platform compatibility concern.

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

709-740: LGTM - Subprocess-based restart for PyInstaller compatibility.

The restart logic now correctly uses subprocess.Popen instead of os.execv, detects PyInstaller frozen state with getattr(sys, 'frozen', False), and uses os._exit(0) to avoid cleanup issues. This addresses the previous concern about PyInstaller builds.

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

244-285: Temp directory not cleaned up on download failure.

If the download fails at lines 274-285, the temporary directory created at line 245 is never cleaned up. The cleanup_path is only passed to start_analysis on success (line 272), leaving orphaned temp directories on failure.

🐛 Proposed fix

             except requests.exceptions.RequestException as ex:
                 self.url_download_progress.visible = False
                 self.url_download_status.value = f"Download failed: {ex}"
                 self.url_download_status.color = "RED"
                 self.update()
                 logger.error(f"URL download failed: {ex}")
+                # Cleanup temp directory on failure
+                try:
+                    shutil.rmtree(temp_dir, ignore_errors=True)
+                except Exception:
+                    pass
             except Exception as ex:
                 self.url_download_progress.visible = False
                 self.url_download_status.value = f"Error: {ex}"
                 self.url_download_status.color = "RED"
                 self.update()
                 logger.error(f"URL download error: {ex}")
+                # Cleanup temp directory on failure
+                try:
+                    shutil.rmtree(temp_dir, ignore_errors=True)
+                except Exception:
+                    pass

tests/test_ui_interactions.py (2)

150-155: Patch location is incorrect for HistoryService.

HistoryService is imported at module level in dashboard_view.py, so patching at the source module won't affect the already-imported reference. Patch where the class is used instead.

🛠️ Fix: patch at the usage location

-    with patch("switchcraft.services.history_service.HistoryService") as MockHistoryService:
+    with patch("switchcraft.gui_modern.views.dashboard_view.HistoryService") as MockHistoryService:

---

481-484: Missing assertion for on_change handler check.

The test logs "CRITICAL" when on_change is missing but doesn't fail the test. This allows silent test passes when the handler is absent.

🛠️ Add assertion

              if tenant_field.on_change:
                  log("SUCCESS: 'Entra Tenant ID' has on_change handler.")
              else:
                  log("CRITICAL: 'Entra Tenant ID' missing on_change handler!")
+                 pytest.fail("'Entra Tenant ID' missing on_change handler")

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

427-433: Exception handling improved but still swallows errors silently.

The change from bare except: to except Exception: is an improvement, but the exception is still being silently ignored. Per the previous review suggestion, consider logging the exception for debugging purposes.

♻️ Add logging for failed snack display

     def _show_snack(self, msg, color="GREEN"):
         try:
             self.app_page.snack_bar = ft.SnackBar(ft.Text(msg), bgcolor=color)
             self.app_page.snack_bar.open = True
             self.app_page.update()
-        except Exception:
-            pass
+        except Exception as ex:
+            logger.debug(f"Failed to show snack: {ex}")

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

63-70: Replace text with label parameter in ft.Tab() constructor.

This issue was previously identified. Flet 0.80.1+ uses label instead of text for Tab display names.

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

500-507: Consider tracking checkbox selection state for future implementation.

The checkboxes in the script list don't have an on_change handler or data association to identify which scripts are selected. When implementing _import_github_scripts and _deploy_github_scripts, you'll need a way to track selections.

Example approach for tracking selections

# Store selected scripts
self.selected_scripts = set()

# In the loop:
def on_script_select(script_path):
    def handler(e):
        if e.control.value:
            self.selected_scripts.add(script_path)
        else:
            self.selected_scripts.discard(script_path)
    return handler

for script_path in ps_files[:50]:
    cb = ft.Checkbox(value=False, on_change=on_script_select(script_path))
    self.github_script_list.controls.append(
        ft.ListTile(
            leading=cb,
            title=ft.Text(script_path),
            trailing=ft.Icon(ft.Icons.DESCRIPTION, color="BLUE_400")
        )
    )

---

440-448: URL parsing could be more robust for edge cases.

The current parsing handles basic GitHub URLs but may fail on variations like:

• github.com/owner/repo (missing protocol)
• https://github.com/owner/repo.git (with .git suffix)
• [email protected]:owner/repo.git (SSH format)

More robust URL parsing

import re

# Parse GitHub URL - handles various formats
match = re.match(
    r'(?:https?://)?(?:www\.)?github\.com[/:]([^/]+)/([^/.]+)(?:\.git)?/?',
    repo_url
)
if not match:
    raise ValueError(i18n.get("invalid_github_url") or "Invalid GitHub URL")
owner, repo = match.groups()

src/switchcraft/gui_modern/utils/flet_compat.py (1)

14-17: Consider logging the initial TypeError for debugging.

When the standard approach fails and fallback succeeds, the original error is silently discarded. If the fallback later exhibits issues, it would be helpful to know the initial failure reason.

Suggested improvement

     try:
         # Standard Flet
         return ft.Tabs(tabs=tabs, **kwargs)
     except TypeError:
+        logger.debug("Standard Tabs creation failed, trying fallback")
         # Fallback for environments where Tabs requires content/length (e.g. tests)

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

24-24: Consider consistent i18n fallback pattern.

The file mixes two fallback patterns: i18n.get("key") or "default" (line 24) and i18n.get("key", default="default") (lines 164-168). The default= parameter is preferable as it also handles empty string returns correctly. This is a minor consistency nit.

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

15-16: Unused imports: tempfile and requests.

These modules are imported but not used anywhere in the file.

♻️ Proposed fix

 import logging
 import threading
-import tempfile
-import requests
 from pathlib import Path

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

104-114: Windows-specific paths in cross-platform code.

The hardcoded C:/Temp and C:/IntuneWin paths are Windows-specific. While the exists() check prevents errors on other platforms, consider using platform-specific paths for better clarity.

♻️ Suggested improvement

+        import sys
+        
         # Common default locations
         user_home = Path.home()
         default_dirs = [
             user_home / "Downloads",
             user_home / "Documents",
             user_home / "Desktop",
-            Path("C:/Temp"),
-            Path("C:/IntuneWin"),
         ]
+        
+        # Add Windows-specific paths
+        if sys.platform == "win32":
+            default_dirs.extend([
+                Path("C:/Temp"),
+                Path("C:/IntuneWin"),
+            ])

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

943-946: Windows-specific os.startfile usage.

os.startfile() is Windows-only. While this file appears to be Windows-focused, consider adding a platform check or try/except for robustness.

♻️ Suggested defensive approach

                 def open_folder(e):
-                    import os
-                    os.startfile(str(source))
+                    try:
+                        import os
+                        os.startfile(str(source))
+                    except AttributeError:
+                        # os.startfile only available on Windows
+                        import subprocess
+                        subprocess.run(['xdg-open', str(source)], check=False)
                     dlg.open = False
                     self.app_page.update()

tests/test_ui_interactions.py (2)

19-19: Duplicate assignment of page.open.

page.open = MagicMock() is assigned twice (lines 19 and 27). Remove one.

♻️ Proposed fix

     # Init empty dialogs
     page.dialog = None
     page.snack_bar = None
-    page.open = MagicMock() # Fix for Flet 0.21+ dialogs

     # Mock window object
     page.window = MagicMock()
     page.theme_mode = ft.ThemeMode.DARK
     page.padding = 10

     # Mock open for dialogs (newer Flet API)
     page.open = MagicMock()
     return page

Also applies to: 27-27

---

63-70: Consider using pytest's built-in logging instead of file-based logging.

Writing to "test_result.log" creates artifacts. Use capfd fixture or pytest's logging capture for cleaner test output.

tests/test_i18n_integrity.py (2)

48-50: Remove extra blank lines inside function.

Lines 49-50 are empty, adding unnecessary vertical space inside the function definition.

♻️ Clean up

         def find_duplicates(filepath):
-
-
             with open(filepath, "r", encoding="utf-8") as f:

---

100-116: Weak heuristic for filtering dynamic keys.

The space check (if " " not in k) is fragile. Consider maintaining an explicit allowlist for known dynamic key patterns or using a more robust detection method.

♻️ Suggested improvement

# Define known dynamic/constructed key patterns to ignore
DYNAMIC_KEY_PATTERNS = [
    # Add patterns like: r'^addon_\w+_name$' if keys are constructed
]

missing = []
for k in found_keys:
    if k not in self.en:
        # Skip if matches known dynamic pattern
        if any(re.match(pattern, k) for pattern in DYNAMIC_KEY_PATTERNS):
            continue
        missing.append(k)

src/switchcraft/gui_modern/views/intune_view.py (3)

110-116: ft.Button with bgcolor, color, and height may not be valid.

Standard Flet button controls (ft.ElevatedButton, ft.TextButton) use the style parameter with ButtonStyle for customization, not direct bgcolor/color properties. This could cause runtime errors.

If this is intentional custom styling, verify the Flet version supports this syntax. Otherwise, consider using:

♻️ Suggested fix using standard Flet API

-        btn_create = ft.Button(
-            i18n.get("btn_create_intunewin") or "Create .intunewin",
-            bgcolor="GREEN_700",
-            color="WHITE",
-            height=50,
-            on_click=self._run_creation
-        )
+        btn_create = ft.ElevatedButton(
+            text=i18n.get("btn_create_intunewin") or "Create .intunewin",
+            style=ft.ButtonStyle(
+                bgcolor=ft.Colors.GREEN_700,
+                color=ft.Colors.WHITE,
+            ),
+            height=50,
+            on_click=self._run_creation
+        )

---

343-348: Previous issue partially addressed but path handling still fragile.

The shell=True was removed, which is good. However, the current format f'/select,{output_file}' as a single argument is correct for Explorer, but paths with spaces may still cause issues since Explorer expects the full /select,<path> as one argument.

Consider quoting the path or using os.path.normpath() for robustness:

♻️ More robust path handling

                 def open_folder(e):
                     import subprocess
                     if os.name == 'nt':
-                        subprocess.run(['explorer', f'/select,{output_file}'])
+                        # Explorer needs /select, and path as single argument
+                        subprocess.run(['explorer', '/select,' + os.path.normpath(output_file)])
                     dlg.open = False
                     self.app_page.update()

---

435-437: Remove the unused placeholder method _open_explorer_select.

This method is defined but never called anywhere in the codebase. Removing it will eliminate dead code.

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

100-105: Localized string may not include manifest path.

The fallback concatenates the path, but if i18n.get("wingetcreate_info") returns a localized string without a {path} placeholder, users won't see the actual path. Consider using format kwargs:

-                            i18n.get("wingetcreate_info") or
-                                "Manifests are saved to: " + str(self.manifest_dir),
+                            i18n.get("wingetcreate_info", path=str(self.manifest_dir)) or
+                                f"Manifests are saved to: {self.manifest_dir}",

And ensure the i18n string uses {path} placeholder.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

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

644-668: Critical bug: Duplicate platform check causes NameError on non-Windows.

This issue was previously flagged but remains unfixed. The nested if sys.platform == "win32": (lines 644-645) means subprocess.run only executes on Windows. On other platforms, result is never defined, causing NameError at line 660. Additionally, line 656 has inconsistent indentation.

🐛 Proposed fix

         def _bg():
             try:
                 cmd = ["winget", "validate", str(self.manifest_dir)]

                 if sys.platform == "win32":
-                    if sys.platform == "win32":
-                        startupinfo = subprocess.STARTUPINFO()
-                        startupinfo.dwFlags |= subprocess.STARTF_USESHOWWINDOW
-                        kwargs = {"startupinfo": startupinfo}
-                    else:
-                        kwargs = {}
-
-                    result = subprocess.run(
-                        cmd,
-                        capture_output=True,
-                        text=True,
-                    timeout=30,
-                        **kwargs
-                )
+                    startupinfo = subprocess.STARTUPINFO()
+                    startupinfo.dwFlags |= subprocess.STARTF_USESHOWWINDOW
+                    kwargs = {"startupinfo": startupinfo}
+                else:
+                    kwargs = {}
+
+                result = subprocess.run(
+                    cmd,
+                    capture_output=True,
+                    text=True,
+                    timeout=30,
+                    **kwargs
+                )

                 output = result.stdout + "\n" + result.stderr

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

217-235: Thread safety pattern inconsistent with connect handler.

While the connect handler (lines 172-202) correctly uses run_task() to marshal UI updates, search_apps (lines 217-235), _copy_metadata_from_supersedence (lines 288-311), and _run_upload (lines 408-438) still call self.update() directly from background threads. Apply the same pattern used in connect for consistency.

src/switchcraft/assets/lang/de.json (1)

774-774: Additional formal "Sie" inconsistency.

Line 774 uses formal form: "Ihr .intunewin-Paket" → should be "Dein .intunewin-Paket" for consistency.

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

263-277: Method structure looks good.

The lazy import pattern for requests is acceptable for optional dependencies. The hardcoded repository values are appropriate for this single-repo use case.

Consider catching ImportError for requests to provide a friendlier error message if the dependency isn't installed, though this is optional if requests is a required dependency.

---

306-325: Exception handling improved, but inconsistent exception type.

The empty exception handler from previous review is now properly addressed (lines 323-325). However, line 317 catches ValueError while line 299 catches json.JSONDecodeError. While JSONDecodeError is a subclass of ValueError, using the same exception type improves consistency and clarity.

Suggested consistency fix

-                    except ValueError: # JSONDecodeError
+                    except json.JSONDecodeError:
                         logger.error(f"Invalid JSON from latest release: {resp.text[:100]}")
                         return False, "Invalid response from GitHub."

src/switchcraft/gui_modern/views/group_manager_view.py (3)

100-111: Hard-coded strings should use i18n for consistency.

Lines 102-103 contain hard-coded English strings ("Entra Group Manager", "Manage your Microsoft Entra ID (Azure AD) groups.") while other UI elements in this file use i18n.get(). This breaks i18n consistency with the rest of the modernized views.

Similarly, the DataTable column headers at lines 84-87 ("Name", "Description", "ID", "Type") are hard-coded.

Consider wrapping these in i18n lookups:

-                    ft.Text("Entra Group Manager", size=28, weight=ft.FontWeight.BOLD),
-                    ft.Text("Manage your Microsoft Entra ID (Azure AD) groups.", color="GREY"),
+                    ft.Text(i18n.get("entra_group_manager_title") or "Entra Group Manager", size=28, weight=ft.FontWeight.BOLD),
+                    ft.Text(i18n.get("entra_group_manager_desc") or "Manage your Microsoft Entra ID (Azure AD) groups.", color="GREY"),

---

132-142: Exception handling logic may misclassify errors.

The exception types are now correctly using requests.exceptions, but the error classification is imprecise:

1. Line 133-134: HTTPError catches all HTTP errors (400, 401, 404, 500, etc.), not just 403 permission errors. A 404 or 500 would incorrectly display as a permission error.

2. Line 139-140: ConnectionError indicates network connectivity issues (DNS failure, connection refused), not authentication failures. The comment and error message are misleading.

3. Line 135: The fallback str(e) if str(e) else "Group.Read.All..." will show raw HTTP error text to users, which may be confusing.

Consider checking the response status code for HTTPError:

♻️ Suggested improvement

             except requests.exceptions.HTTPError as e:
-                # Handle specific permission error (403)
-                logger.error(f"Permission denied loading groups: {e}")
-                missing_perms = str(e) if str(e) else "Group.Read.All, Group.ReadWrite.All"
-                error_msg = i18n.get("graph_permission_error", permissions=missing_perms) or f"Missing Graph API permissions: {missing_perms}"
-                self._show_snack(error_msg, "RED")
+                status_code = e.response.status_code if e.response is not None else None
+                logger.error(f"HTTP error loading groups: {e}")
+                if status_code == 403:
+                    error_msg = i18n.get("graph_permission_error", permissions="Group.Read.All") or "Missing Graph API permissions: Group.Read.All"
+                elif status_code == 401:
+                    error_msg = i18n.get("graph_auth_error") or "Authentication failed. Please check your credentials."
+                else:
+                    error_msg = i18n.get("graph_http_error") or f"HTTP error: {e}"
+                self._show_snack(error_msg, "RED")
             except requests.exceptions.ConnectionError as e:
-                # Handle authentication failure
-                logger.error(f"Authentication failed: {e}")
-                error_msg = i18n.get("graph_auth_error") or "Authentication failed. Please check your credentials."
+                # Handle network connectivity issues
+                logger.error(f"Connection error: {e}")
+                error_msg = i18n.get("graph_connection_error") or "Network connection failed. Please check your internet connection."
                 self._show_snack(error_msg, "RED")

---

203-237: Several dialog strings are hard-coded.

The dialog handling pattern using app_page.open(dlg) is correct and consistent with other views. However, several strings should use i18n for consistency:

• Line 207: "Group Name"
• Line 208: "Description"
• Line 214: "Not connected to Intune"
• Line 229: "Create New Group"
• Line 232: "Cancel"
• Line 233: "Create"

This is optional but would align with the broader i18n expansion in this PR.

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

28-31: dmg_url state variable appears unused.

The dmg_url attribute is initialized but never referenced elsewhere in the class. The URL is read directly from self.url_field.value in _generate_script. Consider removing this dead state variable.

Proposed fix

         # State
-        self.dmg_url = ""
         self.local_file_path = None
         self.generated_script = ""

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

20-20: Unused import SwitchCraftConfig.

This import is not used anywhere in the file and can be removed.

♻️ Suggested fix

 from switchcraft.utils.i18n import i18n
-from switchcraft.utils.config import SwitchCraftConfig

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

561-567: Silent exception handling in _show_snack.

While the pattern matches other views in the codebase, silently swallowing exceptions makes debugging harder. Consider logging at debug level.

♻️ Suggested improvement

     def _show_snack(self, msg, color="GREEN"):
         try:
             self.app_page.snack_bar = ft.SnackBar(ft.Text(msg), bgcolor=color)
             self.app_page.snack_bar.open = True
             self.app_page.update()
-        except Exception:
-            pass
+        except Exception as ex:
+            logger.debug(f"Failed to show snack: {ex}")

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

110-115: Move sys import to module level.

sys is imported inside _get_scan_directories (line 110) and again inside _open_folder (line 297). Move it to module level for consistency and minor efficiency.

♻️ Suggested fix

 import flet as ft
 from switchcraft.utils.config import SwitchCraftConfig
 from switchcraft.utils.i18n import i18n

 import logging
 from datetime import datetime
 from pathlib import Path
 import os
+import sys

 logger = logging.getLogger(__name__)

Then remove the local imports at lines 110 and 297.

.github/workflows/test.yml (1)

30-32: Redundant pytest installation.

pip install .[test,gui] likely already includes pytest as a test dependency. The subsequent pip install pytest pytest-cov may be redundant. Consider consolidating by either adding pytest-cov to your [test] extras or removing the duplicate pytest install.

♻️ Suggested simplification

       - name: Install dependencies
         run: |
           python -m pip install --upgrade pip
           pip install .[test,gui]
-          pip install pytest pytest-cov
+          pip install pytest-cov  # pytest should be in [test] extras

src/switchcraft/gui_modern/views/intune_store_view.py (3)

80-92: Empty fallback branch does nothing.

The else block at lines 87-92 contains only pass with a comment. If neither goto_tab nor go is available, the user gets no feedback. Consider showing a snackbar message to inform users they need to navigate manually.

♻️ Suggested improvement

         else:
-             # Fallback: check if we have a way to signal tab change
-             # ModernApp stores 'app' on page in some instances or we can find the rail
-             # Since this is a view, we usually just show snackbar or let user navigate.
-             # But for best UX, we attempt to find the navigation method.
-             pass
+             self._show_snack("Please navigate to Settings manually.", "ORANGE")

---

163-169: Inconsistent i18n fallback pattern.

Lines 164-168 use default= parameter while line 56 and others use or pattern. For consistency and to leverage i18n's built-in fallback mechanism, prefer using default= throughout.

---

207-213: Silent exception swallowing may hide issues.

The _show_snack method catches all exceptions with a bare pass. While this prevents crashes, it also hides legitimate errors. Consider logging warnings for debugging purposes, consistent with the pattern in intune_view.py (line 448-454).

♻️ Proposed fix matching intune_view.py pattern

     def _show_snack(self, msg, color="GREEN"):
         try:
             self.app_page.snack_bar = ft.SnackBar(ft.Text(msg), bgcolor=color)
             self.app_page.snack_bar.open = True
             self.app_page.update()
-        except Exception:
-             pass
+        except Exception as e:
+            logger.warning(f"Failed to show snackbar: {e}")

tests/test_ui_interactions.py (1)

400-427: Consider extracting find_all_buttons_and_inputs to module level.

This helper is similar to find_buttons but includes TextFields. To improve maintainability and enable reuse, consider extracting it to module level alongside find_buttons.

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

250-252: Redundant import statement.

import os at line 251 is redundant since os is already imported at line 2.

♻️ Remove redundant import

             # Set window icon paths
-            import os
             import sys

src/switchcraft/gui_modern/views/analyzer_view.py (7)

171-185: Verify create_tabs argument order matches expected signature.

The create_tabs helper function signature expects tabs as the first positional argument: def create_tabs(tabs, **kwargs). However, this call passes tabs as a keyword argument. While this works in Python, it's clearer to pass it positionally as intended:

♻️ Suggested refactor for clarity

-        source_tabs = create_tabs(
-            selected_index=0,
-            animation_duration=300,
-            on_change=on_source_tab_change,
-            tabs=[
+        source_tabs = create_tabs(
+            [
                 ft.Tab(
                     label=i18n.get("local_file") or "Local File",
                     icon=ft.Icons.COMPUTER
                 ),
                 ft.Tab(
                     label=i18n.get("download_url") or "URL Download",
                     icon=ft.Icons.LINK
                 )
-            ]
-        )
+            ],
+            selected_index=0,
+            animation_duration=300,
+            on_change=on_source_tab_change
+        )

---

250-267: Use context manager for HTTP response to ensure proper resource cleanup.

The requests.get() response object should be used with a context manager to ensure the connection is properly closed, especially with stream=True.

♻️ Suggested fix

-                # Download with progress
-                response = requests.get(url, stream=True, timeout=60)
-                response.raise_for_status()
-
-                total_size = int(response.headers.get('content-length', 0))
-                downloaded = 0
-
-                with open(temp_path, 'wb') as f:
-                    for chunk in response.iter_content(chunk_size=8192):
-                        if chunk:
-                            f.write(chunk)
-                            downloaded += len(chunk)
-                            if total_size > 0:
-                                pct = downloaded / total_size
-                                self.url_download_progress.value = pct
-                                self.url_download_status.value = f"{i18n.get('downloading') or 'Downloading'}: {int(pct*100)}%"
-                                self.update()
+                # Download with progress
+                with requests.get(url, stream=True, timeout=60) as response:
+                    response.raise_for_status()
+
+                    total_size = int(response.headers.get('content-length', 0))
+                    downloaded = 0
+
+                    with open(temp_path, 'wb') as f:
+                        for chunk in response.iter_content(chunk_size=8192):
+                            if chunk:
+                                f.write(chunk)
+                                downloaded += len(chunk)
+                                if total_size > 0:
+                                    pct = downloaded / total_size
+                                    self.url_download_progress.value = pct
+                                    self.url_download_status.value = f"{i18n.get('downloading') or 'Downloading'}: {int(pct*100)}%"
+                                    self.update()

---

822-866: LGTM - Platform detection properly implemented.

The admin elevation code now includes proper sys.platform == "win32" guards before Windows-specific ctypes.windll calls. This addresses the previous review concern about relying on exception handling for control flow.

Minor: Lines 839-840 have a duplicate comment # Restart as admin.

Remove duplicate comment

-                # Restart as admin
                 # Restart as admin
                 try:

---

952-961: Unused variable result_path - consider removing or renaming.

The result_path variable captures the return value from create_intunewin(), but it's never used. According to the IntuneService, this returns the tool's output text, not the file path. The code correctly searches for the .intunewin file instead, making this variable unnecessary.

♻️ Suggested fix

-                result_path = self.intune_service.create_intunewin(str(source), setup_file, str(output), quiet=True)
+                self.intune_service.create_intunewin(str(source), setup_file, str(output), quiet=True)

---

928-935: Inconsistent dialog opening patterns across methods.

This method correctly uses hasattr(self.app_page, 'open') with fallback for Flet compatibility. However, _show_manual_cmds (lines 1029-1031) still uses the old pattern without this check. Consider standardizing the approach.

♻️ Apply same pattern to _show_manual_cmds

-        self.app_page.dialog = dlg
-        dlg.open = True
-        self.app_page.update()
+        if hasattr(self.app_page, 'open'):
+            self.app_page.open(dlg)
+        else:
+            self.app_page.dialog = dlg
+            dlg.open = True
+            self.app_page.update()

---

1047-1053: Consider logging snackbar failures for debugging.

The exception is silently swallowed, which can hide UI issues during development. Adding a debug log would help troubleshoot snackbar display problems.

♻️ Add debug logging

         except Exception:
-            pass
+            logger.debug("Failed to show snackbar", exc_info=True)

---

226-229: Missing i18n for error message.

The error message "Invalid URL format" should use i18n.get() for consistency with other user-facing strings in this file.

♻️ Use i18n for error message

         if not url.startswith(("http://", "https://")):
-            self._show_snack("Invalid URL format", "RED")
+            self._show_snack(i18n.get("invalid_url_format") or "Invalid URL format", "RED")
             return

Configuration used: defaults

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

src/switchcraft/gui_modern/utils/flet_compat.py (1)

29-52: Incomplete fallback path—extracted tab_contents is never used.

Lines 29-34 iterate over tabs to extract .content into tab_contents, but this list is never passed to the constructed Tabs or TabBarView. Additionally, line 52 returns ft.Tabs(content=ft.TabBar(tabs=tabs), ...) which according to Flet 0.80.1 API is incomplete—the content property requires a single Control containing both TabBar (headers) and TabBarView (body), not just TabBar.

Suggested fix using extracted tab_contents

                 tab_contents = []
                 for t in (tabs or []):
                     if hasattr(t, "content"):
                         tab_contents.append(t.content)
                     else:
                         tab_contents.append(ft.Container()) # Empty placeholder

-                # Create a Column with TabBar and the content view (TabBarView not strict req if we manage visibility?)
-                # Actually, standard pattern is Column([TabBar, Expanded(TabBarView)])
-                # But we can just return a Column that acts as the container.
-
-                # We need to ensure logic works. Tabs usually handles switching.
-                # If we return a Column, existing code might expect .tabs property.
-                # But standard Flet Tabs has .tabs.
-
-                # Let's try to construct a valid ft.Tabs via kwargs, matching 0.80.1 requirement:
-                # ft.Tabs(selected_index=..., animation_duration=..., tabs=[...], expand=...)
-                # If that failed above (TypeError), it implies signature mismatch.
-
-                # Re-try assuming it's the `content` argument issue or similar.
-                # If we really need a fallback:
-
                 length = len(tabs) if tabs else 0
-                return ft.Tabs(content=ft.TabBar(tabs=tabs), length=length, **kwargs)
+                content_column = ft.Column([
+                    ft.TabBar(tabs=tabs),
+                    ft.TabBarView(controls=tab_contents, expand=True)
+                ], expand=True)
+                return ft.Tabs(content=content_column, length=length, **kwargs)

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

248-272: Critical: shlex.quote() double-quoting issue persists.

The past review identified that wrapping shlex.quote() output in additional quotes produces invalid shell syntax. This issue appears unresolved:

• Line 250: FILENAME="{shlex.quote(filename)}" produces FILENAME="'installer.dmg'"
• Line 257: DOWNLOAD_URL="{shlex.quote(url)}" produces DOWNLOAD_URL="'https://...'"
• Line 271: APP_NAME="{shlex.quote(name)}" produces APP_NAME="'My App'"

shlex.quote() already returns a properly quoted string. The additional double quotes break the shell script.

🐛 Fix double-quoting

         if local_file:
             source_type = "local"
             filename = Path(local_file).name
             download_section = f'''# Local file deployment
-# Ensure {shlex.quote(filename)} is copied to the same location as this script
-FILENAME="{shlex.quote(filename)}"
+# Ensure {filename} is copied to the same location as this script
+FILENAME={shlex.quote(filename)}
 FILEPATH="$(dirname "$0")/$FILENAME"
 '''
         else:
             source_type = "url"
             filename = url.split("/")[-1] or "installer"
             download_section = f'''# Download from URL
-DOWNLOAD_URL="{shlex.quote(url)}"
+DOWNLOAD_URL={shlex.quote(url)}
 TEMP_DIR=$(mktemp -d)
 FILENAME=$(basename "$DOWNLOAD_URL")
 FILEPATH="$TEMP_DIR/$FILENAME"

-echo "Downloading {shlex.quote(name)}..."
+echo "Downloading $APP_NAME..."
 curl -L -o "$FILEPATH" "$DOWNLOAD_URL"
 '''

         # Template for DMG/PKG installation
         script = f'''#!/bin/bash
-# Auto-generated by SwitchCraft for {shlex.quote(name)}
-# Source: {"Local file" if source_type == "local" else shlex.quote(url)}
+# Auto-generated by SwitchCraft for {name}
+# Source: {"Local file" if source_type == "local" else url}

-APP_NAME="{shlex.quote(name)}"
+APP_NAME={shlex.quote(name)}
 {download_section}

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

585-588: Security: GitHub token exposed via command-line argument in _update_manifest.

While _generate_new_manifest correctly passes the token via environment variable (lines 491-495), _update_manifest still uses --token as a CLI argument (line 587). This exposes the token in process listings.

🔒 Use environment variable for token

                 # Output directory
                 cmd.extend(["--output", str(self.manifest_dir)])

                 # Submit PR?
+                env = os.environ.copy()
                 if self.upd_submit_pr.value and self.upd_github_token.value:
-                    cmd.extend(["--token", self.upd_github_token.value])
+                    env["WINGET_CREATE_GITHUB_TOKEN"] = self.upd_github_token.value
                     cmd.append("--submit")

                 # Run command
                 self.upd_output.value = f"Running: wingetcreate update {pkg_id}...\n\n"
                 self.update()

                 if sys.platform == "win32":
                     startupinfo = subprocess.STARTUPINFO()
                     startupinfo.dwFlags |= subprocess.STARTF_USESHOWWINDOW
-                    kwargs = {"startupinfo": startupinfo}
+                    kwargs = {"startupinfo": startupinfo, "env": env}
                 else:
-                    kwargs = {}
+                    kwargs = {"env": env}

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

171-185: Tab parameter naming inconsistency with other views.

The code uses label= for Tab parameters (lines 174, 178), while other views in the project (intune_view.py, script_upload_view.py, macos_wizard_view.py) use text=. The create_tabs helper from flet_compat.py handles the compatibility, but for consistency with the codebase, consider using text=.

.github/workflows/test.yml (2)

12-18: Remove unused os matrix variable or use it in runs-on.

The matrix.os is defined but never referenced—runs-on is hardcoded to windows-latest. Either remove the unused matrix variable or reference it properly.

Option 1: Remove unused matrix variable

     strategy:
       fail-fast: true
       max-parallel: 1
       matrix:
-        os: [windows-latest]
         python-version: ["3.13", "3.14"]

Option 2: Use matrix.os in runs-on

   test-backend:
-    runs-on: windows-latest
+    runs-on: ${{ matrix.os }}
     if: inputs.component == 'backend'

---

51-58: Clean up commented code and consider using .[test] extra for consistency.

The commented lines (55-56) add noise. For consistency with the backend job, consider installing the [test] extra instead of pytest separately.

♻️ Suggested cleanup

       - name: Install Core (No GUI)
         run: |
            python -m pip install --upgrade pip
            pip install .
-           # pip install pytest  <-- Included in .[test] or separate step if needed, but here we installed core only.
-           # Assuming we need pytest for testing:
            pip install pytest

Or, if [test] extra doesn't pull GUI dependencies:

       - name: Install Core (No GUI)
         run: |
            python -m pip install --upgrade pip
-           pip install .
-           # pip install pytest  <-- Included in .[test] or separate step if needed, but here we installed core only.
-           # Assuming we need pytest for testing:
-           pip install pytest
+           pip install .[test]

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

304-304: Consider: Response text logging could expose unexpected data.

Lines 304 and 322 log resp.text[:100] on parse failures. While useful for debugging, if GitHub's response ever includes unexpected content (e.g., error pages with tokens), this could leak into logs. This is low risk for GitHub's API but worth noting.

Also applies to: 322-322

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

321-456: Well-implemented members management dialog with proper threading.

The dialog implementation includes:

• Lazy loading of members with progress indicator
• Background threading for all API calls
• Nested dialog for user search and addition
• Proper i18n integration throughout

Minor observation: Line 389 uses if not query: return which will also trigger on whitespace-only input. Consider if not query.strip(): return for robustness.

src/switchcraft/gui_modern/views/script_upload_view.py (3)

447-464: URL parsing may fail for certain valid GitHub URL formats.

The URL parsing logic handles several formats but could fail on:

• SSH URLs like [email protected]:owner/repo.git - the replace(":", "/") would produce [email protected]/owner/repo which lacks https://
• URLs with trailing paths like https://github.com/owner/repo/tree/main

The current parsing at line 447 replaces : with / globally, which could break URLs containing port numbers or other colons.

♻️ Suggested improvement using urllib.parse

+                from urllib.parse import urlparse
+
                 # Parse GitHub URL
-                # Handle formats:
-                # https://github.com/owner/repo
-                # https://github.com/owner/repo.git
-                # github.com/owner/repo
-
-                clean_url = repo_url.replace("git@", "https://").replace(":", "/").rstrip("/")
-                if clean_url.endswith(".git"):
-                    clean_url = clean_url[:-4]
-                if not clean_url.startswith("http"):
-                    clean_url = "https://" + clean_url
-
-                # Split and find owner/repo
-                parts = clean_url.split("/")
-                # Expect https://github.com/owner/repo -> parts: [https:, '', github.com, owner, repo]
-                # Filter out empty strings
-                parts = [p for p in parts if p]
-
-                if len(parts) < 3:
-                     # fallback logic or error
-                     raise ValueError(i18n.get("invalid_github_url") or "Invalid GitHub URL")
-
-                owner = parts[-2]
-                repo = parts[-1]
+                # Handle SSH format: [email protected]:owner/repo.git
+                if repo_url.startswith("git@"):
+                    # Convert [email protected]:owner/repo.git -> github.com/owner/repo
+                    clean_url = repo_url.replace("git@", "").replace(":", "/", 1)
+                else:
+                    clean_url = repo_url
+                
+                if clean_url.endswith(".git"):
+                    clean_url = clean_url[:-4]
+                clean_url = clean_url.rstrip("/")
+                
+                if not clean_url.startswith("http"):
+                    clean_url = "https://" + clean_url
+                
+                parsed = urlparse(clean_url)
+                path_parts = [p for p in parsed.path.split("/") if p]
+                
+                if len(path_parts) < 2:
+                    raise ValueError(i18n.get("invalid_github_url") or "Invalid GitHub URL")
+                
+                owner = path_parts[0]
+                repo = path_parts[1]

---

460-461: Inconsistent indentation.

Line 460 has an extra space of indentation compared to surrounding code. Same issue at lines 545-547.

Fix indentation

                 if len(parts) < 3:
-                     # fallback logic or error
-                     raise ValueError(i18n.get("invalid_github_url") or "Invalid GitHub URL")
+                    # fallback logic or error
+                    raise ValueError(i18n.get("invalid_github_url") or "Invalid GitHub URL")

---

518-528: Script limit of 50 is undocumented to the user.

The code silently limits displayed scripts to 50 (line 518: ps_files[:50]). Users with larger repositories won't see all scripts and may not understand why. Consider showing a message when scripts are truncated.

♻️ Inform user when scripts are truncated

                 else:
                     self.repo_script_items = []
-                    for script_path in ps_files[:50]:  # Limit to 50
+                    MAX_SCRIPTS = 50
+                    for script_path in ps_files[:MAX_SCRIPTS]:
                         cb = ft.Checkbox(value=False)
                         self.repo_script_items.append({"path": script_path, "checkbox": cb})

                         self.github_script_list.controls.append(
                             ft.ListTile(
                                 leading=cb,
                                 title=ft.Text(script_path),
                                 trailing=ft.Icon(ft.Icons.DESCRIPTION, color="BLUE_400")
                             )
                         )
+                    
+                    if len(ps_files) > MAX_SCRIPTS:
+                        self.github_script_list.controls.append(
+                            ft.Text(
+                                f"Showing first {MAX_SCRIPTS} of {len(ps_files)} scripts",
+                                italic=True,
+                                color="ORANGE"
+                            )
+                        )

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

694-700: _show_snack is duplicated across multiple view files.

This identical method exists in at least 6 view files (see relevant_code_snippets). Consider extracting to a shared mixin or utility.

♻️ Extract to shared utility

Create a mixin or utility function:

# In src/switchcraft/gui_modern/utils/snack_helper.py
def show_snack(page, msg, color="GREEN"):
    try:
        page.snack_bar = ft.SnackBar(ft.Text(msg), bgcolor=color)
        page.snack_bar.open = True
        page.update()
    except Exception:
        pass

Then views can use show_snack(self.app_page, msg, color) instead of duplicating the method.

---

26-32: get_manifest_dir creates directories as side effect.

The function name suggests it only retrieves a path, but it also creates directories. This side effect could be unexpected for callers.

♻️ Rename or document the side effect

-def get_manifest_dir():
-    """Get the SwitchCraft manifest directory."""
+def get_or_create_manifest_dir():
+    """Get the SwitchCraft manifest directory, creating it if needed."""

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

293-297: Hardcoded pending navigation indices don't use NavIndex constants.

Lines 293-295 use hardcoded indices (2, 3) for --wizard and --analyzer arguments instead of NavIndex constants. This creates a maintenance burden if indices change.

♻️ Use NavIndex constants

         # Handle Jump List Arguments (Launch Flags)
         import sys
         if "--wizard" in sys.argv:
             # We need to defer this until UI is built
-            self._pending_nav_index = 2 # Wizard index
+            self._pending_nav_index = NavIndex.PACKAGING_WIZARD
         elif "--analyzer" in sys.argv or "--all-in-one" in sys.argv:
-             self._pending_nav_index = 3 # Analyzer index
+             self._pending_nav_index = NavIndex.ANALYZER
         else:
             self._pending_nav_index = None

---

1060-1063: Swallowing RuntimeError silently may hide issues.

The try/except RuntimeError: pass pattern at lines 1060-1063 (and similarly at 1083-1086, 801-804) hides all RuntimeErrors. While this handles "Control not attached" scenarios, it could mask other legitimate errors.

♻️ Log suppressed errors at debug level

         try:
             fade_container.update()
         except RuntimeError:
-            pass
+            pass  # Control not yet attached to page - expected during transitions

tests/test_navigation_integrity.py (1)

69-83: Test uses hardcoded index 21 which may become incorrect.

The test assumes dynamic addons start at index 21 (app._switch_to_tab(21)), but this depends on the number of static destinations. If static destinations change, this test will break. Consider using app.first_dynamic_index instead.

♻️ Use dynamic index calculation

     def test_dynamic_addon_handling(self):
         """Test that dynamic addons logic works (index >= 20)."""
         app = ModernApp(self.mock_page)

         # Mock dynamic addons
         app.dynamic_addons = [{'id': 'test_addon', 'name': 'Test Addon'}]
         # Mock addon service load
         app.addon_service = MagicMock()
         app.addon_service.load_addon_view.return_value = MagicMock()

-        # Dynamic index = 21 (20 + 1)
-        app._switch_to_tab(21)
+        # Use first_dynamic_index to calculate the correct tab index
+        dynamic_tab_index = app.first_dynamic_index  # First dynamic addon
+        app._switch_to_tab(dynamic_tab_index)

         # Check calls
         app.addon_service.load_addon_view.assert_called_with('test_addon')

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

191-202: Consider extracting the hardcoded placeholder message to i18n.

The "Deployment logic coming soon to Store view!" message on line 199 is hardcoded while other UI strings use i18n lookups.

Suggested improvement

-                    on_click=lambda _: self._show_snack("Deployment logic coming soon to Store view!", "BLUE")
+                    on_click=lambda _: self._show_snack(i18n.get("deploy_coming_soon") or "Deployment logic coming soon!", "BLUE")

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

273-284: Missing validation for empty input when adding items to stack.

The _add_item_to_stack method checks if val is truthy but doesn't provide user feedback when the input is empty. Users won't know why nothing happened.

Suggested improvement

     def _add_item_to_stack(self, e):
         if not self.current_stack:
             self._show_snack(i18n.get("select_stack_first") or "Select a stack first", "ORANGE")
             return

         val = self.new_item_field.value
+        if not val or not val.strip():
+            self._show_snack(i18n.get("enter_app_name") or "Please enter an app name", "ORANGE")
+            return
+        val = val.strip()
         if val:
             self.stacks[self.current_stack].append(val)
             self._save_stacks()
             self._select_stack(self.current_stack)  # Refresh list
             self.new_item_field.value = ""
             self.update()

---

330-336: TODO placeholder for deployment logic.

The _execute_deploy method contains a TODO comment indicating the actual Intune deployment logic is not yet implemented. The snackbar message misleadingly says "Deployment started!" when nothing actually happens.

Would you like me to help draft the Intune deployment integration, or should this be tracked as a separate issue?

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

94-131: Directory deduplication uses case-insensitive comparison but stores original case.

The normalization on line 126 uses .lower() for deduplication, which is correct for Windows but may cause issues on case-sensitive filesystems (Linux). Two directories differing only in case would be incorrectly deduplicated.

Platform-aware deduplication

         # Remove duplicates
         seen = set()
         unique_dirs = []
         for d in dirs:
-            d_normalized = os.path.normpath(d).lower()
+            d_normalized = os.path.normpath(d)
+            if sys.platform == "win32":
+                d_normalized = d_normalized.lower()
             if d_normalized not in seen:
                 seen.add(d_normalized)
                 unique_dirs.append(d)

---

154-166: Nested directory scanning could be slow with many subdirectories.

The one-level-down scan iterates all subdirectories without limit. In directories with many subdirectories (e.g., Downloads with hundreds of folders), this could cause performance issues.

Consider limiting subdirectory iteration

                 # Also check one level down (common structure)
-                for subdir in path.iterdir():
+                subdir_count = 0
+                for subdir in path.iterdir():
+                    if subdir_count >= 20:  # Limit subdirectory scanning
+                        break
                     if subdir.is_dir():
+                        subdir_count += 1
                         for file in subdir.glob("*.intunewin"):

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

226-230: Duplicate comment on line 227.

There's a duplicate # Validate URL comment on consecutive lines (227-228).

Remove duplicate comment

-        # Validate URL
         # Validate URL
         if not url.startswith(("http://", "https://")):

---

243-245: URL filename extraction may produce invalid filenames.

The filename extraction on line 243 doesn't sanitize for invalid filesystem characters. URLs with special characters in the path could cause issues.

Add filename sanitization

                 # Get filename from URL
                 filename = url.split("/")[-1].split("?")[0]
+                # Sanitize filename
+                filename = "".join(c for c in filename if c.isalnum() or c in ".-_")
                 if not filename.lower().endswith((".exe", ".msi")):
                     filename = "installer.exe"

tests/test_ui_interactions.py (2)

50-57: Remove dead TabBar handling code.

The TabBar handling block does nothing (just pass). Either implement proper Tab content traversal or remove this dead code.

♻️ Proposed fix

-    # Handle TabBar tabs
-    if isinstance(control, ft.TabBar) and control.tabs:
-        for tab in control.tabs:
-             # Tabs usually have content if they are TabBarView, but Tab control has content/label
-             # We just traverse Tab children? Tab doesn't have children usually unless it stores content
-             # Wait, Tab has 'content' or 'icon' etc.
-             pass
-
     return buttons

---

6-6: Remove unused import.

ModernHomeView is imported but never used in this file.

♻️ Proposed fix

-from switchcraft.gui_modern.views.home_view import ModernHomeView

Configuration used: defaults

Review profile: CHILL

Plan: Pro