Repository navigation
fix(generator): add defensive checks for cross-package type alias namespace resolution - #4931
ddmoney420 wants to merge 4 commits into
Conversation
The runChooserDialog function had a race condition that caused GTK assertion failures and crashes on Linux: 1. InvokeAsync scheduled gtk_dialog_run on the GTK thread 2. After dialog closed, a goroutine was spawned OFF the GTK thread 3. gtk_widget_destroy was called immediately (before goroutine ran) 4. Goroutine tried to call gtk_file_chooser_get_filenames on destroyed widget Fix: - Extract filenames on GTK thread BEFORE destroying widget - Call gtk_widget_destroy on GTK thread AFTER extraction - Goroutine only handles sending Go strings (no GTK calls) Fixes wailsapp#3683 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add typeof window/document checks to all module-level browser API accesses in the TypeScript runtime. This fixes the "window is not defined" error when using @wailsio/runtime with SSR frameworks like Next.js. Files modified: - runtime.ts: guard window.location access - calls.ts: guard window._wails initialization - contextmenu.ts: guard addEventListener call - drag.ts: guard window._wails and drag initialization - events.ts: guard window._wails initialization - index.ts: guard all window._wails setup and invoke calls - dialogs.ts: guard window._wails initialization - utils.ts: guard document.addEventListener for DOMContentLoaded - system.ts: guard IIFE that detects WebView environment Fixes wailsapp#4679 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When rendering JS/TS code for types from external packages, the code was directly looking up the package in m.Imports.External without checking if it existed. If for some reason the package wasn't added during type collection (edge case), this would result in a zero-value ImportInfo with an empty Name, producing invalid code like "$0.Model.createFrom" instead of "data_models$0.Model.createFrom". This fix adds defensive checks in create.go, type.go, and default.go to dynamically add missing packages to the imports map if they're not already present, ensuring valid namespace references are always generated. Fixes wailsapp#4578 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThis pull request addresses import resolution and browser compatibility issues. It fixes incorrect namespace references when generating TypeScript bindings from Go type aliases across packages by dynamically resolving missing imports in the code generator. Additionally, it adds server-side rendering (SSR) guards across the TypeScript runtime to prevent window/document access in non-browser environments, and restructures GTK file chooser result handling on the Go side for thread safety. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Hi @ddmoney420 , I am the original author of the v3 binding generator. Your PR was mistakenly opened against master (v2) so in order to review it I took the liberty to retarget it towards the v3-alpha branch. I noticed that you pushed onto the same branch two commits that look unrelated (although valuable!). Not sure if this was intentional. Before we move forward with this PR, could you please split the additional commits into their own PRs? (I recommend one PR per commit since they solve two different issues). |
|
Now getting to the actual fix.. I see that you tracked down the issue to a missing entry in the import map at render time. Unfortunately your approach of filling in missing entries on the fly is unsound: the rendering phase is parallelized and relies critically on the assumption that input data is mostly immutable. Breaking that assumption will cause data races and potentially nasty bugs. Filling import maps accurately is the collector's task, specifically the This sent me down an unexpected rabbit hole, because nothing in that method looks wrong. Actually, aliases share most of the implementation with named types, which work perfectly. As I started to despair, the light came to me: the culprit is the case *types.Alias, *types.Named:
if visited.Set(typ, true) != nil {
// Break type cycles.
return
}The issue is that Fortunately, we only use this map for aliases and named types, so we can replace it with a regular map indexed by objects: Before merging it will also be essential to add a test case, check that it reproduces the issue and check that it's fixed afterwards. It should be as simple as adding a new sub-package inside My recommendation is to revert revert to upstream first, add the test case, regenerate test results (see task |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v3/internal/runtime/desktop/@wailsio/runtime/src/system.ts (1)
106-162:⚠️ Potential issue | 🟡 MinorUnguarded
windowaccess inIs*functions will throw in SSR.The functions
IsWindows,IsLinux,IsMac,IsAMD64,IsARM,IsARM64, andIsDebugdirectly access(window as any)._wailswithout SSR guards. Calling these in a server context will throwReferenceError: window is not defined.Consider adding guards to return sensible defaults (e.g.,
false) in SSR:💡 Suggested fix for one function (apply pattern to others)
export function IsWindows(): boolean { + if (typeof window === "undefined") return false; return (window as any)._wails?.environment?.OS === "windows"; }
🤖 Fix all issues with AI agents
In `@v3/internal/generator/render/type.go`:
- Around line 213-219: Rendering mutates m.Imports.External via m.Imports.Add
inside render (see m.Imports.External, m.Imports.Add, m.collector.Package,
AddType), causing data races when render runs in parallel; fix it by moving
import population to collection time so render is read-only: in the collector
replace the visited tracking typeutil.Map with a plain map[types.Object]bool and
ensure alias resolution visits aliased types so the collector builds the full
import map (so m.Imports.External contains pkgPath entries before render), then
remove any runtime calls to m.Imports.Add from render code.
In `@v3/internal/runtime/desktop/`@wailsio/runtime/src/runtime.ts:
- Around line 13-14: The SSR fallback sets runtimeURL to an empty string which
causes new URL(runtimeURL) in runtimeCallWithID to throw an "Invalid URL"; add
an early guard in runtimeCallWithID that checks runtimeURL (and whether a custom
transport is configured) and throws a clear, descriptive error (e.g., "No
runtime URL available: set a custom transport for SSR or call from a browser")
instead of letting new URL() blow up; reference runtimeURL and the
runtimeCallWithID function and ensure the guard runs before any new URL(...) or
network/transport logic.
In `@v3/pkg/application/linux_cgo.go`:
- Around line 2119-2141: The code currently limits file chooser results to 1024
items by using a count cap while iterating the GList returned by
gtk_file_chooser_get_filenames, which silently drops additional selections;
remove the artificial count cap and iterate the full GList in the block guarded
by response == C.GTK_RESPONSE_ACCEPT (the loop using iter, iter.next,
buildStringAndFree and C.g_slist_free), or if you must limit, explicitly
record/truncate and send a sentinel or error over the selections channel so
callers are aware; ensure the iteration and freeing still occur on the GTK
thread and that the goroutine sending into selections remains unchanged.
| info := m.Imports.External[pkgPath] | ||
| if info.Name == "" { | ||
| // Package not in imports - this should not happen if AddType was called correctly. | ||
| // Add the package dynamically to avoid generating invalid code. | ||
| m.Imports.Add(m.collector.Package(typ.Obj().Pkg())) | ||
| info = m.Imports.External[pkgPath] | ||
| } |
There was a problem hiding this comment.
Data race: mutating import map during parallel rendering is unsafe.
Per the maintainer's comment on this PR, rendering is parallelized and assumes input data is immutable. Mutating m.Imports.External via m.Imports.Add() during render time can cause data races and non-deterministic bugs when multiple goroutines render concurrently.
The recommended fix is to address the root cause in the collector phase—specifically, replacing typeutil.Map with a plain map[types.Object]bool in the visited tracking so that alias resolution side-effects don't cause the aliased type's package to be skipped when building the import map.
🤖 Prompt for AI Agents
In `@v3/internal/generator/render/type.go` around lines 213 - 219, Rendering
mutates m.Imports.External via m.Imports.Add inside render (see
m.Imports.External, m.Imports.Add, m.collector.Package, AddType), causing data
races when render runs in parallel; fix it by moving import population to
collection time so render is read-only: in the collector replace the visited
tracking typeutil.Map with a plain map[types.Object]bool and ensure alias
resolution visits aliased types so the collector builds the full import map (so
m.Imports.External contains pkgPath entries before render), then remove any
runtime calls to m.Imports.Add from render code.
| // SSR guard: only access window.location in browser environment | ||
| const runtimeURL = typeof window !== "undefined" ? window.location.origin + "/wails/runtime" : ""; |
There was a problem hiding this comment.
Potential runtime error when runtimeURL is empty.
In SSR environments, runtimeURL defaults to an empty string. If a runtime call is made without a custom transport set, Line 110 (new URL(runtimeURL)) will throw "Invalid URL" because an empty string is not a valid URL.
Consider either:
- Adding an early guard in
runtimeCallWithIDto throw a more descriptive error - Documenting that a custom transport must be set for SSR usage
💡 Suggested improvement for clearer error messaging
async function runtimeCallWithID(objectID: number, method: number, windowName: string, args: any): Promise<any> {
// Use custom transport if available
if (customTransport) {
return customTransport.call(objectID, method, windowName, args);
}
+ // Guard against SSR usage without custom transport
+ if (!runtimeURL) {
+ throw new Error("Wails runtime calls require a browser environment or a custom transport");
+ }
+
// Default HTTP fetch transport
let url = new URL(runtimeURL);🤖 Prompt for AI Agents
In `@v3/internal/runtime/desktop/`@wailsio/runtime/src/runtime.ts around lines 13
- 14, The SSR fallback sets runtimeURL to an empty string which causes new
URL(runtimeURL) in runtimeCallWithID to throw an "Invalid URL"; add an early
guard in runtimeCallWithID that checks runtimeURL (and whether a custom
transport is configured) and throws a clear, descriptive error (e.g., "No
runtime URL available: set a custom transport for SSR or call from a browser")
instead of letting new URL() blow up; reference runtimeURL and the
runtimeCallWithID function and ensure the guard runs before any new URL(...) or
network/transport logic.
| response := C.gtk_dialog_run((*C.GtkDialog)(fc)) | ||
| // Extract results on GTK thread BEFORE destroying widget | ||
| var results []string | ||
| if response == C.GTK_RESPONSE_ACCEPT { | ||
| filenames := C.gtk_file_chooser_get_filenames((*C.GtkFileChooser)(fc)) | ||
| iter := filenames | ||
| count := 0 | ||
| for iter != nil && count < 1024 { | ||
| results = append(results, buildStringAndFree(C.gpointer(iter.data))) | ||
| iter = iter.next | ||
| count++ | ||
| } | ||
| C.g_slist_free(filenames) | ||
| } | ||
| // Destroy widget after extracting results (on GTK thread) | ||
| C.gtk_widget_destroy((*C.GtkWidget)(unsafe.Pointer(fc))) | ||
| // Send results from goroutine (safe - no GTK calls) | ||
| go func() { | ||
| defer handlePanic() | ||
| if response == C.GTK_RESPONSE_ACCEPT { | ||
| filenames := C.gtk_file_chooser_get_filenames((*C.GtkFileChooser)(fc)) | ||
| iter := filenames | ||
| count := 0 | ||
| for { | ||
| selections <- buildStringAndFree(C.gpointer(iter.data)) | ||
| iter = iter.next | ||
| if iter == nil || count == 1024 { | ||
| break | ||
| } | ||
| count++ | ||
| } | ||
| for _, result := range results { | ||
| selections <- result | ||
| } | ||
| close(selections) |
There was a problem hiding this comment.
Avoid silently truncating selections at 1024 items.
Line 2126 introduces a hard cap; selecting more than 1024 files will drop the rest without notice. Consider iterating the full list or explicitly signaling truncation.
[suggested fix]
🧩 Remove silent truncation
- count := 0
- for iter != nil && count < 1024 {
+ for iter != nil {
results = append(results, buildStringAndFree(C.gpointer(iter.data)))
iter = iter.next
- count++
}📝 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.
| response := C.gtk_dialog_run((*C.GtkDialog)(fc)) | |
| // Extract results on GTK thread BEFORE destroying widget | |
| var results []string | |
| if response == C.GTK_RESPONSE_ACCEPT { | |
| filenames := C.gtk_file_chooser_get_filenames((*C.GtkFileChooser)(fc)) | |
| iter := filenames | |
| count := 0 | |
| for iter != nil && count < 1024 { | |
| results = append(results, buildStringAndFree(C.gpointer(iter.data))) | |
| iter = iter.next | |
| count++ | |
| } | |
| C.g_slist_free(filenames) | |
| } | |
| // Destroy widget after extracting results (on GTK thread) | |
| C.gtk_widget_destroy((*C.GtkWidget)(unsafe.Pointer(fc))) | |
| // Send results from goroutine (safe - no GTK calls) | |
| go func() { | |
| defer handlePanic() | |
| if response == C.GTK_RESPONSE_ACCEPT { | |
| filenames := C.gtk_file_chooser_get_filenames((*C.GtkFileChooser)(fc)) | |
| iter := filenames | |
| count := 0 | |
| for { | |
| selections <- buildStringAndFree(C.gpointer(iter.data)) | |
| iter = iter.next | |
| if iter == nil || count == 1024 { | |
| break | |
| } | |
| count++ | |
| } | |
| for _, result := range results { | |
| selections <- result | |
| } | |
| close(selections) | |
| response := C.gtk_dialog_run((*C.GtkDialog)(fc)) | |
| // Extract results on GTK thread BEFORE destroying widget | |
| var results []string | |
| if response == C.GTK_RESPONSE_ACCEPT { | |
| filenames := C.gtk_file_chooser_get_filenames((*C.GtkFileChooser)(fc)) | |
| iter := filenames | |
| for iter != nil { | |
| results = append(results, buildStringAndFree(C.gpointer(iter.data))) | |
| iter = iter.next | |
| } | |
| C.g_slist_free(filenames) | |
| } | |
| // Destroy widget after extracting results (on GTK thread) | |
| C.gtk_widget_destroy((*C.GtkWidget)(unsafe.Pointer(fc))) | |
| // Send results from goroutine (safe - no GTK calls) | |
| go func() { | |
| defer handlePanic() | |
| for _, result := range results { | |
| selections <- result | |
| } | |
| close(selections) |
🤖 Prompt for AI Agents
In `@v3/pkg/application/linux_cgo.go` around lines 2119 - 2141, The code currently
limits file chooser results to 1024 items by using a count cap while iterating
the GList returned by gtk_file_chooser_get_filenames, which silently drops
additional selections; remove the artificial count cap and iterate the full
GList in the block guarded by response == C.GTK_RESPONSE_ACCEPT (the loop using
iter, iter.next, buildStringAndFree and C.g_slist_free), or if you must limit,
explicitly record/truncate and send a sentinel or error over the selections
channel so callers are aware; ensure the iteration and freeing still occur on
the GTK thread and that the goroutine sending into selections remains unchanged.
leaanthony
left a comment
leaanthony
left a comment
There was a problem hiding this comment.
This PR mixes several unrelated changes and includes duplicates:
Issues:
-
Duplicate of #4926: The
linux_cgo.gochanges are identical to PR #4926 which is already under review. Please remove these changes from this PR. -
Unrelated SSR guards: The TypeScript runtime SSR guards are a separate concern from the generator namespace resolution fix. Consider splitting these into a separate PR for cleaner review.
The generator fix itself looks good - adding defensive checks for cross-package type alias namespace resolution is the right approach.
Please:
- Remove
v3/pkg/application/linux_cgo.gochanges (they'll come from #4926) - Either remove the SSR guards or split them into a separate PR
This will make the PR focused and easier to review/merge.
|
Thank you both for the thorough feedback and guidance. I really appreciate you taking the time to explain the collector vs render phase distinction. I realize I bit off more than I can chew here - this fix requires deeper knowledge of the Wails build pipeline than I currently have. Rather than risk introducing new bugs, I'm going to close this PR. If someone with more Wails experience wants to pick this up, @fbbdev's explanation is an excellent roadmap. |
|
@ddmoney420 thanks a lot anyways for debugging the issue, it pointed me in the right direction. I opened a PR with the proposed fix at #4948, it works for the test case I created but it would be nice if you could validate it as well on some independent input. Indeed this kind of fix requires in-depth knowledge of the generator pipeline, if anything to avoid concurrency related traps. But it's good for the project if people other than me become familiar with it, and I'm always happy to offer guidance. |
3 participants
Summary
Adds defensive checks in the TypeScript bindings generator to ensure external package imports are properly resolved when handling type aliases across packages.
Fixes #4578
Problem
When a Go type alias from package A points to a type in package B, the TypeScript bindings generator could produce incorrect namespace references like
$0.Model.createFrominstead ofdata_models$0.Model.createFrom.This occurs when
m.Imports.External[pkgPath]returns a zero-valueImportInfo{Name: "", Index: 0}because the package path was not properly registered in the imports map.Solution
Added defensive checks in three files to ensure external package imports exist before using them:
v3/internal/generator/render/create.go- inPostponedCreatesv3/internal/generator/render/type.go- inrenderNamedTypev3/internal/generator/render/default.go- inrenderNamedDefault(two places)If an import is missing, it's now dynamically added to prevent the empty namespace prefix.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores