Skip to content

Conversation

gongzhxu
Copy link

@gongzhxu gongzhxu commented Jul 8, 2025

Summary

  1. Passing the dlv flag can debug the golang program.

Resolves
[Closes https://github.com/wailsapp/wails/issues/2740](#3623)

USAGE
wails dev -dlvflag '--listen=:2345 --headless=true --api-version=2 --accept-multiclient'

IDE Goland

  1. add Shell script to startup wails program
    image

  2. add Go Remote
    image

  3. add Compound to run above two
    image

  4. debug the program
    image

TODO

  1. Supplement and improve the use of VSCode
  2. Automatically generate .run debugging configuration files during project initialization

Summary by CodeRabbit

  • New Features
    • Added a debug flag to development mode to pass options to the Go debugger (Delve).
  • Performance
    • Optimized byte-to-string conversions across the app to reduce allocations and improve responsiveness.
  • Bug Fixes
    • Corrected asset retrieval during development to improve reliability of rebuilds and hot-reload.
  • Documentation
    • Updated changelog to note the new debug flag and standardized string conversions.

Copy link
Contributor

coderabbitai bot commented Jul 8, 2025

Walkthrough

Introduces a new dlvflag option for dev runs and updates restart logic to optionally launch via Delve. Adds an internal conv package with BytesToString and replaces many string([]byte) conversions across the codebase. Fixes a byte-to-string conversion in a dev watcher. Updates changelog.

Changes

Cohort / File(s) Summary
Delve debug integration
v2/cmd/wails/flags/dev.go, v2/cmd/wails/internal/dev/dev.go
Adds Dev.DlvFlag; restartApp conditionally runs app via dlv with parsed flags; logs executed command; retains existing cleanup/error handling.
New conversion utility
v2/internal/conv/conv.go
Adds conv.BytesToString([]byte) using unsafe conversion (read-only).
Frontend string conversions
v2/internal/frontend/desktop/darwin/clipboard.go, v2/internal/frontend/desktop/darwin/frontend.go, v2/internal/frontend/desktop/linux/frontend.go, v2/internal/frontend/desktop/windows/frontend.go
Replaces string(b) with conv.BytesToString(...) for clipboard/Callback paths.
Single-instance conversions
v2/internal/frontend/desktop/linux/single_instance.go, v2/internal/frontend/desktop/windows/single_instance.go
Uses conv.BytesToString(...) for serialized message payloads; minor import reorders.
Dispatcher conversions
v2/internal/frontend/dispatcher/calls.go, v2/internal/frontend/dispatcher/securecalls.go
Uses conv.BytesToString(...) for marshaled JSON in logs/returns.
Menu manager conversions
v2/internal/menumanager/contextmenu.go, v2/internal/menumanager/processedMenu.go, v2/internal/menumanager/traymenu.go
Uses conv.BytesToString(...) when returning JSON strings.
Watcher/read conversion fixes
v2/cmd/wails/internal/dev/dev.go, v2/cmd/wails/generate.go
Replaces string(content) with conv.BytesToString(...) for assetDir and package-lock.json reads.
Utilities and build paths
v2/internal/s/s.go, v2/pkg/commands/build/base.go, v2/pkg/git/git.go, v2/tools/release/release.go
Standardizes byte-to-string via conv.BytesToString(...) in loaders, build error output, git escape, and release tooling.
Tests updated for conv
v2/internal/binding/binding_test/binding_conflicting_package_name_test.go, v2/internal/binding/binding_test/binding_returned_promises_test.go, v2/internal/binding/binding_test/binding_type_alias_test.go, v2/pkg/commands/bindings/bindings_test.go
Tests now use conv.BytesToString(...) for file content comparisons; added conv imports; import order tweaks.
Docs
website/src/pages/changelog.mdx
Adds Unreleased entries for dlvflag and standardized string conversions.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Dev as Dev CLI
  participant Restart as restartApp
  participant OS as OS Process

  Dev->>Restart: run (appBinary, args, DlvFlag)
  alt DlvFlag empty
    Note over Restart: Launch app directly
    Restart->>OS: StartProcess(appBinary, args)
  else DlvFlag provided
    Note over Restart: Launch via Delve
    Restart->>Restart: dlvArgs = shlex.Split(DlvFlag)
    Restart->>Restart: newArgs = dlvArgs + ["exec", appBinary] + ["--", args...]
    Restart->>OS: StartProcess("dlv", newArgs)
  end
  OS-->>Restart: pid / error
  Restart-->>Dev: result (with existing cleanup/error handling)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

lgtm

Suggested reviewers

  • leaanthony

Poem

I nibbled on bytes with whiskered delight,
Turned slices to strings in the moon’s gentle light.
A hop to the dlv trail—debuggers awake!
Quiet conversions, no copies to make.
Changelog sprinkled, carrots aligned—
Ship it, dear devs, with a hop so refined! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning The linked issue (#2740) requests a runtime mechanism to open devtools via a function, but this PR instead implements a dlvflag for Delve debugging and does not provide any devtools-opening functionality, so it does not satisfy the linked issue’s objectives. Clarify which issue this PR targets—if it intends to implement Delve debugging, link to the correct issue for dlvflag support; otherwise, implement the devtools-opening function described in issue #2740.
Out of Scope Changes Check ⚠️ Warning In addition to adding dlvflag support, the PR introduces widespread replacements of string() conversions with conv.BytesToString() across many unrelated packages, which is not part of the dlvflag requirement and therefore constitutes out-of-scope changes. Please remove or move the conv.BytesToString refactoring into a separate PR so this change set focuses solely on adding the dlvflag feature.
Description Check ⚠️ Warning The description does not follow the repository’s template: it omits sections such as “Type of change,” “How Has This Been Tested?”, the required “Fixes #” syntax, and the checklist items, and it uses nonstandard headings instead of the prescribed ones. Please reformat the description to match the template by adding a “Description” section with the proper issue reference using “Fixes #,” selecting the “Type of change,” detailing testing steps under “How Has This Been Tested?”, and completing the checklist items at the bottom.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title is concise, follows the conventional “feat:” prefix, and clearly summarizes the primary change of adding a dlvflag option for Go debugging without extraneous detail or noise.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. Enhancement New feature or request go Pull requests that update Go code labels Jul 8, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
v2/cmd/wails/flags/dev.go (1)

34-34: Consider enhancing the flag description for better user experience.

The flag addition is implemented correctly and follows the established pattern. However, the description could be more informative about the expected format or provide usage examples.

Consider updating the description to be more descriptive:

-	DlvFlag              string `flag:"dlvflag" description:"Flags to pass to dlv"`
+	DlvFlag              string `flag:"dlvflag" description:"Flags to pass to dlv (space-separated, e.g., '--listen=:40000 --headless')"`
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e753157 and 739c436.

📒 Files selected for processing (2)
  • v2/cmd/wails/flags/dev.go (1 hunks)
  • v2/cmd/wails/internal/dev/dev.go (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
v2/cmd/wails/internal/dev/dev.go (1)
v2/internal/process/process.go (1)
  • NewProcess (16-24)

Comment on lines 312 to 327

var cmd string
if len(f.DlvFlag) == 0 {
cmd = appBinary
} else {
cmd = "dlv"
newArgs := append(strings.Split(f.DlvFlag, " "), "exec", appBinary)
if len(args) > 0 {
newArgs = append(newArgs, "--")
args = append(newArgs, args...)
}
args = newArgs
}

buildOptions.Logger.Print("NewProcess cmd: %s, args: %v", cmd, args)
newProcess := process.NewProcess(cmd, args...)
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve argument parsing robustness and add basic validation.

The core logic for dlv integration is correct and follows the proper command structure. However, there are opportunities for improvement:

  1. Argument parsing: Using strings.Split(f.DlvFlag, " ") doesn't handle quoted arguments properly, which could break with complex flags like --listen=:40000 --api-version=2.

  2. Missing validation: No check if dlv is available on the system.

Consider this improved implementation:

 	var cmd string
+	var newArgs []string
 	if len(f.DlvFlag) == 0 {
 		cmd = appBinary
 	} else {
 		cmd = "dlv"
-		newArgs := append(strings.Split(f.DlvFlag, " "), "exec", appBinary)
+		// Use shlex to properly handle quoted arguments
+		dlvArgs, err := shlex.Split(f.DlvFlag)
+		if err != nil {
+			buildOptions.Logger.Fatal("Unable to parse dlvflag: %s", err.Error())
+		}
+		newArgs = append(dlvArgs, "exec", appBinary)
 		if len(args) > 0 {
 			newArgs = append(newArgs, "--")
-			args = append(newArgs, args...)
+			newArgs = append(newArgs, args...)
 		}
-		args = newArgs
+		args = newArgs
 	}

This uses the already imported shlex package for proper argument parsing and fixes the argument construction logic.

🤖 Prompt for AI Agents
In v2/cmd/wails/internal/dev/dev.go around lines 312 to 327, the current code
uses strings.Split to parse f.DlvFlag, which fails to handle quoted arguments
correctly, and it lacks validation to check if the 'dlv' command is available on
the system. To fix this, replace strings.Split with the shlex package to
properly parse f.DlvFlag into arguments respecting quotes, and add a check
before running the command to verify that 'dlv' is installed and accessible,
returning an error or logging appropriately if not found.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gongzhxu ☝️ This is a good suggestion. Please could you add an entry to the changelog located at website/src/pages/changelog.mdx?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@leaanthony leaanthony added the awaiting feedback More information is required from the requestor label Jul 11, 2025
@leaanthony
Copy link
Member

Thanks 🙏 Please could you add an entry to the changelog located at website/src/pages/changelog.mdx?
Also there's a merge conflict 👍

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jul 19, 2025
Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fb092e6 and 353783a.

📒 Files selected for processing (23)
  • v2/cmd/wails/generate.go (2 hunks)
  • v2/cmd/wails/internal/dev/dev.go (3 hunks)
  • v2/internal/binding/binding_test/binding_conflicting_package_name_test.go (2 hunks)
  • v2/internal/binding/binding_test/binding_returned_promises_test.go (2 hunks)
  • v2/internal/binding/binding_test/binding_type_alias_test.go (2 hunks)
  • v2/internal/conv/conv.go (1 hunks)
  • v2/internal/frontend/desktop/darwin/clipboard.go (2 hunks)
  • v2/internal/frontend/desktop/darwin/frontend.go (2 hunks)
  • v2/internal/frontend/desktop/linux/frontend.go (2 hunks)
  • v2/internal/frontend/desktop/linux/single_instance.go (2 hunks)
  • v2/internal/frontend/desktop/windows/frontend.go (1 hunks)
  • v2/internal/frontend/desktop/windows/single_instance.go (2 hunks)
  • v2/internal/frontend/desktop/windows/winc/w32/utils.go (1 hunks)
  • v2/internal/frontend/dispatcher/calls.go (3 hunks)
  • v2/internal/frontend/dispatcher/securecalls.go (2 hunks)
  • v2/internal/menumanager/contextmenu.go (2 hunks)
  • v2/internal/menumanager/processedMenu.go (2 hunks)
  • v2/internal/menumanager/traymenu.go (3 hunks)
  • v2/internal/s/s.go (2 hunks)
  • v2/pkg/commands/bindings/bindings_test.go (2 hunks)
  • v2/pkg/commands/build/base.go (2 hunks)
  • v2/pkg/git/git.go (2 hunks)
  • v2/tools/release/release.go (3 hunks)
✅ Files skipped from review due to trivial changes (9)
  • v2/pkg/commands/build/base.go
  • v2/internal/frontend/desktop/darwin/clipboard.go
  • v2/cmd/wails/generate.go
  • v2/internal/menumanager/traymenu.go
  • v2/internal/menumanager/contextmenu.go
  • v2/internal/frontend/dispatcher/calls.go
  • v2/tools/release/release.go
  • v2/pkg/git/git.go
  • v2/internal/frontend/dispatcher/securecalls.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • v2/cmd/wails/internal/dev/dev.go
🧰 Additional context used
🧠 Learnings (8)
v2/internal/binding/binding_test/binding_returned_promises_test.go (1)
Learnt from: fbbdev
PR: wailsapp/wails#4045
File: v3/internal/generator/render/info.go:28-68
Timestamp: 2025-02-04T23:59:43.956Z
Learning: In the Wails v3 project, internal functions are designed to panic on nil parameters as they represent contract violations, rather than adding defensive nil checks.
v2/pkg/commands/bindings/bindings_test.go (1)
Learnt from: fbbdev
PR: wailsapp/wails#4045
File: v3/internal/generator/render/info.go:28-68
Timestamp: 2025-02-04T23:59:43.956Z
Learning: In the Wails v3 project, internal functions are designed to panic on nil parameters as they represent contract violations, rather than adding defensive nil checks.
v2/internal/menumanager/processedMenu.go (2)
Learnt from: leaanthony
PR: wailsapp/wails#4031
File: v3/pkg/application/menu.go:199-202
Timestamp: 2025-01-24T22:41:18.566Z
Learning: In the Wails menu system (v3/pkg/application/menu.go), shared state between menus is intentionally designed and desirable. Methods like `Append()` and `Prepend()` should maintain shared references to menu items rather than creating deep copies.
Learnt from: fbbdev
PR: wailsapp/wails#4066
File: v3/pkg/application/messageprocessor_call.go:174-174
Timestamp: 2025-02-13T01:05:02.267Z
Learning: When handling JSON marshaling errors in Wails v3, the error message from json.Marshal provides sufficient debugging context. Logging raw data is unnecessary and could make logs harder to read.
v2/internal/frontend/desktop/windows/single_instance.go (1)
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:91-113
Timestamp: 2025-02-24T06:08:55.645Z
Learning: The JSON quote handling in Windows notifications' activation arguments (v3/pkg/services/notifications/notifications_windows.go) has a known limitation with single-quote collisions that needs to be addressed after initial testing.
v2/internal/frontend/desktop/linux/frontend.go (2)
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:91-113
Timestamp: 2025-02-24T06:08:55.645Z
Learning: The JSON quote handling in Windows notifications' activation arguments (v3/pkg/services/notifications/notifications_windows.go) has a known limitation with single-quote collisions that needs to be addressed after initial testing.
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:113-164
Timestamp: 2025-02-24T06:23:10.967Z
Learning: In Windows Toast notifications (notifications_windows.go), metadata must be appended to each action's Arguments because the toast activated callback only returns the Arguments string of the clicked action. This is achieved by appending the same escaped JSON data (with single quotes) to all actions using the ":::" delimiter.
v2/internal/frontend/desktop/windows/frontend.go (2)
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_windows.go:91-113
Timestamp: 2025-02-24T06:08:55.645Z
Learning: The JSON quote handling in Windows notifications' activation arguments (v3/pkg/services/notifications/notifications_windows.go) has a known limitation with single-quote collisions that needs to be addressed after initial testing.
Learnt from: fbbdev
PR: wailsapp/wails#4045
File: v3/internal/generator/render/info.go:28-68
Timestamp: 2025-02-04T23:59:43.956Z
Learning: In the Wails v3 project, internal functions are designed to panic on nil parameters as they represent contract violations, rather than adding defensive nil checks.
v2/internal/frontend/desktop/darwin/frontend.go (2)
Learnt from: fbbdev
PR: wailsapp/wails#4045
File: v3/internal/generator/render/info.go:28-68
Timestamp: 2025-02-04T23:59:43.956Z
Learning: In the Wails v3 project, internal functions are designed to panic on nil parameters as they represent contract violations, rather than adding defensive nil checks.
Learnt from: leaanthony
PR: wailsapp/wails#4031
File: v3/pkg/application/menu.go:199-202
Timestamp: 2025-01-24T22:41:18.566Z
Learning: In the Wails menu system (v3/pkg/application/menu.go), shared state between menus is intentionally designed and desirable. Methods like `Append()` and `Prepend()` should maintain shared references to menu items rather than creating deep copies.
v2/internal/s/s.go (1)
Learnt from: fbbdev
PR: wailsapp/wails#4045
File: v3/internal/generator/render/info.go:28-68
Timestamp: 2025-02-04T23:59:43.956Z
Learning: In the Wails v3 project, internal functions are designed to panic on nil parameters as they represent contract violations, rather than adding defensive nil checks.
🧬 Code Graph Analysis (11)
v2/internal/binding/binding_test/binding_conflicting_package_name_test.go (1)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
v2/internal/binding/binding_test/binding_returned_promises_test.go (1)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
v2/pkg/commands/bindings/bindings_test.go (1)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
v2/internal/binding/binding_test/binding_type_alias_test.go (1)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
v2/internal/menumanager/processedMenu.go (1)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
v2/internal/frontend/desktop/windows/winc/w32/utils.go (1)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
v2/internal/frontend/desktop/windows/single_instance.go (2)
v2/internal/frontend/desktop/windows/winc/w32/user32.go (1)
  • SendMessage (371-379)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
v2/internal/frontend/desktop/linux/frontend.go (1)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
v2/internal/frontend/desktop/windows/frontend.go (1)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
v2/internal/frontend/desktop/darwin/frontend.go (1)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
v2/internal/s/s.go (1)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
🔇 Additional comments (17)
v2/pkg/commands/bindings/bindings_test.go (1)

11-11: Inconsistent with PR objectives - these changes are unrelated to dlvflag debugging feature.

The PR objectives state this is about adding dlvflag for golang debug, but these changes are replacing byte-to-string conversions with an unsafe utility function. This appears to be a completely different set of changes.

Additionally, while the unsafe conversion is safe here since goMod is not reused after line 65, using unsafe optimizations in test code provides minimal benefit and adds unnecessary risk.

Also applies to: 65-65

Likely an incorrect or invalid review comment.

v2/internal/s/s.go (1)

14-14: Safe but unrelated to PR objectives.

While this unsafe conversion is safe (the byte slice is not reused), these changes are unrelated to the stated PR objective of adding dlvflag for golang debugging.

Also applies to: 267-267

Likely an incorrect or invalid review comment.

v2/internal/binding/binding_test/binding_conflicting_package_name_test.go (1)

13-13: Unrelated to PR objectives.

These changes replace string conversion with unsafe optimization, which is unrelated to the stated PR objective of adding dlvflag for golang debugging.

Also applies to: 61-61

Likely an incorrect or invalid review comment.

v2/internal/binding/binding_test/binding_returned_promises_test.go (1)

9-9: Unrelated to PR objectives.

These changes are part of the byte-to-string conversion optimization pattern, not the dlvflag debugging feature mentioned in the PR objectives.

Also applies to: 78-78

Likely an incorrect or invalid review comment.

v2/internal/conv/conv.go (1)

1-11: This file is completely unrelated to the PR objectives of adding dlvflag for golang debugging.

The PR description mentions adding debugging support with Delve, but this file introduces an unsafe string conversion utility. This appears to be an entirely different feature.

Additionally, while the unsafe conversion can improve performance by avoiding allocation, it comes with significant risks that should be better documented:

Consider adding more comprehensive documentation:

 // BytesToString converts []byte to string without memory allocation.
 // WARNING: The returned string must be used as read-only!
+// 
+// SAFETY: This function is only safe to use when:
+// 1. The byte slice will not be modified after conversion
+// 2. The byte slice will outlive the returned string
+// 3. The caller does not store the string beyond the lifetime of the byte slice
+//
+// DO NOT use this function with:
+// - Byte slices that may be modified
+// - Temporary byte slices that may be garbage collected
+// - Byte slices from pools that may be reused
 func BytesToString(b []byte) string {

Likely an incorrect or invalid review comment.

v2/internal/binding/binding_test/binding_type_alias_test.go (2)

8-9: LGTM: Import reorganization follows Go conventions.

The external package imports were correctly moved below standard library imports, and the new conv package import was added appropriately.


61-61: LGTM: Consistent byte-to-string conversion optimization.

The change from string(rawGeneratedBindings) to conv.BytesToString(rawGeneratedBindings) is correct and aligns with the broader refactoring to use zero-allocation string conversions throughout the codebase.

v2/internal/frontend/desktop/windows/single_instance.go (2)

11-14: LGTM: Import reorganization improves code structure.

The external package imports were correctly grouped together at the bottom of the import block, following Go conventions.


67-67: LGTM: Optimized byte-to-string conversion in IPC context.

The change from string(serialized) to conv.BytesToString(serialized) is appropriate for the JSON-marshaled data being passed to SendMessage. This optimization eliminates unnecessary allocation during inter-process communication.

v2/internal/menumanager/processedMenu.go (2)

6-6: LGTM: Added required import for conversion utility.

The conv package import was correctly added to support the new byte-to-string conversion function.


138-138: LGTM: Zero-allocation JSON marshaling optimization.

The change from string(menuAsJSON) to conv.BytesToString(menuAsJSON) optimizes the conversion of JSON-marshaled bytes to string without affecting the method's functionality or error handling.

v2/internal/frontend/desktop/linux/frontend.go (2)

7-7: LGTM: Cleaned up cgo directive formatting.

Removed trailing space in the cgo directive, improving code cleanliness without affecting functionality.


494-494: LGTM: Optimized string conversion in JavaScript callback.

The change from string(escaped) to conv.BytesToString(escaped) eliminates allocation when converting JSON-escaped message data for JavaScript execution, consistent with the broader optimization effort.

v2/internal/frontend/desktop/linux/single_instance.go (2)

12-13: LGTM: Import reorganization follows Go conventions.

The reordering of imports to separate standard library from third-party packages is correct.


72-72: LGTM: Efficient byte-to-string conversion optimization.

The replacement of string(serialized) with conv.BytesToString(serialized) provides zero-allocation conversion for the D-Bus message payload, which is appropriate since the string is used immediately in the API call.

v2/internal/frontend/desktop/darwin/frontend.go (2)

29-29: LGTM: Conv package import added for optimization.

The addition of the conv package import supports the byte-to-string conversion optimization.


428-428: LGTM: Optimized callback message conversion.

The replacement of string(escaped) with conv.BytesToString(escaped) provides zero-allocation conversion for the JSON callback payload, improving performance in the JavaScript execution path.

}
f.mainWindow.Invoke(func() {
f.chromium.Eval(`window.wails.Callback(` + string(escaped) + `);`)
f.chromium.Eval(`window.wails.Callback(` + conv.BytesToString(escaped) + `);`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

LGTM: Optimized callback conversion, but verify import.

The replacement of string(escaped) with conv.BytesToString(escaped) provides efficient conversion for the JavaScript callback. However, ensure the conv package import is properly added to this file.


🏁 Script executed:

#!/bin/bash
# Description: Verify conv package import in windows frontend file
# Expected: Should find the conv import declaration

rg -A 5 -B 5 "github.com/wailsapp/wails/v2/internal/conv" v2/internal/frontend/desktop/windows/frontend.go

Length of output: 106


To confirm whether the conv package is actually imported and how it’s referenced, let’s inspect the import block and any occurrences of conv in the file:


🏁 Script executed:

#!/bin/bash
# Show the import section (with context)
rg -A20 -B0 "^import" v2/internal/frontend/desktop/windows/frontend.go

# List all lines where conv is referenced
rg -n "conv\." v2/internal/frontend/desktop/windows/frontend.go

Length of output: 677


Missing conv import in Windows frontend

The call to conv.BytesToString on line 812 will fail without importing the conv package. Please add the following import to v2/internal/frontend/desktop/windows/frontend.go:

 import (
   // existing imports…
+  "github.com/wailsapp/wails/v2/internal/conv"
 )

This ensures conv.BytesToString(escaped) resolves correctly.

📝 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.

Suggested change
f.chromium.Eval(`window.wails.Callback(` + conv.BytesToString(escaped) + `);`)
import (
// existing imports…
+ "github.com/wailsapp/wails/v2/internal/conv"
)
🤖 Prompt for AI Agents
In v2/internal/frontend/desktop/windows/frontend.go at line 812, the code uses
conv.BytesToString but the conv package is not imported. To fix this, add an
import statement for the conv package at the top of the file so that
conv.BytesToString(escaped) resolves correctly and the code compiles without
errors.

@leaanthony
Copy link
Member

@gongzhxu - do you still want to merge this in? There's a few comments to address above 🙏

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
v2/cmd/wails/internal/dev/dev.go (1)

504-504: Avoid unsafe zero-copy here or document the guarantee.

Using conv.BytesToString is unsafe unless the backing []byte is guaranteed immutable thereafter. Here it’s likely fine, but the perf gain is negligible and readability/safety suffer.

Apply this safer change:

-							assetDir = conv.BytesToString(content)
+							assetDir = string(content)

If you keep the zero-copy, add a brief comment stating the slice will not be mutated after this point.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 353783a and 8d2af56.

📒 Files selected for processing (5)
  • v2/cmd/wails/flags/dev.go (1 hunks)
  • v2/cmd/wails/internal/dev/dev.go (3 hunks)
  • v2/internal/frontend/desktop/darwin/frontend.go (2 hunks)
  • v2/internal/frontend/desktop/linux/frontend.go (1 hunks)
  • v2/internal/frontend/desktop/windows/frontend.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • v2/internal/frontend/desktop/darwin/frontend.go
  • v2/internal/frontend/desktop/windows/frontend.go
  • v2/cmd/wails/flags/dev.go
  • v2/internal/frontend/desktop/linux/frontend.go
🧰 Additional context used
🧬 Code graph analysis (1)
v2/cmd/wails/internal/dev/dev.go (2)
v2/internal/process/process.go (1)
  • NewProcess (16-24)
v2/internal/conv/conv.go (1)
  • BytesToString (9-11)
🔇 Additional comments (2)
v2/cmd/wails/internal/dev/dev.go (2)

26-26: Import looks fine.

The conv package is used below (Line 504).


318-334: Fix loss of app args and robustly parse dlv flags; also validate dlv availability.

  • Bug: Original app args are dropped when dlv is enabled (Line 327 sets args but later overwritten by Line 329).
  • Use shlex.Split to correctly handle quoted dlv flags.
  • Treat whitespace-only dlv flags as empty.
  • Validate that dlv exists in PATH before attempting to run.

Apply this diff:

-	var command string
-	if len(f.DlvFlag) == 0 {
-		command = appBinary
-	} else {
-		command = "dlv"
-		newArgs := append(strings.Split(f.DlvFlag, " "), "exec", appBinary)
-		if len(args) > 0 {
-			newArgs = append(newArgs, "--")
-			args = append(newArgs, args...)
-		}
-		args = newArgs
-	}
+	var command string
+	if strings.TrimSpace(f.DlvFlag) == "" {
+		command = appBinary
+	} else {
+		// Ensure dlv is installed
+		if _, lookErr := exec.LookPath("dlv"); lookErr != nil {
+			buildOptions.Logger.Fatal("Delve (dlv) not found in PATH: %s", lookErr.Error())
+		}
+		command = "dlv"
+		dlvArgs, splitErr := shlex.Split(f.DlvFlag)
+		if splitErr != nil {
+			buildOptions.Logger.Fatal("Unable to parse dlvflag: %s", splitErr.Error())
+		}
+		newArgs := append(dlvArgs, "exec", appBinary)
+		if len(args) > 0 {
+			newArgs = append(newArgs, "--")
+			newArgs = append(newArgs, args...)
+		}
+		args = newArgs
+	}

Note: Document that users should not include the “exec” subcommand in -dlvflag; it is injected by Wails.

Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c32b76e and bd16850.

📒 Files selected for processing (1)
  • website/src/pages/changelog.mdx (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T13:15:20.339Z
Learnt from: APshenkin
PR: wailsapp/wails#4484
File: v2/internal/frontend/utils/urlValidator.go:25-31
Timestamp: 2025-08-08T13:15:20.339Z
Learning: In Wails v2 (PR wailsapp/wails#4484), for BrowserOpenURL URL validation (v2/internal/frontend/utils/urlValidator.go), maintainers (APshenkin) prefer not to restrict schemes to an http/https allowlist because supported schemes may vary. The allowlist suggestion is declined; continue with the existing denylist approach and generic validation.

Applied to files:

  • website/src/pages/changelog.mdx

Comment on lines +24 to +25
- Add `dlvflag` for golang debug in [PR](https://github.com/wailsapp/wails/pull/4410) by @gongzhxu
- Standardized string conversions to avoid subtle memory/runtime in [PR](https://github.com/wailsapp/wails/pull/4410) by @gongzhxu
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Clarify the second changelog bullet

The sentence currently stops at “memory/runtime”, so readers don’t know what was avoided. Please finish the thought (e.g. “…issues”).

-- Standardized string conversions to avoid subtle memory/runtime in [PR](https://github.com/wailsapp/wails/pull/4410) by @gongzhxu
+- Standardized string conversions to avoid subtle memory/runtime issues in [PR](https://github.com/wailsapp/wails/pull/4410) by @gongzhxu
📝 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.

Suggested change
- Add `dlvflag` for golang debug in [PR](https://github.com/wailsapp/wails/pull/4410) by @gongzhxu
- Standardized string conversions to avoid subtle memory/runtime in [PR](https://github.com/wailsapp/wails/pull/4410) by @gongzhxu
- Add `dlvflag` for golang debug in [PR](https://github.com/wailsapp/wails/pull/4410) by @gongzhxu
- Standardized string conversions to avoid subtle memory/runtime issues in [PR](https://github.com/wailsapp/wails/pull/4410) by @gongzhxu
🤖 Prompt for AI Agents
In website/src/pages/changelog.mdx around lines 24 to 25, the second changelog
bullet ends with “memory/runtime” and is incomplete; edit the sentence to finish
the thought (for example: “Standardized string conversions to avoid subtle
memory/runtime issues in [PR](https://github.com/wailsapp/wails/pull/4410) by
@gongzhxu”), ensuring the phrasing is clear and grammatically complete.

@gongzhxu
Copy link
Author

@gongzhxu - do you still want to merge this in? There's a few comments to address above 🙏

Thanks to @leaanthony for the reply. I just saw this message and have made the improvements. Please help review it again, and I will make timely adjustments if there are any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback More information is required from the requestor Enhancement New feature or request go Pull requests that update Go code size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants