Conversation
|
I see what you're doing here |
|
🤯 |
StephaneDelcroix
left a comment
StephaneDelcroix
left a comment
There was a problem hiding this comment.
Findings
🔴 CRITICAL -- build.yml -- Missing with: key on Upload Linux AppImage step (5/5 models)
name:, path:, retention-days: are direct children of uses: instead of nested under with:. Step will fail.
🔴 CRITICAL -- build.yml -- Duplicate if: key on create-release job (5/5 models)
Two consecutive if: lines. Invalid YAML.
🟡 MODERATE -- PolyPilot.Gtk/MauiProgram.cs -- IDemoService not registered (3/5 models)
Demo mode will throw InvalidOperationException at runtime.
🟡 MODERATE -- NotificationManagerService.cs -- Sync block in async method + process leak (3/5 models)
WaitForExit(3000) blocks thread. Process.Start() return never disposed.
Action: ⚠️ Request changes
- Fix missing with: on upload step
- Remove duplicate if:
- Register IDemoService
- Fix blocking WaitForExit and process leak
|
Thanks for the review! Verified each finding against the code: 1. Missing 2. Duplicate 3. 4. Process leak in |
Redth
force-pushed
the
feature/linux-gtk-head
branch
from
f43fcc2 to
8ad8c81
Compare
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
PR Review Squad — Consensus Report (5-model dispatch, 2+ agreement filter)
CI Status: ⚠️ No CI checks ran
🟡 MODERATE — create-release hard-gates on publish-linux (3/5 models)
.github/workflows/build.yml — Adding publish-linux to needs: [notarize-maccatalyst, publish-windows, publish-android, publish-linux] means any Linux CI failure blocks all platform releases. Since Linux GTK is experimental, it should not gate Mac/Windows/Android releases. Fix: either remove publish-linux from needs: or add continue-on-error: true to the Linux job.
🟡 MODERATE — Wrong workload for Linux GTK build (3/5 models)
.github/workflows/build.yml (publish-linux) — dotnet workload install maui-android is wrong for a Linux/GTK build. All other jobs use maui; only the Android job needs maui-android. The GTK project targets net10.0 with UseMaui=true. Fix: dotnet workload install maui.
🟢 MINOR — NotificationManagerService.InitializeAsync blocks calling thread (2/5 models)
PolyPilot.Gtk/Services/NotificationManagerService.cs:29 — proc?.WaitForExit(3000) is synchronous inside an async method. Should use await Task.Run(...) or WaitForExitAsync.
🟢 MINOR — CSS scroll overrides apply globally, not just GTK (2/5 models)
PolyPilot/wwwroot/app.css — .content.settings-content-active override (overflow-y: auto !important) applies on all platforms, not just Linux. Could subtly change scroll on Mac/Windows.
✅ Confirmed Good
PlatformHelper.cs#elselogic correctly classifies Linux as desktopCopilotService.csreturn info;fix is correct (method returnsTask<AgentSessionInfo>)- GTK project structure and
<Compile Include>links follow existing patterns IDemoService— NOT a DI issue (public constructor chains tonew DemoService()internally)
Recommended Action: ⚠️ Request changes
Fix the two CI issues before merge: wrong workload + release gate dependency.
Add a new app head project for running PolyPilot natively on Linux using Platform.Maui.Linux.Gtk4 (0.1.0-preview.1). The GTK head shares all code, Razor components, and wwwroot assets from the main PolyPilot project via Compile/RazorComponent/Content Include links. Key pieces: - Program.cs: GtkMauiApplication entry point with WebKit init and MauiDevFlow agent wiring - MauiProgram.cs: GTK-specific MAUI builder with BlazorWebView handler registration and Gtk4 essentials - Services/: Linux stub implementations for QrScannerService and NotificationManagerService - Post-build target copies _framework Blazor assets and generates unscoped CSS bundle for GTK WebKit compatibility Also updates: - PolyPilot.slnx: add PolyPilot.Gtk project - dotnet-tools.json: bump maui-devflow CLI to 0.8.1 - README.md: add Linux build/run instructions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PlatformHelper.IsDesktop only checked MACCATALYST and WINDOWS preprocessor directives, causing Linux GTK builds to fall into the false branch. This hid all desktop UI sections (Transport Mode, Persistent Server, DevTunnel, etc.) and limited the app to Remote mode only. Add a runtime OperatingSystem fallback in the #else branch so any non-mobile platform (including Linux) is correctly detected as desktop. Also set a reasonable default window size (1400x900) on Linux since the GTK default is too small for the desktop layout. Update PlatformHelperTests to reflect that the test host (Linux) now correctly reports IsDesktop=true. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The settings page used a nested scroll container (.settings-page with overflow-y: auto inside .content with overflow: hidden) which caused content to be unreachable on the GTK4 WebKit WebView. Fix by: - Changing .settings-page to overflow-y: visible / min-height: 100% so it sizes to its content instead of clipping - Adding .settings-content-active class to article.content when the settings route is active, enabling overflow-y: auto at that level - Resetting scroll position on settings page init and cleanup Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Update MauiDevFlow CLI, NuGet packages, and skill to 0.12.1 - Update Platform.Maui.Linux.Gtk4 packages to 0.5.1 - Update GitHub.Copilot.SDK to 0.1.26, Markdig to 1.0.0 - Update MauiVersion to 10.0.41 in GTK head - Add CommunityToolkit.Maui for speech-to-text support - Register missing services (TutorialService, UsageStatsService, ISpeechToText) - Pin sqlite-net-pcl and SQLitePCLRaw versions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Build .deb, AppImage, and Flatpak artifacts for both arm64 and x64 using a matrix strategy with native runners. Follows the same pattern as MAUI.Sherpa's publish-linux job. - Uses ubuntu-latest (x64) and ubuntu-24.04-arm (arm64) runners - Installs GTK4/WebKit deps, Flatpak SDK - Publishes self-contained with CreateAppImage/CreateDeb/CreateFlatpak - Uploads tar.gz, .deb, .AppImage, .flatpak as separate artifacts - Updates create-release to include all Linux artifacts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the stub NotificationManagerService with a real implementation that uses notify-send (libnotify) for desktop notifications. Checks for notify-send availability on init and sends notifications with --app-name=PolyPilot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add using to Process.Start() in SendNotificationAsync so the process handle is disposed after firing the notification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-error - Fix publish-linux workload: maui-android → maui (GTK needs full workload) - Make NotificationManagerService.InitializeAsync truly async via Task.Run - Scope GTK CSS scroll overrides to body[data-platform='linux'] - Add PlatformHelper.PlatformName and set data-platform attribute on body - Add continue-on-error on Linux artifact downloads in create-release Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Redth
force-pushed
the
feature/linux-gtk-head
branch
from
8ad8c81 to
3f96695
Compare
|
Addressed all 4 review findings from the latest review:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Reverted the workload thing - AI is wrong, we need maui-android only since the other ones can't install on linux |
3 participants
Adds a Linux/GTK app head for PolyPilot using
Platform.Maui.Linux.Gtk4.Changes
PolyPilot.Gtk/) — shares all components/services from main project via<Compile Include>linksPlatformHelperrecognizes Linux as a desktop platformNotificationManagerServiceandQrScannerServicefor LinuxTested