feat(tui): mark restart-required settings and add confirmation modal by SuperCoolPencil · Pull Request #417 · SurgeDM/Surge

SuperCoolPencil · 2026-04-26T08:26:15Z

Greptile Summary

This PR adds UI markers for restart-required settings and a confirmation modal that gates a clean process re-exec behind an explicit user choice. The cross-platform restart implementation is solid — syscall.Exec on Unix and a detached child-plus-natural-exit on Windows, with the lock explicitly released before spawning — and the reflection-based change detection against a snapshotted baseline is a clean approach. Remaining findings are all P2.

Confidence Score: 5/5

Safe to merge; all remaining findings are style/comment P2s with no impact on runtime correctness

No P0 or P1 issues found. The cross-platform restart logic is correct, the ANSI-in-WrapText and other substantive issues were addressed in previous review rounds, and the new test coverage is adequate for the happy path.

internal/utils/run_windows.go (misleading comment), internal/utils/open.go (trivial docstrings)

Important Files Changed

Filename	Overview
cmd/root.go	Wires restart signal from TUI model into performRestart(); uses build-constrained utils.Run cleanly
internal/utils/run_windows.go	New Windows restart implementation; spawns child and returns nil — correct, but contains a misleading comment about os.Exit and parent processes
internal/utils/run_unix.go	New Unix restart via syscall.Exec; straightforward and correct
internal/utils/open.go	Adds Run() dispatcher and trivial 'what' docstrings that restate function names
internal/config/settings.go	Adds ui_restart struct tag and RequiresRestart field to SettingMeta; reflection-based metadata population looks correct
internal/tui/helpers.go	snapshotSettings/checkRestartRequirement use pointer receivers correctly; reflection traversal of ui_restart tags is sound
internal/tui/model.go	Adds RestartConfirmState, SettingsBaseline, and RestartRequested; iota comments are now correct; RestartRequested carries a stale [NEW] annotation
internal/tui/update_modals.go	updateRestartConfirm correctly sets RestartRequested=true before calling shutdownCmd; cancelRestart clears SettingsBaseline
internal/tui/update_settings.go	Adds defensive snapshot on entry and restart-check on close; settings persist before confirmation modal which is intentional
internal/tui/view.go	viewRestartConfirm renders the confirmation modal; reuses quitConfirmFocused for button state
internal/tui/view_settings.go	Prepends ANSI-styled restartNotice to plain desc before WrapText — known ANSI-width corruption issue for narrow panes (flagged previously)
internal/tui/settings_restart_test.go	Tests cover snapshot, detection, revert, defensive snapshot, and baseline cleanup; happy paths only
internal/tui/update_dashboard.go	Adds snapshotSettings() call when entering settings from dashboard; straightforward
internal/tui/update.go	Routes RestartConfirmState to updateRestartConfirm; no issues

Prompt To Fix All With AI

This is a comment left during a code review.
Path: internal/utils/run_windows.go
Line: 19-22

Comment:
**Misleading comment about `os.Exit` and the parent process**

`os.Exit(0)` terminates the *current* process, not the parent. The real reason to avoid calling it here is that it would bypass any deferred functions registered in `cmd/root.go` or the Cobra command chain. The current behavior (returning `nil` and letting `main()` exit naturally) is correct, but the comment misdirects anyone who reads it later.

```suggestion
	// Do NOT call os.Exit(0) here.
	// Returning nil lets the normal Cobra/main() teardown path run,
	// executing any deferred cleanup before the process exits.
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: internal/utils/open.go
Line: 9

Comment:
**"What" docstrings that restate function names**

The newly added comments for `OpenFile`, `OpenContainingFolder`, `openWithSystem`, and `buildOpenCommand` each restate exactly what the function name already conveys. Per the project's comment style, comments should explain *why* code exists rather than repeat the name in prose form. For public functions that truly need a doc comment, a single sentence describing a non-obvious constraint (e.g. why it uses `Start` instead of `Run`) is more valuable than a paraphrase of the signature.

This pattern appears on lines 9, 27, 53, and 65.

**Rule Used:** What: Comments must explain *why* code exists, not... ([source](https://app.greptile.com/review/custom-context?memory=4e45ef13-32c2-4753-8060-a44ef767144d))

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: internal/tui/model.go
Line: 206

Comment:
**Stale `[NEW]` annotation**

The `[NEW]` marker will be meaningless the moment this PR is merged, and adds noise without explaining *why* the field exists or its invariants (e.g. "set only inside the TUI shutdown path, read by `cmd/root.go` to trigger `performRestart`").

```suggestion
	RestartRequested bool // Set by updateRestartConfirm; signals cmd/root.go to exec a fresh process after TUI shutdown
```

**Rule Used:** What: Comments must explain *why* code exists, not... ([source](https://app.greptile.com/review/custom-context?memory=4e45ef13-32c2-4753-8060-a44ef767144d))

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (5): Last reviewed commit: "feat: implement platform-specific proces..." | Re-trigger Greptile}

Context used:

Rule used - What: Comments must explain why code exists, not... (source)

github-actions · 2026-04-26T08:26:54Z

Binary Size Analysis

⚠️ Size Increased

Version	Human Readable	Raw Bytes
Main	19.38 MB	`20316452`
PR	19.54 MB	`20488484`
Difference	168.00 KB	`172032`

… snapshotting

greptile-apps · 2026-04-27T09:03:09Z

+package main
+import "syscall"
+func main() {
+	syscall.Exec("", nil, nil)
+}


Scratch file breaks Windows build

Go compiles files matching *_windows.go exclusively on Windows. This file will therefore be compiled only on Windows, but it calls syscall.Exec — a POSIX-only function that does not exist in Go's syscall package on Windows. Any go build ./... on a Windows runner will fail with syscall.Exec undefined. This looks like a leftover from exploratory testing and should be deleted before merging.

Prompt To Fix With AI

This is a comment left during a code review. Path: scratch/test_windows.go Line: 1-5 Comment: **Scratch file breaks Windows build** Go compiles files matching `*_windows.go` exclusively on Windows. This file will therefore be compiled only on Windows, but it calls `syscall.Exec` — a POSIX-only function that does not exist in Go's `syscall` package on Windows. Any `go build ./...` on a Windows runner will fail with `syscall.Exec undefined`. This looks like a leftover from exploratory testing and should be deleted before merging. How can I resolve this? If you propose a fix, please make it concise.

… Unix compatibility

SuperCoolPencil force-pushed the require-restart branch from 1335fa1 to 75073ad Compare April 26, 2026 08:28

greptile-apps Bot reviewed Apr 26, 2026

View reviewed changes

Comment thread internal/tui/update_modals.go

Comment thread internal/tui/model.go Outdated

Comment thread internal/tui/helpers.go

Comment thread internal/tui/update_settings.go

SuperCoolPencil force-pushed the require-restart branch from 75073ad to 577a1f0 Compare April 26, 2026 08:36

feat(tui): mark restart-required settings and add confirmation modal

93bced4

SuperCoolPencil force-pushed the require-restart branch from 577a1f0 to 93bced4 Compare April 26, 2026 08:39

test(tui): add regression tests for restart requirement and defensive…

5ed4066

… snapshotting

greptile-apps Bot reviewed Apr 26, 2026

View reviewed changes

Comment thread internal/tui/view_settings.go

chore: go fmt

bb6b630

greptile-apps Bot reviewed Apr 26, 2026

View reviewed changes

Comment thread cmd/root.go

greptile-apps Bot reviewed Apr 27, 2026

View reviewed changes

feat: implement platform-specific process replacement for Windows and…

036775c

… Unix compatibility

SuperCoolPencil force-pushed the require-restart branch from 09c7cdb to 036775c Compare April 27, 2026 09:16

SuperCoolPencil merged commit 44074cf into main Apr 27, 2026
16 checks passed

SuperCoolPencil deleted the require-restart branch April 27, 2026 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(tui): mark restart-required settings and add confirmation modal#417

feat(tui): mark restart-required settings and add confirmation modal#417
SuperCoolPencil merged 4 commits intomainfrom
require-restart

SuperCoolPencil commented Apr 26, 2026 •

edited by greptile-apps Bot

Loading

Uh oh!

github-actions Bot commented Apr 26, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps Bot Apr 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

SuperCoolPencil commented Apr 26, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

github-actions Bot commented Apr 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Analysis

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps Bot Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

SuperCoolPencil commented Apr 26, 2026 •

edited by greptile-apps Bot

Loading

github-actions Bot commented Apr 26, 2026 •

edited

Loading