[Fix] Exit Stream Navigation#248
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR introduces a centralized stream exit mechanism (ExitStreamPage), refactors error and status handling in the streaming startup pipeline, adds loading state tracking to the renderer, enhances logging with formatted stage names, improves host wake polling, and adjusts UI components to display progress information. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Streaming/moonlight_xbox_dxMain.cpp:
- Around line 547-558: The loop reading rootFrame->Content after calling
rootFrame->GoBack() can race because Content updates only after navigation
completes; update the logic in the loop that uses rootFrame, CanGoBack, GoBack,
Content and AppPage: either inspect rootFrame->BackStack entries (check each
entry's SourcePageType for AppPage) to determine if an AppPage exists before
calling GoBack, or convert the loop to trigger GoBack and wait for the
Frame::Navigated event (or a short completion wait tied to Navigated) before
re-reading rootFrame->Content and continuing; ensure you stop iterating when
CanGoBack is false or when an AppPage is found and keep maxSteps control.
🧹 Nitpick comments (1)
Streaming/moonlight_xbox_dxMain.cpp (1)
533-575: Consider adding minimal error logging for debuggability.The broad
catch(...)blocks silently swallow all exceptions, which aligns with the PR's goal of safe returns. However, this makes debugging difficult when issues occur.Adding a single log line in the outer catch would preserve the safe-return behavior while providing visibility:
📝 Suggested logging addition
void moonlight_xbox_dxMain::ExitStreamPage() { try { // ... existing code ... - } catch (...) { + } catch (...) { + Utils::Logf("ExitStreamPage: unexpected exception during navigation\n"); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Streaming/moonlight_xbox_dxMain.cppStreaming/moonlight_xbox_dxMain.h
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mpotrykus
Repo: TheElixZammuto/moonlight-xbox PR: 246
File: Pages/StreamPage.xaml.cpp:77-91
Timestamp: 2026-01-10T18:24:38.508Z
Learning: In the moonlight-xbox project's StreamPage::Page_Loaded, the inner DISPATCH_UI macro (lines 83-91 in StreamPage.xaml.cpp) is necessary to ensure the loading screen displays correctly. Even though the outer RunAsync executes on the UI thread, wrapping the heavy initialization (CreateDeviceDependentResources, CreateWindowSizeDependentResources, StartRenderLoop) in a separate DISPATCH_UI breaks the work into two message queue items, allowing the UI to update and show the loading screen between the moonlight_xbox_dxMain construction and the resource initialization.
📚 Learning: 2026-01-10T18:24:38.508Z
Learnt from: mpotrykus
Repo: TheElixZammuto/moonlight-xbox PR: 246
File: Pages/StreamPage.xaml.cpp:77-91
Timestamp: 2026-01-10T18:24:38.508Z
Learning: In the moonlight-xbox project's StreamPage::Page_Loaded, the inner DISPATCH_UI macro (lines 83-91 in StreamPage.xaml.cpp) is necessary to ensure the loading screen displays correctly. Even though the outer RunAsync executes on the UI thread, wrapping the heavy initialization (CreateDeviceDependentResources, CreateWindowSizeDependentResources, StartRenderLoop) in a separate DISPATCH_UI breaks the work into two message queue items, allowing the UI to update and show the loading screen between the moonlight_xbox_dxMain construction and the resource initialization.
Applied to files:
Streaming/moonlight_xbox_dxMain.hStreaming/moonlight_xbox_dxMain.cpp
📚 Learning: 2025-12-14T22:36:59.945Z
Learnt from: andygrundman
Repo: TheElixZammuto/moonlight-xbox PR: 239
File: Streaming/moonlight_xbox_dxMain.cpp:150-159
Timestamp: 2025-12-14T22:36:59.945Z
Learning: In Streaming/moonlight_xbox_dxMain.cpp, initialize lastFramePts to 0 because the first decoded frame's PTS is never 0, enabling safe detection of frame changes in the render loop. Include a clarifying comment about this assumption. If this assumption could change in the future, add a one-time initialization using the first non-zero PTS to guard against regressions.
Applied to files:
Streaming/moonlight_xbox_dxMain.cpp
🧬 Code graph analysis (2)
Streaming/moonlight_xbox_dxMain.h (1)
Streaming/moonlight_xbox_dxMain.cpp (2)
ExitStreamPage(532-576)ExitStreamPage(532-532)
Streaming/moonlight_xbox_dxMain.cpp (1)
Pages/AppPage.xaml.cpp (1)
AppPage(27-34)
🪛 Cppcheck (2.19.0)
Streaming/moonlight_xbox_dxMain.cpp
[error] 569-569: syntax error
(syntaxError)
🔇 Additional comments (4)
Streaming/moonlight_xbox_dxMain.h (1)
33-33: LGTM!The new
ExitStreamPage()declaration is appropriately placed alongside related navigation methods (Disconnect,CloseApp).Streaming/moonlight_xbox_dxMain.cpp (3)
7-8: LGTM!Required includes for the navigation logic in
ExitStreamPage().
194-196: LGTM!Correct use of
DISPATCH_UIto dispatch the navigation call to the UI thread from the background render loop worker. This properly replaces the previous inline navigation logic.
569-569: Static analysis hint is likely a false positive.Cppcheck reports a syntax error here, but
HostSelectorPage::typeidis valid C++/CX syntax for obtaining type information. Cppcheck doesn't fully support C++/CX language extensions.Verify this compiles successfully in your build environment.
Streaming/moonlight_xbox_dxMain.cpp
Outdated
| for (unsigned int step = 0; step < maxSteps && rootFrame->CanGoBack; ++step) { | ||
| auto current = dynamic_cast<AppPage ^>(rootFrame->Content); | ||
| if (current != nullptr) { | ||
| reachedAppPage = true; | ||
| break; | ||
| } | ||
| try { | ||
| rootFrame->GoBack(); | ||
| } catch (...) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
UWP Windows::UI::Xaml::Controls::Frame GoBack synchronous or asynchronous
💡 Result:
Frame.GoBack() is a synchronous method (returns void) — there is no built‑in GoBackAsync API. Callers can invoke GoBack() directly; if you need to await completion of the navigation, listen for the Frame.Navigated (or NavigationService) events and/or use a TaskCompletionSource to produce an awaitable Task. [1][2]
Sources:
[1] Frame class (Windows.UI.Xaml.Controls) — Microsoft Docs.
[2] Back navigation for UWP apps — Microsoft Docs.
🌐 Web query:
UWP Frame GoBack Content property updated immediately or after navigation complete
💡 Result:
Short answer: Frame.Content is updated only when the navigation completes — after the Frame raises its Navigated event (not instantly at the moment you call GoBack()). See the Frame/navigation lifecycle docs which show the frame fires Navigating → (page OnNavigatingFrom/OnNavigatingTo) → Navigated when the new content is ready. [1][2]
Sources:
- Microsoft: Peer-to-peer navigation / navigation lifecycle (Frame.Navigate, Navigated, Navigating). [1]
- Tutorial/teaching note describing event order (Navigate → Navigating → page OnNavigatingFrom → Navigated → page OnNavigatingTo). [2]
Potential race condition with asynchronous navigation.
Frame::GoBack() is synchronous (returns immediately), but Frame.Content is only updated after the navigation completes (when the Navigated event fires). Reading rootFrame->Content on the next loop iteration may not reflect the new page yet, causing the loop to overshoot or miss the AppPage.
Consider either:
- Checking
BackStackentries directly to findAppPage(viaSourcePageType) - Using
Navigatedevents to confirm navigation completion before readingContent
🤖 Prompt for AI Agents
In @Streaming/moonlight_xbox_dxMain.cpp around lines 547 - 558, The loop reading
rootFrame->Content after calling rootFrame->GoBack() can race because Content
updates only after navigation completes; update the logic in the loop that uses
rootFrame, CanGoBack, GoBack, Content and AppPage: either inspect
rootFrame->BackStack entries (check each entry's SourcePageType for AppPage) to
determine if an AppPage exists before calling GoBack, or convert the loop to
trigger GoBack and wait for the Frame::Navigated event (or a short completion
wait tied to Navigated) before re-reading rootFrame->Content and continuing;
ensure you stop iterating when CanGoBack is false or when an AppPage is found
and keep maxSteps control.
|
Isn't my approach more reliable in that it doesn't depend on a stack of pages and navigating back? I liken it to using the back button in a web browser vs clicking the link that goes to the home page. The point from CR about async back navigation also makes it seem a lot more fragile to use your method. |
Open to suggestions. This is just attempting a safe "back". Swallowing errors, and if it can't be done, go back to host like we know works. I tried a more direct approach, like you had, of going straight to the host's app page, but it messed with the order of selected pages, and hitting the back button or "b" would put you back into the stream page. Unless I'm misunderstanding, we have to use "back" to keep history in line, otherwise we have to rewire all back functions to go exactly where we want instead of just back. |
|
OK, that's a fair point about breaking B-button back. I will apply this and test it out. |
|
When I tested this by restarting Sunshine, I got sent back to an empty "apps page", I think this is why I didn't like using back, because you have to go back to the screen above this, the "host list" page. After quite a while sitting on the empty apps page I got a popup that said "Disconnected / Connection to host was lost". Hitting OK on that took me to the list of hosts, where my host had the disconnection icon. Restarting the app is the only thing you can do at this point. One thing I would suggest is to log everything that ends up in one of the catch blocks. There could be useful info in those errors. I would also try to wait on the GoBack() call if it's async. |
|
@andygrundman Interesting! I was wondering what issue you were running into. Mine goes to a filled app page every time. So that tells me that you are being sent back to the app page, but you lost the host connected to it. Two things come to mind for me on that:
You said you got the "Disconnected/Connection to host was lost". This is normal with some of the new UI changes I threw in. The current MDNS handler seems to kill the queries and connections after the app being open for a while. I added a fix that "should" handle that in my "mdns improvements" PR. That being said, are you getting this after waiting a long time? Or immediate? Immediate could mean the app lost the host connection, If it sits a while and then that message appears (like it would in a normal scenario) that could mean you still have the host, but apps tiles are not being rendered. They could have missed the window to appear.
AppPage should NEVER be used without a host, so I think this makes sense. Thoughts on all that? |
So anyways, I did this. I did some forced "null host" tests. Let me know if it fixes it for you. Should redirect right back to HostSelectorPage after stream. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Streaming/moonlight_xbox_dxMain.cpp:
- Around line 545-553: The check of rootFrame->Content immediately after calling
rootFrame->GoBack() is unreliable because GoBack() initiates navigation
asynchronously; replace the immediate Content check and instead subscribe to
rootFrame->Navigated (and optionally NavigationFailed/NavigationStopped) to
detect completion and set reachedAppPage when the new Content is an AppPage;
implement a short-lived event handler or a TaskCompletionSource-like pattern
tied to the Navigated event that awaits navigation completion before inspecting
rootFrame->Content, and ensure you unsubscribe the handler after it fires to
avoid leaks.
🧹 Nitpick comments (1)
Streaming/moonlight_xbox_dxMain.cpp (1)
555-561: Fallback to null content is a last resort but leaves app in a broken state.Setting
rootFrame->Content = nullptr(line 559) when both GoBack and Navigate fail will leave the app with no visible UI. While this is an extreme edge case and better than crashing, users would see a blank screen with no way to recover.Consider whether a more graceful fallback is possible, such as attempting to create a new HostSelectorPage instance directly:
Possible alternative fallback
try { rootFrame->Navigate(Windows::UI::Xaml::Interop::TypeName(HostSelectorPage::typeid)); } catch (...) { - rootFrame->Content = nullptr; + try { + rootFrame->Content = ref new HostSelectorPage(); + } catch (...) { + rootFrame->Content = nullptr; + } Utils::Log("ExitStreamPage: Failed to return to HostSelectorPage\n"); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Pages/AppPage.xaml.cppStreaming/moonlight_xbox_dxMain.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-10T18:24:38.508Z
Learnt from: mpotrykus
Repo: TheElixZammuto/moonlight-xbox PR: 246
File: Pages/StreamPage.xaml.cpp:77-91
Timestamp: 2026-01-10T18:24:38.508Z
Learning: In the moonlight-xbox project's StreamPage::Page_Loaded, the inner DISPATCH_UI macro (lines 83-91 in StreamPage.xaml.cpp) is necessary to ensure the loading screen displays correctly. Even though the outer RunAsync executes on the UI thread, wrapping the heavy initialization (CreateDeviceDependentResources, CreateWindowSizeDependentResources, StartRenderLoop) in a separate DISPATCH_UI breaks the work into two message queue items, allowing the UI to update and show the loading screen between the moonlight_xbox_dxMain construction and the resource initialization.
Applied to files:
Pages/AppPage.xaml.cppStreaming/moonlight_xbox_dxMain.cpp
📚 Learning: 2025-12-14T22:36:59.945Z
Learnt from: andygrundman
Repo: TheElixZammuto/moonlight-xbox PR: 239
File: Streaming/moonlight_xbox_dxMain.cpp:150-159
Timestamp: 2025-12-14T22:36:59.945Z
Learning: In Streaming/moonlight_xbox_dxMain.cpp, initialize lastFramePts to 0 because the first decoded frame's PTS is never 0, enabling safe detection of frame changes in the render loop. Include a clarifying comment about this assumption. If this assumption could change in the future, add a one-time initialization using the first non-zero PTS to guard against regressions.
Applied to files:
Streaming/moonlight_xbox_dxMain.cpp
🧬 Code graph analysis (2)
Pages/AppPage.xaml.cpp (2)
Pages/AppPage.xaml.h (1)
MoonlightHost(31-35)Utils.hpp (3)
Log(13-13)Log(14-14)Logf(15-15)
Streaming/moonlight_xbox_dxMain.cpp (3)
Pages/AppPage.xaml.cpp (1)
AppPage(28-35)Utils.cpp (4)
Log(57-82)Log(57-57)Log(84-88)Log(84-84)Utils.hpp (2)
Log(13-13)Log(14-14)
🪛 Cppcheck (2.19.0)
Pages/AppPage.xaml.cpp
[error] 45-45: syntax error
(syntaxError)
Streaming/moonlight_xbox_dxMain.cpp
[error] 557-557: syntax error
(syntaxError)
🔇 Additional comments (6)
Pages/AppPage.xaml.cpp (3)
5-5: LGTM!The include is required to support the new redirect logic when navigating with a null host.
41-74: Solid defensive navigation logic with proper exception handling.The implementation correctly uses
WeakReferenceto safely access the page in the deferred lambda, preventing dangling pointer issues if the page is destroyed before the dispatcher callback executes. The nested try/catch blocks ensure exceptions are logged rather than crashing the app.Note: The Cppcheck syntax error at line 45 is a false positive—the
HostSelectorPage::typeidsyntax is valid C++/CX for obtaining a Windows Runtime type.
358-370: LGTM!Formatting-only changes with no functional impact.
Streaming/moonlight_xbox_dxMain.cpp (3)
7-8: LGTM!Required includes for the new
ExitStreamPage()functionality.
194-196: LGTM!Clean refactoring that centralizes stream exit navigation into a dedicated function, improving maintainability and enabling reuse.
532-566: Good defensive implementation overall.The function centralizes stream exit navigation with broad exception handling that prevents crashes, aligning with the PR objective to "swallow any errors and safely return." The logic correctly handles edge cases and provides fallback paths.
Note: The Cppcheck syntax error at line 557 is a false positive—
HostSelectorPage::typeidis valid C++/CX syntax.
| try { | ||
| rootFrame->GoBack(); | ||
| } catch (...) { | ||
| Utils::Log("ExitStreamPage: Failed to GoBack()\n"); | ||
| } | ||
|
|
||
| if (!reachedAppPage) { | ||
| if (dynamic_cast<AppPage ^>(rootFrame->Content) != nullptr) reachedAppPage = true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
UWP Frame.GoBack() synchronous asynchronous timing behavior
💡 Result:
Short answer
- Frame.GoBack() is a synchronous call (void) that immediately starts a navigation operation but does not block until the new page is loaded — the actual navigation completion happens later and is reported via Frame events (Navigated, NavigationStopped, NavigationFailed). [1][2]
How to know when the back navigation finished
- Handle Frame.Navigated (or NavigationFailed/NavigationStopped) to detect when the navigation is complete/failed. [1][2]
- If you need an awaitable pattern, create a TaskCompletionSource and complete it from a one-time Navigated handler (subscribe, call GoBack(), await the TCS, then unsubscribe). Example patterns are shown in community posts. [3]
References
- Microsoft docs — Back navigation / Frame navigation (Navigated etc.). [1]
- Microsoft docs — Navigation / peer-to-peer navigation (Frame behavior). [2]
- Community examples showing using Frame.Navigated to get the page instance or await navigation. [3]
Fix unreliable navigation state check after GoBack().
Frame::GoBack() is synchronous in return but asynchronous in effect—it immediately returns after initiating the navigation operation, but the actual page transition and Content update happen later. The code checks rootFrame->Content at line 552 immediately after calling GoBack() at line 546, but the Content property still reflects the old page at that point. This check is unreliable and explains why navigation failures were observed.
To properly detect when the back navigation completes, subscribe to Frame.Navigated (or handle NavigationFailed/NavigationStopped) instead of checking Content immediately. Alternatively, if you need the content to be updated before proceeding, you'll need to await navigation completion using a TaskCompletionSource pattern tied to the Navigated event.
🤖 Prompt for AI Agents
In @Streaming/moonlight_xbox_dxMain.cpp around lines 545 - 553, The check of
rootFrame->Content immediately after calling rootFrame->GoBack() is unreliable
because GoBack() initiates navigation asynchronously; replace the immediate
Content check and instead subscribe to rootFrame->Navigated (and optionally
NavigationFailed/NavigationStopped) to detect completion and set reachedAppPage
when the new Content is an AppPage; implement a short-lived event handler or a
TaskCompletionSource-like pattern tied to the Navigated event that awaits
navigation completion before inspecting rootFrame->Content, and ensure you
unsubscribe the handler after it fires to avoid leaks.
…xbox into fix-exit-stream-navigation
|
Buffed up this PR with error-handling on StartStreaming navigation exits. Also completely marshalled the process between UI and background threads so UI updates can appear. 
|
|
@andygrundman if you get the time, please review and let me know if you have questions on why I did anything the way I did to the StartStreaming process. |
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
State/MoonlightClient.cpp (1)
396-406:⚠️ Potential issue | 🟠 Major
sprintfwith%sand a potentially nullLiGetFormattedStageNameresult is undefined behavior; alsofailingPortsis silently dropped from the format string.Two issues here:
LiGetFormattedStageName(stage)returnsnullptrwhenLiGetStageNamereturns null (see pch.h line 240). Passingnullptrtosprintf%sis UB (typically a crash).- The format string has 3 specifiers (
%d,%s,%d) but 4 arguments are passed —failingPortsis computed and populated but never printed.Proposed fix
- sprintf(message, "Stage %d: '%s' - Failed with error: %d.\n", stage, LiGetFormattedStageName(stage), err, failingPorts); + const char* name = LiGetFormattedStageName(stage); + sprintf(message, "Stage %d: '%s' - Failed with error: %d. Ports: %s\n", stage, name ? name : "Unknown", err, failingPorts);Pages/HostSelectorPage.xaml.cpp (1)
347-347:⚠️ Potential issue | 🟡 MinorCatch
std::exceptionby const reference, not by value.Catching by value slices derived exception types, losing the actual error information from
what().Proposed fix
- catch (std::exception ex) { + catch (const std::exception& ex) {
🤖 Fix all issues with AI agents
In `@App.xaml.cpp`:
- Line 108: Remove the stray trailing character 'F' after the XML doc end tag in
App.xaml.cpp (the line containing "/// </summary>F"); edit the XML documentation
comment so it reads "/// </summary>" with no extra characters to restore
well-formed XML docs for the surrounding method/class documentation.
In `@moonlight-xbox-dx.vcxproj`:
- Line 674: The ErrorText XML element in moonlight-xbox-dx.vcxproj contains
mojibake ("ù", "è"); open the project file, correct the string in the
<ErrorText> tag to use the proper Italian characters ("più", "è") and then save
the file with UTF-8 encoding (ensure the XML declaration or your editor
specifies UTF-8 or include a UTF-8 BOM if your tooling requires it) so the
characters persist; verify by re-opening the file and checking the <ErrorText>
value reads "Questo progetto fa riferimento a uno o più pacchetti NuGet che non
sono presenti in questo computer. Usare lo strumento di ripristino dei pacchetti
NuGet per scaricarli. Per altre informazioni, vedere
http://go.microsoft.com/fwlink/?LinkID=322105. Il file mancante è {0}."
In `@Pages/StreamPage.xaml`:
- Line 77: The TextBlock named StepText uses an invalid FontSize string
("12px"); change the FontSize attribute on StepText to a numeric value without
the "px" suffix (e.g., FontSize="12") so UWP XAML parses correctly and uses
device-independent pixels.
In `@setup-dev.ps1`:
- Around line 58-65: The call to vswhere piped through ConvertFrom-Json returns
an array, so accessing $vsJson.installationPath and
$vsJson.catalog.productLineVersion can be null; update the logic that reads
$vsJson to index the first element (or select the first object) before using its
properties (e.g. replace direct uses of $vsJson.installationPath and
$vsJson.catalog.productLineVersion with references into the first array element
of the ConvertFrom-Json result) so you reliably obtain installationPath and
productLineVersion from the first returned installation.
- Around line 242-244: The recursive Get-ChildItem may find a third-party
.vcxproj; update the Try-Step "Updating .vcxproj with certificate info" to
target the known project filename or constrain recursion. Replace the generic
Get-ChildItem -Recurse -Filter *.vcxproj with a search for the specific file
(e.g. Get-ChildItem -Recurse -Filter "moonlight-xbox-dx.vcxproj" -File |
Select-Object -First 1) or limit depth (use -Depth 1/2 if available) before
assigning $proj and keep the existing null check.
- Around line 134-137: Detect-VS can return $null causing $CMakeGenerator to be
empty and later cmake calls to run with -G ""; update the script to bail out or
fail fast when no generator was detected by checking $CMakeGenerator (the
variable set from Detect-VS) before any cmake-dependent steps (e.g., inside
Try-Step "Configuring moonlight-common-c" and other Step 5 cmake blocks) and
throw or exit with a clear error like "No CMake generator available" so
subsequent cmake invocations are not reached with an empty -G argument.
- Around line 226-276: The $cert created in Try-Step "Generating UWP signing
certificate" is defined in a child scope and becomes null in Try-Step "Updating
.vcxproj with certificate info"; change the variable to script scope by
assigning it as $script:cert where New-SelfSignedCertificate is called (in the
"Generating UWP signing certificate" Try-Step) and update references in the
"Updating .vcxproj with certificate info" Try-Step to use $script:cert (e.g.,
$script:cert.Thumbprint), or alternatively merge the two Try-Step scriptblocks
so $cert remains in scope.
In `@State/MoonlightClient.cpp`:
- Around line 366-371: The function connection_status_update calls
LiGetFormattedStageName(status) twice and passes its return directly into
sprintf %s, which risks a nullptr dereference and wastes the previously captured
stageName; fix by reusing the captured stageName variable, check if stageName is
nullptr and substitute a safe placeholder (e.g. "<unknown>") before calling
sprintf/Utils::Log, and prefer a bounded/safer formatting function or ensure the
buffer size is respected when writing the message.
- Around line 373-376: The function connection_status_completed calls
LiGetFormattedStageName(status) and passes its return directly into sprintf
which risks dereferencing a nullptr (and possible overflow); update
connection_status_completed to first capture the return of
LiGetFormattedStageName into a local pointer, check it for nullptr and
substitute a safe fallback string (e.g. "<unknown>") if null, and use a bounded
formatting call (e.g. snprintf) or ensure the buffer size is respected before
calling Utils::Log so neither a null pointer nor buffer overflow can occur.
In `@Streaming/moonlight_xbox_dxMain.cpp`:
- Around line 20-23: The include for the C++/CX XAML header
Common/ModalDialog.xaml.h must be moved out of the extern "C" block because it
declares ref types/classes that cannot have C linkage; update the top of the
file so extern "C" only wraps the C header Limelight.h (keep `#include`
<Limelight.h> inside extern "C") and place `#include` <Common/ModalDialog.xaml.h>
outside/after the extern "C" block to restore correct C++/CX linkage for the
ModalDialog types.
- Around line 49-50: The continuations attached to
concurrency::create_task(::moonlight_xbox_dx::ModalDialog::ShowOnceAsync(...))
call t.get() without handling thrown exceptions; wrap the body of each lambda
continuation (the lambda capturing self, showLogsDialog and the other one at
lines ~82–83) in a try/catch that catches std::exception (or
Platform::Exception^ if appropriate for the project), log or handle the error
consistently with the surrounding codebase, and avoid letting the exception
escape the continuation (e.g., early-return after logging) so that cancelled or
failed dialogs do not produce unobserved task exceptions.
- Around line 115-131: The DISPATCH_UI lambda inside client->OnCompleted
currently calls Sleep(500), which blocks the UI thread; remove the Sleep(500)
call and replace it with a non-blocking delay (e.g., start a DispatcherTimer or
use a dispatch-with-delay) before setting streamPage->m_progressRing->IsActive =
false and streamPage->m_progressView->Visibility = Collapsed; locate the lambda
referenced by DISPATCH_UI in the OnCompleted handler (the closure that accesses
streamPage and modifies m_progressRing/m_progressView) and implement a
DispatcherTimer or equivalent delayed dispatch to perform the UI updates without
blocking.
- Around line 682-684: Replace the unsafe `[&]` lambda capture used in the
DISPATCH_UI calls inside ToggleLogs and ToggleStats with an explicit `[this]`
capture so member pointers remain valid when the lambda runs asynchronously;
update the lambdas that reference m_LogRenderer and m_statsTextRenderer (inside
DISPATCH_UI) to capture `this` instead of `[&]` in the moonlight_xbox_dxMain
methods ToggleLogs/ToggleStats to avoid dangling references if the object is
destroyed before the dispatched lambda executes.
- Around line 67-72: The loop comment says "last 24 lines" but the condition
(int)lines.size() - i < 6 actually picks the last 5 lines; update either the
comment or the filter: if you want the last 24 lines change the numeric literal
in the condition (used in the loop that appends to m_text) from 6 to 25 (or
otherwise use a named constant like LAST_N_LINES = 24 and adjust the check to
(int)lines.size() - i <= LAST_N_LINES), or if the intention was to keep 5 lines,
change the comment to match the code.
In `@Streaming/VideoRenderer.cpp`:
- Around line 98-100: Change the render guard so rendering only proceeds when
loading is both complete and successful: replace the current conjunction check
that uses m_loadingComplete and m_loadingSuccessful (in the render path inside
VideoRenderer::Render or the function containing that if) with a condition that
returns early unless m_loadingComplete.load(...) &&
m_loadingSuccessful.load(...); if you intentionally want to keep spinning after
a failed StartStreaming (where m_loadingComplete=true and
m_loadingSuccessful=false) to let IsConnectionTerminated() break the loop,
instead add a clarifying comment near the guard explaining that behavior.
🧹 Nitpick comments (6)
moonlight-xbox-dx.vcxproj (1)
139-142: Hardcoded certificate password in project file.The plaintext password
moonlightis committed to source control. While this is a local development signing certificate (generated bysetup-dev.ps1) and not a production secret, committing it to a public repo means anyone can sign packages with this identity. If this is intentional for dev convenience, consider adding a brief comment in the file or README clarifying that the cert is for local development only.setup-dev.ps1 (2)
226-238: No idempotency — re-running creates duplicate certificates.Each run of the script creates a new self-signed certificate in
Cert:\CurrentUser\Mywithout checking for or removing a prior one. Over time this clutters the cert store. Consider removing an existing cert with the same subject before creating a new one.Proposed fix
Try-Step "Generating UWP signing certificate" { + # Remove any existing cert with the same subject to keep the store clean + Get-ChildItem Cert:\CurrentUser\My | Where-Object { $_.Subject -eq $CertSubject } | Remove-Item -Force -ErrorAction SilentlyContinue + $script:cert = New-SelfSignedCertificate ` -Type CodeSigningCert ` -Subject $CertSubject ` -CertStoreLocation "Cert:\CurrentUser\My"
16-18: Certificate password is hardcoded in the script.
$CertPassword = "moonlight"is fine for a local dev signing cert, but it's worth adding a brief comment explaining this is intentionally a non-secret value for local development. This matches the same password committed in the.vcxprojat line 142.State/MoonlightClient.cpp (1)
292-300: Good addition — surfacinggs_errordetails to the caller viaOnFailed.One minor note: consider using
snprintfinstead ofsprintffor theerrorMessagebuffer to guard against an unexpectedly longgs_errorstring.pch.h (1)
238-252:thread_localreturn pattern is fragile — returnedc_str()is invalidated by the next call on the same thread.In the current callers (immediate
sprintf), this works fine. But line 369 in MoonlightClient.cpp callsLiGetFormattedStageNametwice in the same expression — once storing the result in an unusedstageName, then again inline insprintf. If a single expression ever needs two different stage names, the second call would overwrite the first. Worth a brief comment on the function documenting this constraint.Streaming/VideoRenderer.h (1)
79-79: Nit: inconsistent indentation — spaces vs. tabs.Line 79 uses spaces while the surrounding lines (e.g., Line 78) use tabs.
| <muxc:ProgressRing x:Name="MainProgressRing" Width="64" Height="64" IsActive="True" Margin="0,0,0,16" /> | ||
| <TextBlock x:Name="StatusText" Text="Initializing Moonlight..."/> | ||
| <TextBlock x:Name="StatusText" TextAlignment="Center" Text="Initializing Moonlight..."/> | ||
| <TextBlock x:Name="StepText" FontSize="12px" Foreground="DimGray" TextAlignment="Center" Text="" Margin="0,16,0,0" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
UWP XAML FontSize "px" suffix support documentation
💡 Result:
UWP XAML does not support a "px" suffix for FontSize (or other double-typed size properties). In UWP, FontSize is a double value expressed in pixels (effective/device-independent pixels in practice), and XAML attribute values for primitives are parsed from plain numeric strings (so "12" works; "12px" won’t). [1][2]
Use:
<TextBlock FontSize="12" />Related documentation:
- UWP/WinUI API docs show
FontSizeasdoubleand “measured in pixels” (example:FontIconSource.FontSize). [1] - UWP XAML syntax docs describe primitive properties (like
double) being set via attribute syntax using the built-in string-to-primitive conversion (no unit suffixes described). [2]
Sources
- Microsoft Learn —
FontIconSource.FontSize(UWP):double, measured in pixels. (learn.microsoft.com) - Microsoft Learn — UWP “XAML syntax guide” (primitive types like
doubleuse built-in parser conversion from strings). (learn.microsoft.com)
Citations:
- 1: https://learn.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.fonticonsource.fontsize?view=winrt-26100&utm_source=openai
- 2: https://learn.microsoft.com/en-us/windows/uwp/xaml-platform/xaml-syntax-guide?utm_source=openai
FontSize="12px" — UWP XAML does not support the px suffix; use FontSize="12" instead.
In UWP XAML, FontSize expects a bare numeric value (device-independent pixels). The "12px" string will cause a XAML parse error at runtime.
Proposed fix
- <TextBlock x:Name="StepText" FontSize="12px" Foreground="DimGray" TextAlignment="Center" Text="" Margin="0,16,0,0" />
+ <TextBlock x:Name="StepText" FontSize="12" Foreground="DimGray" TextAlignment="Center" Text="" Margin="0,16,0,0" />📝 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.
| <TextBlock x:Name="StepText" FontSize="12px" Foreground="DimGray" TextAlignment="Center" Text="" Margin="0,16,0,0" /> | |
| <TextBlock x:Name="StepText" FontSize="12" Foreground="DimGray" TextAlignment="Center" Text="" Margin="0,16,0,0" /> |
🤖 Prompt for AI Agents
In `@Pages/StreamPage.xaml` at line 77, The TextBlock named StepText uses an
invalid FontSize string ("12px"); change the FontSize attribute on StepText to a
numeric value without the "px" suffix (e.g., FontSize="12") so UWP XAML parses
correctly and uses device-independent pixels.
setup-dev.ps1
Outdated
| $vsJson = & $vswhere -latest -format json | ConvertFrom-Json | ||
| if (-not $vsJson) { | ||
| Record-Failure "No Visual Studio installation detected" | ||
| return $null | ||
| } | ||
|
|
||
| $installPath = $vsJson.installationPath | ||
| $version = $vsJson.catalog.productLineVersion |
There was a problem hiding this comment.
vswhere -latest -format json returns a JSON array — index into it.
ConvertFrom-Json on an array yields a PowerShell array, so $vsJson.installationPath may return $null or behave unexpectedly. Index into the first element:
Proposed fix
- $vsJson = & $vswhere -latest -format json | ConvertFrom-Json
- if (-not $vsJson) {
+ $vsAll = & $vswhere -latest -format json | ConvertFrom-Json
+ if (-not $vsAll) {
Record-Failure "No Visual Studio installation detected"
return $null
}
+ $vsJson = $vsAll[0]
$installPath = $vsJson.installationPath
$version = $vsJson.catalog.productLineVersion📝 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.
| $vsJson = & $vswhere -latest -format json | ConvertFrom-Json | |
| if (-not $vsJson) { | |
| Record-Failure "No Visual Studio installation detected" | |
| return $null | |
| } | |
| $installPath = $vsJson.installationPath | |
| $version = $vsJson.catalog.productLineVersion | |
| $vsAll = & $vswhere -latest -format json | ConvertFrom-Json | |
| if (-not $vsAll) { | |
| Record-Failure "No Visual Studio installation detected" | |
| return $null | |
| } | |
| $vsJson = $vsAll[0] | |
| $installPath = $vsJson.installationPath | |
| $version = $vsJson.catalog.productLineVersion |
🤖 Prompt for AI Agents
In `@setup-dev.ps1` around lines 58 - 65, The call to vswhere piped through
ConvertFrom-Json returns an array, so accessing $vsJson.installationPath and
$vsJson.catalog.productLineVersion can be null; update the logic that reads
$vsJson to index the first element (or select the first object) before using its
properties (e.g. replace direct uses of $vsJson.installationPath and
$vsJson.catalog.productLineVersion with references into the first array element
of the ConvertFrom-Json result) so you reliably obtain installationPath and
productLineVersion from the first returned installation.
setup-dev.ps1
Outdated
| $CMakeGenerator = Detect-VS | ||
| if ($CMakeGenerator) { | ||
| Write-Host "Using CMake generator: $CMakeGenerator" -ForegroundColor Yellow | ||
| } |
There was a problem hiding this comment.
Script continues with $null generator after VS detection failure.
If Detect-VS returns $null (e.g., missing VS or missing components), $CMakeGenerator will be $null and the cmake invocations in Step 5 will receive -G "", which will produce confusing cmake errors. Consider bailing out early or recording a clear failure before reaching the cmake steps.
Proposed fix — guard before cmake steps
$CMakeGenerator = Detect-VS
if ($CMakeGenerator) {
Write-Host "Using CMake generator: $CMakeGenerator" -ForegroundColor Yellow
}
+else {
+ Record-Failure "Cannot proceed without a valid CMake generator (Visual Studio not detected)"
+}Then, in Steps 5 (and any cmake-dependent step), guard on $CMakeGenerator:
Try-Step "Configuring moonlight-common-c" {
if (-not $CMakeGenerator) { throw "No CMake generator available — skipping" }
# ... cmake invocation ...
}📝 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.
| $CMakeGenerator = Detect-VS | |
| if ($CMakeGenerator) { | |
| Write-Host "Using CMake generator: $CMakeGenerator" -ForegroundColor Yellow | |
| } | |
| $CMakeGenerator = Detect-VS | |
| if ($CMakeGenerator) { | |
| Write-Host "Using CMake generator: $CMakeGenerator" -ForegroundColor Yellow | |
| } | |
| else { | |
| Record-Failure "Cannot proceed without a valid CMake generator (Visual Studio not detected)" | |
| } |
🤖 Prompt for AI Agents
In `@setup-dev.ps1` around lines 134 - 137, Detect-VS can return $null causing
$CMakeGenerator to be empty and later cmake calls to run with -G ""; update the
script to bail out or fail fast when no generator was detected by checking
$CMakeGenerator (the variable set from Detect-VS) before any cmake-dependent
steps (e.g., inside Try-Step "Configuring moonlight-common-c" and other Step 5
cmake blocks) and throw or exit with a clear error like "No CMake generator
available" so subsequent cmake invocations are not reached with an empty -G
argument.
| concurrency::create_task(::moonlight_xbox_dx::ModalDialog::ShowOnceAsync(dialog1)).then([self, showLogsDialog](concurrency::task<Windows::UI::Xaml::Controls::ContentDialogResult> t) { | ||
| auto result = t.get(); |
There was a problem hiding this comment.
Unhandled exception from t.get() in .then() continuations.
Both ShowOnceAsync continuations call t.get() without a try/catch. If the dialog is cancelled or fails, t.get() throws and the exception propagates as an unobserved task exception. Wrap these in try/catch to match the defensive error handling approach used throughout this PR.
Proposed fix (apply to both continuations)
concurrency::create_task(::moonlight_xbox_dx::ModalDialog::ShowOnceAsync(dialog1)).then([self, showLogsDialog](concurrency::task<Windows::UI::Xaml::Controls::ContentDialogResult> t) {
- auto result = t.get();
+ Windows::UI::Xaml::Controls::ContentDialogResult result;
+ try { result = t.get(); } catch (...) {
+ self->StopRenderLoop();
+ self->ExitStreamPage();
+ return;
+ }
if (result == Windows::UI::Xaml::Controls::ContentDialogResult::Primary) {Also applies to: 82-83
🤖 Prompt for AI Agents
In `@Streaming/moonlight_xbox_dxMain.cpp` around lines 49 - 50, The continuations
attached to
concurrency::create_task(::moonlight_xbox_dx::ModalDialog::ShowOnceAsync(...))
call t.get() without handling thrown exceptions; wrap the body of each lambda
continuation (the lambda capturing self, showLogsDialog and the other one at
lines ~82–83) in a try/catch that catches std::exception (or
Platform::Exception^ if appropriate for the project), log or handle the error
consistently with the surrounding codebase, and avoid letting the exception
escape the continuation (e.g., early-return after logging) so that cancelled or
failed dialogs do not produce unobserved task exceptions.
| for (int i = 0; i < (int)lines.size(); i++) { | ||
| // Get only the last 24 lines | ||
| if ((int)lines.size() - i < 6) { | ||
| m_text += lines[i]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Comment says "last 24 lines" but code filters to last 5.
The condition (int)lines.size() - i < 6 selects entries where i > size - 6, which yields the last 5 lines, not 24.
Fix either the comment or the filter
- // Get only the last 24 lines
- if ((int)lines.size() - i < 6) {
+ // Get only the last 24 lines
+ if ((int)lines.size() - i < 25) {Or update the comment to match the code:
- // Get only the last 24 lines
- if ((int)lines.size() - i < 6) {
+ // Get only the last 5 lines
+ if ((int)lines.size() - i < 6) {🤖 Prompt for AI Agents
In `@Streaming/moonlight_xbox_dxMain.cpp` around lines 67 - 72, The loop comment
says "last 24 lines" but the condition (int)lines.size() - i < 6 actually picks
the last 5 lines; update either the comment or the filter: if you want the last
24 lines change the numeric literal in the condition (used in the loop that
appends to m_text) from 6 to 25 (or otherwise use a named constant like
LAST_N_LINES = 24 and adjust the check to (int)lines.size() - i <=
LAST_N_LINES), or if the intention was to keep 5 lines, change the comment to
match the code.
Streaming/moonlight_xbox_dxMain.cpp
Outdated
| client->OnCompleted = ([this, streamPage, configuration]() { | ||
| concurrency::create_task([this]() { | ||
| while (this->m_sceneRenderer && !this->m_sceneRenderer->IsLoadingComplete() && !this->moonlightClient->IsConnectionTerminated()) { | ||
| Sleep(50); | ||
| } | ||
| }).then([this, streamPage, configuration](concurrency::task<void> t) { | ||
|
|
||
| if (this->m_sceneRenderer && this->m_sceneRenderer->IsLoadingSuccessful()) { | ||
| DISPATCH_UI(([streamPage]() { | ||
| Sleep(500); | ||
| streamPage->m_progressRing->IsActive = false; | ||
| streamPage->m_progressView->Visibility = Windows::UI::Xaml::Visibility::Collapsed; | ||
| })); | ||
| } | ||
|
|
||
| }); | ||
| }); |
There was a problem hiding this comment.
Sleep(500) blocks the UI thread.
Line 124 calls Sleep(500) inside a DISPATCH_UI lambda, which freezes the UI for half a second. Use a timer or dispatch-delayed pattern instead, or simply remove the sleep if it's not strictly needed.
Proposed fix — remove the sleep
if (this->m_sceneRenderer && this->m_sceneRenderer->IsLoadingSuccessful()) {
DISPATCH_UI(([streamPage]() {
- Sleep(500);
streamPage->m_progressRing->IsActive = false;
streamPage->m_progressView->Visibility = Windows::UI::Xaml::Visibility::Collapsed;
}));🤖 Prompt for AI Agents
In `@Streaming/moonlight_xbox_dxMain.cpp` around lines 115 - 131, The DISPATCH_UI
lambda inside client->OnCompleted currently calls Sleep(500), which blocks the
UI thread; remove the Sleep(500) call and replace it with a non-blocking delay
(e.g., start a DispatcherTimer or use a dispatch-with-delay) before setting
streamPage->m_progressRing->IsActive = false and
streamPage->m_progressView->Visibility = Collapsed; locate the lambda referenced
by DISPATCH_UI in the OnCompleted handler (the closure that accesses streamPage
and modifies m_progressRing/m_progressView) and implement a DispatcherTimer or
equivalent delayed dispatch to perform the UI updates without blocking.
| DISPATCH_UI([&] { | ||
| m_LogRenderer->ToggleVisible(); | ||
| }); |
There was a problem hiding this comment.
[&] capture in DISPATCH_UI is dangerous if the lambda outlives the caller.
ToggleLogs and ToggleStats use [&] capture in DISPATCH_UI, which dispatches asynchronously to the UI thread. If moonlight_xbox_dxMain is destroyed before the dispatched lambda runs, the captured references are dangling. Use [this] instead — m_LogRenderer and m_statsTextRenderer are members, so this capture is sufficient and explicit.
Proposed fix
- DISPATCH_UI([&] {
+ DISPATCH_UI([this] {
m_LogRenderer->ToggleVisible();
});- DISPATCH_UI([&] {
+ DISPATCH_UI([this] {
m_statsTextRenderer->ToggleVisible();
});Also applies to: 692-694
🤖 Prompt for AI Agents
In `@Streaming/moonlight_xbox_dxMain.cpp` around lines 682 - 684, Replace the
unsafe `[&]` lambda capture used in the DISPATCH_UI calls inside ToggleLogs and
ToggleStats with an explicit `[this]` capture so member pointers remain valid
when the lambda runs asynchronously; update the lambdas that reference
m_LogRenderer and m_statsTextRenderer (inside DISPATCH_UI) to capture `this`
instead of `[&]` in the moonlight_xbox_dxMain methods ToggleLogs/ToggleStats to
avoid dangling references if the object is destroyed before the dispatched
lambda executes.
| if (!m_loadingComplete.load(std::memory_order_acquire) && !m_loadingSuccessful.load(std::memory_order_acquire)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Render guard condition allows rendering after a failed load.
The condition !m_loadingComplete && !m_loadingSuccessful skips rendering only when both are false. After a failed StartStreaming (m_loadingComplete=true, m_loadingSuccessful=false), the guard falls through and attempts to render with no valid stream. While this likely won't crash (null texture check at Line 114 returns false), the intent seems clearer as an OR:
- if (!m_loadingComplete.load(std::memory_order_acquire) && !m_loadingSuccessful.load(std::memory_order_acquire)) {
+ if (!m_loadingComplete.load(std::memory_order_acquire) || !m_loadingSuccessful.load(std::memory_order_acquire)) {This way, rendering only proceeds when loading is both complete and successful. If the intent is to allow the render loop to continue spinning (so IsConnectionTerminated() can break it), consider adding a comment explaining why the && is deliberate.
📝 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 (!m_loadingComplete.load(std::memory_order_acquire) && !m_loadingSuccessful.load(std::memory_order_acquire)) { | |
| return true; | |
| } | |
| if (!m_loadingComplete.load(std::memory_order_acquire) || !m_loadingSuccessful.load(std::memory_order_acquire)) { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In `@Streaming/VideoRenderer.cpp` around lines 98 - 100, Change the render guard
so rendering only proceeds when loading is both complete and successful: replace
the current conjunction check that uses m_loadingComplete and
m_loadingSuccessful (in the render path inside VideoRenderer::Render or the
function containing that if) with a condition that returns early unless
m_loadingComplete.load(...) && m_loadingSuccessful.load(...); if you
intentionally want to keep spinning after a failed StartStreaming (where
m_loadingComplete=true and m_loadingSuccessful=false) to let
IsConnectionTerminated() break the loop, instead add a clarifying comment near
the guard explaining that behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Streaming/moonlight_xbox_dxMain.cpp`:
- Around line 633-639: The catch-all handler for
rootFrame->Navigate(HostSelectorPage::typeid) currently nukes the UI by setting
rootFrame->Content = nullptr and logs only a generic message; instead, catch the
exception, log the actual exception details via Utils::Log (include what failed
and exception info) and perform a graceful recovery: attempt an alternative
recovery path such as calling Window::Current->Close() or navigating to a
lightweight fallback page rather than clearing rootFrame->Content; update the
catch block around the Navigate call in the ExitStreamPage flow to implement
these changes (referencing rootFrame, Navigate, HostSelectorPage, Utils::Log and
Window::Current->Close()).
- Around line 67-70: The for-loop that intends to show the "last 8 lines" uses
the condition (int)lines.size() - i < 8 which actually selects 7 lines; update
the condition in the loop that iterates over lines (the block containing the
comment and the if using lines.size() - i) to use <= 8 (or < 9) so the last 8
lines are included, and ensure the inline comment matches the corrected
condition.
- Around line 38-97: The two std::shared_ptr lambdas showErrorDialog and
showLogsDialog form a reference cycle and leak; break it by making one direction
a std::weak_ptr (e.g., declare showLogsDialog as std::shared_ptr but capture a
std::weak_ptr to it inside the showErrorDialog lambda, or vice versa) and lock()
it before invoking, or alternatively call reset() on the other shared_ptr
immediately after you invoke it in each branch; update the captures in the
enclosing DISPATCH_UI lambda to use the weak_ptr and adjust invocation sites
(the closures referenced by showErrorDialog and showLogsDialog) to lock() and
check for emptiness before calling to avoid the cycle.
| DISPATCH_UI(([msgCopy, self, appName]() { | ||
| auto showErrorDialog = std::make_shared<std::function<void()>>(); | ||
| auto showLogsDialog = std::make_shared<std::function<void()>>(); | ||
|
|
||
| *showErrorDialog = [self, msgCopy, showLogsDialog, appName]() { | ||
| auto dialog1 = ref new Windows::UI::Xaml::Controls::ContentDialog(); | ||
| dialog1->Title = L"Failed to start " + appName; | ||
| dialog1->Content = Utils::StringFromStdString(msgCopy); | ||
| dialog1->PrimaryButtonText = L"OK"; | ||
| dialog1->SecondaryButtonText = L"Show Logs"; | ||
|
|
||
| concurrency::create_task(::moonlight_xbox_dx::ModalDialog::ShowOnceAsync(dialog1)).then([self, showLogsDialog](concurrency::task<Windows::UI::Xaml::Controls::ContentDialogResult> t) { | ||
| auto result = t.get(); | ||
| if (result == Windows::UI::Xaml::Controls::ContentDialogResult::Primary) { | ||
| self->StopRenderLoop(); | ||
| self->ExitStreamPage(); | ||
| } | ||
| else if (result == Windows::UI::Xaml::Controls::ContentDialogResult::Secondary) { | ||
| (*showLogsDialog)(); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| *showLogsDialog = [self, showErrorDialog]() { | ||
| auto dialog2 = ref new Windows::UI::Xaml::Controls::ContentDialog(); | ||
|
|
||
| std::wstring m_text = L""; | ||
| std::vector<std::wstring> lines = Utils::GetLogLines(); | ||
|
|
||
| for (int i = 0; i < (int)lines.size(); i++) { | ||
| // Get only the last 8 lines | ||
| // More than that cannot be fully viewed on the screen at the current scaling | ||
| if ((int)lines.size() - i < 8) { | ||
| m_text += lines[i]; | ||
| } | ||
| } | ||
|
|
||
| Utils::showLogs = true; | ||
|
|
||
| dialog2->MaxWidth = 600; | ||
| dialog2->Title = "Logs"; | ||
| dialog2->Content = ref new Platform::String(m_text.c_str()); | ||
| dialog2->PrimaryButtonText = L"OK"; | ||
| dialog2->SecondaryButtonText = L"Show Error"; | ||
|
|
||
| concurrency::create_task(::moonlight_xbox_dx::ModalDialog::ShowOnceAsync(dialog2)).then([self, showErrorDialog](concurrency::task<Windows::UI::Xaml::Controls::ContentDialogResult> t) { | ||
| auto result = t.get(); | ||
| if (result == Windows::UI::Xaml::Controls::ContentDialogResult::Primary) { | ||
| self->StopRenderLoop(); | ||
| self->ExitStreamPage(); | ||
| } | ||
| else if (result == Windows::UI::Xaml::Controls::ContentDialogResult::Secondary) { | ||
| (*showErrorDialog)(); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| // Start by showing the error dialog | ||
| (*showErrorDialog)(); | ||
| })); |
There was a problem hiding this comment.
Circular shared_ptr reference between showErrorDialog and showLogsDialog leaks memory.
showErrorDialog captures showLogsDialog (line 42) and showLogsDialog captures showErrorDialog (line 61). Since both are std::shared_ptr, neither ref count reaches zero — they keep each other alive indefinitely after the enclosing lambda returns.
Break the cycle by resetting the other pointer inside each branch once it's no longer needed, or by using a weak_ptr for one direction.
Proposed fix — reset after use to break the cycle
*showErrorDialog = [self, msgCopy, showLogsDialog, appName]() {
// ...
concurrency::create_task(::moonlight_xbox_dx::ModalDialog::ShowOnceAsync(dialog1)).then([self, showLogsDialog](concurrency::task<Windows::UI::Xaml::Controls::ContentDialogResult> t) {
auto result = t.get();
if (result == Windows::UI::Xaml::Controls::ContentDialogResult::Primary) {
self->StopRenderLoop();
self->ExitStreamPage();
}
else if (result == Windows::UI::Xaml::Controls::ContentDialogResult::Secondary) {
(*showLogsDialog)();
}
});
};
-*showLogsDialog = [self, showErrorDialog]() {
+auto weakError = std::weak_ptr<std::function<void()>>(showErrorDialog);
+*showLogsDialog = [self, weakError]() {
// ...
- concurrency::create_task(::moonlight_xbox_dx::ModalDialog::ShowOnceAsync(dialog2)).then([self, showErrorDialog](concurrency::task<Windows::UI::Xaml::Controls::ContentDialogResult> t) {
+ concurrency::create_task(::moonlight_xbox_dx::ModalDialog::ShowOnceAsync(dialog2)).then([self, weakError](concurrency::task<Windows::UI::Xaml::Controls::ContentDialogResult> t) {
auto result = t.get();
if (result == Windows::UI::Xaml::Controls::ContentDialogResult::Primary) {
self->StopRenderLoop();
self->ExitStreamPage();
}
else if (result == Windows::UI::Xaml::Controls::ContentDialogResult::Secondary) {
- (*showErrorDialog)();
+ if (auto err = weakError.lock()) (*err)();
}
});
};🤖 Prompt for AI Agents
In `@Streaming/moonlight_xbox_dxMain.cpp` around lines 38 - 97, The two
std::shared_ptr lambdas showErrorDialog and showLogsDialog form a reference
cycle and leak; break it by making one direction a std::weak_ptr (e.g., declare
showLogsDialog as std::shared_ptr but capture a std::weak_ptr to it inside the
showErrorDialog lambda, or vice versa) and lock() it before invoking, or
alternatively call reset() on the other shared_ptr immediately after you invoke
it in each branch; update the captures in the enclosing DISPATCH_UI lambda to
use the weak_ptr and adjust invocation sites (the closures referenced by
showErrorDialog and showLogsDialog) to lock() and check for emptiness before
calling to avoid the cycle.
| if (!reachedAppPage) { | ||
| try { | ||
| rootFrame->Navigate(Windows::UI::Xaml::Interop::TypeName(HostSelectorPage::typeid)); | ||
| } catch (...) { | ||
| rootFrame->Content = nullptr; | ||
| Utils::Log("ExitStreamPage: Failed to return to HostSelectorPage\n"); | ||
| } |
There was a problem hiding this comment.
Fallback to HostSelectorPage with Content = nullptr on failure is harsh.
If Navigate(HostSelectorPage) throws, setting rootFrame->Content = nullptr (line 637) leaves the app with a blank frame and no recovery path. Consider logging the exception details (not just a generic message) and, if possible, attempting Window::Current->Close() or another graceful shutdown rather than blanking the frame.
🧰 Tools
🪛 Cppcheck (2.19.0)
[error] 635-635: syntax error
(syntaxError)
🤖 Prompt for AI Agents
In `@Streaming/moonlight_xbox_dxMain.cpp` around lines 633 - 639, The catch-all
handler for rootFrame->Navigate(HostSelectorPage::typeid) currently nukes the UI
by setting rootFrame->Content = nullptr and logs only a generic message;
instead, catch the exception, log the actual exception details via Utils::Log
(include what failed and exception info) and perform a graceful recovery:
attempt an alternative recovery path such as calling Window::Current->Close() or
navigating to a lightweight fallback page rather than clearing
rootFrame->Content; update the catch block around the Navigate call in the
ExitStreamPage flow to implement these changes (referencing rootFrame, Navigate,
HostSelectorPage, Utils::Log and Window::Current->Close()).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
moonlight-xbox-dx.sln (1)
7-18:⚠️ Potential issue | 🔴 CriticalFix stale ProjectDependencies: three GUIDs no longer match declared projects.
The
ProjectSection(ProjectDependencies)on lines 8–10 references three GUIDs that don't match any currently declared projects in the solution:
- Line 8:
{842AAF8B-4505-3D3B-AAE5-CFF297F90298}— unmatched- Line 9:
{873D03B2-B99F-3917-AFDC-9FB954F9962E}— unmatched- Line 10:
{E025A414-0DA7-3C61-A890-9487ED6E1D91}— unmatchedThese are likely remnants from before the project GUIDs were regenerated. Line 11's
{F4776924-619C-42C7-88B2-82C947CCC9E7}correctly references DirectXTK (line 20) and should remain.Since Visual Studio resolves dependencies by GUID, the three stale entries are silently ignored, breaking the intended build order. Remove these three lines, or if they should reference
moonlight-common-c,gamestream, andenet, update them to the current GUIDs:
moonlight-common-c:{56F17673-A685-3B16-9094-8B11198EF0BC}gamestream:{B089477D-7A5A-3EF1-8139-BC31842134E6}enet:{2CB3B76D-BADC-3BF6-90C9-9D52F4B53A93}State/MoonlightClient.cpp (1)
396-406:⚠️ Potential issue | 🟡 Minor
failingPortsis computed but never printed; same nullptr risk withLiGetFormattedStageName.The format string on line 402 has three specifiers (
%d,%s,%d) but four arguments are passed —failingPortsis silently ignored bysprintf. If you intend to log failing ports, add a%sspecifier; otherwise, remove the dead code (failingPortscomputation +LiStringifyPortFlagscall). Also guardLiGetFormattedStageNameagainst nullptr.Proposed fix (include failing ports)
- sprintf(message, "Stage %d: '%s' - Failed with error: %d.\n", stage, LiGetFormattedStageName(stage), err, failingPorts); + const char* stageName = LiGetFormattedStageName(stage); + sprintf(message, "Stage %d: '%s' - Failed with error: %d. Ports: %s\n", stage, stageName ? stageName : "Unknown", err, failingPorts);
🤖 Fix all issues with AI agents
In `@Pages/StreamPage.xaml.cpp`:
- Around line 106-115: The try/catch allows StopRenderLoop() to throw and skips
m_main.reset(), leaking the resource and leaving the "m_main reset" log
incorrect; refactor Page_Unloaded so m_main.reset() always runs regardless of
StopRenderLoop() outcome (e.g., call StopRenderLoop() in its try/catch but move
m_main.reset() into a single post-catch cleanup section or call m_main.reset()
in each catch), preserve logging of exceptions using the existing catch blocks
(Utils::Logf/Utils::Log) and only log "m_main reset" after m_main.reset()
actually executes; update references in the method using StopRenderLoop,
m_main.reset and the catch handlers accordingly.
- Around line 75-95: The catch blocks inside the DISPATCH_UI lambda that
initializes m_main (created via new moonlight_xbox_dxMain and started with
StartRenderLoop) only log errors and leave the UI stuck; update each catch
(std::exception, std::string, Platform::Exception ^, ...) to perform recovery by
calling the existing ExitStreamPage() method (or at minimum hide/stop the
progress view and re-enable back navigation) after logging so the user can leave
the loading screen; ensure you call ExitStreamPage() (or invoke the same cleanup
code used elsewhere to stop the progress ring and restore UI state) and avoid
throwing from the catch handlers.
🧹 Nitpick comments (3)
moonlight-xbox-dx.sln (1)
3-4: Consider team's Visual Studio version support when keeping v18.Visual Studio Version 18 (VS 2026) exists and is stable as of February 2026 (v18.2 released Jan 20). VS 2022 developers can still open this solution—they'll receive an upgrade prompt but can continue working, and the
.slnformat itself hasn't changed. If your team primarily uses VS 2022, you may want to keepVisualStudioVersionat 17 to avoid version mismatch prompts for all contributors; otherwise, v18 is acceptable.Pages/AppPage.xaml.cpp (1)
152-181: NewappNamederivation looks correct; pre-existing debug artifact on line 179.The
GetAppByIdusage and fallback to"App"on line 161 are sound. The extended config population (lines 165–173) mirrors the host settings cleanly.Line 179 (
printf("C")) is a pre-existing debug artifact — consider replacing it with a properUtils::Logcall for consistency with the rest of the error-handling improvements in this PR.Streaming/moonlight_xbox_dxMain.h (1)
32-32: Declaration looks good; implementation in the .cpp has broad catch-alls.The
ExitStreamPage()public method fits well in this class. Looking at the implementation inStreaming/moonlight_xbox_dxMain.cpp(lines 609–643), theGoBack()call on line 623 usescatch (...)which discards the actual error. Consider catchingPlatform::Exception^andstd::exception&first (as done elsewhere in this PR) to log the specific failure reason — this will help diagnose the navigation issues discussed in the PR comments.
| try { | ||
| this->m_main->StopRenderLoop(); | ||
| this->m_main.reset(); | ||
| } catch (std::exception &ex) { | ||
| Utils::Logf("StreamPage::Page_Unloaded m_main threw an exception: %s\n", ex.what()); | ||
| } catch (...) { | ||
| Utils::Log("StreamPage::Page_Unloaded StopRenderLoop threw an exception\n"); | ||
| Utils::Log("StreamPage::Page_Unloaded m_main threw an exception\n"); | ||
| } | ||
|
|
||
| this->m_main.reset(); | ||
| Utils::Log("StreamPage::Page_Unloaded m_main reset\n"); |
There was a problem hiding this comment.
m_main.reset() is skipped if StopRenderLoop() throws — resource leak.
If StopRenderLoop() throws, execution jumps to the catch block and m_main.reset() on line 108 is never reached, yet line 115 still logs "m_main reset". Move the reset into a finally-style pattern (or duplicate it in the catch blocks) so the resource is always released.
Proposed fix
try {
this->m_main->StopRenderLoop();
- this->m_main.reset();
} catch (std::exception &ex) {
Utils::Logf("StreamPage::Page_Unloaded m_main threw an exception: %s\n", ex.what());
} catch (...) {
Utils::Log("StreamPage::Page_Unloaded m_main threw an exception\n");
}
+
+ this->m_main.reset();
Utils::Log("StreamPage::Page_Unloaded m_main reset\n");🤖 Prompt for AI Agents
In `@Pages/StreamPage.xaml.cpp` around lines 106 - 115, The try/catch allows
StopRenderLoop() to throw and skips m_main.reset(), leaking the resource and
leaving the "m_main reset" log incorrect; refactor Page_Unloaded so
m_main.reset() always runs regardless of StopRenderLoop() outcome (e.g., call
StopRenderLoop() in its try/catch but move m_main.reset() into a single
post-catch cleanup section or call m_main.reset() in each catch), preserve
logging of exceptions using the existing catch blocks (Utils::Logf/Utils::Log)
and only log "m_main reset" after m_main.reset() actually executes; update
references in the method using StopRenderLoop, m_main.reset and the catch
handlers accordingly.
| if (lines.size() - i < 24) { | ||
| m_text += lines[i]; | ||
| } | ||
| DISPATCH_THREADPOOL(([this, devRes = m_deviceResources, cfg = configuration] { |
There was a problem hiding this comment.
I specifically pulled this out of being async, why add it back? I haven't tried this yet but will see what happens.
|
Does this fix the disconnected hosts after 5 minutes issue? I was thinking, why do we even sit there and spam the servers? It should simply always let me connect to my server, whether it's up or not. Let me try at least, and then fail if it's down. |
|
My very first try with this branch, and HDR is completely broken. Please remove any changes to the video code. :( |
|
I already spent way too much time debugging the HDR issues, I don't want to do it again. Here's a log if you want to compare. The reason the HDR section is missing in yours is because the async change you made causes things to happen out of order, it needs to wait for StartStreaming to finish. Good: Broken: |
| concurrency::create_task(::moonlight_xbox_dx::ModalDialog::ShowOnceAsync(confirm)); | ||
| } | ||
| else { | ||
| if (!success) { |
There was a problem hiding this comment.
Snuck this in. Removed the WoL Success dialog. This just feels silly to have now that we have a visual polling feedback.
|
@andygrundman Woops! Looks like the SetHDR got taken out when I merged in master. I added it back with a flag check for safety. Just tested. Sorry about that. The entire point of the new edits is to marshal the work between UI and backend. StartStreaming is 95% of what's happening, so that was the important one. Most of the other work needs to happen on the UI thread to grab screen information, but StartStreaming can happen on the background to free up the UI thread to make updates. Without it, the UI is in a frozen state until all changes have been made. This was particularly bad on errors, because some of the library calls have cleanup after a return. So everything just hung for ~20 seconds. I did not WANT to make any of it async, but the only safe way to offload work to another thread without deadlocking was to send the async call and check flags for completion. So to put it bluntly, we needed to pick an executing thread and send the needed work to the other. |
I'm working on this one next as part of the mdns improvements. Good thought. I believe this issue is the network utility actually shuts down though. So it's less of a reporting issues and more of a "I'm gone, you can't even use me" scenario. My current theory is the network is being taken down on a "sleep" and not spinning up, but I still have to test. I'll try this idea out though, to see if we get lucky. |
|
With everything this now touches, converting to a draft until all concerns are addressed and there is ample time to test. |
OK, this does fix HDR. I guess I've never ran into a "hung for 20 seconds" issue, probably my network is too good or something. |
|
Releasing this PR. Been playing with it for a while with no issues. I also played with a few different ways to marshall threads. This option worked the best in terms of performance and reliability. |
andygrundman
left a comment
andygrundman
left a comment
There was a problem hiding this comment.
I've been running this for a while and it's generally fine. There are still the same old mostly rare issues that have been around in all versions, so I'm good with this PR.
3 participants
Swallows any errors and safely returns. If there are issues, safely return to HostSelectorPage.
---------UPDATE---------
This PR is now a 2-part fix.
On exiting a stream, it will send you back to the AppPage. On the chance of a null host, the page load event now checks for host, if none is found for any reason, you are directed back to the HostSelectorPage.
The threads were remarshalled when starting a stream, in order to free the UI thread. This fixes a couple things:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
UI/UX