| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Claude finished @MartinZikmund's task in 3m 58s —— View job PR Review: WASM MT
SummaryVerdict: fix-first — one correctness bug must be addressed before merge; three style regressions also flagged.
Findings
Cross-cutting notesThe overall architecture is sound: moving InitializeExports() to an explicit await in WebAssemblyBrowserHost.InitializeAsync() (before the run loop starts) is a cleaner ordering than the old setImmediate-based workaround, and the StorageFolder.makePersistent simplification is safe given the new call sequence. The OnResizeAsync / IsAutoEnableAccessibilityAsync Task-returning [JSExport] pattern is the correct approach for MT WASM interop. |
Sorry, something went wrong.
| export class WebAssemblyThreading { | ||
| public static isThreadingEnabled() { | ||
| return (<any>globalThis).crossOriginIsolated && | ||
| typeof SharedArrayBuffer !== undefined && |
There was a problem hiding this comment.
Bug: typeof comparison is always true
typeof always returns a string ("function", "undefined", etc.), so comparing it to the value undefined (not the string 'undefined') is always true — SharedArrayBuffer is effectively never checked.
| typeof SharedArrayBuffer !== undefined && | |
| return (<any>globalThis).crossOriginIsolated && | |
| typeof SharedArrayBuffer !== 'undefined' && | |
| (<any>Module).PThread !== undefined; |
With the current code, isThreadingEnabled() treats SharedArrayBuffer as always present when crossOriginIsolated is true, which can mis-classify single-threaded environments as multi-threaded.
Sorry, something went wrong.
| @@ -0,0 +1,33 @@ | |||
| using System.Runtime.InteropServices.JavaScript; | |||
There was a problem hiding this comment.
Missing #nullable enable directive
Project convention requires a per-file #nullable enable at the top of every new/refactored file (see .claude/rules/code-style.md). Without it, WindowObject (line 23) is an unguarded non-nullable reference that the compiler silently allows to be null before Initialize() is called.
| using System.Runtime.InteropServices.JavaScript; | |
| #nullable enable | |
| using System.Runtime.InteropServices.JavaScript; |
Sorry, something went wrong.
| /// Use this for JSImports calls inside JSWebWorker. | ||
| /// See: JSProxyContext.SealJSImportCapturing(). | ||
| /// </summary> | ||
| public static JSObject WindowObject { get; private set; } |
There was a problem hiding this comment.
WindowObject should be nullable
After adding #nullable enable, this property needs a ? annotation (or a null! initializer if callers are always post-Initialize()). Before Initialize() is called, WindowObject is null and the property type makes no promise that it won't be.
| public static JSObject WindowObject { get; private set; } | |
| public static JSObject? WindowObject { get; private set; } |
Sorry, something went wrong.
| #nullable enable | ||
|
|
||
| using System.Runtime.InteropServices.JavaScript; | ||
| using System.Runtime.InteropServices.JavaScript; |
There was a problem hiding this comment.
#nullable enable was removed
The prior version of this file had #nullable enable at the top; this commit drops it. Please restore it — nullable annotations must not regress.
| using System.Runtime.InteropServices.JavaScript; | |
| #nullable enable | |
| using System.Runtime.InteropServices.JavaScript; | |
| using System.Threading.Tasks; |
Sorry, something went wrong.
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResizeAsync; | ||
| } | ||
| else { | ||
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; |
There was a problem hiding this comment.
Missing indentation in else body
The OnResize assignment is at the same level as the else keyword rather than indented inside the block.
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; | |
| else { | |
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; | |
| } |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24103/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
The build 228350 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 122 Details
|
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 2m 13s —— View job PR Review: WASM MT
SummaryVerdict: fix-first — the same findings from the previous review cycle remain unaddressed. No new issues were found in the new commits (OnResizeAsync, IsAutoEnableAccessibilityAsync, InitializeExports sequencing).
All five inline comments from the prior review have been re-posted on the current commit, since none were resolved:
What's good in the new commits
|
Sorry, something went wrong.
| export class WebAssemblyThreading { | ||
| public static isThreadingEnabled() { | ||
| return (<any>globalThis).crossOriginIsolated && | ||
| typeof SharedArrayBuffer !== undefined && |
There was a problem hiding this comment.
Bug (unresolved): typeof comparison against value undefined is always true
typeof always returns a string — comparing it to the value undefined (not the string 'undefined') always evaluates to true. As a result, isThreadingEnabled() never actually checks whether SharedArrayBuffer is available; it only checks crossOriginIsolated and Module.PThread.
| typeof SharedArrayBuffer !== undefined && | |
| typeof SharedArrayBuffer !== 'undefined' && |
This was flagged in the previous review cycle and is still unaddressed.
Sorry, something went wrong.
| @@ -0,0 +1,33 @@ | |||
| using System.Runtime.InteropServices.JavaScript; | |||
There was a problem hiding this comment.
Style (unresolved): missing #nullable enable
Project convention requires #nullable enable at the top of every new/refactored file (.claude/rules/code-style.md). Without it the compiler silently permits WindowObject (line 23) to be null before Initialize() is called.
| using System.Runtime.InteropServices.JavaScript; | |
| #nullable enable | |
| using System.Runtime.InteropServices.JavaScript; |
Sorry, something went wrong.
| /// Use this for JSImports calls inside JSWebWorker. | ||
| /// See: JSProxyContext.SealJSImportCapturing(). | ||
| /// </summary> | ||
| public static JSObject WindowObject { get; private set; } |
There was a problem hiding this comment.
Style (unresolved): WindowObject should be nullable
Before Initialize() is called, WindowObject is null but the property type makes no such promise. Once #nullable enable is added (line 1), this must be annotated with ? to match the actual lifecycle.
| public static JSObject WindowObject { get; private set; } | |
| public static JSObject? WindowObject { get; private set; } |
Sorry, something went wrong.
| #nullable enable | ||
|
|
||
| using System.Runtime.InteropServices.JavaScript; | ||
| using System.Runtime.InteropServices.JavaScript; |
There was a problem hiding this comment.
Style (unresolved): #nullable enable was removed
The previous version of this file had #nullable enable; this PR drops it. Nullable annotations must not regress.
| using System.Runtime.InteropServices.JavaScript; | |
| #nullable enable | |
| using System.Runtime.InteropServices.JavaScript; | |
| using System.Threading.Tasks; |
Sorry, something went wrong.
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResizeAsync; | ||
| } | ||
| else { | ||
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; |
There was a problem hiding this comment.
Style (unresolved): missing indentation inside else body
this.onResize is at the same tab depth as the else keyword rather than indented one level inside the block. Should be 4 tabs to match the if branch on line 44.
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; | |
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24103/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 2m 42s —— View job PR Review: WASM MT
SummaryVerdict: fix-first — the same five findings from the prior two review cycles remain unaddressed after this synchronize. No new issues were introduced by the latest commits.
Findings (all pre-existing, all still open)
All five inline comments from the previous cycle have been re-posted on the current commit since none were resolved. Each has a one-click suggestion block ready to apply. |
Sorry, something went wrong.
| export class WebAssemblyThreading { | ||
| public static isThreadingEnabled() { | ||
| return (<any>globalThis).crossOriginIsolated && | ||
| typeof SharedArrayBuffer !== undefined && |
There was a problem hiding this comment.
Bug (still unresolved): typeof comparison against value undefined is always true
typeof always returns a string ("undefined", "function", etc.). Comparing a string with the value undefined (not the string 'undefined') always evaluates to true, so this check never actually gates on whether SharedArrayBuffer exists.
| typeof SharedArrayBuffer !== undefined && | |
| typeof SharedArrayBuffer !== 'undefined' && |
This has been flagged in both previous review cycles and is still unaddressed.
Sorry, something went wrong.
| @@ -0,0 +1,33 @@ | |||
| using System.Runtime.InteropServices.JavaScript; | |||
There was a problem hiding this comment.
Style (still unresolved): missing #nullable enable
Project convention requires #nullable enable at the top of every new/refactored file (.claude/rules/code-style.md). Without it the compiler silently allows WindowObject (line 23) to be null before Initialize() is called.
| using System.Runtime.InteropServices.JavaScript; | |
| #nullable enable | |
| using System.Runtime.InteropServices.JavaScript; |
Sorry, something went wrong.
| /// Use this for JSImports calls inside JSWebWorker. | ||
| /// See: JSProxyContext.SealJSImportCapturing(). | ||
| /// </summary> | ||
| public static JSObject WindowObject { get; private set; } |
There was a problem hiding this comment.
Style (still unresolved): WindowObject must be nullable
Before Initialize() is called, WindowObject is null, but the property type makes no such promise. Once #nullable enable is added (line 1), this must be annotated with ? to reflect the pre-initialization lifecycle.
| public static JSObject WindowObject { get; private set; } | |
| public static JSObject? WindowObject { get; private set; } |
Sorry, something went wrong.
| #nullable enable | ||
|
|
||
| using System.Runtime.InteropServices.JavaScript; | ||
| using System.Runtime.InteropServices.JavaScript; |
There was a problem hiding this comment.
Style (still unresolved): #nullable enable was removed
The previous version of this file had #nullable enable; this PR drops it. Nullable annotations must not regress.
| using System.Runtime.InteropServices.JavaScript; | |
| #nullable enable | |
| using System.Runtime.InteropServices.JavaScript; | |
| using System.Threading.Tasks; |
Sorry, something went wrong.
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResizeAsync; | ||
| } | ||
| else { | ||
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; |
There was a problem hiding this comment.
Style (still unresolved): missing indentation inside else body
this.onResize is at the same tab depth as the else keyword rather than indented one level inside the block. Should be 4 tabs to match the if branch on line 44.
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; | |
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24103/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
The build 229242 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 158 Details
|
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24103/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 2m 5s —— View job PR Review: WASM MT
SummaryVerdict: fix-first — the same five findings from the previous three review cycles remain unaddressed after this synchronize. No new issues were introduced by the latest commits (fix(wasm): Disable dotnet.js fingerprinting on the SamplesApp head).
All five inline comments have been re-posted on the current commit since none were resolved across four review cycles. Each has a one-click suggestion block ready to apply.
What's good in the new commits
|
Sorry, something went wrong.
| export class WebAssemblyThreading { | ||
| public static isThreadingEnabled() { | ||
| return (<any>globalThis).crossOriginIsolated && | ||
| typeof SharedArrayBuffer !== undefined && |
There was a problem hiding this comment.
Bug (4th cycle, still unresolved): typeof comparison against value undefined is always true
typeof always returns a string ("undefined", "function", etc.). Comparing that string with the value undefined (not the string 'undefined') always evaluates to true, so this check never actually gates on whether SharedArrayBuffer exists. In cross-origin-isolated environments where SharedArrayBuffer is absent, isThreadingEnabled() will incorrectly return true (assuming Module.PThread is also present).
| typeof SharedArrayBuffer !== undefined && | |
| typeof SharedArrayBuffer !== 'undefined' && |
Sorry, something went wrong.
| @@ -0,0 +1,33 @@ | |||
| using System.Runtime.InteropServices.JavaScript; | |||
There was a problem hiding this comment.
Style (4th cycle, still unresolved): missing #nullable enable
Project convention (.claude/rules/code-style.md) requires #nullable enable at the top of every new/refactored file. Without it the compiler silently permits WindowObject (line 23) to be assigned null before Initialize() is called, and callers get no nullability diagnostics.
| using System.Runtime.InteropServices.JavaScript; | |
| #nullable enable | |
| using System.Runtime.InteropServices.JavaScript; |
Sorry, something went wrong.
| /// Use this for JSImports calls inside JSWebWorker. | ||
| /// See: JSProxyContext.SealJSImportCapturing(). | ||
| /// </summary> | ||
| public static JSObject WindowObject { get; private set; } |
There was a problem hiding this comment.
Style (4th cycle, still unresolved): WindowObject must be nullable
Before Initialize() is called WindowObject holds its default value of null, but the property type JSObject makes no such promise. Once #nullable enable is added at line 1, this must carry a ? annotation to match the actual pre-initialization lifecycle — and all callers will be able to guard against it being null.
| public static JSObject WindowObject { get; private set; } | |
| public static JSObject? WindowObject { get; private set; } |
Sorry, something went wrong.
| #nullable enable | ||
|
|
||
| using System.Runtime.InteropServices.JavaScript; | ||
| using System.Runtime.InteropServices.JavaScript; |
There was a problem hiding this comment.
Style (4th cycle, still unresolved): #nullable enable was removed
The previous version of this file had #nullable enable. This PR drops it while adding a new Task-returning JSImport — nullable coverage must not regress.
| using System.Runtime.InteropServices.JavaScript; | |
| #nullable enable | |
| using System.Runtime.InteropServices.JavaScript; | |
| using System.Threading.Tasks; |
Sorry, something went wrong.
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResizeAsync; | ||
| } | ||
| else { | ||
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; |
There was a problem hiding this comment.
Style (4th cycle, still unresolved): missing indentation inside else body
this.onResize is at the same tab depth as the else keyword rather than indented one level inside the block. It should be at 4 tabs to match the if branch on line 44.
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; | |
| this.onResize = WebAssemblyWindowWrapper.assemblyExports.Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.OnResize; |
Sorry, something went wrong.
|
The build 229435 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 171 Details
|
Sorry, something went wrong.
|
⚠️⚠️ The build 228350 has failed on Uno.UI - CI. |
Sorry, something went wrong.
|
⚠️⚠️ The build 229242 has failed on Uno.UI - CI. |
Sorry, something went wrong.
|
The build 228918 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 420 Details
|
Sorry, something went wrong.
|
⚠️⚠️ The build 228918 has failed on Uno.UI - CI. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
…drop"
This reverts commit e7518b8, reversing
changes made to 4990906.
Conflicts:
src/Directory.Build.propsGitHub Issue: closes
PR Type:
What changed? 🚀
PR Checklist ✅