bug fixes for new features #47
Conversation
github-actions
bot
added
documentation
📝 WalkthroughWalkthroughAdds defensive version-generation and release workflow fallbacks; restructures Intune docs; hardens GUI thread-safety and UI update paths; improves build script Inno Setup discovery; tightens Intune service error propagation; adds many UI-focused tests and updates app version to 2026.1.2. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@docs/INTUNE.md`:
- Around line 18-21: Remove the stray character "w" that appears before the
"[!IMPORTANT]" Intune OMA-URI section in INTUNE.md; open the block containing
the "[!IMPORTANT]" heading (the Intune OMA-URI Configuration text) and delete
the lone "w" so the document starts directly with the "[!IMPORTANT]" directive
and the following lines ("Intune OMA-URI Configuration" and "Critical: Ensure
you use the String (XML) Data Type...").
In `@src/switchcraft/gui_modern/views/intune_store_view.py`:
- Around line 214-224: Reverse the current fallback logic: instead of calling
_update_ui() directly and using run_task as a fallback, use
self.app_page.run_task(_update_ui) as the primary method to marshal UI updates
to the page event loop (when hasattr(self.app_page, 'run_task')). If run_task is
not available, then fallback to calling _update_ui() inside a try/except and log
exceptions with logger.exception; remove or update the misleading "Flet's
update() is thread-safe" comment. Ensure errors from run_task are also caught
and logged (use logger.exception around the run_task call).
In `@tests/test_all_buttons.py`:
- Line 240: The assertion assert len(buttons) >= 0 is meaningless because list
length is never negative; update the test in tests/test_all_buttons.py to assert
a real expectation about the buttons variable (for example, check that buttons
is a list and that it contains elements) by replacing the no-op assertion with
meaningful checks such as verifying type (isinstance(buttons, list)) and/or that
len(buttons) > 0 or another domain-specific condition that ensures the view
created the expected buttons.
- Around line 115-140: The test test_all_buttons_have_handlers improperly calls
another test function test_all_views_have_buttons(), creating a fragile
dependency, and ends with assert True which makes the test meaningless; refactor
by extracting the shared logic into a pure helper (e.g., collect_all_buttons or
a pytest fixture) that returns the all_buttons dict, update
test_all_buttons_have_handlers to call that helper (not the test function),
compute buttons_without_handlers from all_buttons, log them using the existing
print block, and then assert that buttons_without_handlers is empty (e.g.,
assert not buttons_without_handlers, f"Buttons without handlers:
{buttons_without_handlers}") or use pytest.skip/xfail for known exceptions;
update references to test_all_views_have_buttons and buttons_without_handlers
accordingly.
In `@tests/test_intune_store_search.py`:
- Around line 133-172: The test test_intune_search_timeout_mechanism ends with a
trivial assert True and must be changed to actually validate the timeout
behavior: update the test to either (A) assert that the view's timeout handling
was invoked by checking that the mocked view._show_error (or error_calls)
contains the expected timeout message after simulating the long-running
mock_intune_service.search_apps, or (B) refactor ModernIntuneStoreView (or
_run_search) to accept an injectable timeout value so the test can set a short
timeout and then assert that view._show_error was called and search_completed
did not occur; if you prefer not to test timeouts here, remove the test
entirely. Reference: test_intune_search_timeout_mechanism,
ModernIntuneStoreView, view._run_search, mock_intune_service.search_apps, and
view._show_error.
- Around line 36-71: The test test_intune_search_shows_timeout_error is
ineffective: patch threading.Thread.join before calling
ModernIntuneStoreView._run_search so the started search thread uses the mocked
join, simulate a join timeout (e.g., mock_join.return_value or side_effect to
return immediately/raise or no-op to mimic timeout behavior), remove the final
pass and add assertions that view._show_error was invoked with the expected
timeout message (inspect error_calls or assert a call to _show_error), and
ensure the slow_search replacement on mock_intune_service.search_apps and
list_apps remains in place so the timeout path is exercised.
In `@tests/test_language_change.py`:
- Around line 65-67: The test's assertion expecting "restart" can fail because
_on_lang_change in settings_view.py shows "Language changed. UI updated." when
switchcraft_app is present; update the test (using snack_calls) to assert the
actual success message variant instead of always checking for "restart" — for
example, check that any snack contains either "language changed" or "ui updated"
(or allow both the "restart" fallback and the "Language changed. UI updated."
happy path) so the assertion works for both the mock-provided switchcraft_app
path in _on_lang_change and the fallback.
In `@tests/test_notification_bell.py`:
- Around line 50-71: The test test_notification_bell_toggles_drawer should
accept that closing the drawer may set mock_page.end_drawer to None; update the
final assertion (after calling ModernApp._toggle_notification_drawer the second
time) to allow either end_drawer being None or its open attribute being False
(e.g., assert mock_page.end_drawer is None or mock_page.end_drawer.open is
False) so the test passes whether the implementation clears the reference or
merely closes it.
🧹 Nitpick comments (20)
docs/Intune_Configuration_Guide.md (1)
194-194: Consider hyphenating "Top-Level" as a compound adjective.Minor style suggestion: "Top Level Settings" should be "Top-Level Settings" for grammatical correctness when used as a compound modifier.
✏️ Proposed fix
-### Top Level Settings +### Top-Level Settingsscripts/build_release.ps1 (2)
332-352: Expanded Inno Setup discovery is good, but contains redundant paths.The path array includes both hardcoded paths (e.g.,
"C:\Program Files (x86)\Inno Setup 6\ISCC.exe") and environment variable paths (e.g.,"${env:ProgramFiles(x86)}\Inno Setup 6\ISCC.exe") that typically resolve to the same location. This isn't a functional issue—just minor redundancy that adds a few extraTest-Pathcalls.♻️ Optional: Remove redundant hardcoded paths
$PossiblePaths = @( - "C:\Program Files (x86)\Inno Setup 6\ISCC.exe", - "C:\Program Files\Inno Setup 6\ISCC.exe", - "C:\Program Files (x86)\Inno Setup 5\ISCC.exe", - "C:\Program Files\Inno Setup 5\ISCC.exe", "${env:ProgramFiles(x86)}\Inno Setup 6\ISCC.exe", "$env:ProgramFiles\Inno Setup 6\ISCC.exe", "${env:ProgramFiles(x86)}\Inno Setup 5\ISCC.exe", - "$env:ProgramFiles\Inno Setup 5\ISCC.exe" + "$env:ProgramFiles\Inno Setup 5\ISCC.exe", + # Fallback for edge cases where env vars aren't set + "C:\Program Files (x86)\Inno Setup 6\ISCC.exe", + "C:\Program Files\Inno Setup 6\ISCC.exe" )
406-422: Duplicated Inno Setup discovery logic.This path discovery block is identical to lines 332-349. Consider extracting this into a helper function to reduce duplication and ensure consistent behavior (e.g., the "Found Inno Setup at" message is missing here).
♻️ Extract helper function for Inno Setup discovery
Add this helper function near the other helper functions (around line 205):
function Find-InnoSetup { $IsccPath = (Get-Command "iscc" -ErrorAction SilentlyContinue).Source if ($IsccPath) { Write-Host "Found Inno Setup in PATH: $IsccPath" -ForegroundColor Gray return $IsccPath } $PossiblePaths = @( "${env:ProgramFiles(x86)}\Inno Setup 6\ISCC.exe", "$env:ProgramFiles\Inno Setup 6\ISCC.exe", "${env:ProgramFiles(x86)}\Inno Setup 5\ISCC.exe", "$env:ProgramFiles\Inno Setup 5\ISCC.exe" ) foreach ($p in $PossiblePaths) { if (Test-Path $p) { Write-Host "Found Inno Setup at: $p" -ForegroundColor Gray return $p } } return $null }Then replace both discovery blocks with:
$IsccPath = Find-InnoSetup.github/workflows/release.yml (1)
41-50: Good defense-in-depth version validation.The shell-level validation complements the Python script's built-in fallback, providing an extra safety net. The regex correctly validates the basic
MAJOR.MINOR.PATCHformat.Note: The hardcoded fallback
"2026.1.2"here must be kept in sync withFALLBACK_VERSIONingenerate_release_version.py. Consider defining this as a workflow-level environment variable to ensure consistency.♻️ Optional: Define fallback as workflow-level env var
on: workflow_dispatch: inputs: release_type: # ... +env: + FALLBACK_VERSION: "2026.1.2" + permissions: contents: writeThen reference as
${{ env.FALLBACK_VERSION }}in the validation step.src/switchcraft/services/intune_service.py (1)
394-403: Remove the duplicate RequestException handler.
The second block is unreachable and leaves conflicting error semantics.♻️ Suggested cleanup
except requests.exceptions.RequestException as e: logger.error(f"Network error in list_apps: {e}") raise - except requests.exceptions.RequestException as e: - logger.error(f"Network error while listing apps: {e}") - raise Exception(f"Network error: {str(e)}")tests/test_github_login.py (2)
61-64: Remove unusedget_pagefunction definitions.These
get_pagefunctions are defined but never used—only the property assignment on line 64/102/146 is effective. The function definitions are dead code.♻️ Suggested cleanup
- # Mock the page property to avoid RuntimeError - def get_page(): - return mock_page type(view).page = property(lambda self: mock_page)Also applies to: 99-102, 143-146
72-73: Consider usingpytest-timeoutor event-based synchronization instead oftime.sleep.Using fixed
time.sleep(0.5)calls for async synchronization can lead to flaky tests—too short may cause failures on slow CI, too long wastes time. Consider using threading Events or polling with shorter sleeps and a max retry count for more reliable tests.Also applies to: 116-117, 167-168
src/switchcraft/gui_modern/app.py (2)
188-205: Retry logic may cause redundant operations.The retry path at lines 195-197 calls
_open_notifications_drawer(e)again after it already failed in the try block (line 189). If the failure is deterministic (e.g., missing permissions, invalid state), retrying immediately won't help and wastes resources. Consider adding a different recovery strategy or removing the redundant retry.♻️ Suggested simplification
try: self._open_notifications_drawer(e) # Force update to ensure drawer is visible self.page.update() except Exception as ex: logger.exception(f"Error opening notification drawer: {ex}") - # Try again with simpler approach - try: - self._open_notifications_drawer(e) - self.page.update() - except Exception as ex2: - logger.error(f"Failed to open drawer after retry: {ex2}") - self.page.snack_bar = ft.SnackBar( - content=ft.Text(f"Failed to open notifications: {ex2}"), - bgcolor="RED" - ) - self.page.snack_bar.open = True - self.page.update() + self.page.snack_bar = ft.SnackBar( + content=ft.Text(f"Failed to open notifications: {ex}"), + bgcolor="RED" + ) + self.page.snack_bar.open = True + self.page.update()
1006-1009: Triplepage.update()calls are unusual.Calling
page.update()three times consecutively suggests potential underlying rendering issues. While this might work around a Flet quirk, it's a code smell. Consider investigating the root cause or adding a comment explaining why this is necessary.tests/test_intune_store_search.py (1)
19-21: Remove unusedget_pagefunction.The
get_pagefunction is defined but never called—only the property assignment matters.♻️ Suggested cleanup
- # Mock page property to avoid RuntimeError - def get_page(): - return page type(page).page = property(lambda self: page)tests/test_winget_details.py (1)
19-21: Remove unusedget_pagefunction.Same issue as in other test files—the function is defined but never called.
♻️ Suggested cleanup
- # Mock page property to avoid RuntimeError - def get_page(): - return page type(page).page = property(lambda self: page)tests/test_all_buttons.py (3)
41-46: Mock page is missingrun_taskand other attributes needed by some views.Compared to other test fixtures in the codebase (e.g.,
test_button_functionality.pylines 12-28), this mock is missingrun_task,dialog,end_drawer,snack_bar,open, andclose. Some views likeModernIntuneStoreViewandModernWingetViewmay fail or behave unexpectedly during instantiation.Suggested enhancement
mock_page = MagicMock(spec=ft.Page) mock_page.update = MagicMock() mock_page.switchcraft_app = MagicMock() mock_page.switchcraft_app.goto_tab = MagicMock() mock_page.switchcraft_app._current_tab_index = 0 + mock_page.switchcraft_app._view_cache = {} + mock_page.dialog = None + mock_page.end_drawer = None + mock_page.snack_bar = None + mock_page.open = MagicMock() + mock_page.close = MagicMock() + mock_page.run_task = lambda func: func() # Execute immediately for testing type(mock_page).page = property(lambda self: mock_page)
143-145: Same test coupling issue as above.
test_button_handlers_are_callablealso callstest_all_views_have_buttons()directly, creating the same fragile dependency.
30-112: Test function acts as both test and data provider - consider refactoring.
test_all_views_have_buttonsreturnsall_buttons(line 112), which is used by other tests. However, pytest ignores test return values. This dual-purpose design leads to the coupling issues noted above.Consider extracting the view instantiation and button discovery logic into a pytest fixture that can be reused across tests.
src/switchcraft/gui_modern/views/settings_view.py (2)
703-709: Redundant close dialog wrapper.The
close_dlgfunction wraps_closeand usesrun_taskif available, but since_show_dialog_with_flowis already called from the main thread (viarun_taskat line 792), the extrarun_taskcheck is unnecessary here.
1058-1068: Direct call with run_task fallback is reasonable but inconsistent with other patterns.The comment states "Flet's update() is thread-safe" (line 1058), so calling
_reload_app()directly is attempted first. However, other parts of this file (e.g.,_start_github_login) userun_taskas the primary path. Consider keeping the pattern consistent across the codebase for maintainability.tests/test_language_change.py (1)
7-7: Unused import.
timeis imported but not used in this file.-import timetests/test_button_functionality.py (3)
8-9: Unused imports.
inspectandimportlibare imported but not used in this file.-import inspect -import importlib
42-49: Duplicatedfind_clickableshelper function.The same helper function is defined twice (lines 42-49 and 86-93). Extract it to module level or as a fixture to reduce duplication.
Extract to module level
+def find_clickables(control, clickables): + """Recursively find all controls with on_click handlers.""" + if hasattr(control, 'on_click') and control.on_click is not None: + clickables.append(control) + if hasattr(control, 'content'): + find_clickables(control.content, clickables) + if hasattr(control, 'controls'): + for child in control.controls: + find_clickables(child, clickables) + def test_home_view_buttons(mock_page): """Test that Home View buttons work.""" from switchcraft.gui_modern.views.home_view import ModernHomeView navigate_calls = [] def track_navigate(idx): navigate_calls.append(idx) view = ModernHomeView(mock_page, on_navigate=track_navigate) - # Find all clickable containers - def find_clickables(control, clickables): - if hasattr(control, 'on_click') and control.on_click is not None: - clickables.append(control) - if hasattr(control, 'content'): - find_clickables(control.content, clickables) - if hasattr(control, 'controls'): - for child in control.controls: - find_clickables(child, clickables) - clickables = [] find_clickables(view, clickables)Also applies to: 86-93
175-175: Weak assertions:assert Trueprovides no verification.Both
test_winget_view_search_buttonandtest_intune_store_search_buttonend withassert True, which always passes and doesn't verify any behavior.Consider verifying that the search was triggered or that the view state changed:
- assert True # Just verify view was created + # Verify view was created and search can be triggered + assert view is not NoneOr remove the assertion entirely if the test's purpose is just to ensure no exceptions are raised during the flow.
Also applies to: 200-200
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/scripts/generate_release_version.py.github/workflows/release.ymldata/stacks.jsondocs/INTUNE.mddocs/IntuneConfig.mddocs/Intune_Configuration_Guide.mddocs/PolicyDefinitions/README.mdscripts/build_release.ps1src/switchcraft/gui_modern/app.pysrc/switchcraft/gui_modern/views/category_view.pysrc/switchcraft/gui_modern/views/intune_store_view.pysrc/switchcraft/gui_modern/views/settings_view.pysrc/switchcraft/gui_modern/views/winget_view.pysrc/switchcraft/services/intune_service.pyswitchcraft_legacy.issswitchcraft_modern.isstests/test_all_buttons.pytests/test_button_functionality.pytests/test_github_login.pytests/test_intune_store_search.pytests/test_language_change.pytests/test_loading_screen.pytests/test_notification_bell.pytests/test_winget_details.py
💤 Files with no reviewable changes (1)
- docs/IntuneConfig.md
🧰 Additional context used
🧬 Code graph analysis (8)
tests/test_language_change.py (1)
src/switchcraft/gui_modern/views/settings_view.py (2)
ModernSettingsView(17-1765)_on_lang_change(968-1155)
tests/test_intune_store_search.py (1)
src/switchcraft/gui_modern/views/intune_store_view.py (5)
ModernIntuneStoreView(14-623)_get_token(119-125)_show_error(228-241)_run_search(127-226)_update_list(243-286)
src/switchcraft/gui_modern/views/settings_view.py (3)
src/switchcraft/services/auth_service.py (3)
initiate_device_flow(27-44)poll_for_token(47-90)save_token(93-95)src/switchcraft/gui_modern/utils/view_utils.py (1)
_show_snack(9-20)tests/test_github_login.py (1)
run_task(21-22)
tests/test_notification_bell.py (1)
src/switchcraft/gui_modern/app.py (1)
_toggle_notification_drawer(140-222)
tests/test_button_functionality.py (5)
src/switchcraft/gui_modern/views/home_view.py (1)
ModernHomeView(6-156)src/switchcraft/gui_modern/views/category_view.py (1)
CategoryView(41-116)src/switchcraft/gui_modern/views/settings_view.py (1)
ModernSettingsView(17-1765)src/switchcraft/gui_modern/views/winget_view.py (1)
ModernWingetView(13-1033)src/switchcraft/gui_modern/views/intune_store_view.py (2)
ModernIntuneStoreView(14-623)_get_token(119-125)
src/switchcraft/gui_modern/views/intune_store_view.py (2)
src/switchcraft/utils/i18n.py (1)
get(179-206)src/switchcraft/gui/views/intune_store_view.py (1)
_show_error(106-109)
src/switchcraft/gui_modern/views/winget_view.py (1)
src/switchcraft/debug_views.py (1)
update(19-19)
tests/test_loading_screen.py (3)
src/switchcraft/gui_modern/app.py (1)
main(1814-1824)tests/test_github_login.py (1)
mock_page(12-33)src/switchcraft/debug_views.py (5)
add(20-20)update(19-19)width(28-28)height(30-30)open(22-22)
🪛 LanguageTool
docs/Intune_Configuration_Guide.md
[style] ~3-~3: Try moving the adverb to make the sentence clearer.
Context: ...uration Guide This guide describes how to correctly configure SwitchCraft policies using Microsoft Intune Custom Profiles ...
(SPLIT_INFINITIVE)
[uncategorized] ~194-~194: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...HUMBPRINT_HEX_STRING"/> ``` --- ### Top Level Settings #### 1. Debug Mode (`DebugMod...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
docs/PolicyDefinitions/README.md
53-53: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-backend / test-backend (3.13)
🔇 Additional comments (43)
data/stacks.json (1)
1-1: LGTM!Valid initialization file for the Stack Manager feature. The empty JSON object provides a proper starting state for storing stack configurations, as referenced in the documentation.
switchcraft_legacy.iss (1)
8-13: LGTM!Version constants updated consistently to 2026.1.2. The
#ifndefguards properly allow CI/CD override while providing sensible defaults.switchcraft_modern.iss (1)
8-13: LGTM!Version constants updated consistently to 2026.1.2, matching the legacy installer.
docs/Intune_Configuration_Guide.md (1)
1-202: Well-structured Intune configuration guide.The documentation is comprehensive, covering common errors, prerequisites, and all OMA-URI settings with proper XML payload examples. This will be valuable for enterprise deployments.
docs/PolicyDefinitions/README.md (3)
63-76: LGTM! Clear and consistent table structure.The Configuration Reference table effectively communicates the String data type requirement with consistent formatting and helpful XML examples for each policy setting.
79-79: LGTM! Consistent XML examples throughout.The XML examples consistently use String DataType with properly CDATA-wrapped payloads, aligning with the guidance in the IMPORTANT note and Configuration Reference table.
Also applies to: 93-94, 107-108, 170-171
47-52: Technical guidance verified as accurate per Microsoft documentation.The documentation correctly states that ADMX-backed policies in Intune must use String (or String (XML)) data type with XML-encoded payloads, never Integer or Boolean. This aligns with Microsoft Learn and official Intune support guidance on OMA-URI data type requirements. The IMPORTANT note appropriately emphasizes this critical configuration requirement.
.github/scripts/generate_release_version.py (2)
6-6: LGTM!The
sysimport is correctly added to support stderr output for warnings and errors.
74-88: Good defensive error handling with consistent fallback.The try/except pattern with validation provides robust fallback behavior. Using
sys.stderrfor warnings/errors while keeping stdout clean for version output is the correct approach for CI pipelines.One minor note: the regex
r'^\d+\.\d+\.\d+'(without end anchor$) will match versions like1.2.3-betaand1.2.3-dev-abc123which is correct given thecalculate_next_versionfunction produces such suffixes.scripts/build_release.ps1 (2)
155-174: LGTM!The fallback version handling is well-structured: defaults are set upfront, and all failure modes (parse failure, missing file) are handled with informative warnings.
380-393: Read-Host may hang in CI/CD if Inno Setup is missing and winget is available.The
Read-Hostprompt only executes when Inno Setup is not found but winget is available. Since GitHub Actions Windows runners don't reliably pre-install Inno Setup, this interactive prompt could block CI/CD pipelines. For automated workflows, either pre-install Inno Setup or conditionally disable theInstallertarget.For local development, the current behavior is acceptable and provides helpful UX.
.github/workflows/release.yml (2)
169-175: Automated fallback version propagation is well-designed.The sed commands correctly escape the
$characters for shell interpolation. The approach of calculatingFALLBACK_VERSIONfrom the release version ensures consistency.One consideration: the sed pattern for
build_release.ps1matches\$AppVersion = \".*\"which could theoretically match multiple lines if there were other variables with similar names. In the current script, this appears safe since$AppVersionand$AppVersionNumericare the only matches on lines 156-157.
177-180: LGTM!The git operations correctly stage all modified files, and the commit message clearly documents both the development version and fallback version updates. The
[skip ci]tag appropriately prevents triggering another workflow run.src/switchcraft/gui_modern/views/category_view.py (1)
90-94: LGTM on the explicit alignment.
Keeps the icon centered while making the positioning explicit.Please sanity-check against the current Flet alignment API.
tests/test_loading_screen.py (1)
10-53: Good coverage for loading-screen presence.
Catches regressions in initial add/update behavior and basic loading/progress markers.Please confirm the assertions still match the current
main()initialization flow.src/switchcraft/services/intune_service.py (1)
448-465: Nice propagation of network errors in fallback paths.
Re-raising Timeout/RequestException keeps callers aware of network failures.Please confirm this aligns with the expected
requestsexception flow in your runtime.src/switchcraft/gui_modern/views/winget_view.py (2)
243-245: Good normalization of empty results.
Guarantees stable list handling when the helper returns None.Please confirm this matches the Winget helper’s current return types.
Also applies to: 277-281
685-712: Details pane refresh flow looks solid.
Rebuilding the Column and reassigning it should force a reliable redraw.Please sanity-check against current Flet update behavior.
tests/test_notification_bell.py (1)
10-47: Good coverage for opening the notification drawer.
Validates drawer creation/open + update behavior.Please confirm this still reflects the latest drawer-open flow.
tests/test_github_login.py (4)
11-33: Well-structured mock page fixture.The fixture correctly mocks
run_taskto execute synchronously and simulates dialog opening behavior. The pattern of capturing dialog state viamock_openis appropriate for testing dialog-based flows.
43-86: Test coverage for dialog flow looks comprehensive.The test validates dialog opening, content verification (user code and verification URI), and UI update calls. Good coverage of the happy path for device-flow initiation.
89-121: Error path test is well-structured.Properly tests the failure scenario when device flow initiation returns
None, verifying that an error snack is shown with appropriate messaging.
124-178: Success flow test validates complete token handling.Good coverage verifying token save, UI update via
_update_sync_ui, and success message display. The tracking pattern using wrapper functions is appropriate.src/switchcraft/gui_modern/app.py (3)
165-172: Good defensive programming for drawer reference handling.Saving
drawer_refbefore clearing and using it for subsequent operations prevents issues with stale references and null pointer scenarios.
1225-1230: Defensive drawer open enforcement is reasonable.Forcing the drawer open flag when it's unexpectedly false is a pragmatic workaround for potential Flet state synchronization issues. The warning log helps with debugging.
1823-1824: Exposing app instance on page enables cross-view communication.This assignment is already done in
__init__at line 42. Having it in both places ensures the reference is set regardless of how the app is initialized, which is fine for robustness.src/switchcraft/gui_modern/views/intune_store_view.py (3)
131-140: Good defensive UI update with fallback.The try-except pattern that falls back to updating just the results list when the full view update fails is a reasonable approach to handle partially-attached control states.
185-186: Appropriate timeout logging.Warning log when the search thread times out helps with debugging slow network conditions or hung requests.
195-199: Good defensive completion state handling.Ensuring
completedis set and providing a default error message when no response is received prevents the UI from getting stuck in a loading state indefinitely.tests/test_intune_store_search.py (2)
74-98: Network error test validates error handling correctly.Good test structure: mocks the exception, triggers the search, and asserts that the error callback receives an appropriate message.
101-130: Success path test properly validates result handling.Correctly tests that search results are passed to
_update_listwith the expected structure and content.tests/test_winget_details.py (4)
37-89: Comprehensive test for details loading and display.Good coverage: validates details callback, control structure, and right pane visibility. The wrapper pattern for tracking
_show_details_uicalls is appropriate.
92-119: Loading state test correctly validates immediate UI feedback.Verifying that a
ProgressBarappears immediately and the right pane becomes visible before async loading completes is good UX validation.
145-156: Clever recursive control traversal for error text detection.The
collect_texthelper recursively traverses nested controls to find error messages, which handles Flet's nested container structure well. This is a good testing pattern.
159-203: UI update tracking test validates rendering calls.Good test structure: wraps both view and page update methods to verify that UI updates are triggered during the details loading flow.
tests/test_all_buttons.py (1)
14-27: LGTM! Recursive button discovery helper is well-structured.The helper correctly handles multiple Flet button types and traverses
controls,content, andactionsattributes.src/switchcraft/gui_modern/views/settings_view.py (4)
672-695: Background flow initiation with proper error handling - LGTM.The
_init_flowfunction correctly usesrun_taskto dispatch UI error notifications to the main thread when available, with a fallback for direct calls.
757-761: Defensive dialog open check is good.The check at lines 757-761 ensures the dialog is actually opened even if the initial attempt didn't set the flag correctly. This is good defensive programming.
764-783: Token polling and UI update on main thread - well implemented.The polling runs in a background thread and correctly uses
run_taskto dispatch UI updates (dialog close, snackbar) to the main thread.
786-809: Good orchestration with proper thread-safety fallbacks.The
_flow_completefunction properly sequences: (1) background network call, (2) main-thread dialog display viarun_task, with fallback to direct call ifrun_taskis unavailable.tests/test_language_change.py (1)
25-45: Test correctly verifies config and i18n updates.The test properly patches dependencies and verifies that
set_user_preferenceandset_languageare called with the expected arguments.tests/test_button_functionality.py (2)
12-28: Well-structured mock page fixture.This fixture includes all necessary attributes (
run_task,dialog,open,close, etc.) that views may depend on. Good alignment with other test files in the codebase.
137-175: Test correctly mocks AddonService and WingetHelper chain.The patching correctly sets up the mock chain for
AddonService().import_addon_module().WingetHelper(). The search for the button and conditional click handling is defensive and appropriate.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/switchcraft/services/intune_service.py (1)
484-489: Inconsistent exception handling: wraps Timeout in generic Exception.In
list_apps(line 396),Timeoutis re-raised as itself, but here it's wrapped in a genericException. This inconsistency prevents callers from catchingTimeoutspecifically.🐛 Suggested fix for consistency
except requests.exceptions.Timeout: logger.error(f"Request timed out while getting app details for {app_id}") - raise Exception("Request timed out. The server took too long to respond.") + raise requests.exceptions.Timeout("Request timed out. The server took too long to respond.") except requests.exceptions.RequestException as e: logger.error(f"Network error getting app details: {e}") - raise Exception(f"Network error: {str(e)}") + raisesrc/switchcraft/gui_modern/views/winget_view.py (1)
376-416: Remove incorrect thread-safety assumption and usepage.run_task()as the primary approach for UI updates from background threads.The comment "Flet's update() is thread-safe" contradicts Flet's official documentation. Since
_fetch()runs in a background thread (line 419:threading.Thread(target=_fetch, daemon=True).start()), callingupdate()directly can cause race conditions. Flet requires UI mutations to be marshalled back to the main/app context.Reverse the logic: use
page.run_task(_show_ui)as the primary path and remove the direct call, or remove the direct call entirely in favor of run_task.
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 28-35: The job-level if currently references the matrix context
(matrix.python-version) which isn’t available at job evaluation; remove that
matrix-based condition from the job if and either (a) split the Python 3.15 run
into a separate job (e.g., python-3-15) with its own job-level if guarded by
github.event_name == 'push' && github.ref == 'refs/heads/main', or (b) keep the
matrix and move the 3.15-specific gating into step-level if checks;
additionally, when creating the separate 3.15 job (or when adding 3.15 to
setup-python), update the actions/setup-python@v6 usage to include with:
allow-prereleases: true so prerelease Python 3.15 is accepted.
In @.github/workflows/release.yml:
- Around line 41-50: The VERSION validation uses an unanchored grep which lets
malformed strings like "1.2.3.4" or "1.2.3foo" pass; update the grep check that
references the VERSION variable in the if condition to use a strict anchored
regex that matches only MAJOR.MINOR.PATCH and optional pre-release/build
identifiers (e.g. anchor with ^ and $ and allow an optional -pre and +build
suffix pattern), so the fallback to "2026.1.2" triggers for any invalid format.
In @.github/workflows/test.yml:
- Around line 13-23: The job-level if currently references matrix.python-version
(and uses matrix in the job if), which is invalid; instead either (A) move the
Python-version-specific logic into step-level conditions by keeping the matrix
in the job and adding an if on the test step (use matrix.python-version and
github.event_name/github.ref inside the step's if), or (B) encode the allowed
combinations using the matrix include/exclude feature in the matrix definition
so the job-level if only uses allowed contexts (e.g., inputs/component); update
references to matrix.python-version in the job-level if to be removed and place
them on the relevant step's if or into the matrix include/exclude block to
restrict Python 3.15 runs to main push events.
In `@scripts/build_release.ps1`:
- Around line 377-395: The script currently uses Read-Host (at the $response
prompt) which blocks non-interactive CI; wrap the interactive prompt and
subsequent winget install attempt in a guard that only runs for
local/interactive sessions (e.g. check that neither $env:CI nor
$env:GITHUB_ACTIONS is set), so replace the current unconditional Read-Host
block with an if (-not $env:CI -and -not $env:GITHUB_ACTIONS) { $response =
Read-Host ...; if ($response -in 'y','Y') { try { winget install --id
JRSoftware.InnoSetup ... } catch { Write-Warning ... } } } else { Write-Host
"Skipping interactive Inno Setup install in CI." -ForegroundColor Yellow } to
ensure $wingetAvailable, Read-Host and the winget install are only executed
interactively.
In `@src/switchcraft/gui_modern/views/intune_store_view.py`:
- Line 131: The current call to i18n.get("searching") can return the key itself
when a translation is missing, so the "or" fallback won't work; update the code
that appends to self.results_list.controls (the ft.Text creation) to pass a
default/fallback directly to i18n.get (e.g., use the i18n.get signature that
accepts a default like i18n.get("searching", "Searching...")) so ft.Text always
receives a human-friendly string even when the translation is absent.
In `@tests/test_all_three_issues.py`:
- Around line 182-186: The loop that assigns lang_dd by iterating only
general_tab.controls can miss nested Dropdowns; change the lookup to perform a
recursive search through general_tab's descendants (same approach used in
test_language_change_updates_ui) to find the first ft.Dropdown, so that lang_dd
is set even when the dropdown is nested instead of a direct child of
general_tab.controls.
In `@tests/test_intune_store_search.py`:
- Around line 35-88: The test test_intune_search_shows_timeout_error has
redundant local imports and a race/global-patch issue: remove the local imports
of os and threading, stop globally reassigning threading.Thread, and ensure the
mocked thread accurately simulates a running thread so slow_search is never
invoked; instead use a local Thread subclass or context-managed patching to
override join and is_alive for threads created by view._run_search. Concretely,
replace the direct assignment threading.Thread = mock_thread with a scoped
monkeypatch/patch of threading.Thread (or use unittest.mock.patch) when creating
mock_thread, ensure mock_thread returns a Thread instance whose is_alive returns
True and join returns immediately, and keep slow_search and
view._get_token/view._show_error as-is so the timeout branch in
ModernIntuneStoreView._run_search is exercised without global side effects.
♻️ Duplicate comments (1)
src/switchcraft/gui_modern/views/intune_store_view.py (1)
201-220: Align the UI-thread comment with the run_task path.The comment says to avoid
run_task, but the code now usesrun_taskas the primary marshalling mechanism. Update/remove the comment to avoid misleading future edits.🛠️ Suggested fix
- # Update UI on main thread - MUST use page.update() directly, not run_task + # Update UI on the page event loop; prefer run_task when available
🧹 Nitpick comments (11)
docs/Intune_Configuration_Guide.md (1)
1-202: Optional: Consider adding a troubleshooting section.The guide is comprehensive and well-structured. To further enhance it, consider adding:
- A troubleshooting section with additional common errors and solutions
- Guidance on validating that policies are applied correctly
- Links to related documentation (e.g., the INTUNE.md and PolicyDefinitions/README.md mentioned in the PR context)
These additions would make the guide even more valuable for administrators, but the current content is already solid and production-ready.
src/switchcraft/gui_modern/views/winget_view.py (1)
451-461: Consistent with the pattern above - same thread-safety concern applies.The same fallback pattern is used here for error UI. Consider consolidating this into a helper method to reduce duplication.
♻️ Suggested helper to reduce duplication
def _run_on_ui_thread(self, func): """Execute function on UI thread with fallback.""" try: func() except Exception as ex: logger.debug(f"Direct call failed: {ex}") if hasattr(self.app_page, 'run_task'): try: self.app_page.run_task(func) except Exception: passtests/test_winget_details.py (2)
70-78: Usingtime.sleep()for async synchronization is flaky.Tests that rely on
time.sleep()can fail intermittently on slow CI runners or pass despite bugs on fast machines. Consider using polling with a timeout or mocking the threading entirely.♻️ Suggested improvement using polling
import time def wait_for(condition, timeout=2.0, interval=0.05): """Wait for condition to be true, polling at interval.""" start = time.time() while time.time() - start < timeout: if condition(): return True time.sleep(interval) return False # Usage in test: assert wait_for(lambda: len(details_shown) > 0), "Details should be loaded"
174-196: Test verifies update calls but assertion at line 196 is weak.The assertion
assert len(update_calls) > 0 or len(page_update_calls) > 0passes if either is called, but doesn't verify that updates happened at the right time or in the right sequence.tests/test_all_buttons.py (1)
260-264: Swallowing handler exceptions may hide real bugs.Printing failures but not failing the test means broken handlers go unnoticed. Consider collecting failures and asserting at the end, or using
pytest.warns/pytest.raisesselectively.♻️ Suggested improvement to track failures
+ handler_failures = [] for button in buttons: if hasattr(button, 'on_click') and button.on_click is not None: try: mock_event = MagicMock() mock_event.control = button mock_event.data = "true" button.on_click(mock_event) except Exception as e: - print(f"Button handler failed in {view_name}: {e}") + handler_failures.append(f"{view_name}: {e}") assert view is not None + # Optionally: assert not handler_failures, f"Handler failures: {handler_failures}"tests/test_all_three_issues.py (1)
50-67: Mock AuthService fixture provides complete device flow mocking.All necessary return values are configured for the GitHub device flow. However, there's redundancy:
mock_serviceis created but onlymockis returned.♻️ Simplify fixture
`@pytest.fixture` def mock_auth_service(): """Mock AuthService responses.""" with patch('switchcraft.gui_modern.views.settings_view.AuthService') as mock: - mock_service = MagicMock() mock.initiate_device_flow.return_value = { "device_code": "test_code", "user_code": "ABC-123", "verification_uri": "https://github.com/login/device", "interval": 5, "expires_in": 900 } mock.poll_for_token.return_value = "test_token_123" mock.save_token = MagicMock() - mock_service.initiate_device_flow = mock.initiate_device_flow - mock_service.poll_for_token = mock.poll_for_token - mock_service.save_token = mock.save_token - return mock + yield mocktests/test_ui_interactions_critical.py (1)
85-87: Avoid fixed sleeps in UI tests; poll for state instead.Fixed waits add flakiness; polling for the dialog/open state (or goto_tab call) makes the tests more deterministic.
♻️ Suggested approach (apply similarly to the other sleep)
- time.sleep(1.0) + for _ in range(20): + if mock_page.dialog and mock_page.dialog.open: + break + time.sleep(0.1)Also applies to: 136-137
tests/test_github_login_real.py (1)
72-73: Replace fixed sleep with polling to reduce flakiness.♻️ Suggested fix
- time.sleep(0.5) + for _ in range(10): + if mock_page.dialog and mock_page.dialog.open: + break + time.sleep(0.05)tests/test_button_functionality.py (2)
56-67: Don’t silently swallow all click failures.Right now the tests can pass even if every handler crashes. Consider tracking successful clicks and asserting at least one succeeds (apply the same pattern to the other loops).
♻️ Example adjustment (apply similarly to other loops)
- for clickable in clickables: + successful_clicks = 0 + for clickable in clickables: if hasattr(clickable, 'on_click') and clickable.on_click: try: mock_event = MagicMock() mock_event.control = clickable clickable.on_click(mock_event) + successful_clicks += 1 except Exception as e: # Some might fail due to missing dependencies, that's OK pass @@ - assert len(clickables) > 0 + assert len(clickables) > 0 + assert successful_clicks > 0, "Expected at least one clickable handler to succeed"Also applies to: 91-100, 117-127
167-168: Strengthen the “button works” assertions.
assert True/view is not Nonewon’t catch regressions in button wiring. A minimal improvement is to assert the handler exists and is callable.♻️ Suggested assertions
- assert True # Just verify view was created + assert search_button is not None, "Search button should exist" + assert callable(search_button.on_click), "Search button must have on_click handler" @@ - assert view is not None + assert view.btn_search is not None, "Search button should exist" + assert callable(view.btn_search.on_click), "Search button must have on_click handler"Also applies to: 192-192
tests/test_intune_store_search.py (1)
150-216: Redundant imports and potential flakiness in timeout test.
Redundant imports:
threading(line 153) andos(line 154) are already imported at module level.Potential flakiness: The assertion
search_started.wait(timeout=1.0)at line 205 may fail intermittently. Thesearch_started.set()is called insideslow_search, butslow_searchis assigned tomock_intune_service.search_apps(line 170). The actual_run_searchmethod may callsearch_appsorlist_appsdepending on the query value. Sincelist_appsis not mocked with this behavior in this test, if the implementation path changes, the test could break.Improved from past review: The test now has meaningful assertions (lines 211-215) instead of
assert True.Remove redundant imports
def test_intune_search_timeout_mechanism(mock_page, mock_intune_service): """Test that Intune search properly times out after 60 seconds.""" from switchcraft.gui_modern.views.intune_store_view import ModernIntuneStoreView - import threading - import os # Skip in CI to avoid 65 second wait
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.github/workflows/ci.yml.github/workflows/docs_preview.yml.github/workflows/release.yml.github/workflows/test.ymldocs/.vitepress/config.mtsdocs/INTUNE.mddocs/Intune_Configuration_Guide.mdscripts/build_release.ps1src/switchcraft/gui_modern/app.pysrc/switchcraft/gui_modern/views/analyzer_view.pysrc/switchcraft/gui_modern/views/intune_store_view.pysrc/switchcraft/gui_modern/views/settings_view.pysrc/switchcraft/gui_modern/views/stack_manager_view.pysrc/switchcraft/gui_modern/views/winget_view.pysrc/switchcraft/gui_modern/views/wingetcreate_view.pysrc/switchcraft/services/intune_service.pytests/test_all_buttons.pytests/test_all_three_issues.pytests/test_button_functionality.pytests/test_github_login.pytests/test_github_login_real.pytests/test_intune_store_search.pytests/test_language_change.pytests/test_notification_bell.pytests/test_ui_interactions_critical.pytests/test_winget_details.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_language_change.py
- tests/test_notification_bell.py
- tests/test_github_login.py
- docs/INTUNE.md
🧰 Additional context used
🧬 Code graph analysis (9)
tests/test_ui_interactions_critical.py (7)
tests/test_github_login.py (4)
mock_page(13-34)run_task(22-23)mock_open(27-31)mock_auth_service(38-41)tests/test_language_change.py (1)
mock_page(10-21)tests/test_notification_bell.py (2)
mock_page(12-24)test_notification_bell_opens_drawer(27-52)src/switchcraft/gui_modern/app.py (3)
goto_tab(1100-1108)ModernApp(25-1812)_toggle_notification_drawer(140-216)src/switchcraft/services/auth_service.py (3)
initiate_device_flow(27-44)poll_for_token(47-90)save_token(93-95)src/switchcraft/gui_modern/views/settings_view.py (2)
ModernSettingsView(17-1802)_build_general_tab(96-167)src/switchcraft/gui_modern/views/script_upload_view.py (1)
on_change(54-62)
tests/test_all_buttons.py (3)
tests/test_button_functionality.py (1)
mock_page(11-26)tests/test_github_login.py (2)
mock_page(13-34)run_task(22-23)tests/test_github_login_real.py (2)
mock_page(12-34)run_task(22-23)
tests/test_all_three_issues.py (3)
src/switchcraft/gui_modern/app.py (1)
_toggle_notification_drawer(140-216)src/switchcraft/gui_modern/views/settings_view.py (3)
_start_github_login(667-830)_build_general_tab(96-167)_build_cloud_sync_section(169-182)src/switchcraft/services/notification_service.py (1)
get_notifications(115-116)
tests/test_winget_details.py (1)
src/switchcraft/gui_modern/views/winget_view.py (3)
ModernWingetView(13-1053)_show_details_ui(465-767)_load_details(335-463)
tests/test_button_functionality.py (3)
src/switchcraft/gui_modern/views/home_view.py (1)
ModernHomeView(6-156)src/switchcraft/gui_modern/views/category_view.py (1)
CategoryView(41-116)src/switchcraft/gui_modern/views/winget_view.py (1)
ModernWingetView(13-1053)
tests/test_github_login_real.py (2)
tests/test_github_login.py (4)
mock_page(13-34)run_task(22-23)mock_open(27-31)mock_auth_service(38-41)src/switchcraft/services/auth_service.py (2)
initiate_device_flow(27-44)poll_for_token(47-90)
src/switchcraft/gui_modern/views/analyzer_view.py (1)
src/switchcraft/utils/i18n.py (1)
get(179-206)
tests/test_intune_store_search.py (2)
src/switchcraft/gui_modern/views/intune_store_view.py (4)
_get_token(119-125)_show_error(234-247)_run_search(127-232)_update_list(249-292)src/switchcraft/services/intune_service.py (2)
search_apps(404-464)list_apps(372-402)
src/switchcraft/gui_modern/views/intune_store_view.py (2)
src/switchcraft/utils/i18n.py (1)
get(179-206)src/switchcraft/gui_modern/views/intune_view.py (1)
_update_ui(540-542)
🪛 actionlint (1.7.10)
.github/workflows/test.yml
15-15: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
16-16: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
.github/workflows/ci.yml
29-29: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
30-30: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🪛 LanguageTool
docs/Intune_Configuration_Guide.md
[style] ~3-~3: Try moving the adverb to make the sentence clearer.
Context: ...uration Guide This guide describes how to correctly configure SwitchCraft policies using Microsoft Intune Custom Profiles ...
(SPLIT_INFINITIVE)
🔇 Additional comments (63)
.github/workflows/test.yml (2)
38-44: LGTM!Adding explicit
CIandGITHUB_ACTIONSenvironment variables is a reasonable practice for tests that may check these values, even though GitHub Actions typically sets them automatically.
52-55: Python 3.14 is stable and well-supported; however, Python 3.15 is currently alpha-only.
actions/setup-python@v6supports Python 3.14 (stable versions available: 3.14.2, 3.14.1, 3.14.0) and aligns with the test-backend matrix. However, Python 3.15 is only available as pre-release alpha versions (3.15.0-alpha.5 through 3.15.0-alpha.1). The test-backend job's conditional logic (limiting 3.15 runs to main branch pushes) appears to account for this instability. If using 3.15 alpha is intentional, ensure CI expectations account for potential breaking changes in future releases.docs/.vitepress/config.mts (1)
152-154: The configurationcors: falseis appropriate for a VitePress documentation site in local development. While it is technically more restrictive thancors: true, Vite's security guidance recommends against usingcors: truedue to the security risk of allowing any origin. For a documentation site that doesn't require cross-origin requests during local development,cors: falseis the correct choice.Likely an incorrect or invalid review comment.
docs/Intune_Configuration_Guide.md (3)
1-16: Excellent error guidance and clear introduction!The prominent warning about error
-2016281112and the explanation of the Data Type confusion is extremely valuable. This will save administrators significant troubleshooting time. The distinction between selecting "String" in the Intune Portal while providing XML content as the value is clearly articulated.Note: The static analysis hint about "split infinitive" on line 3 is a false positive—"how to correctly configure" is perfectly acceptable and idiomatic English.
17-23: LGTM! Clear prerequisite documentation.The ADMX ingestion prerequisite is clearly documented with the necessary OMA-URI path and data type.
24-202: Excellent structure and consistency across all policy settings!The policy documentation is well-organized and consistently formatted:
- Clear hierarchical structure by category
- All XML snippets follow the same pattern (
<enabled/>,<disabled/>, or<enabled/>with<data>elements)- Appropriate use of placeholders for sensitive data (API keys, client secrets, GUIDs, thumbprints)
- Valid values documented for dropdown fields (Theme, UpdateChannel, AIProvider)
- Each policy includes OMA-URI path, data type, and XML examples
The examples make it easy to copy and adapt for actual deployments.
.github/workflows/release.yml (1)
151-179: LGTM: fallback propagation during the dev bump looks consistent.scripts/build_release.ps1 (4)
155-174: LGTM: clearer fallback defaults and warnings.
228-231: No review comment (whitespace-only change).
234-255: Nice extraction of Inno Setup discovery into a helper.
354-356: Good reuse of Get-InnoSetupPath for both installer flows.Also applies to: 406-408
.github/workflows/docs_preview.yml (1)
48-55: LGTM — preview base path configured.src/switchcraft/gui_modern/views/analyzer_view.py (1)
492-496: LGTM — SFX notice layout tweak looks good.src/switchcraft/gui_modern/views/stack_manager_view.py (1)
41-55: LGTM — description row expansion should prevent clipping.src/switchcraft/gui_modern/views/wingetcreate_view.py (1)
100-118: LGTM — manifest info row expansion looks good..github/workflows/ci.yml (2)
3-17: LGTM — workflow triggers re-enabled with docs/markdown ignores.
55-56: LGTM — explicit CI flags set for tests.src/switchcraft/gui_modern/views/winget_view.py (3)
243-245: Good defensive handling for None results.Normalizing
Noneto an empty list before calling_show_listprevents downstream errors. This aligns well with the guard added in_show_list.
277-281: LGTM - Defensive None guard.This guard ensures stable behavior when
resultsisNone, preventingTypeErroronlen(results)and during filtering.
694-732: Robust UI refresh pattern with proper sequencing.Creating a new
Columninstance and re-assigning bothdetails_areaandright_pane.contentis the correct approach to force Flet to recognize changes. The sequential update order (child → parent → view → page) is appropriate.The extensive exception handling with debug logging is good for diagnosing issues without crashing.
src/switchcraft/services/intune_service.py (2)
394-402: Good: Explicit re-raising of network exceptions enables proper error handling upstream.Re-raising
TimeoutandRequestExceptionseparately allows callers to handle these cases distinctly (e.g., show timeout-specific UI messages).
445-464: Correct early propagation of network errors prevents unnecessary fallback attempts.Re-raising
TimeoutandRequestExceptionimmediately is the right approach—there's no point falling back to client-side filtering if the network is unavailable.tests/test_winget_details.py (2)
143-154: Good recursive error text collection pattern.The
collect_texthelper effectively traverses the control tree to find error messages. The assertion correctly checks for error indicators.
24-32: The mock chain is correctly configured and will properly interceptWingetHelper. The fixture matches the actual access pattern inwinget_view.py:
AddonService()→mock_addon.return_value.import_addon_module(...)→mock_addon_instance.import_addon_module.return_value.WingetHelper()→mock_addon_instance.import_addon_module.return_value.WingetHelper.return_valueThe code accesses these attributes in the exact order the mock chain assumes, so the mock will be applied correctly.
Likely an incorrect or invalid review comment.
tests/test_all_buttons.py (4)
14-28: Good recursive button discovery helper.The function correctly handles multiple container types (
controls,content,actions). Type checking for various button types is thorough.
30-47: Previous review feedback addressed: Test coupling fixed.The shared logic is now extracted into
collect_all_buttons()helper function, andtest_all_views_have_buttons()no longer returns a value. This addresses the coupling issue from the previous review.Also applies to: 49-126
147-153: Improved: Now asserts on buttons without handlers instead of always passing.This is a significant improvement over the previous
assert True. The test now properly fails if buttons lack handlers.
245-247: Previous review feedback addressed: Meaningful assertions added.The weak
assert len(buttons) >= 0has been replaced with proper type checking and a requirement for at least one button. Good improvement.tests/test_all_three_issues.py (4)
14-47: Comprehensive mock_page fixture covers all necessary attributes.The fixture properly mocks
dialog,end_drawer,run_task, andopenbehaviors. Themock_openhelper correctly handles bothAlertDialogandNavigationDrawertypes.
77-79: Good practice: CI environment skip for time-dependent tests.Skipping tests with
time.sleep()in CI environments prevents flaky failures. Consider also using@pytest.mark.slowto allow selective execution.Also applies to: 128-130
107-118: Recursive dropdown finder is robust.The
find_dropdownfunction correctly traverses nested controls and checks for language-related labels. This handles various UI structures.
195-220: Existence tests provide good baseline coverage.These tests verify that critical UI elements exist and have handlers. They're fast and don't require
time.sleep(), making them reliable for CI.src/switchcraft/gui_modern/views/intune_store_view.py (1)
185-200: Timeout completion guard looks solid.The additional timeout logging and default-error fallback make the background search behavior deterministic.
tests/test_ui_interactions_critical.py (6)
14-47: Mock page fixture covers the required UI hooks.
51-67: AuthService patching fixture looks good.
143-170: Drawer interaction test reads well.
173-193: Good guardrail around language dropdown wiring.
195-209: Login button existence test is clear and focused.
211-220: Notification button wiring check looks good.tests/test_github_login_real.py (4)
11-34: Mock page fixture is well-scoped for the tests.
37-54: AuthService fixture setup is solid.
82-100: Language change refresh assertion is straightforward.
103-121: Notification drawer test is clear and consistent.src/switchcraft/gui_modern/app.py (6)
165-199: Drawer open/close hardening is solid.
278-305: Open sequence and post-open verification look good.
815-818: Loading-screen sequencing comment is helpful.
1008-1009: Post-add update ensures the layout renders immediately.
1225-1231: Extra drawer-open enforcement is a good safeguard.
1823-1824: Explicit app exposure on the page is clear.tests/test_button_functionality.py (2)
10-26: Mock page fixture is comprehensive for UI tests.
30-38: Recursive clickable discovery helper is clear.tests/test_intune_store_search.py (4)
13-23: LGTM - Mock page fixture is well-structured.The fixture correctly mocks
ft.Pagewithrun_taskexecuting immediately for synchronous testing.
26-32: LGTM - IntuneService mock fixture is appropriate.The context manager properly patches at the import location and yields the instance.
91-115: LGTM - Network error test is well-structured.The test correctly mocks a
RequestException, triggers search, and asserts error handling.
118-147: LGTM - Successful results test validates the happy path.The test properly mocks search results and verifies they propagate to
_update_list.src/switchcraft/gui_modern/views/settings_view.py (8)
123-127: LGTM - Improved handler binding pattern.Using a named function
_handle_lang_changewith a guard check fore.control.valueis cleaner and safer than a direct lambda assignment.
667-708: Good defensive pattern for GitHub login initialization.The flow correctly shows a loading dialog immediately, then runs network operations in background. Error handling properly closes the loading dialog before showing error snacks.
One minor observation: The
_init_flowfunction is called from a background thread (_flow_complete), but it also callsself.app_page.update()on lines 697 and 705. While this appears to work in practice with Flet, it's technically a UI update from a background thread.Consider wrapping UI updates in
_init_flowwithrun_taskfor consistency:def _update_on_error(): loading_dlg.open = False self.app_page.update() self._show_snack("Login init failed", "RED") if hasattr(self.app_page, 'run_task'): self.app_page.run_task(_update_on_error) else: _update_on_error()
730-738: Good UX improvement - Selectable text fields.Making the verification URL and user code selectable (lines 734, 736) improves usability for manual copy operations.
741-778: Comprehensive dialog presentation fallbacks.The multiple fallback paths (page.open → manual assignment → secondary fallback) ensure the dialog opens across different Flet versions and configurations. The logging statements aid debugging.
780-800: Token polling correctly uses background thread with main-thread UI update.The
_poll_tokenruns in a daemon thread and properly usesrun_taskto marshal UI updates back to the main thread.
802-830: Flow completion handles threading correctly with appropriate fallbacks.The orchestration properly starts the flow in a background thread and uses
run_taskto show the dialog on the main thread. Multiple fallback paths handle edge cases.
998-1008: Good addition of logging for language change debugging.The debug/info log statements help trace language change flow issues.
1056-1105: Language reload follows consistent pattern with appropriate fallbacks.The
_reload_appfunction and its invocation viarun_taskwith fallbacks mirrors the GitHub login pattern, ensuring main-thread safety for UI updates. All error paths consistently show a success/fallback message to the user.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Python 3.15 only on main branch push (not PRs) to save time | ||
| if: | | ||
| matrix.python-version == '3.14' || | ||
| (matrix.python-version == '3.15' && github.event_name == 'push' && github.ref == 'refs/heads/main') | ||
| strategy: | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest] | ||
| python-version: ["3.11", "3.12"] | ||
| python-version: ["3.14", "3.15"] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Actions job level if condition matrix context available documentation
💡 Result:
Short answer: You cannot use the matrix context inside a job-level if because GitHub Actions evaluates jobs.<job_id>.if before applying the job's strategy.matrix; the matrix is therefore unavailable at that level. Use a step-level if (steps can see matrix), or move the conditional into the matrix definition (include/exclude) or use a preceding job to set outputs and gate the job with needs. [1][2]
Sources:
- Workflow syntax: jobs.<job_id>.if is evaluated before strategy.matrix is applied. [1]
- Contexts reference: table of context availability showing jobs.<job_id>.if does not include the matrix context. [2]
References:
[1] https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions
[2] https://docs.github.com/en/actions/reference/accessing-contextual-information-about-workflow-runs
🌐 Web query:
actions/setup-python@v6 Python 3.15 support ubuntu windows latest
💡 Result:
Short answer: Yes — actions/setup-python@v6 can install Python 3.15 on both ubuntu-latest and windows-latest, but as of Jan 16, 2026 Python 3.15 is still a prerelease (alpha) and therefore not guaranteed to be present in the runners' tool cache. Use the action's allow-prereleases option (or specify an exact prerelease build) so the action will fall back to/download the prerelease from releases if it's not cached.
How to (example):
- uses: actions/setup-python@v6
with:
python-version: '3.15'
allow-prereleases: true
Sources:
- actions/setup-python README (v6) — general behavior and inputs. [1]
- setup-python advanced usage: allow-prereleases behavior (falls back to prereleases when GA not available). [2]
- Python 3.15 release schedule / current prerelease status (3.15 is in alpha/beta phases as of Jan 2026). [3]
Fix job-level if that references matrix, and configure prerelease support for Python 3.15.
matrix context isn't available in job-level if (it's evaluated before strategy.matrix is applied), so this condition is invalid and will cause the job to be skipped or rejected. Split the 3.15 run into a separate job gated by github.*, or move the condition into step-level guards.
Additionally, Python 3.15 is currently a prerelease (alpha as of January 2026) and won't be available in the runner tool cache without explicit prerelease support. Add allow-prereleases: true to the setup-python action, or this will fail at runtime.
🔧 Minimal patch (then add a separate 3.15 job)
- if: |
- matrix.python-version == '3.14' ||
- (matrix.python-version == '3.15' && github.event_name == 'push' && github.ref == 'refs/heads/main')
strategy:
matrix:
os: [ubuntu-latest, windows-latest]
- python-version: ["3.14", "3.15"]
+ python-version: ["3.14"]For the separate 3.15 job (or when re-adding 3.15 to this matrix), use:
- uses: actions/setup-python@v6
with:
python-version: '3.15'
allow-prereleases: true🧰 Tools
🪛 actionlint (1.7.10)
29-29: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
30-30: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 Prompt for AI Agents
In @.github/workflows/ci.yml around lines 28 - 35, The job-level if currently
references the matrix context (matrix.python-version) which isn’t available at
job evaluation; remove that matrix-based condition from the job if and either
(a) split the Python 3.15 run into a separate job (e.g., python-3-15) with its
own job-level if guarded by github.event_name == 'push' && github.ref ==
'refs/heads/main', or (b) keep the matrix and move the 3.15-specific gating into
step-level if checks; additionally, when creating the separate 3.15 job (or when
adding 3.15 to setup-python), update the actions/setup-python@v6 usage to
include with: allow-prereleases: true so prerelease Python 3.15 is accepted.
| run: | | ||
| VERSION=$(python .github/scripts/generate_release_version.py --type ${{ github.event.inputs.release_type }}) | ||
| # Fallback to 2026.1.2 if version generation fails or returns invalid version | ||
| if [ -z "$VERSION" ] || [ "$VERSION" = "0.0.0" ] || ! echo "$VERSION" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+'; then | ||
| echo "Warning: Version generation failed or returned invalid version '$VERSION', using fallback: 2026.1.2" | ||
| VERSION="2026.1.2" | ||
| fi | ||
| echo "version=$VERSION" >> $GITHUB_OUTPUT | ||
| echo "is_prerelease=${{ github.event.inputs.release_type != 'stable' }}" >> $GITHUB_OUTPUT | ||
| echo "Generated version: $VERSION" |
There was a problem hiding this comment.
Tighten the version regex so malformed versions don’t bypass the fallback.
The current pattern only checks a prefix, so values like 1.2.3.4 or 1.2.3foo will pass. If you want strict MAJOR.MINOR.PATCH (optionally with pre-release/build), anchor the regex.
🛠️ Suggested fix
- if [ -z "$VERSION" ] || [ "$VERSION" = "0.0.0" ] || ! echo "$VERSION" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+'; then
+ if [ -z "$VERSION" ] || [ "$VERSION" = "0.0.0" ] || ! echo "$VERSION" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?(\+[0-9A-Za-z.-]+)?$'; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| VERSION=$(python .github/scripts/generate_release_version.py --type ${{ github.event.inputs.release_type }}) | |
| # Fallback to 2026.1.2 if version generation fails or returns invalid version | |
| if [ -z "$VERSION" ] || [ "$VERSION" = "0.0.0" ] || ! echo "$VERSION" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+'; then | |
| echo "Warning: Version generation failed or returned invalid version '$VERSION', using fallback: 2026.1.2" | |
| VERSION="2026.1.2" | |
| fi | |
| echo "version=$VERSION" >> $GITHUB_OUTPUT | |
| echo "is_prerelease=${{ github.event.inputs.release_type != 'stable' }}" >> $GITHUB_OUTPUT | |
| echo "Generated version: $VERSION" | |
| run: | | |
| VERSION=$(python .github/scripts/generate_release_version.py --type ${{ github.event.inputs.release_type }}) | |
| # Fallback to 2026.1.2 if version generation fails or returns invalid version | |
| if [ -z "$VERSION" ] || [ "$VERSION" = "0.0.0" ] || ! echo "$VERSION" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?(\+[0-9A-Za-z.-]+)?$'; then | |
| echo "Warning: Version generation failed or returned invalid version '$VERSION', using fallback: 2026.1.2" | |
| VERSION="2026.1.2" | |
| fi | |
| echo "version=$VERSION" >> $GITHUB_OUTPUT | |
| echo "is_prerelease=${{ github.event.inputs.release_type != 'stable' }}" >> $GITHUB_OUTPUT | |
| echo "Generated version: $VERSION" |
🤖 Prompt for AI Agents
In @.github/workflows/release.yml around lines 41 - 50, The VERSION validation
uses an unanchored grep which lets malformed strings like "1.2.3.4" or
"1.2.3foo" pass; update the grep check that references the VERSION variable in
the if condition to use a strict anchored regex that matches only
MAJOR.MINOR.PATCH and optional pre-release/build identifiers (e.g. anchor with ^
and $ and allow an optional -pre and +build suffix pattern), so the fallback to
"2026.1.2" triggers for any invalid format.
| if: | | ||
| inputs.component == 'backend' && | ||
| ( | ||
| matrix.python-version == '3.14' || | ||
| (matrix.python-version == '3.15' && github.event_name == 'push' && github.ref == 'refs/heads/main') | ||
| ) | ||
| strategy: | ||
| fail-fast: true | ||
| max-parallel: 1 | ||
| matrix: | ||
| python-version: ["3.13", "3.14"] | ||
| python-version: ["3.14", "3.15"] |
There was a problem hiding this comment.
matrix context is not available at the job-level if condition.
The matrix context can only be accessed within job steps, not in the job-level if expression. As flagged by actionlint, only github, inputs, needs, and vars contexts are available here. This will cause the workflow to fail or behave unexpectedly.
To conditionally run specific matrix combinations, use include/exclude in the matrix definition or move the version-specific condition to a step-level if.
🔧 Suggested fix using step-level conditions
test-backend:
runs-on: windows-latest
- if: |
- inputs.component == 'backend' &&
- (
- matrix.python-version == '3.14' ||
- (matrix.python-version == '3.15' && github.event_name == 'push' && github.ref == 'refs/heads/main')
- )
+ if: inputs.component == 'backend'
strategy:
fail-fast: true
max-parallel: 1
matrix:
python-version: ["3.14", "3.15"]
+ exclude:
+ - python-version: "3.15"
+ include:
+ - python-version: "3.15"
+ run-condition: ${{ github.event_name == 'push' && github.ref == 'refs/heads/main' }}Alternatively, add a step-level condition to skip steps for Python 3.15 on non-main pushes:
- name: Run All Python Tests
if: matrix.python-version == '3.14' || (github.event_name == 'push' && github.ref == 'refs/heads/main')
run: |
pytest tests/ -v --tb=short🧰 Tools
🪛 actionlint (1.7.10)
15-15: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
16-16: context "matrix" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 Prompt for AI Agents
In @.github/workflows/test.yml around lines 13 - 23, The job-level if currently
references matrix.python-version (and uses matrix in the job if), which is
invalid; instead either (A) move the Python-version-specific logic into
step-level conditions by keeping the matrix in the job and adding an if on the
test step (use matrix.python-version and github.event_name/github.ref inside the
step's if), or (B) encode the allowed combinations using the matrix
include/exclude feature in the matrix definition so the job-level if only uses
allowed contexts (e.g., inputs/component); update references to
matrix.python-version in the job-level if to be removed and place them on the
relevant step's if or into the matrix include/exclude block to restrict Python
3.15 runs to main push events.
| Write-Host "`nTo build installers, please install Inno Setup:" -ForegroundColor Yellow | ||
| Write-Host " - Download from: https://jrsoftware.org/isdl.php" -ForegroundColor Yellow | ||
| Write-Host " - Or install via winget: winget install JRSoftware.InnoSetup" -ForegroundColor Yellow | ||
| Write-Host " - Or install via chocolatey: choco install innosetup" -ForegroundColor Yellow | ||
|
|
||
| # Offer to install via winget if available | ||
| $wingetAvailable = (Get-Command "winget" -ErrorAction SilentlyContinue) | ||
| if ($wingetAvailable) { | ||
| $response = Read-Host "`nWould you like to install Inno Setup via winget now? (y/N)" | ||
| if ($response -eq 'y' -or $response -eq 'Y') { | ||
| Write-Host "Installing Inno Setup via winget..." -ForegroundColor Cyan | ||
| try { | ||
| winget install --id JRSoftware.InnoSetup --silent --accept-package-agreements --accept-source-agreements | ||
| Write-Host "Inno Setup installed successfully. Please run the build script again." -ForegroundColor Green | ||
| } catch { | ||
| Write-Warning "Failed to install Inno Setup via winget: $_" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid interactive prompts in CI/non-interactive shells.
Line 385 uses Read-Host, which can block GitHub Actions or other non-interactive runs. Gate the prompt to interactive sessions (or local-only runs) and skip in CI.
🛠️ Suggested fix
- $response = Read-Host "`nWould you like to install Inno Setup via winget now? (y/N)"
- if ($response -eq 'y' -or $response -eq 'Y') {
- Write-Host "Installing Inno Setup via winget..." -ForegroundColor Cyan
- try {
- winget install --id JRSoftware.InnoSetup --silent --accept-package-agreements --accept-source-agreements
- Write-Host "Inno Setup installed successfully. Please run the build script again." -ForegroundColor Green
- } catch {
- Write-Warning "Failed to install Inno Setup via winget: $_"
- }
- }
+ if (-not $env:CI -and -not $env:GITHUB_ACTIONS -and $Host.UI -and $Host.UI.RawUI) {
+ $response = Read-Host "`nWould you like to install Inno Setup via winget now? (y/N)"
+ if ($response -match '^[yY]$') {
+ Write-Host "Installing Inno Setup via winget..." -ForegroundColor Cyan
+ try {
+ winget install --id JRSoftware.InnoSetup --silent --accept-package-agreements --accept-source-agreements
+ Write-Host "Inno Setup installed successfully. Please run the build script again." -ForegroundColor Green
+ } catch {
+ Write-Warning "Failed to install Inno Setup via winget: $_"
+ }
+ }
+ } else {
+ Write-Host "Skipping interactive install prompt in CI/non-interactive shell." -ForegroundColor Gray
+ }🤖 Prompt for AI Agents
In `@scripts/build_release.ps1` around lines 377 - 395, The script currently uses
Read-Host (at the $response prompt) which blocks non-interactive CI; wrap the
interactive prompt and subsequent winget install attempt in a guard that only
runs for local/interactive sessions (e.g. check that neither $env:CI nor
$env:GITHUB_ACTIONS is set), so replace the current unconditional Read-Host
block with an if (-not $env:CI -and -not $env:GITHUB_ACTIONS) { $response =
Read-Host ...; if ($response -in 'y','Y') { try { winget install --id
JRSoftware.InnoSetup ... } catch { Write-Warning ... } } } else { Write-Host
"Skipping interactive Inno Setup install in CI." -ForegroundColor Yellow } to
ensure $wingetAvailable, Read-Host and the winget install are only executed
interactively.
| self.results_list.controls.append(ft.ProgressBar()) | ||
| self.results_list.controls.append(ft.Text("Searching...", color="GREY_500", italic=True)) | ||
| self.update() | ||
| self.results_list.controls.append(ft.Text(i18n.get("searching") or "Searching...", color="GREY_500", italic=True)) |
There was a problem hiding this comment.
Ensure i18n fallback works when translations are missing.
i18n.get() returns the key when a translation is missing, so the or "Searching..." fallback won’t trigger. Pass a default to avoid showing the raw key.
🛠️ Suggested fix
- self.results_list.controls.append(ft.Text(i18n.get("searching") or "Searching...", color="GREY_500", italic=True))
+ self.results_list.controls.append(
+ ft.Text(i18n.get("searching", default="Searching..."), color="GREY_500", italic=True)
+ )🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/views/intune_store_view.py` at line 131, The
current call to i18n.get("searching") can return the key itself when a
translation is missing, so the "or" fallback won't work; update the code that
appends to self.results_list.controls (the ft.Text creation) to pass a
default/fallback directly to i18n.get (e.g., use the i18n.get signature that
accepts a default like i18n.get("searching", "Searching...")) so ft.Text always
receives a human-friendly string even when the translation is absent.
| lang_dd = None | ||
| for control in general_tab.controls: | ||
| if isinstance(control, ft.Dropdown): | ||
| lang_dd = control | ||
| break |
There was a problem hiding this comment.
Language dropdown search may miss nested dropdowns.
Unlike test_language_change_updates_ui which uses a recursive finder, this test only checks direct children of general_tab.controls. This could cause the test to fail if the dropdown is nested.
🐛 Suggested fix: Use recursive search
general_tab = view._build_general_tab()
lang_dd = None
- for control in general_tab.controls:
- if isinstance(control, ft.Dropdown):
- lang_dd = control
- break
+ def find_dropdown(control):
+ if isinstance(control, ft.Dropdown):
+ return control
+ if hasattr(control, 'controls'):
+ for child in control.controls:
+ result = find_dropdown(child)
+ if result:
+ return result
+ return None
+ lang_dd = find_dropdown(general_tab)🤖 Prompt for AI Agents
In `@tests/test_all_three_issues.py` around lines 182 - 186, The loop that assigns
lang_dd by iterating only general_tab.controls can miss nested Dropdowns; change
the lookup to perform a recursive search through general_tab's descendants (same
approach used in test_language_change_updates_ui) to find the first ft.Dropdown,
so that lang_dd is set even when the dropdown is nested instead of a direct
child of general_tab.controls.
| def test_intune_search_shows_timeout_error(mock_page, mock_intune_service): | ||
| """Test that Intune search shows timeout error after 60 seconds.""" | ||
| from switchcraft.gui_modern.views.intune_store_view import ModernIntuneStoreView | ||
| import os | ||
| import threading | ||
|
|
||
| # Skip in CI to avoid 70 second wait | ||
| if os.environ.get('CI') == 'true' or os.environ.get('GITHUB_ACTIONS') == 'true': | ||
| pytest.skip("Skipping test with long time.sleep in CI environment") | ||
|
|
||
| # Mock slow search that times out | ||
| def slow_search(token, query): | ||
| time.sleep(70) # Simulate timeout (longer than 60 second timeout) | ||
| return [] | ||
| mock_intune_service.search_apps = slow_search | ||
| mock_intune_service.list_apps = slow_search | ||
|
|
||
| # Mock token | ||
| view = ModernIntuneStoreView(mock_page) | ||
| view._get_token = lambda: "mock_token" | ||
|
|
||
| # Track error calls | ||
| error_calls = [] | ||
| def track_error(msg): | ||
| error_calls.append(msg) | ||
| view._show_error = track_error | ||
|
|
||
| # Mock Thread.join to simulate timeout immediately | ||
| original_thread = threading.Thread | ||
| def mock_thread(target=None, daemon=False, **kwargs): | ||
| thread = original_thread(target=target, daemon=daemon, **kwargs) | ||
| # Override join to simulate timeout | ||
| original_join = thread.join | ||
| def mock_join(timeout=None): | ||
| # Simulate timeout by returning immediately (thread still alive) | ||
| return None | ||
| thread.join = mock_join | ||
| return thread | ||
| threading.Thread = mock_thread | ||
|
|
||
| try: | ||
| # Start search | ||
| view.search_field.value = "test" | ||
| view._run_search(None) | ||
|
|
||
| # Wait a bit for the timeout handling to complete | ||
| time.sleep(0.2) | ||
|
|
||
| # Check that timeout error was shown | ||
| assert len(error_calls) > 0, "Timeout error should have been shown" | ||
| assert any("timeout" in str(msg).lower() or "60 seconds" in str(msg) for msg in error_calls), \ | ||
| f"Expected timeout message, but got: {error_calls}" | ||
| finally: | ||
| threading.Thread = original_thread |
There was a problem hiding this comment.
Test has race condition and redundant imports.
Issues identified:
-
Redundant imports:
os(line 38) andthreading(line 39) are already imported at module level (lines 7, 10). -
Race condition: The
slow_searchfunction (lines 46-48) withtime.sleep(70)is defined but never actually invoked becausemock_joinreturns immediately (line 70). The test relies onis_alive()returningTrueby default on the unmocked thread, but there's no explicit mock foris_alive()in this test (unliketest_intune_search_timeout_mechanism). -
Global monkey-patch risk: Directly assigning to
threading.Thread(line 73) affects the global namespace. While wrapped in try/finally, if an exception occurs before line 73, the finally block will restore the wrong value.
Suggested improvements
def test_intune_search_shows_timeout_error(mock_page, mock_intune_service):
"""Test that Intune search shows timeout error after 60 seconds."""
from switchcraft.gui_modern.views.intune_store_view import ModernIntuneStoreView
- import os
- import threading
# Skip in CI to avoid 70 second wait
if os.environ.get('CI') == 'true' or os.environ.get('GITHUB_ACTIONS') == 'true':
@@ -63,6 +61,10 @@
def mock_thread(target=None, daemon=False, **kwargs):
thread = original_thread(target=target, daemon=daemon, **kwargs)
# Override join to simulate timeout
original_join = thread.join
def mock_join(timeout=None):
# Simulate timeout by returning immediately (thread still alive)
return None
thread.join = mock_join
+ # Make is_alive return True to simulate timeout
+ def mock_is_alive():
+ return True
+ thread.is_alive = mock_is_alive
return thread🤖 Prompt for AI Agents
In `@tests/test_intune_store_search.py` around lines 35 - 88, The test
test_intune_search_shows_timeout_error has redundant local imports and a
race/global-patch issue: remove the local imports of os and threading, stop
globally reassigning threading.Thread, and ensure the mocked thread accurately
simulates a running thread so slow_search is never invoked; instead use a local
Thread subclass or context-managed patching to override join and is_alive for
threads created by view._run_search. Concretely, replace the direct assignment
threading.Thread = mock_thread with a scoped monkeypatch/patch of
threading.Thread (or use unittest.mock.patch) when creating mock_thread, ensure
mock_thread returns a Thread instance whose is_alive returns True and join
returns immediately, and keep slow_search and view._get_token/view._show_error
as-is so the timeout branch in ModernIntuneStoreView._run_search is exercised
without global side effects.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.