jim800121chen
8cd5751ce3
依 R5 五輪決策把 visionA-local 從「Wails 內嵌 Next.js」重構為「Wails
本機伺服器控制台 + 瀏覽器 Web UI」模式（類比 Docker Desktop / Ollama）。
程式碼變動
- M8-1 砍 yt-dlp 全套（後端 resolver / URL handler / 前端 URL tab /
Makefile vendor / installer / bootstrap / CI workflow，-555 行）
- M8-2 砍 Mock 模式全套（driver/mock、mock_camera、Settings runtimeMode、
VISIONA_MOCK 環境變數，-528 行）
- M8-3 ffmpeg 從 GPL 切換到 LGPL 混合方案：Windows/Linux 用 BtbN 現成
LGPL binary，macOS 自 build minimal decoder-only 進 git
（vendor/ffmpeg/macos/ffmpeg 5.7MB + ffprobe 5.6MB，比 GPL 版省 85% 空間）
- M8-4 Wails Server Controller：state machine、log ring buffer 2000 行、
preferences.json atomic write、boot-id、Gin SkipPaths、shutdown 7+1 秒、
notify_*.go 三平台 OS 通知、watchServer 改 Error state 不 os.Exit
- M8-4b 啟動階段管線 R5-E：6 階段進度 event、20s soft / 60s hard timeout、
stage 5/6 skip 規則、sentinel file、RestartStartupSequence 5 步驟
- M8-5 Wails 控制台 vanilla HTML/JS/CSS（9 檔 ~2012 行）取代 M7-B splash：
state 視覺、log panel、startup progress panel、Stage 6 manual CTA
pulse、shutdown modal、Settings、Dark Mode、i18n 中英雙語
- M8-6 上傳影片副檔名擴充（mp4/avi/mov/mpeg/mpg）
- M8-7 Web UI Server Offline Overlay（role=alertdialog + focus trap +
wsEverConnected 容錯 + Page Visibility）
- M8-8 CORS middleware（127.0.0.1/localhost only + suffix attack 防護）+
ws/origin.go 獨立 WebSocket CheckOrigin 避 package cycle
- MAJ-4 server:shutdown-imminent WebSocket broadcast 機制
（/ws/system endpoint + notifyShutdownImminent helper）
- M8-9 Boot-ID + 瀏覽器 tab 自動重連（sessionStorage loop guard）
品質
- ~105+ 新 unit test + race detector (-count=2) 全綠
- 10 個 milestone 全部通過 Reviewer 審查
- 三方 v2 + v2.1 文件（PRD / Design Spec / TDD）+ 交叉互審紀錄
收錄在 .autoflow/
交付前待處理（M8-10）
- 重跑 make payload-macos 把舊 GPL 77MB binary 換成新 LGPL
- 三平台 end-to-end build 驗證
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
230 lines
15 KiB
Markdown
230 lines
15 KiB
Markdown
# Reviewer 審查 M8-4b 補丁（2026-04-15）
|
||
|
||
## 摘要
|
||
|
||
- 審查對象：M8-4b 補丁（3 Major + Stage 4 timeout 修復）
|
||
- 審查檔案：`visiona-local/startup_pipeline.go`（L385-423 新增 helpers）、`visiona-local/server_control.go`（L167-223 startInternal 守門、L630-652 probe timeout、L867-937 RestartStartupSequence + test hook）、`visiona-local/app.go`（L109-119 struct、L1676-1693 openBrowser var）、`visiona-local/startup_pipeline_test.go`（L393-660 4 個 regression test）
|
||
- 總結論：**✅ 通過** — 3 Major 全部正確修復、Stage 4 timeout 改好、4 個 regression test 完整覆蓋且全綠。Build/Vet/Test/Race（count=2）在 `visiona-local` 與 `server` 兩個 module 都 PASS。
|
||
- 3 Major 修復狀況：M-1 ✅ / M-2 ✅ / M-3 ✅
|
||
- 問題統計：Critical 0 / Major 0 / Minor 2 / Suggestion 1
|
||
- 阻擋 M8-10 嗎：**否**，可推進 M8-10。
|
||
|
||
---
|
||
|
||
## A. HasFailedStage helper 正確性
|
||
|
||
位置：`visiona-local/startup_pipeline.go` L385-404
|
||
|
||
- 邏輯正確 ✅：`current == -1`（`FailStage` 會把 current 設為 -1）或迴圈檢查 `stages[1..6].status == "failed"`（defensive path）。
|
||
- Lock 保護 ✅：`p.mu.Lock()` + defer `Unlock()`。`mu` 在 struct 是 `sync.Mutex`（L98），沒 RLock 可用 — 任務敘述提到的「RLock」不適用，用 `Lock` 是正確選擇。效能影響可忽略（只在 startInternal error 路徑讀一次）。
|
||
- 文件註解清楚 ✅：L386-391 解釋語義與對 M-1 的作用。
|
||
- Test 覆蓋 4 case ✅：`TestStartupPipeline_HasFailedStage`（L537-566）分別驗證：new / running / FailStage 後（current=-1）/ 某 stage status=failed 但 current 未同步（defensive）。
|
||
|
||
---
|
||
|
||
## B. IsInColdStart helper 正確性
|
||
|
||
位置：`visiona-local/startup_pipeline.go` L406-423
|
||
|
||
- 邏輯正確 ✅：`current >= 1 && current <= startupTotalStages`（startupTotalStages=6），stage 7（ready）與 current=-1（failed）、current=0（未 Start）都回 false。
|
||
- Lock 保護 ✅：同 `HasFailedStage`，用 `Lock/Unlock`。
|
||
- Test 覆蓋邊界 ✅：`TestStartupPipeline_IsInColdStart`（L578-606）覆蓋 current ∈ {0, 1..6, 7, -1}，迴圈確認 1..6 全 true，其餘全 false。
|
||
|
||
---
|
||
|
||
## C. startInternal error 分支守門
|
||
|
||
位置：`visiona-local/server_control.go` L167-194
|
||
|
||
- 正確用 `pipeline.HasFailedStage()` ✅（L176-179）。
|
||
- `pipeline == nil` fallback ✅：`pipelineHandled` 預設 false → 走原 setState + emit + sendCrashNotification 邏輯。
|
||
- 非冷啟動路徑（RestartServer 等，pipeline 已 ready current=7，若此刻 Start 失敗 HasFailedStage=false）fallback 正常發通知 ✅。
|
||
- 註解（L168-175）解釋完整：說明為什麼守門 + 哪些路徑仍需 fallback。
|
||
- 不破壞 RestartServer（走 `ctrl.Restart()` → `ctrl.Stop()` + `ctrl.Start()`）的錯誤處理 ✅：RestartServer 發生時 pipeline.current==7 或 0，`HasFailedStage()` 回 false。
|
||
|
||
**小提醒**（非 Major）：此守門建立在「pipeline FailStage 一定早於 startInternal return err」的假設上。實作上 `startServerV2` 每個 stage 失敗都立即 `FailStage`（見 server_control.go L437-448 / L554-566），再 return err，順序正確。若未來新增一條「不透過 FailStage 的錯誤返回路徑」，守門會被繞過。建議記為 Minor 追蹤（見 §L）。
|
||
|
||
---
|
||
|
||
## D. openBrowser 守門
|
||
|
||
位置：`visiona-local/server_control.go` L202-222
|
||
|
||
- `pipeline.IsInColdStart() == false` 才執行 R5-D3 openBrowser ✅（L213-216）。
|
||
- RestartServer 路徑（pipeline.current==7，`IsInColdStart()` 回 false）仍 openBrowser ✅。
|
||
- `pipeline == nil` 路徑 `inColdStart` 維持 false，fallback 仍 openBrowser ✅（L214-216）。
|
||
- 註解（L204-212）清楚列出三種場景（冷啟動 / RestartServer / RestartStartupSequence）的預期行為，可維護性高。
|
||
|
||
---
|
||
|
||
## E. openBrowser package-level var 改動
|
||
|
||
位置：`visiona-local/app.go` L1676-1693
|
||
|
||
- 改為 `var openBrowser = openBrowserExec` ✅（L1681）。預設值是實作函式，production 透過 var 呼叫只多一個 indirection，效能影響可忽略。
|
||
- 所有呼叫端透過 var 呼叫：`app.go` L268（runStartupStage5）、`server_control.go` L219（startInternal R5-D3）。Test 可替換 var 做 stub。
|
||
- `openBrowserExec` 保留為獨立函式（L1684-1693）作為預設實作，macOS/Windows/Linux 三分支原封不動 ✅。
|
||
- Test 重置良好 ✅：`TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce` 用 `defer func() { openBrowser = original }()`（L627）保證 test 結束還原。
|
||
|
||
---
|
||
|
||
## F. RestartStartupSequence test 重構
|
||
|
||
位置：`visiona-local/startup_pipeline_test.go` L393-522
|
||
|
||
- **直接呼叫 `a.RestartStartupSequence()`** ✅（L439）— 不是手動複製 5 步驟。
|
||
- 用 `restartStartFn` test hook 攔截 Step 6 ✅（L424-433），spy 記錄呼叫、檢查 sentinel 是否已清、snapshot 新 pipeline。
|
||
- `callLog` 驗證順序 ✅（L497-516）：`pipelineCancel` 必須在 `startFn` 之前。
|
||
- 驗證新 pipeline 建立 ✅（L466-486）：stage1=completed / stage2=running / current=2。
|
||
- `pipelineCancelFn` 的 `context.CancelFunc` 型別允許 assign 任意 `func()` ✅（L407-410）。
|
||
- 其他 side effect 驗證完整：proc 被清為 nil（L453-458 ForceKill 效果）、sentinel file 已刪除（L461-463）、啟動前 ctrl state 設為 Error 模擬 Retry 前置條件（L418）。
|
||
- 「若未來把 Step 4 移到 Step 2 之前」的概念驗證：test 雖無法直接抓 Step 4 ↔ Step 2 相對順序，但能抓「Step 4 在 Step 6 之前」（`sentinelClearedBeforeStart` L489-491），滿足原 Reviewer 對 M-3 的核心期待（驗證真的呼叫 method、不是手動複製）。
|
||
|
||
**Minor F-1**（見 §L）：`newPipelineTestApp` 的 `a.ctx == nil`，所以 RestartStartupSequence Step 5 的 watcher goroutine 分支不會啟動，L518-521 的「清理新啟動的 watcher」註解有點誤導（實際上 pipelineCancelFn 已在 Step 1 被清為 nil，之後也沒有被重設）。無功能影響，純註解細節。
|
||
|
||
---
|
||
|
||
## G. TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce
|
||
|
||
位置：`visiona-local/startup_pipeline_test.go` L615-660
|
||
|
||
- 正確 stub `openBrowser` var ✅（L621-627），defer 還原。
|
||
- `AutoOpenBrowser=true`（L618）確保 runStartupStage5 會走 openBrowser 分支（不 skip）。
|
||
- `restartStartFn` 內模擬 server 啟動成功：設 fake proc port=12345、setState(Running)、手動推進 pipeline stages 到 stage 5 running ✅（L630-645）。這讓 `runStartupStage5` 能從 `ctrl.proc.port` 取到 URL 並呼叫 openBrowser。
|
||
- 斷言 `openCount == 1` ✅（L652-654），錯誤訊息清楚標示 want 1。
|
||
- 驗證冷啟動路徑下「startInternal 的 R5-D3 open」不會發生 — 因為 restartStartFn 替換了 `ctrl.Start()`，完全 bypass startInternal，所以實際上這個 test 驗證的是 runStartupStage5 本身只會 open 一次。
|
||
|
||
**Minor G-1**（見 §L）：此 test 沒有實際經過 startInternal 的 R5-D3 分支（restartStartFn 完全替換 ctrl.Start），所以嚴格來說它驗證的是「runStartupStage5 本身的 open 次數」，而不是「冷啟動中 startInternal + runStartupStage5 的 open 次數總和」。要完整驗證 M-2，需要一個能跑 startInternal 的整合 test（但 startInternal 會 spawn python server 無法 unit test）。現況可接受，因為 IsInColdStart 邏輯已被 B 節 `TestStartupPipeline_IsInColdStart` 單獨驗證，startInternal 的守門條件 `!inColdStart` 是純布林邏輯，靜態即可推得正確。
|
||
|
||
---
|
||
|
||
## H. Stage 4 probe timeout 2s
|
||
|
||
位置：`visiona-local/server_control.go` L630-652
|
||
|
||
- 註解說明原因 ✅：L634-636 解釋 TDD §3「秒回即算完成」+「2s 足以涵蓋正常 latency」。
|
||
- 不破壞既有 test ✅：Stage 4 相關 test 全綠（`go test -count=1` PASS）。
|
||
- 逾時仍算完成 ✅（L647-651 不論 err 或 status 都 CompleteStage(4)），符合 TDD §3 設計意圖。
|
||
|
||
---
|
||
|
||
## I. Build/Test/Race 結果
|
||
|
||
| 指令 | 結果 |
|
||
|------|------|
|
||
| `cd visiona-local && go build .` | PASS（無輸出） |
|
||
| `cd visiona-local && go vet ./...` | PASS（無輸出） |
|
||
| `cd visiona-local && go test -count=1 ./...` | PASS（8.210s） |
|
||
| `cd visiona-local && go test -race -count=2 ./...` | **PASS（16.225s）** |
|
||
| `cd server && go build ./...` | PASS |
|
||
| `cd server && go vet ./...` | PASS |
|
||
| `cd server && go test -count=1 ./...` | PASS（api/handlers/ws/device/model 全通過） |
|
||
| `cd server && go test -race ./...` | PASS |
|
||
|
||
4 個新 test 單獨跑 verbose：
|
||
```
|
||
--- PASS: TestStartupPipeline_RestartStartupSequence_StepsExecution (0.00s)
|
||
--- PASS: TestStartupPipeline_HasFailedStage (0.00s)
|
||
--- PASS: TestStartupPipeline_IsInColdStart (0.00s)
|
||
--- PASS: TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce (0.00s)
|
||
```
|
||
|
||
---
|
||
|
||
## J. 合併檢查
|
||
|
||
`git diff --stat` 看過全部變更，無衝突標記、無重複區塊。合併狀況：
|
||
|
||
- **M8-4 原版** + **M8-4 補丁**（Stop/ForceKill/handleWatchFailure/logPump）：`ForceKill` L273-291 的 cancelWatcher 順序修復仍在，`RestartStartupSequence` Step 2 正確利用 ✅。
|
||
- **M8-4b 原版**（startServerV2 hook + probe + RestartStartupSequence）：補丁新增 helpers 與守門邏輯未動到既有 pipeline struct 欄位與 stage hook 位置 ✅。
|
||
- **MAJ-4 補丁**（shutdown-imminent broadcast + notifyShutdownImminent）：不觸及 startup pipeline / server_control 的 startInternal / RestartStartupSequence 區塊 ✅。
|
||
- `grep` 未見 `<<<<<<<` / `>>>>>>>` 合併標記。
|
||
|
||
---
|
||
|
||
## K. 3 Major 症狀驗證
|
||
|
||
### M-1（重複 OS 通知）
|
||
|
||
症狀：冷啟動 stage 3 失敗 → `emitError` 發通知 A → startInternal error 分支再發通知 B。
|
||
|
||
修復後的流程：
|
||
1. `startServerV2` 失敗 → `pipeline.FailStage(3, err)` → `emitError` 發通知「第 3 階段失敗」+ setState(Error) + current = -1
|
||
2. startInternal 收 err → `HasFailedStage()` 回 true → `pipelineHandled = true` → **skip** setState/emit/sendCrashNotification
|
||
3. 只剩一個通知 ✅
|
||
|
||
**症狀消失確認**：Major M-1 修復路徑正確，唯一前提是「startServerV2 的所有失敗路徑都走 FailStage」— 已確認 L437-448（stage 2）、L554-566（stage 3）、stage 4 只在 timeout 仍 complete 不 fail，符合條件。
|
||
|
||
### M-2（重複開瀏覽器）
|
||
|
||
症狀：冷啟動成功 → runStartupStage5 的 openBrowser 呼叫 1 次 + startInternal R5-D3 的 openBrowser 呼叫 1 次 = 2 次。
|
||
|
||
修復後的流程：
|
||
1. 冷啟動 → startServerV2 成功 → startInternal 檢查 `IsInColdStart()`（此時 current ∈ 1..6）→ 回 true → **skip** R5-D3 openBrowser
|
||
2. startServerV2 返回後，app.startup 呼叫 `runStartupStage5` → openBrowser 1 次 ✅
|
||
3. RestartServer 路徑：pipeline.current==7 → IsInColdStart 回 false → R5-D3 正常 open ✅
|
||
|
||
**症狀消失確認**：冷啟動 openBrowser 只被呼叫一次。`TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce` 驗證 openCount==1。
|
||
|
||
### M-3（Retry test 重構）
|
||
|
||
症狀：test 手動複製 Step 1-5，改步驟順序抓不到。
|
||
|
||
修復後：直接呼叫 `a.RestartStartupSequence()` + callLog spy。若未來調整步驟順序（cancel 晚於 start），callLog 順序斷言會 fail ✅；若把 sentinel 清除改到 start 之後（Step 4 → Step 6 之後），`sentinelClearedBeforeStart` 斷言會 fail ✅。
|
||
|
||
**概念驗證**：test 能抓到「cancel 必須早於 start」與「sentinel 必須在 start 前清」兩個順序不變量，涵蓋 M-3 的核心意圖。
|
||
|
||
---
|
||
|
||
## L. 問題清單
|
||
|
||
### Critical
|
||
（無）
|
||
|
||
### Major
|
||
（無）
|
||
|
||
### Minor
|
||
|
||
| # | 檔案 | 行 | 問題 | 建議 |
|
||
|---|------|----|------|------|
|
||
| m-1 | `visiona-local/startup_pipeline_test.go` | L518-521 | 註解說「清理新啟動的 watcher goroutine」，但 `newPipelineTestApp` 的 `a.ctx == nil`，Step 5 的 watcher 分支不會啟動，pipelineCancelFn 在 Step 1 已被清為 nil。註解誤導但無功能影響 | 改註解為「若 ctx 存在則清理重建的 watcher（此 test 走 nil ctx 分支，為 safety）」 |
|
||
| m-2 | `visiona-local/server_control.go` | L167-194 | `pipelineHandled` 守門建立在「startServerV2 所有失敗路徑都走 FailStage」的隱性假設上，若未來新增一條不透過 FailStage return err 的路徑，M-1 症狀會復活 | 在 `startServerV2` 函式開頭加註解「所有 err return 前必須先 FailStage」；或改用 explicit flag（如 `a.ctrl.suppressFallbackError`）由 `emitError` 設，startInternal 讀後 clear |
|
||
|
||
### Suggestion
|
||
|
||
| # | 內容 |
|
||
|---|------|
|
||
| s-1 | `TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce` 嚴格來說驗證的是 runStartupStage5 本身只 open 一次（因為 restartStartFn 替換了 ctrl.Start，完全 bypass 了 startInternal 的 R5-D3 分支）。M-2 的守門邏輯（`IsInColdStart() == false` 才 open）是靠 `TestStartupPipeline_IsInColdStart` 單獨驗證的 pure boolean 邏輯。兩個 test 合起來足以推得正確性，但若希望更貼近「真實 M-2 症狀」，可另加一個 test 透過 `a.ctrl.startInternal` 直接呼叫（需要先在 ctrl 加 test-only hook 允許跳過 spawn），非必要 |
|
||
|
||
---
|
||
|
||
## M. 結論
|
||
|
||
**判定：✅ 通過**
|
||
|
||
補丁對 M8-4b 原版 Reviewer 提出的 3 Major 全部正確修復：
|
||
|
||
1. **M-1（重複通知）** — 新增 `HasFailedStage()` helper + `startInternal` error 分支守門，邏輯正確、lock 保護得當，regression test（`TestStartupPipeline_HasFailedStage`）覆蓋 4 case 全通過。
|
||
2. **M-2（重複開瀏覽器）** — 新增 `IsInColdStart()` helper + `startInternal` R5-D3 守門 + `openBrowser` 改 package-level var 以便 stub，regression test（`TestStartupPipeline_IsInColdStart` + `TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce`）驗證完整。
|
||
3. **M-3（Retry test 重構）** — 新增 `restartStartFn` test hook + 重構 `TestStartupPipeline_RestartStartupSequence_StepsExecution` 為直接呼叫 method + callLog spy 驗證順序，抓得到 cancel↔start 與 sentinel↔start 的順序不變量。
|
||
|
||
**Stage 4 probe timeout** 從 5s 改為 2s，註解說明符合 TDD 原意。
|
||
|
||
**工程品質**：
|
||
- Build / Vet / Test / Race（-count=2）在 visiona-local 與 server 兩個 module 全綠
|
||
- 合併乾淨，無衝突標記
|
||
- 註解詳盡（每個 helper 都有中文註解解釋「為什麼」與「哪些場景走 fallback」）
|
||
- Lock 使用正確（`sync.Mutex` + `Lock/Unlock`，沒有 race condition）
|
||
- Test hook 透過 struct field 注入，不污染正式路徑（`restartStartFn` 預設 nil）
|
||
|
||
**不阻擋 M8-10**。Minor 2 項 + Suggestion 1 項可記為技術債，不強制當下處理。
|
||
|
||
**優點**：
|
||
- Helper 命名清楚（`HasFailedStage` / `IsInColdStart` 語義一眼就懂）
|
||
- 守門邏輯對 `pipeline == nil` fallback 處理完整，不會因單測環境或非冷啟動路徑被誤傷
|
||
- 4 個 regression test 覆蓋面完整：helper 本身 + 實際 RestartStartupSequence 流程 + openBrowser 呼叫次數
|
||
- 透過改 `openBrowser` 為 package-level var 解決測試 stub 問題，是 Go idiomatic 的做法
|
||
- `restartStartFn` test hook 設計乾淨：預設 nil → 走正式 `a.ctrl.Start()`，單測替換即可，不會意外漏到 production
|
||
- 2s probe timeout 的註解說明了「為什麼不是 5s」— 文件化設計決策
|