View & CI fixes #49
View & CI fixes #49
Conversation
📝 WalkthroughWalkthroughAdds robust error handling and UI-safety utilities across the modern GUI, centralizes version parsing in the release script, hardens addon ZIP handling, expands localization greetings, modifies CI auto-merge to pre-check review comments, and adds/updates many tests and docs. Changes
Sequence Diagram(s)(omitted) 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/switchcraft/services/addon_service.py (1)
210-222: Zip Slip protection uses string prefix check which can be bypassed with sibling directory paths.The check at line 220 is vulnerable to path confusion when a zip contains members in sibling directories. For example, if
targetresolves to/home/user/addons/foo, a member like../foobar/file.txtwould be normalized and resolved to/home/user/addons/foobar/file.txt, which passes thestartswithcheck even though it's outside the target directory (sibling, not child).The project requires Python 3.9+, so use
Path.relative_to()to safely validate path containment:🔒 Safer Zip Slip protection
# Ensure the resolved path starts with the target directory (prevent Zip Slip) - if not str(file_path).startswith(str(target.resolve())): + try: + file_path.relative_to(target.resolve()) + except ValueError: logger.error(f"Security Alert: Attempted Zip Slip with {member.filename}") continue
🤖 Fix all issues with AI agents
In @.github/workflows/review-auto-merge.yml:
- Around line 76-113: The notification double-counts CodeRabbit inline comments
because INLINE_REVIEW_COMMENTS includes all inline comments while
TOTAL_CODERABBIT_COMMENTS also sums CodeRabbit inline comments; update the
messaging to avoid overlap by: stop using TOTAL_CODERABBIT_COMMENTS in the
notification summary, instead report INLINE_REVIEW_COMMENTS for all inline
comments and separately report CODERABBIT_INLINE (how many of those inline
comments are from CodeRabbit) and CODERABBIT_PR_COMMENTS (CodeRabbit non-inline
PR comments), and adjust the BODY construction (replace uses of
TOTAL_CODERABBIT_COMMENTS) so it mentions CODERABBIT_INLINE and
CODERABBIT_PR_COMMENTS distinctly; keep existing variables
INLINE_REVIEW_COMMENTS, CODERABBIT_INLINE, CODERABBIT_PR_COMMENTS,
COMMENT_EXISTS and BODY to locate the changes.
In `@scripts/build_release.ps1`:
- Around line 172-186: The parsed pyproject version may be malformed so
$VersionInfo.Numeric can be empty; after extracting with Extract-VersionInfo
(when reading $PyProjectFile and matching $VersionLine) validate that
$VersionInfo.Numeric is non-empty and well-formed, and if not, reassign
$VersionInfo by calling Extract-VersionInfo -VersionString $FallbackVersion (or
otherwise set $AppVersion/$AppVersionNumeric/$AppVersionInfo from
$FallbackVersion), then Write-Host a warning mentioning the fallback; update
references to $AppVersion, $AppVersionNumeric, and $AppVersionInfo accordingly
to ensure installer metadata never receives an empty numeric version.
In `@src/switchcraft_winget/utils/winget.py`:
- Around line 208-215: get_package_details now raises on "not found", but the
legacy GUI in winget_view.py does not catch those exceptions (it runs in a
daemon thread), causing silent failures; update the legacy view
(src/switchcraft/gui/views/winget_view.py) to wrap calls to get_package_details
in a try/except that catches the exception raised for missing packages and
handles it like the modern GUI (e.g., show a "not found" state or log and
continue), or if the old view is deprecated, document/confirm deprecation and
remove its usage; specifically locate the call sites in winget_view.py where
get_package_details is invoked and add explicit exception handling consistent
with the behavior in src/switchcraft/gui_modern/views/winget_view.py so
missing-package exceptions are handled rather than causing silent thread
failures.
In `@src/switchcraft/gui_modern/utils/view_utils.py`:
- Around line 194-206: The fallback incorrectly calls func() directly (which
returns a coroutine) when page.run_task(func) fails; replace that direct call
with proper coroutine execution: import asyncio and attempt to run the coroutine
by using asyncio.run(func()) when no event loop is running, otherwise schedule
it on the running loop (e.g., loop.create_task(func()) or
asyncio.create_task(func())); update the code inside the except block that
currently references func() to detect a running loop and either
asyncio.run(func()) or create_task(func()) so the coroutine actually executes.
In `@src/switchcraft/gui_modern/views/intune_store_view.py`:
- Around line 213-215: The stray logger.exception call referencing an undefined
ex must be removed or moved into the corresponding except block; locate the call
in intune_store_view.py near the UI update code (around the call to
self._run_task_safe(_update_ui)) and either delete the line
"logger.exception(f\"Failed to update UI: {ex}\")" or wrap the scheduling in a
try/except that logs the actual exception variable (e.g., except Exception as
ex: logger.exception("Failed to update UI", exc_info=ex)); ensure no reference
to ex exists outside an except block and that logging uses exc_info or the
caught exception variable.
In `@src/switchcraft/gui_modern/views/settings_view.py`:
- Around line 1063-1064: The assignment to app must be inside the hasattr check
to avoid a syntax/control-flow bug: indent "app = self.app_page.switchcraft_app"
so it is executed only when hasattr(self.app_page, 'switchcraft_app') is True
(i.e., make it part of the if block in settings_view.py), and if subsequent code
expects app to exist consider adding an else or guard using that same attribute
check where functions/methods reference app.
- Around line 1354-1358: The current duplicate-check uses `if handler not in
root_logger.handlers` which fails because `handler` is a new instance; instead
check for existing handler by type: before adding, use something like `if not
any(isinstance(h, FletLogHandler) for h in root_logger.handlers)` (or compare
`h.__class__ is handler.__class__`) and only then call
`root_logger.addHandler(handler)` and `logger.info(...)`; update the block
around `root_logger`, `handler`, and the info log to use this type-based
presence check.
In `@src/switchcraft/utils/logging_handler.py`:
- Around line 123-126: The code in set_debug_mode uses an undefined variable
`logger`, causing a NameError at runtime; fix it by replacing `logger` with the
existing `root_logger` (used elsewhere in this module) or by adding a
module-level logger (e.g., set `logger = logging.getLogger(__name__)`) and then
use that `logger` inside set_debug_mode to emit the two info messages about
debug mode. Ensure the chosen symbol (`root_logger` or newly defined `logger`)
is in scope for the set_debug_mode function.
In `@tests/test_gui_views.py`:
- Around line 99-105: The test replaces GroupManagerView._has_credentials with a
zero-arg function which will raise TypeError when bound and also leaks the
override globally; change mock_has_credentials to accept self (def
mock_has_credentials(self): return True) and apply it using
unittest.mock.patch.object (or patch.object as a context manager/decorator)
targeting GroupManagerView._has_credentials so the override is scoped to the
test and automatically restored after the test finishes; update the test to
create the view inside the patch scope (where view = GroupManagerView(page) is
executed).
🧹 Nitpick comments (12)
src/switchcraft/services/addon_service.py (1)
164-180: Manifest search logic has redundant condition.At line 174, checking
normalized == 'manifest.json'is redundant since this case is already handled by line 165. If we reach theelseblock (lines 167-180), then"manifest.json" in fileswas already false, meaning exact root-level match won't exist.The
endswith('/manifest.json')pattern is correct for subdirectory manifests. The logic still works but has unnecessary branching.♻️ Simplified manifest search
- # Check for manifest.json at root first - if "manifest.json" in files: - manifest_path = "manifest.json" - else: - # Check for manifest.json in common subdirectories - # Some ZIPs might have it in a subfolder - for file_path in files: - # Normalize path separators - normalized = file_path.replace('\\', '/') - # Check if it's manifest.json at any level (but prefer root) - if normalized.endswith('/manifest.json') or normalized == 'manifest.json': - # Prefer root-level, but accept subdirectory if root not found - if manifest_path is None or normalized == 'manifest.json': - manifest_path = file_path - # If we found root-level, stop searching - if normalized == 'manifest.json': - break + # Check for manifest.json at root first + if "manifest.json" in files: + manifest_path = "manifest.json" + else: + # Check for manifest.json in subdirectories + for file_path in files: + normalized = file_path.replace('\\', '/') + if normalized.endswith('/manifest.json'): + manifest_path = file_path + break # Use first found subdirectory manifestsrc/switchcraft/gui_modern/views/settings_view.py (2)
124-144: Double-wrapped error handler is redundant.
safe_lang_handlerwraps_handle_lang_change, which already has its own try/except. If an exception occurs in_handle_lang_change, it's caught at line 132-134, showing a snack. Thensafe_lang_handlerat lines 140-142 catches nothing (since the inner handler already caught it) and would only trigger if the snack itself fails.Consider consolidating to a single error handling layer.
♻️ Consolidated error handler
- # Set on_change handler - wrap in safe handler to catch errors - def _handle_lang_change(e): - try: - if e.control.value: - logger.info(f"Language dropdown changed to: {e.control.value}") - self._on_lang_change(e.control.value) - else: - logger.warning("Language dropdown changed but value is None/empty") - except Exception as ex: - logger.exception(f"Error handling language change: {ex}") - self._show_snack(f"Failed to change language: {ex}", "RED") - - # Wrap handler to catch exceptions and show in error view - def safe_lang_handler(e): - try: - _handle_lang_change(e) - except Exception as ex: - logger.exception(f"Error in language change handler: {ex}") - self._show_error_view(ex, "Language dropdown change") - - lang_dd.on_change = safe_lang_handler + def _handle_lang_change(e): + try: + if e.control.value: + logger.info(f"Language dropdown changed to: {e.control.value}") + self._on_lang_change(e.control.value) + else: + logger.warning("Language dropdown changed but value is None/empty") + except Exception as ex: + logger.exception(f"Error handling language change: {ex}") + self._show_snack(f"Failed to change language: {ex}", "RED") + + lang_dd.on_change = _handle_lang_change
1813-1859: GPO certificate detection logic improved but has verification gap.The logic now properly checks
is_managed()first (lines 1819) before attempting to use GPO values. However, when verification fails (line 1850-1859), the code unconditionally treats GPO as configured even if the thumbprint/path is empty or invalid, which could lead to signing failures later.Consider adding validation that the GPO-configured value is actually usable before returning success.
src/switchcraft/gui_modern/views/home_view.py (2)
36-38: Moveimport randomto module level.Importing
randominside the method on each call is inefficient. Move this to the top of the file with other imports.♻️ Suggested refactor
At the top of the file (after line 2):
import randomThen remove line 37.
98-103: Defensive bounds check is good, but arrays are always equal length.The check on line 101 (
if selected_index < len(default_greetings)) is defensive but sincegreeting_keysanddefault_greetingsare defined together with matching lengths in each branch, this condition will always be true. The defensive check doesn't hurt, but consider usingzipor a list of tuples to guarantee parity:♻️ Alternative approach using paired tuples
# Example for one time period: greetings = [ ("greeting_morning_1", "Good Morning"), ("greeting_morning_2", "Morning!"), # ... ] greeting_key, default_greeting = random.choice(greetings)This eliminates the possibility of mismatched array lengths.
src/switchcraft/gui_modern/utils/view_utils.py (2)
5-5: Module-level traceback import is unused directly.The
tracebackimport at line 5 is shadowed byimport traceback as tbinside_show_error_view(line 24). Either remove line 5 or use it consistently instead of re-importing.♻️ Remove unused import
-import tracebackThe module-level import is not used; the method imports it locally as
tb.
151-243: Thorough fallback handling, but high complexity.The method handles many edge cases (no page, page without run_task, sync vs async functions) with multiple fallback paths. While comprehensive, the nested try/except blocks make this difficult to follow.
Consider documenting the expected fallback chain in the docstring or extracting some paths into helper methods.
src/switchcraft/gui_modern/app.py (2)
27-84: Duplication withViewMixin._show_error_view.This static method largely duplicates the logic in
ViewMixin._show_error_viewfromview_utils.py. Consider refactoring to share code:
- Extract the core crash view display logic into a shared utility function
- Have both methods call the shared function
The main difference is
page.views[-1].controlsaccess (lines 61-64) vs ViewMixin'sself.controlsapproach.♻️ Consider extracting shared logic
# In view_utils.py or a new crash_utils.py def display_crash_view(page: ft.Page, error: Exception, context: str = None): """Shared crash view display logic.""" # Common implementation here pass # Then in app.py: `@staticmethod` def _show_runtime_error(page, error, context=None): from switchcraft.gui_modern.utils.view_utils import display_crash_view display_crash_view(page, error, context)
369-383: Doublepage.update()may cause visual flicker.Lines 377 and 381 both call
page.update(). The second update (lines 379-383) with a try/except is defensive, but calling update twice in quick succession may cause visual flicker. Consider consolidating or adding a comment explaining why the second update is necessary.src/switchcraft_winget/utils/winget.py (1)
340-350: Use_get_subprocess_kwargsto avoid duplicated window‑hiding logic.
Install/verify paths still rebuildstartupinfo/creationflagsmanually (Lines 340–349, 668–677). Since_get_subprocess_kwargsnow centralizes this, reusing it keeps behavior consistent and reduces drift.♻️ Consolidate kwargs usage
- startupinfo = self._get_startup_info() - kwargs = {} - if startupinfo: - kwargs['startupinfo'] = startupinfo - import sys - if sys.platform == "win32": - if hasattr(subprocess, 'CREATE_NO_WINDOW'): - kwargs['creationflags'] = subprocess.CREATE_NO_WINDOW - else: - kwargs['creationflags'] = 0x08000000 # CREATE_NO_WINDOW constant - proc = subprocess.run(cmd, capture_output=True, text=True, timeout=300, **kwargs) + kwargs = self._get_subprocess_kwargs() + proc = subprocess.run(cmd, capture_output=True, text=True, timeout=300, **kwargs)- startupinfo = self._get_startup_info() - kwargs = {} - if startupinfo: - kwargs['startupinfo'] = startupinfo - import sys - if sys.platform == "win32": - if hasattr(subprocess, 'CREATE_NO_WINDOW'): - kwargs['creationflags'] = subprocess.CREATE_NO_WINDOW - else: - kwargs['creationflags'] = 0x08000000 - proc = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="ignore", timeout=30, **kwargs) + kwargs = self._get_subprocess_kwargs() + proc = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="ignore", timeout=30, **kwargs)Also applies to: 668-678
src/switchcraft/gui_modern/views/library_view.py (1)
136-197: Directory scans on the UI thread may still block the view.
Lines 136–197 do filesystem IO synchronously. For large folders this can freeze the UI. Consider moving the scan to a background thread and marshal UI updates via_run_task_safe.src/switchcraft/assets/lang/en.json (1)
558-594: LGTM! Comprehensive time-of-day greeting variants.The expanded greeting keys provide good UX variety and align with the German localization additions. The structure is consistent and the JSON is valid.
One minor semantic note:
greeting_night_2(line 592) uses "Evening!" which is slightly inconsistent with the "night" category—consider using a night-specific phrase like "Good night!" or "Nighttime!" for consistency.🔧 Optional: Align night greeting variant
- "greeting_night_2": "Evening!", + "greeting_night_2": "Good night!",
| if enabled: | ||
| logger.info("Debug mode enabled - all log levels (DEBUG, INFO, WARNING, ERROR, CRITICAL) will be captured") | ||
| else: | ||
| logger.info("Debug mode disabled - only INFO and above will be captured") |
There was a problem hiding this comment.
Undefined logger variable will cause NameError at runtime.
The logger variable used on lines 124 and 126 is never defined in this module. When set_debug_mode is called, this will raise a NameError.
🐛 Proposed fix
Either use root_logger which is already available in scope:
if enabled:
- logger.info("Debug mode enabled - all log levels (DEBUG, INFO, WARNING, ERROR, CRITICAL) will be captured")
+ root_logger.info("Debug mode enabled - all log levels (DEBUG, INFO, WARNING, ERROR, CRITICAL) will be captured")
else:
- logger.info("Debug mode disabled - only INFO and above will be captured")
+ root_logger.info("Debug mode disabled - only INFO and above will be captured")Or define a module-level logger at the top of the file:
logger = logging.getLogger(__name__)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if enabled: | |
| logger.info("Debug mode enabled - all log levels (DEBUG, INFO, WARNING, ERROR, CRITICAL) will be captured") | |
| else: | |
| logger.info("Debug mode disabled - only INFO and above will be captured") | |
| if enabled: | |
| root_logger.info("Debug mode enabled - all log levels (DEBUG, INFO, WARNING, ERROR, CRITICAL) will be captured") | |
| else: | |
| root_logger.info("Debug mode disabled - only INFO and above will be captured") |
🤖 Prompt for AI Agents
In `@src/switchcraft/utils/logging_handler.py` around lines 123 - 126, The code in
set_debug_mode uses an undefined variable `logger`, causing a NameError at
runtime; fix it by replacing `logger` with the existing `root_logger` (used
elsewhere in this module) or by adding a module-level logger (e.g., set `logger
= logging.getLogger(__name__)`) and then use that `logger` inside set_debug_mode
to emit the two info messages about debug mode. Ensure the chosen symbol
(`root_logger` or newly defined `logger`) is in scope for the set_debug_mode
function.
github-actions
bot
added
the
documentation
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/switchcraft/gui_modern/utils/view_utils.py (2)
168-188: Handle async funcs in the no-page fallbackLines 174 and 182 call
func()directly; iffuncis async, it won't execute and you'll leak a coroutine. Apply the same async handling pattern already used elsewhere in this method (lines 190+): checkinspect.iscoroutinefunction(func), then useasyncio.create_task()for running event loops orasyncio.run()as fallback.🔧 Proposed fix
except (RuntimeError, AttributeError): # No page available, try direct call as fallback try: - func() + if inspect.iscoroutinefunction(func): + try: + loop = asyncio.get_running_loop() + asyncio.create_task(func()) + except RuntimeError: + asyncio.run(func()) + else: + func() return True except Exception as e: logger.warning(f"Failed to execute function directly (no page): {e}", exc_info=True) return False if not page: # No page available, try direct call as fallback try: - func() + if inspect.iscoroutinefunction(func): + try: + loop = asyncio.get_running_loop() + asyncio.create_task(func()) + except RuntimeError: + asyncio.run(func()) + else: + func() return True except Exception as e: logger.warning(f"Failed to execute function directly (page None): {e}", exc_info=True) return False
359-454: Run fallback even without a page + respect async fallback types in sync pathLine 371 returns early when no page is present, completely skipping the promised fallback execution since
fallback_funcis only assigned at line 373. Additionally, the sync path (line 420) callsfallback_func()directly without checking if it's a coroutine function, while the else branch correctly handles this at line 430. This creates an inconsistency: iftask_funcis sync butfallback_funcis async, line 420 will call it without awaiting, causing a coroutine object to be created and discarded rather than executed.Move
fallback_funcassignment before the page check and add async handling to the sync path fallback to match the pattern already used in the else branch.src/switchcraft/gui_modern/views/group_manager_view.py (1)
153-265: Re-enable the list on error pathsLine 162 disables
list_container, but the HTTPError, ConnectionError, and generic Exception handlers (lines 188–244) only display error messages without re-enabling it, leaving the list permanently disabled after a failure. Only the BaseException handler and the success path re-enable it.Proposed fix
def show_error(): try: self._show_snack(error_msg, "RED") + self.list_container.disabled = False + self.update() except (RuntimeError, AttributeError) as e: logger.debug(f"Control not added to page (RuntimeError/AttributeError): {e}") @@ def show_error(): try: self._show_snack(error_msg, "RED") + self.list_container.disabled = False + self.update() except (RuntimeError, AttributeError) as e: logger.debug(f"Control not added to page (RuntimeError/AttributeError): {e}") @@ def show_error(): try: self._show_snack(error_msg, "RED") # Also update the list to show error if hasattr(self, 'groups_list'): self.groups_list.controls.clear() self.groups_list.controls.append( ft.Container( content=ft.Column([ ft.Icon(ft.Icons.ERROR_OUTLINE, color="RED", size=48), ft.Text(error_msg, color="RED", text_align=ft.TextAlign.CENTER) ], horizontal_alignment=ft.CrossAxisAlignment.CENTER), alignment=ft.alignment.center, padding=20 ) ) self.groups_list.update() + self.list_container.disabled = False + self.update() except (RuntimeError, AttributeError) as e: logger.debug(f"Control not added to page (RuntimeError/AttributeError): {e}")
🤖 Fix all issues with AI agents
In @.github/workflows/review-auto-merge.yml:
- Around line 81-88: The gh api calls that compute INLINE_REVIEW_COMMENTS,
CODERABBIT_INLINE, and CODERABBIT_PR_COMMENTS need pagination to avoid
undercounting; update each gh api invocation used to populate
INLINE_REVIEW_COMMENTS, CODERABBIT_INLINE and CODERABBIT_PR_COMMENTS to include
the --paginate flag so results are aggregated across all pages (keeping the same
--jq filters), ensuring the final counts reflect all pages of pull request
review comments and issue comments.
In `@src/switchcraft/gui_modern/app.py`:
- Around line 42-49: The code uses traceback.format_exc() which relies on an
active exception context; instead format the passed-in exception object `error`
in the helper (where `tb_str` is created). Replace tb.format_exc() with
formatting of the exception, e.g. use tb.format_exception(type(error), error,
error.__traceback__) (or tb.TracebackException.from_exception(error).format())
and join the result into a single string assigned to `tb_str`, preserving the
existing `if context:` prefix logic and leaving CrashDumpView usage unchanged.
In `@src/switchcraft/gui_modern/views/library_view.py`:
- Around line 273-283: The grid currently shows nothing when a search yields
zero matches; after computing filtered_files from self.all_files using
self.search_val, detect if filtered_files is empty and append a suitable "no
results" control to self.grid.controls instead of tiles (e.g., a centered
label/container with "No results" or similar). Update the block that iterates
over filtered_files and the call to self._create_tile(item) so that when
filtered_files is empty you create and append the empty-state control (and skip
creating tiles) to provide clear feedback to the user.
In `@src/switchcraft/gui_modern/views/settings_view.py`:
- Around line 714-719: The login button is set to "Starting..." with
ft.Icons.HOURGLASS_EMPTY in the block that references self.login_btn but never
restored on init/poll failures; update the failure and completion paths to reset
the button state. Specifically, in the init routine(s) and any poll/error
callbacks that handle initialization or polling failures (the methods that call
or wrap the block setting self.login_btn.text/icon), ensure you set
self.login_btn.text back to the original label, restore the original icon (or
None), and call self.login_btn.update() on both error paths and on successful
completion of polling so the button is never left stuck in the "Starting..."
state.
- Around line 1059-1061: There's a stray opening triple-quote that is turning
the body of _on_lang_change into a multiline string; remove the unintended
triple-quote so the subsequent lines (logger.info and logger.debug) and the rest
of the method become executable again, ensuring the method _on_lang_change
contains the logger.info(f"Language change requested: {val}") and
logger.debug(...) calls as real statements rather than part of a string.
In `@src/switchcraft/services/addon_service.py`:
- Around line 162-205: The manifest search currently accepts a nested
manifest_path but extraction still preserves the ZIP subfolder, so
read_manifest() fails expecting target/manifest.json; update the extraction
logic where files are written (the code that uses manifest_path and extracts ZIP
entries) to compute a manifest_dir =
os.path.dirname(manifest_path.replace('\\','/')) and, if manifest_dir is
non-empty, strip that leading prefix from every extracted file path (rebase
extraction into manifest_dir) so manifest.json and entry points end up at
target/manifest.json; also update the user-facing Exception message that lists
root_files to indicate if manifest was found under a subdirectory and show that
subdirectory; apply the same change to the other extraction branch referenced
(the block around lines 215-241) so both paths normalize extraction to the
manifest directory.
In `@tests/test_critical_ui_fixes.py`:
- Around line 141-145: The test currently contains a no-op assertion ("assert
True") which masks failures; replace it with a concrete assertion that verifies
the refresh action triggered data loading: patch or mock the _load_data method
before creating the view (or attach a spy), trigger the refresh button, wait for
the background thread, and assert that _load_data was called (or assert that
dir_info or the grid state changed accordingly). Alternatively, assert that
helper actions like goto_tab or the language setter were invoked by mocking
those methods and checking their call state instead of using assert True.
In `@tests/test_github_login_integration.py`:
- Around line 13-35: The fallback import block is missing a poll_until
implementation which will raise NameError if conftest import fails; add a
minimal poll_until function alongside is_ci_environment, skip_if_ci and
mock_page that accepts a predicate (callable), timeout and interval parameters,
polls the predicate until it returns True or the timeout elapses, and raises a
TimeoutError (or returns False) on timeout so tests depending on poll_until
behave consistently; ensure the symbol name is poll_until and its signature
matches how tests call it.
♻️ Duplicate comments (1)
scripts/build_release.ps1 (1)
182-187: Strengthen numeric validation (non-empty isn’t enough).
Values like1.2or1.2.still pass and can yield inconsistent metadata. ValidateMAJOR.MINOR.PATCH(or normalize) before accepting.🔧 Suggested hardening
- # Validate that the parsed version is non-empty and well-formed - if ([string]::IsNullOrWhiteSpace($VersionInfo.Numeric)) { - Write-Warning "Parsed version from pyproject.toml has empty numeric component, using fallback: $FallbackVersion" - $VersionInfo = Extract-VersionInfo -VersionString $FallbackVersion - } + # Validate numeric format (MAJOR.MINOR.PATCH) + if ($VersionInfo.Numeric -notmatch '^\d+\.\d+\.\d+$') { + Write-Warning "Invalid version format '$($VersionInfo.Full)' in pyproject.toml, using fallback: $FallbackVersion" + $VersionInfo = Extract-VersionInfo -VersionString $FallbackVersion + }
🧹 Nitpick comments (6)
src/switchcraft/utils/logging_handler.py (1)
111-127: PreviousNameErroris fixed, but line 123 is redundant.The fix for the undefined
loggervariable is correct—usingroot_loggerthroughout resolves the runtime error. However, line 123 reassignsroot_logger = logging.getLogger()unnecessarily since it's already in scope from line 113.♻️ Remove redundant reassignment
for handler in root_logger.handlers: if hasattr(handler, 'setLevel'): handler.setLevel(level) - root_logger = logging.getLogger() if enabled: root_logger.info("Debug mode enabled - all log levels (DEBUG, INFO, WARNING, ERROR, CRITICAL) will be captured") else: root_logger.info("Debug mode disabled - only INFO and above will be captured")switchcraft_modern.spec (1)
45-60: Filtergui_modernsubmodules with the same exclusion rules.Right now
collect_submodules('switchcraft.gui_modern')bypassesexcluded_modules, which can reintroduce optional/addon modules and their deps. Consider applying the same filter and centralizing the explicit app inclusion to keep one consistent path.♻️ Suggested refactor
-# Collect everything from gui_modern explicitly to ensure app.py is included -try: - gui_modern_submodules = collect_submodules('switchcraft.gui_modern') - hidden_imports += gui_modern_submodules -except Exception as e: - print(f"WARNING: Failed to collect gui_modern submodules: {e}") +# Filter out modules that were moved to addons or don't exist +excluded_modules = [ + 'switchcraft.utils.winget', 'switchcraft.gui.views.ai_view', + 'switchcraft_winget', 'switchcraft_ai', 'switchcraft_advanced', + 'switchcraft.utils.updater' +] + +def _not_excluded(m): + return not any(m.startswith(ex) for ex in excluded_modules) + +# Collect everything from gui_modern explicitly to ensure app.py is included +try: + gui_modern_submodules = collect_submodules('switchcraft.gui_modern') + hidden_imports += [m for m in gui_modern_submodules if _not_excluded(m)] +except Exception as e: + print(f"WARNING: Failed to collect gui_modern submodules: {e}") @@ -all_submodules = collect_submodules('switchcraft') -# Filter out modules that were moved to addons or don't exist -excluded_modules = ['switchcraft.utils.winget', 'switchcraft.gui.views.ai_view', 'switchcraft_winget', 'switchcraft_ai', 'switchcraft_advanced', 'switchcraft.utils.updater'] -# Ensure app.py is explicitly included (it might be filtered out otherwise) -filtered_submodules = [m for m in all_submodules if not any(m.startswith(ex) for ex in excluded_modules)] -# Explicitly add app.py if it's not already in the list -if 'switchcraft.gui_modern.app' not in filtered_submodules: - filtered_submodules.append('switchcraft.gui_modern.app') +all_submodules = collect_submodules('switchcraft') +filtered_submodules = [m for m in all_submodules if _not_excluded(m)] +filtered_submodules.append('switchcraft.gui_modern.app')src/switchcraft/gui/views/winget_view.py (1)
193-208: Localize the new error-state labels.The new user-facing strings (“Package ID”, “Version”, and the error line) are hardcoded; consider i18n keys so this view stays consistent with the rest of the localized UI.
tests/reproduce_addon_issue.py (1)
49-94: Convert print-only checks into real assertions.The tests currently catch exceptions and only print failures, so they won’t fail in CI even when behavior regresses. Prefer direct assertions and let exceptions propagate.
✅ Example tightening (pytest-style assertions)
- try: - # Attempt install - service.install_addon(str(zip_path)) - - # Verify installation - installed_path = service.addons_dir / "test.addon" - if installed_path.exists() and (installed_path / "manifest.json").exists(): - print("SUCCESS: Addon installed and manifest found.") - # Check if script was extracted - if (installed_path / "script.py").exists(): - print("SUCCESS: Subfiles extracted correctly.") - else: - print("FAILURE: script.py not found in installed folder.") - else: - print("FAILURE: Addon folder or manifest not found after install.") - - except Exception as e: - print(f"FAILURE: Install raised exception: {e}") - import traceback - traceback.print_exc() + service.install_addon(str(zip_path)) + installed_path = service.addons_dir / "test.addon" + assert installed_path.exists() + assert (installed_path / "manifest.json").exists() + assert (installed_path / "script.py").exists()- try: - service.install_addon(str(zip_path)) - installed_path = service.addons_dir / "test.addon.root" - if installed_path.exists() and (installed_path / "manifest.json").exists(): - print("SUCCESS: Root addon installed.") - else: - print("FAILURE: Root addon failed.") - except Exception as e: - print(f"FAILURE: Root install raised exception: {e}") + service.install_addon(str(zip_path)) + installed_path = service.addons_dir / "test.addon.root" + assert installed_path.exists() + assert (installed_path / "manifest.json").exists()src/switchcraft/gui_modern/views/winget_view.py (2)
360-365: Localize the new loading text.The new “Loading package details...” string is hardcoded. Consider an i18n key for consistent localization.
💡 Example
- loading_area.controls.append(ft.Text("Loading package details...", color="GREY_500", italic=True)) + loading_area.controls.append( + ft.Text(i18n.get("winget_loading_details") or "Loading package details...", color="GREY_500", italic=True) + )
742-758: Remove the duplicateright_pane.update()call.You update the same container twice back-to-back; consider keeping a single update to avoid redundant refresh work.
♻️ Suggested cleanup
- try: - # Then update right pane container - CRITICAL for visibility - self.right_pane.update() - logger.debug("right_pane.update() called successfully") - except Exception as ex: - logger.error(f"Error updating right_pane: {ex}", exc_info=True) + # right_pane.update() already called above; keep a single update to reduce redundant refreshes.
| INLINE_REVIEW_COMMENTS=$(gh api "repos/$REPO/pulls/$PR_NUMBER/comments" --jq '[.[] | select(.position != null or .original_position != null) | select(.user.login != "github-actions[bot]")] | length') | ||
| # Check for CodeRabbit review comments specifically (both inline and general) | ||
| CODERABBIT_INLINE=$(gh api "repos/$REPO/pulls/$PR_NUMBER/comments" --jq '[.[] | select(.user.login == "coderabbitai[bot]" and (.position != null or .original_position != null))] | length') | ||
| # Also check for CodeRabbit review comments in PR comments (not just inline) | ||
| CODERABBIT_PR_COMMENTS=$(gh api "repos/$REPO/issues/$PR_NUMBER/comments" --jq '[.[] | select(.user.login == "coderabbitai[bot]" and (.body | test("suggestion|review|feedback|issue|problem|error|warning"; "i")))] | length') | ||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/review-auto-merge.yml | sed -n '75,95p'Repository: FaserF/SwitchCraft
Length of output: 1890
🏁 Script executed:
cat -n .github/workflows/review-auto-merge.yml | sed -n '70,130p'Repository: FaserF/SwitchCraft
Length of output: 4632
🌐 Web query:
GitHub CLI gh api pagination default behavior --paginate flag
💡 Result:
Short answer: by default gh api does NOT paginate — it returns the single page from the API. Use --paginate to have gh automatically follow pages until there are no more; that will issue additional requests and output each page sequentially (multiple JSON documents). Add --slurp to combine pages into one outer JSON array. [1][2]
Sources:
- gh api manual (describes default and --paginate / --slurp behavior). [1]
- GH CLI discussion/issue noting --paginate returns multiple JSON documents and recommending --jq or --slurp to combine. [2]
🏁 Script executed:
# Test the jq aggregation syntax proposed in the fix
echo '[{"id": 1}, {"id": 2}]' | jq '.[]' | jq -s '[.[] | select(.id > 0)] | length'Repository: FaserF/SwitchCraft
Length of output: 63
Add pagination to comment API calls to prevent undercounting.
GitHub CLI's gh api returns only the first page (30 items) by default. If a PR has more than 30 comments, or if CodeRabbit feedback appears beyond the first page, the review comment detection can return zero and allow auto-merge to proceed incorrectly. Add --paginate and aggregate results across pages.
🔧 Suggested fix
-INLINE_REVIEW_COMMENTS=$(gh api "repos/$REPO/pulls/$PR_NUMBER/comments" --jq '[.[] | select(.position != null or .original_position != null) | select(.user.login != "github-actions[bot]")] | length')
+INLINE_REVIEW_COMMENTS=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/comments" --jq '.[]' | jq -s '[.[] | select(.position != null or .original_position != null) | select(.user.login != "github-actions[bot]")] | length')
-CODERABBIT_INLINE=$(gh api "repos/$REPO/pulls/$PR_NUMBER/comments" --jq '[.[] | select(.user.login == "coderabbitai[bot]" and (.position != null or .original_position != null))] | length')
+CODERABBIT_INLINE=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/comments" --jq '.[]' | jq -s '[.[] | select(.user.login == "coderabbitai[bot]" and (.position != null or .original_position != null))] | length')
-CODERABBIT_PR_COMMENTS=$(gh api "repos/$REPO/issues/$PR_NUMBER/comments" --jq '[.[] | select(.user.login == "coderabbitai[bot]" and (.body | test("suggestion|review|feedback|issue|problem|error|warning"; "i")))] | length')
+CODERABBIT_PR_COMMENTS=$(gh api --paginate "repos/$REPO/issues/$PR_NUMBER/comments" --jq '.[]' | jq -s '[.[] | select(.user.login == "coderabbitai[bot]" and (.body | test("suggestion|review|feedback|issue|problem|error|warning"; "i")))] | length')📝 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.
| INLINE_REVIEW_COMMENTS=$(gh api "repos/$REPO/pulls/$PR_NUMBER/comments" --jq '[.[] | select(.position != null or .original_position != null) | select(.user.login != "github-actions[bot]")] | length') | |
| # Check for CodeRabbit review comments specifically (both inline and general) | |
| CODERABBIT_INLINE=$(gh api "repos/$REPO/pulls/$PR_NUMBER/comments" --jq '[.[] | select(.user.login == "coderabbitai[bot]" and (.position != null or .original_position != null))] | length') | |
| # Also check for CodeRabbit review comments in PR comments (not just inline) | |
| CODERABBIT_PR_COMMENTS=$(gh api "repos/$REPO/issues/$PR_NUMBER/comments" --jq '[.[] | select(.user.login == "coderabbitai[bot]" and (.body | test("suggestion|review|feedback|issue|problem|error|warning"; "i")))] | length') | |
| INLINE_REVIEW_COMMENTS=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/comments" --jq '.[]' | jq -s '[.[] | select(.position != null or .original_position != null) | select(.user.login != "github-actions[bot]")] | length') | |
| # Check for CodeRabbit review comments specifically (both inline and general) | |
| CODERABBIT_INLINE=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/comments" --jq '.[]' | jq -s '[.[] | select(.user.login == "coderabbitai[bot]" and (.position != null or .original_position != null))] | length') | |
| # Also check for CodeRabbit review comments in PR comments (not just inline) | |
| CODERABBIT_PR_COMMENTS=$(gh api --paginate "repos/$REPO/issues/$PR_NUMBER/comments" --jq '.[]' | jq -s '[.[] | select(.user.login == "coderabbitai[bot]" and (.body | test("suggestion|review|feedback|issue|problem|error|warning"; "i")))] | length') | |
🤖 Prompt for AI Agents
In @.github/workflows/review-auto-merge.yml around lines 81 - 88, The gh api
calls that compute INLINE_REVIEW_COMMENTS, CODERABBIT_INLINE, and
CODERABBIT_PR_COMMENTS need pagination to avoid undercounting; update each gh
api invocation used to populate INLINE_REVIEW_COMMENTS, CODERABBIT_INLINE and
CODERABBIT_PR_COMMENTS to include the --paginate flag so results are aggregated
across all pages (keeping the same --jq filters), ensuring the final counts
reflect all pages of pull request review comments and issue comments.
| try: | ||
| import traceback as tb | ||
| from switchcraft.gui_modern.views.crash_view import CrashDumpView | ||
|
|
||
| # Get traceback | ||
| tb_str = tb.format_exc() | ||
| if context: | ||
| tb_str = f"Context: {context}\n\n{tb_str}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the complete function context
head -100 src/switchcraft/gui_modern/app.py | tail -60Repository: FaserF/SwitchCraft
Length of output: 3043
🏁 Script executed:
# Let's find the complete _show_runtime_error function
rg -A 30 "def _show_runtime_error" src/switchcraft/gui_modern/app.pyRepository: FaserF/SwitchCraft
Length of output: 1282
🏁 Script executed:
# Look for all calls to _show_runtime_error to understand when it's invoked
rg "_show_runtime_error" src/switchcraft/gui_modern/Repository: FaserF/SwitchCraft
Length of output: 299
🏁 Script executed:
# Let me search for calls to _show_runtime_error more comprehensively
rg -B 3 -A 3 "_show_runtime_error\(" src/switchcraft/Repository: FaserF/SwitchCraft
Length of output: 559
🏁 Script executed:
# Also check if it's a method defined on a class or static method
rg -B 2 "def _show_runtime_error" src/switchcraft/gui_modern/app.pyRepository: FaserF/SwitchCraft
Length of output: 163
🏁 Script executed:
# Search for all calls to _show_runtime_error - it's a static method so could be called as ModernApp._show_runtime_error or similar
rg "show_runtime_error" src/switchcraft/ -B 2 -A 2Repository: FaserF/SwitchCraft
Length of output: 880
🏁 Script executed:
# Let me check the entire file structure to understand the class better
rg -B 5 "@staticmethod" src/switchcraft/gui_modern/app.py | head -30Repository: FaserF/SwitchCraft
Length of output: 196
🏁 Script executed:
# Search for actual invocations - look for ModernApp._show_runtime_error or just _show_runtime_error with context
rg "ModernApp\._show_runtime_error|_show_runtime_error\(" src/switchcraft/ --type pyRepository: FaserF/SwitchCraft
Length of output: 178
🏁 Script executed:
# Let's also check git history to see if there are any error handling wrappers or error callbacks
git log --oneline -n 20 -- src/switchcraft/gui_modern/app.pyRepository: FaserF/SwitchCraft
Length of output: 86
🏁 Script executed:
# Check if there's an error handler callback that might call this
rg "on_error|error_handler|exception" src/switchcraft/gui_modern/app.py -B 2 -A 2 | head -50Repository: FaserF/SwitchCraft
Length of output: 2573
🏁 Script executed:
# Let's check the entire git diff for this commit to see context of when this was added
git show ab9b527 -- src/switchcraft/gui_modern/app.py | head -200Repository: FaserF/SwitchCraft
Length of output: 8541
🏁 Script executed:
# Let's check how CrashDumpView is used and if it receives traceback_str
rg -B 5 -A 10 "class CrashDumpView" src/switchcraft/Repository: FaserF/SwitchCraft
Length of output: 1838
🏁 Script executed:
# Let's check if there are any event handlers or callbacks that might invoke _show_runtime_error
rg "page.on_error|try:|except" src/switchcraft/gui_modern/app.py -B 3 -A 3 | head -100Repository: FaserF/SwitchCraft
Length of output: 3994
🏁 Script executed:
# Check the CrashDumpView implementation to see how it uses traceback_str
cat src/switchcraft/gui_modern/views/crash_view.py | head -80Repository: FaserF/SwitchCraft
Length of output: 4067
🏁 Script executed:
# Let me verify if error objects passed to this function would have __traceback__ attributes
python3 << 'EOF'
import traceback
import sys
# Simulate a typical calling pattern
def test_format_exc():
"""Test format_exc outside of an exception context"""
try:
raise ValueError("Test error")
except ValueError as e:
# This works because we're in an except block
exc_outside = e
formatted = traceback.format_exc()
print("=== format_exc() inside except block ===")
print(formatted[:100])
# Now outside the except block - what does format_exc return?
print("\n=== format_exc() outside except block ===")
formatted_outside = traceback.format_exc()
print(f"Result: {repr(formatted_outside)}")
# But the error object still has __traceback__
print(f"\n=== error.__traceback__ exists: {exc_outside.__traceback__ is not None}")
# Using format_exception with the error
print("\n=== format_exception with error object ===")
formatted_proper = "".join(traceback.format_exception(type(exc_outside), exc_outside, exc_outside.__traceback__))
print(formatted_proper[:100])
test_format_exc()
EOFRepository: FaserF/SwitchCraft
Length of output: 456
🏁 Script executed:
# Let me search for where this function might be called or if there's a pattern where it's used
rg "except|try:" src/switchcraft/gui_modern/app.py | grep -A 5 -B 5 "_show_runtime_error"Repository: FaserF/SwitchCraft
Length of output: 44
🏁 Script executed:
# Check if there's any page.on_error handler setup
rg "on_error|page\." src/switchcraft/gui_modern/app.py | head -30Repository: FaserF/SwitchCraft
Length of output: 1821
🏁 Script executed:
# Let me check if there's a setup_page method or any event handlers
rg "def setup_page" src/switchcraft/gui_modern/app.py -A 30Repository: FaserF/SwitchCraft
Length of output: 1301
🏁 Script executed:
# Check if _show_runtime_error is perhaps being set as a callback or referenced as a string
rg "show_runtime_error" src/switchcraft/ -iRepository: FaserF/SwitchCraft
Length of output: 299
🏁 Script executed:
# Let me check the entire file to see if there's any page-level error handler or callback registration
wc -l src/switchcraft/gui_modern/app.pyRepository: FaserF/SwitchCraft
Length of output: 100
🏁 Script executed:
# Check for any page callbacks or event handlers that might indirectly call this
rg "page\." src/switchcraft/gui_modern/app.py | grep -E "callback|handler|on_" | head -20Repository: FaserF/SwitchCraft
Length of output: 662
🏁 Script executed:
# Let me check if there are any tests or examples calling this function
find src/switchcraft -name "*test*" -type fRepository: FaserF/SwitchCraft
Length of output: 119
🏁 Script executed:
# Check the modern_main.py to see how ModernApp is instantiated
rg "ModernApp" src/switchcraft/ -B 2 -A 2Repository: FaserF/SwitchCraft
Length of output: 4806
Use the passed exception's traceback instead of format_exc().
format_exc() depends on an active exception context; since this helper receives error directly, it should format that instead. Without an active exception, the call returns "NoneType: None", losing the passed error's traceback.
💡 Suggested fix
- tb_str = tb.format_exc()
+ if error is not None and getattr(error, "__traceback__", None) is not None:
+ tb_str = "".join(tb.format_exception(type(error), error, error.__traceback__))
+ else:
+ tb_str = tb.format_exc()📝 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.
| try: | |
| import traceback as tb | |
| from switchcraft.gui_modern.views.crash_view import CrashDumpView | |
| # Get traceback | |
| tb_str = tb.format_exc() | |
| if context: | |
| tb_str = f"Context: {context}\n\n{tb_str}" | |
| try: | |
| import traceback as tb | |
| from switchcraft.gui_modern.views.crash_view import CrashDumpView | |
| # Get traceback | |
| if error is not None and getattr(error, "__traceback__", None) is not None: | |
| tb_str = "".join(tb.format_exception(type(error), error, error.__traceback__)) | |
| else: | |
| tb_str = tb.format_exc() | |
| if context: | |
| tb_str = f"Context: {context}\n\n{tb_str}" |
🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/app.py` around lines 42 - 49, The code uses
traceback.format_exc() which relies on an active exception context; instead
format the passed-in exception object `error` in the helper (where `tb_str` is
created). Replace tb.format_exc() with formatting of the exception, e.g. use
tb.format_exception(type(error), error, error.__traceback__) (or
tb.TracebackException.from_exception(error).format()) and join the result into a
single string assigned to `tb_str`, preserving the existing `if context:` prefix
logic and leaving CrashDumpView usage unchanged.
| # Filter files based on search | ||
| filtered_files = [] | ||
| for item in self.all_files: | ||
| name = item.get('filename', '').lower() | ||
| if not self.search_val or self.search_val in name: | ||
| filtered_files.append(item) | ||
|
|
||
| # Add tiles for filtered files | ||
| for item in filtered_files: | ||
| self.grid.controls.append(self._create_tile(item)) | ||
|
|
There was a problem hiding this comment.
Show an empty-state when the search filters out all items.
Currently, a search with zero matches leaves a blank grid (no message), which is confusing. Consider showing a “No results” message when filtered_files is empty.
💡 Suggested fix
- # Add tiles for filtered files
- for item in filtered_files:
- self.grid.controls.append(self._create_tile(item))
+ if not filtered_files:
+ self.grid.controls.append(
+ ft.Text(i18n.get("no_search_results") or "No results found.", color="GREY_500")
+ )
+ else:
+ for item in filtered_files:
+ self.grid.controls.append(self._create_tile(item))📝 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.
| # Filter files based on search | |
| filtered_files = [] | |
| for item in self.all_files: | |
| name = item.get('filename', '').lower() | |
| if not self.search_val or self.search_val in name: | |
| filtered_files.append(item) | |
| # Add tiles for filtered files | |
| for item in filtered_files: | |
| self.grid.controls.append(self._create_tile(item)) | |
| # Filter files based on search | |
| filtered_files = [] | |
| for item in self.all_files: | |
| name = item.get('filename', '').lower() | |
| if not self.search_val or self.search_val in name: | |
| filtered_files.append(item) | |
| if not filtered_files: | |
| self.grid.controls.append( | |
| ft.Text(i18n.get("no_search_results") or "No results found.", color="GREY_500") | |
| ) | |
| else: | |
| for item in filtered_files: | |
| self.grid.controls.append(self._create_tile(item)) |
🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/views/library_view.py` around lines 273 - 283, The
grid currently shows nothing when a search yields zero matches; after computing
filtered_files from self.all_files using self.search_val, detect if
filtered_files is empty and append a suitable "no results" control to
self.grid.controls instead of tiles (e.g., a centered label/container with "No
results" or similar). Update the block that iterates over filtered_files and the
call to self._create_tile(item) so that when filtered_files is empty you create
and append the empty-state control (and skip creating tiles) to provide clear
feedback to the user.
| # Immediate visual feedback | ||
| if hasattr(self, 'login_btn'): | ||
| self.login_btn.text = "Starting..." | ||
| self.login_btn.icon = ft.Icons.HOURGLASS_EMPTY | ||
| self.login_btn.update() | ||
|
|
There was a problem hiding this comment.
Reset the login button state on failure.
The “Starting…” label/icon is set but never restored on init/poll failures, leaving the button stuck. Consider restoring in the failure paths and after polling completes.
🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/views/settings_view.py` around lines 714 - 719,
The login button is set to "Starting..." with ft.Icons.HOURGLASS_EMPTY in the
block that references self.login_btn but never restored on init/poll failures;
update the failure and completion paths to reset the button state. Specifically,
in the init routine(s) and any poll/error callbacks that handle initialization
or polling failures (the methods that call or wrap the block setting
self.login_btn.text/icon), ensure you set self.login_btn.text back to the
original label, restore the original icon (or None), and call
self.login_btn.update() on both error paths and on successful completion of
polling so the button is never left stuck in the "Starting..." state.
| """ | ||
| logger.info(f"Language change requested: {val}") | ||
| from switchcraft.utils.config import SwitchCraftConfig | ||
| from switchcraft.utils.i18n import i18n | ||
|
|
||
| # Save preference | ||
| SwitchCraftConfig.set_user_preference("Language", val) | ||
| logger.debug(f"Language preference saved: {val}") | ||
|
|
||
| # Actually update the i18n singleton | ||
| i18n.set_language(val) | ||
| logger.debug(f"i18n language updated: {val}") | ||
|
|
||
| # Immediately refresh the current view to apply language change | ||
| # Get current tab index and reload the view | ||
| if hasattr(self.app_page, 'switchcraft_app'): | ||
| app = self.app_page.switchcraft_app | ||
| current_idx = getattr(app, '_current_tab_index', 0) | ||
| logger.debug(f"Current app_page: {getattr(self, 'app_page', 'Not Set')}, type: {type(getattr(self, 'app_page', None))}") |
There was a problem hiding this comment.
Remove the stray triple‑quote that swallows the method body.
Line 1059 starts a new multiline string, which makes most of _on_lang_change inert until the next """.
🐛 Proposed fix
- """📝 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.
| """ | |
| logger.info(f"Language change requested: {val}") | |
| from switchcraft.utils.config import SwitchCraftConfig | |
| from switchcraft.utils.i18n import i18n | |
| # Save preference | |
| SwitchCraftConfig.set_user_preference("Language", val) | |
| logger.debug(f"Language preference saved: {val}") | |
| # Actually update the i18n singleton | |
| i18n.set_language(val) | |
| logger.debug(f"i18n language updated: {val}") | |
| # Immediately refresh the current view to apply language change | |
| # Get current tab index and reload the view | |
| if hasattr(self.app_page, 'switchcraft_app'): | |
| app = self.app_page.switchcraft_app | |
| current_idx = getattr(app, '_current_tab_index', 0) | |
| logger.debug(f"Current app_page: {getattr(self, 'app_page', 'Not Set')}, type: {type(getattr(self, 'app_page', None))}") | |
| logger.info(f"Language change requested: {val}") | |
| logger.debug(f"Current app_page: {getattr(self, 'app_page', 'Not Set')}, type: {type(getattr(self, 'app_page', None))}") |
🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/views/settings_view.py` around lines 1059 - 1061,
There's a stray opening triple-quote that is turning the body of _on_lang_change
into a multiline string; remove the unintended triple-quote so the subsequent
lines (logger.info and logger.debug) and the rest of the method become
executable again, ensuring the method _on_lang_change contains the
logger.info(f"Language change requested: {val}") and logger.debug(...) calls as
real statements rather than part of a string.
| manifest_path = None | ||
|
|
||
| # TODO: Support nested, but let's strict for now | ||
| if not valid: | ||
| raise Exception("Invalid addon: manifest.json missing from root") | ||
| # Check for manifest.json at root first | ||
| if "manifest.json" in files: | ||
| manifest_path = "manifest.json" | ||
| else: | ||
| # Check for manifest.json in common subdirectories | ||
| # Some ZIPs might have it in a subfolder | ||
| for file_path in files: | ||
| # Normalize path separators | ||
| normalized = file_path.replace('\\', '/') | ||
| # Check if it's manifest.json in a subdirectory (root already checked above) | ||
| if normalized.endswith('/manifest.json'): | ||
| # Accept subdirectory manifest if root not found | ||
| if manifest_path is None: | ||
| manifest_path = file_path | ||
|
|
||
| if not manifest_path: | ||
| # Fallback: Recursive search (max depth 2) | ||
| for file_in_zip in files: | ||
| parts = file_in_zip.replace('\\', '/').split('/') | ||
| if len(parts) <= 3 and parts[-1].lower() == 'manifest.json': | ||
| manifest_path = file_in_zip | ||
| break | ||
|
|
||
| if not manifest_path: | ||
| # Provide helpful error message | ||
| root_files = [f for f in files if '/' not in f.replace('\\', '/') or f.replace('\\', '/').count('/') == 0] | ||
| raise Exception( | ||
| f"Invalid addon: manifest.json not found in ZIP archive.\n" | ||
| f"The addon ZIP must contain a manifest.json file at the root level.\n" | ||
| f"Root files found: {', '.join(root_files[:10]) if root_files else 'none'}" | ||
| ) | ||
|
|
||
| # Check ID from manifest (use found path, not hardcoded) | ||
| with z.open(manifest_path) as f: | ||
| try: | ||
| data = json.load(f) | ||
| except json.JSONDecodeError as e: | ||
| raise Exception(f"Invalid manifest.json: JSON parse error - {e}") | ||
|
|
||
| # Check ID from manifest | ||
| with z.open("manifest.json") as f: | ||
| data = json.load(f) | ||
| addon_id = data.get("id") | ||
| if not addon_id: | ||
| raise Exception("Invalid manifest: missing id") | ||
| raise Exception("Invalid manifest.json: missing required field 'id'") |
There was a problem hiding this comment.
Nested manifest installs still extract into a subfolder.
You now accept manifest.json in a subdirectory, but extraction preserves the original path. That leaves manifest.json (and entry points) under target/<subdir>/..., while read_manifest() expects it at target/manifest.json. This will still break addon discovery and the new tests.
🛠️ Suggested fix: rebase extraction to the manifest directory (and update the error message)
- if not manifest_path:
+ if not manifest_path:
# Provide helpful error message
root_files = [f for f in files if '/' not in f.replace('\\', '/') or f.replace('\\', '/').count('/') == 0]
raise Exception(
f"Invalid addon: manifest.json not found in ZIP archive.\n"
- f"The addon ZIP must contain a manifest.json file at the root level.\n"
+ f"The addon ZIP must contain a manifest.json file at the root level or inside a single top-level folder.\n"
f"Root files found: {', '.join(root_files[:10]) if root_files else 'none'}"
)
# Secure extraction
# If manifest was in a subdirectory, we need to handle path normalization
target_resolved = target.resolve()
+ base_prefix = Path(manifest_path).parent.as_posix()
+ if base_prefix in ("", "."):
+ base_prefix = None
for member in z.infolist():
# Normalize path separators for cross-platform compatibility
normalized_name = member.filename.replace('\\', '/')
+ if base_prefix:
+ if not normalized_name.startswith(f"{base_prefix}/"):
+ continue
+ normalized_name = normalized_name[len(base_prefix) + 1:]
+ if not normalized_name:
+ continue
# Resolve the target path for this member
file_path = (target / normalized_name).resolve()Also applies to: 215-241
🤖 Prompt for AI Agents
In `@src/switchcraft/services/addon_service.py` around lines 162 - 205, The
manifest search currently accepts a nested manifest_path but extraction still
preserves the ZIP subfolder, so read_manifest() fails expecting
target/manifest.json; update the extraction logic where files are written (the
code that uses manifest_path and extracts ZIP entries) to compute a manifest_dir
= os.path.dirname(manifest_path.replace('\\','/')) and, if manifest_dir is
non-empty, strip that leading prefix from every extracted file path (rebase
extraction into manifest_dir) so manifest.json and entry points end up at
target/manifest.json; also update the user-facing Exception message that lists
root_files to indicate if manifest was found under a subdirectory and show that
subdirectory; apply the same change to the other extraction branch referenced
(the block around lines 215-241) so both paths normalize extraction to the
manifest directory.
| # Wait a bit for background thread to start | ||
| time.sleep(0.1) | ||
|
|
||
| # Verify that _load_data was triggered (check if dir_info was updated or grid was refreshed) | ||
| assert True, "Refresh button should trigger data load" |
There was a problem hiding this comment.
Avoid no-op assertions that always pass.
assert True and ... or True make these tests succeed even if the behavior breaks. Consider asserting a concrete effect (e.g., patch _load_data before view creation and assert it was called; or assert goto_tab/language setter was invoked).
Also applies to: 351-351
🤖 Prompt for AI Agents
In `@tests/test_critical_ui_fixes.py` around lines 141 - 145, The test currently
contains a no-op assertion ("assert True") which masks failures; replace it with
a concrete assertion that verifies the refresh action triggered data loading:
patch or mock the _load_data method before creating the view (or attach a spy),
trigger the refresh button, wait for the background thread, and assert that
_load_data was called (or assert that dir_info or the grid state changed
accordingly). Alternatively, assert that helper actions like goto_tab or the
language setter were invoked by mocking those methods and checking their call
state instead of using assert True.
| try: | ||
| from conftest import is_ci_environment, skip_if_ci, poll_until, mock_page | ||
| except ImportError: | ||
| def is_ci_environment(): | ||
| return ( | ||
| os.environ.get('CI') == 'true' or | ||
| os.environ.get('GITHUB_ACTIONS') == 'true' or | ||
| os.environ.get('GITHUB_RUN_ID') is not None | ||
| ) | ||
| def skip_if_ci(reason="Test not suitable for CI environment"): | ||
| if is_ci_environment(): | ||
| pytest.skip(reason) | ||
| @pytest.fixture | ||
| def mock_page(): | ||
| page = MagicMock(spec=ft.Page) | ||
| page.dialog = None | ||
| page.update = MagicMock() | ||
| page.snack_bar = MagicMock(spec=ft.SnackBar) | ||
| page.snack_bar.open = False | ||
| page.open = MagicMock() | ||
| page.run_task = lambda func: func() | ||
| return page | ||
|
|
There was a problem hiding this comment.
Fallback block defines no poll_until, causing NameError if conftest import fails.
Add a minimal fallback implementation alongside skip_if_ci and mock_page.
🐛 Proposed fix
def skip_if_ci(reason="Test not suitable for CI environment"):
if is_ci_environment():
pytest.skip(reason)
`@pytest.fixture`
def mock_page():
page = MagicMock(spec=ft.Page)
page.dialog = None
page.update = MagicMock()
page.snack_bar = MagicMock(spec=ft.SnackBar)
page.snack_bar.open = False
page.open = MagicMock()
page.run_task = lambda func: func()
return page
+ def poll_until(condition, timeout=2.0, interval=0.05):
+ start = time.time()
+ while time.time() - start < timeout:
+ if condition():
+ return True
+ time.sleep(interval)
+ return False🤖 Prompt for AI Agents
In `@tests/test_github_login_integration.py` around lines 13 - 35, The fallback
import block is missing a poll_until implementation which will raise NameError
if conftest import fails; add a minimal poll_until function alongside
is_ci_environment, skip_if_ci and mock_page that accepts a predicate (callable),
timeout and interval parameters, polls the predicate until it returns True or
the timeout elapses, and raises a TimeoutError (or returns False) on timeout so
tests depending on poll_until behave consistently; ensure the symbol name is
poll_until and its signature matches how tests call it.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.