Conversation
gtsiolis
commented
Mar 13, 2026
gtsiolis
commented
| BEFORE | AFTER |
|---|---|
 |
 |
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
📝 WalkthroughWalkthroughThe PR adds web app URL propagation to post-start setup routines, introduces decorative message styling for endpoint information, and adds multi-install detection for the lstk binary with version and running status tracking. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ 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 CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/container/start.go (1)
144-149: Keep the bullets andTip:label out of the domain event payload.These strings are presentation details, and they already leak into
internal/ui/components/message.go, which now has to parse prefixes to recover styling semantics. A typed output event or structured payload here would let the plain and TUI renderers own the formatting instead.Based on learnings, "Event payloads should be domain facts (phase/status/progress), not pre-rendered UI strings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/container/start.go` around lines 144 - 149, emitPostStartPointers is emitting pre-formatted UI strings via output.EmitInfo (using output.Sink), which leaks presentation into event payloads; change it to emit a structured domain event (e.g., a PostStartPointers event/struct with fields Endpoint, WebAppURL, and Tip or TipKey) instead of the bullet/“Tip:” strings, and update calls to output.EmitInfo (or add a new output.EmitEvent) to send that structured payload so presenters (plain/TUI) can render bullets and the “Tip:” label themselves; update consumers that parse these strings (e.g., message rendering code) to handle the new typed event.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/update/install_method.go`:
- Around line 209-237: The code currently runs exec.Command("npm", "root", "-g")
and treats its output as an install prefix, producing incorrect paths; change
the command to exec.Command("npm", "prefix", "-g") to get the actual global
prefix, then rebuild the candidate paths from that prefix (e.g.
filepath.Join(prefix, "bin", "lstk") for Unix global bin and
filepath.Join(prefix, "lstk.exe") for Windows), and for package locations use
filepath.Join(prefix, "lib", "node_modules", "@localstack", "lstk", "bin",
"lstk") (or the Windows/node_modules variant) instead of joining an already
node_modules path into another node_modules; update the path entries currently
appended where prefix is used (the slice additions around where cmd.Output() is
handled and the subsequent fallbackPrefixes logic) accordingly.
- Around line 143-149: Replace the non-portable inode-based duplicate detection:
remove use of inodeKey and the info.Sys().(*syscall.Stat_t) branch in the
function that processes file info and instead compare file identity with
os.SameFile by keeping the previously-encountered os.FileInfo (or its
os.FileInfo result) and calling os.SameFile(prevInfo, info) to skip duplicates;
update references to seenInodes to store os.FileInfo (or a map of
string->os.FileInfo) keyed by resolved path. Also update collectCandidatePaths
to include Windows executables by probing both "lstk" and "lstk.exe" (and
generally append ".exe" on Windows for candidate names) so Windows installs
aren't missed. Ensure changes touch the symbols seenInodes, inodeKey,
collectCandidatePaths, and any variable named resolved or info.
---
Nitpick comments:
In `@internal/container/start.go`:
- Around line 144-149: emitPostStartPointers is emitting pre-formatted UI
strings via output.EmitInfo (using output.Sink), which leaks presentation into
event payloads; change it to emit a structured domain event (e.g., a
PostStartPointers event/struct with fields Endpoint, WebAppURL, and Tip or
TipKey) instead of the bullet/“Tip:” strings, and update calls to
output.EmitInfo (or add a new output.EmitEvent) to send that structured payload
so presenters (plain/TUI) can render bullets and the “Tip:” label themselves;
update consumers that parse these strings (e.g., message rendering code) to
handle the new typed event.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 44d5babb-ffe3-4592-8e85-c88ea20b137b
📒 Files selected for processing (5)
internal/container/start.gointernal/container/start_test.gointernal/ui/components/message.gointernal/ui/components/message_test.gointernal/update/install_method.go
| if stat, ok := info.Sys().(*syscall.Stat_t); ok { | ||
| key := inodeKey{dev: uint64(stat.Dev), ino: uint64(stat.Ino)} | ||
| if _, ok := seenInodes[key]; ok { | ||
| continue | ||
| } | ||
| seenInodes[key] = resolved | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Go define syscall.Stat_ton Windows, and isos.SameFilethe portable standard-library way to tell whether twoos.Stat results refer to the same file?
💡 Result:
-
syscall.Stat_ton Windows: No.syscall.Stat_tis a Unix/POSIX-stylestatstruct; in Go’s standard library it is not defined forGOOS=windows(the WindowssyscallAPI surface instead exposes Windows-specific structs such assyscall.Win32FileAttributeData). (pkg.go.dev) -
Portable “same file” check: Yes—
os.SameFile(fi1, fi2)is the standard-library, portable way to ask whether twoos.Statresults refer to the same file. Note its documented limitations: it only applies to results returned byos.Stat(otherwise it returns false), and on non-Unix systems the decision “may be based on the path names.” (pkg.go.dev)
Citations:
Replace inode-based duplicate detection with os.SameFile for Windows portability.
Line 143 uses syscall.Stat_t, which is not defined on Windows and causes the windows-latest build to fail. The inode-based duplicate detection (lines 143–149) is not portable. Use os.SameFile instead—the standard-library portable way to check whether two os.Stat results refer to the same file.
Also: collectCandidatePaths only probes lstk, so lstk.exe installs on Windows will still be missed after fixing the portability issue.
🧰 Tools
🪛 GitHub Check: Integration Tests (windows-latest)
[failure] 143-143:
undefined: syscall.Stat_t
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/update/install_method.go` around lines 143 - 149, Replace the
non-portable inode-based duplicate detection: remove use of inodeKey and the
info.Sys().(*syscall.Stat_t) branch in the function that processes file info and
instead compare file identity with os.SameFile by keeping the
previously-encountered os.FileInfo (or its os.FileInfo result) and calling
os.SameFile(prevInfo, info) to skip duplicates; update references to seenInodes
to store os.FileInfo (or a map of string->os.FileInfo) keyed by resolved path.
Also update collectCandidatePaths to include Windows executables by probing both
"lstk" and "lstk.exe" (and generally append ".exe" on Windows for candidate
names) so Windows installs aren't missed. Ensure changes touch the symbols
seenInodes, inodeKey, collectCandidatePaths, and any variable named resolved or
info.
| // Try to get npm prefix via command | ||
| cmd := exec.Command("npm", "root", "-g") | ||
| out, err := cmd.Output() | ||
| if err == nil { | ||
| prefix := strings.TrimSpace(string(out)) | ||
| paths = append(paths, filepath.Join(prefix, ".bin", "lstk")) | ||
| paths = append(paths, filepath.Join(prefix, "node_modules", "@localstack", "lstk", "bin", "lstk")) | ||
| paths = append(paths, filepath.Join(prefix, "node_modules", "@localstack", "lstk_darwin_arm64", "lstk")) | ||
| paths = append(paths, filepath.Join(prefix, "node_modules", "@localstack", "lstk_darwin_amd64", "lstk")) | ||
| paths = append(paths, filepath.Join(prefix, "node_modules", "@localstack", "lstk_linux_amd64", "lstk")) | ||
| paths = append(paths, filepath.Join(prefix, "node_modules", "@localstack", "lstk_windows_amd64", "lstk.exe")) | ||
| } | ||
|
|
||
| // Fallback to common npm global prefixes | ||
| fallbackPrefixes := []string{ | ||
| filepath.Join(os.Getenv("HOME"), ".local", "share", "mise", "installs"), | ||
| filepath.Join(os.Getenv("HOME"), ".nvm", "versions"), | ||
| "/usr/local", | ||
| "/opt/homebrew", | ||
| } | ||
| for _, prefix := range fallbackPrefixes { | ||
| if prefix == "" { | ||
| continue | ||
| } | ||
| paths = append(paths, | ||
| filepath.Join(prefix, "lib", "node_modules", "@localstack", "lstk", "bin", "lstk"), | ||
| filepath.Join(prefix, "lib", "node_modules", "@localstack", "lstk", "lstk"), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What does npm root -greturn compared withnpm prefix -gornpm config get prefix, and which command should be used to locate the global bin directory versus the global package directory?
💡 Result:
npm root -gprints the globalnode_modulesdirectory (i.e., where global packages are installed). In global mode, that’s typically{prefix}/lib/node_moduleson Unix and{prefix}/node_moduleson Windows. [1][3]npm prefix -gprints the global prefix (the base install directory that global installs are rooted under), e.g./usr/local(Unix) or%AppData%\npm(Windows). [2][3]npm config get prefixprints the configured value of theprefixsetting (the same value thatnpm prefix -greports when you’re asking for the global prefix). [2][4]
Which to use
- To locate the global package directory: use
npm root -g. [1] - To locate the global bin directory (where global executables are linked/installed): use
npm bin -g(equivalently, it’s under the prefix:{prefix}/binon Unix,{prefix}on Windows). [5][3]
Sources
[1] npm-root docs (npm Docs) (docs.npmjs.com)
[2] npm-prefix docs (npm Docs) (docs.npmjs.com)
[3] “Folders” / prefix & global install layout (npm Docs) (docs.npmjs.com)
[4] npm config files and $PREFIX usage (npm Docs) (docs.npmjs.com)
[5] npm-bin docs (npm Docs) (docs.npmjs.com)
Citations:
- 1: https://docs.npmjs.com/cli/v8/commands/npm-root
- 2: https://docs.npmjs.com/cli/v8/commands/npm-prefix
- 3: https://docs.npmjs.com/cli/v11/configuring-npm/folders
- 4: https://docs.npmjs.com/cli/v8/using-npm/config
- 5: https://docs.npmjs.com/cli/v7/commands/npm-bin/
Use npm prefix -g instead of npm root -g to get the actual install prefix.
npm root -g returns the global node_modules directory (e.g., /usr/local/lib/node_modules on Unix), not the install prefix. The current code treats it as a prefix, which creates invalid paths like {node_modules}/node_modules/@localstack/lstk for lines 215–219. Use npm prefix -g to get the actual prefix, then construct paths as:
- Global bin:
{prefix}/bin/lstk(Unix) or{prefix}/lstk.exe(Windows) - Global packages:
{prefix}/lib/node_modules/@localstack/lstk/bin/lstk(Unix) or{prefix}/node_modules/@localstack/lstk/bin/lstk(Windows)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/update/install_method.go` around lines 209 - 237, The code currently
runs exec.Command("npm", "root", "-g") and treats its output as an install
prefix, producing incorrect paths; change the command to exec.Command("npm",
"prefix", "-g") to get the actual global prefix, then rebuild the candidate
paths from that prefix (e.g. filepath.Join(prefix, "bin", "lstk") for Unix
global bin and filepath.Join(prefix, "lstk.exe") for Windows), and for package
locations use filepath.Join(prefix, "lib", "node_modules", "@localstack",
"lstk", "bin", "lstk") (or the Windows/node_modules variant) instead of joining
an already node_modules path into another node_modules; update the path entries
currently appended where prefix is used (the slice additions around where
cmd.Output() is handled and the subsequent fallbackPrefixes logic) accordingly.
1 participant
