Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the bridge file handling system to improve safety and maintainability. The key improvements include replacing printf-style string formatting with Go's text/template package for shell scripts, consolidating duplicate file setup logic into reusable Lua scripts, and changing from concatenated Lua scripts with global variables to individual scripts that receive arguments directly via ExecLua.
Changes:
- Replaced printf-based template interpolation with Go's text/template package for safer script generation
- Consolidated file setup logic into setup_remote_file.lua and setup_remote_file_on_init.lua
- Refactored bridge initialization to use batch execution with explicit argument passing instead of concatenated scripts with global variables
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/client/main.go | Refactored prepareRemoteNvim to use batch execution and explicit argument passing; added pathWithBatExtension helper; removed getBrowserScript and getEditorScript functions |
| src/bridge_files/main.go | Added ReadFileWithTemplate function to handle Go template rendering |
| src/bridge_files/shell/nvrh-editor.bat | Converted from printf format to Go template syntax; changed %% to % for batch variables |
| src/bridge_files/shell/nvrh-editor | Converted from printf format to Go template syntax; removed printf escape comments |
| src/bridge_files/shell/nvrh-browser.bat | Converted from printf format to Go template syntax; changed %% to % for batch variables |
| src/bridge_files/shell/nvrh-browser | Converted from printf format to Go template syntax; removed printf escape comments |
| src/bridge_files/lua/types/nvrh.lua | Replaced global variable declarations with _G._nvrh_is_initialized flag |
| src/bridge_files/lua/setup_remote_file_on_init.lua | New consolidated script for creating remote files during initialization |
| src/bridge_files/lua/setup_remote_file.lua | New consolidated script for creating remote files |
| src/bridge_files/lua/setup_port_scanner.lua | Changed to receive should_map_ports as argument; updated initialization guard |
| src/bridge_files/lua/setup_nvim_launcher.lua | Removed (functionality replaced by setup_remote_file.lua) |
| src/bridge_files/lua/setup_editor_script.lua | Removed (functionality replaced by setup_remote_file_on_init.lua) |
| src/bridge_files/lua/setup_browser_script.lua | Removed (functionality replaced by setup_remote_file_on_init.lua) |
| src/bridge_files/lua/session_automap_ports.lua | Changed to receive channel_id as argument |
| src/bridge_files/lua/rpc_tunnel_port.lua | Updated initialization guard to use _G._nvrh_is_initialized |
| src/bridge_files/lua/rpc_open_url.lua | Changed to receive browser_script_path as argument; updated initialization guard |
| src/bridge_files/lua/rpc_open_file.lua | Changed to receive editor_script_path as argument; updated initialization guard |
| src/bridge_files/lua/init_nvrh.lua | Changed to receive all parameters as arguments; updated initialization guard |
| src/bridge_files/lua/init_bridge.lua | Removed (initialization logic moved to individual scripts) |
| src/bridge_files/lua/finalize_bridge.lua | New script to set _G._nvrh_is_initialized flag after bridge setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set "LOCK_FILE=%TEMP%\nvrh-editor-%RANDOM%.lock" | ||
| set "LOCK_FILE=%LOCK_FILE:\=\\%" |
There was a problem hiding this comment.
While the conversion from double percent signs to single percent signs is correct on this line, note that line 30 (outside the diff) still uses double percent signs (%%LOCK_FILE%%), which will cause a runtime error. When this file was processed by fmt.Sprintf, double percent signs were converted to single ones, but text/template doesn't do this conversion. Line 30 should be changed to use single percent signs (%LOCK_FILE%) to match Windows batch file syntax.
src/client/main.go
Outdated
|
|
||
| batch.ExecLua(bridge_files.ReadFileWithoutError("lua/rpc_tunnel_port.lua"), nil) | ||
| batch.ExecLua(bridge_files.ReadFileWithoutError("lua/setup_port_scanner.lua"), nil, nvrhContext.AutomapPorts) | ||
| batch.ExecLua(bridge_files.ReadFileWithoutError("lua/session_automap_ports.lua"), nil) |
There was a problem hiding this comment.
Missing required argument for session_automap_ports.lua. The Lua script expects a channel_id argument (line 1 of session_automap_ports.lua: local channel_id = ...), but no argument is being passed here. This should pass nv.ChannelID() as an argument, similar to how it was used in the original code.
| batch.ExecLua(bridge_files.ReadFileWithoutError("lua/session_automap_ports.lua"), nil) | |
| batch.ExecLua(bridge_files.ReadFileWithoutError("lua/session_automap_ports.lua"), nil, nv.ChannelID()) |
| func ReadFileWithTemplate(filename string, data any) string { | ||
| fileContent := ReadFileWithoutError(filename) | ||
|
|
||
| tmpl, err := template.New("file").Parse(fileContent) | ||
| if err != nil { | ||
| slog.Error("Error parsing template", "filename", filename, "err", err) | ||
| return "" | ||
| } | ||
|
|
||
| var renderedContentBytes bytes.Buffer | ||
| err = tmpl.Execute(&renderedContentBytes, data) | ||
| if err != nil { | ||
| slog.Error("Error executing template", "filename", filename, "err", err) | ||
| return "" | ||
| } | ||
|
|
||
| return renderedContentBytes.String() | ||
| } |
There was a problem hiding this comment.
ReadFileWithTemplate returns an empty string on error, which could lead to silent failures where invalid or empty scripts are written to disk. Consider either panicking on template errors (since they indicate programming errors) or returning an error that the caller must handle. This would make debugging template issues easier and prevent runtime failures from malformed scripts.
🤖 I have created a release *beep* *boop* --- ## [0.7.0](v0.6.0...v0.7.0) (2026-02-01) ### Features * Custom open file handler ([#74](#74)) ([4649eb0](4649eb0)) ### Bug Fixes * Document configuration file ([#73](#73)) ([53bca4e](53bca4e)) * Refactor bridge files ([#76](#76)) ([91dba24](91dba24)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
There was a few things being repeated when it came to bridge files:
A lot of the
setup_*lua scripts were really just "make sure this remote file exists, potentially with these permissions" and has been replaced withsetup_remote_file/setup_remote_file_on_init.Using printf for templates was really more annoying when it came to
.batfiles, so printf formatting has been replace withtext/templateand the functionReadFileWithTemplate.Also, I wanted to stop doing interpolation when creating a remote file.
ExecLualets you pass args directly, which feels safer. Unfortunately, all the bridge scripts were written under the assumption that they would be concated together. So I had to refactor those as well.should_initializebecame_G._nvrh_is_initialized ~= true, and a lot of arguments had to be moved around and double checked.