Skip to content

fix: update flow bugs wave1 (#182, #183, #184, #185) - #187

Merged
cheerc merged 2 commits into
mainfrom
fix/update-flow-bugs-wave1
Jun 29, 2026
Merged

fix: update flow bugs wave1 (#182, #183, #184, #185)#187
cheerc merged 2 commits into
mainfrom
fix/update-flow-bugs-wave1

Conversation

@cheerc

@cheerc cheerc commented Jun 29, 2026

Copy link
Copy Markdown
Owner

What

Fix four update-flow issues in a single batch PR:

How

Scope

Follows dispatch spec exactly — 7 files, all within update flow boundaries.

Lessons

  • ConfigManager has no pop() method — used set(key, None) instead. Caught by ./deploy.sh verify runtime log check, not by unit tests (which use MagicMock that auto-accepts any method call)
  • MagicMock silently accepts any attribute access — runtime verification (./deploy.sh verify) is essential for catching API mismatches that mocks hide

Closes #182
Closes #183
Closes #184
Closes #185

Build Verification

  • ./deploy.sh verify PASS
  • Log: ✓ PASS: Build verification — log clean, no errors detected

Test Evidence

  • ruff check: PASS (All checks passed!)
  • pytest (255 passed): PASS
  • test_frozen_paths.py (17 passed): PASS
  • frontend build (tsc + vite): PASS

🤖 Generated with Claude Code

@cheerc DeepSeek
fix: update flow bugs wave1 (#182, #183, #184, #185)
e2ac1e4 
- #182: Update self._progress in _download_asset() loop so get_progress()
  returns real values instead of always 0
- #183: Support i18n-reactive {key, params} message format in Toast;
  center title text and action buttons
- #184: Auto-clear skip_version on manual check; don't pass it to
  UpdateChecker so user sees real update status
- #185: Confirm clear-skip feature has no temporary/phase markers
  (doc-only — code already uses permanent wording)

Closes t-20260629141836493120-40453-1

Co-Authored-By: DeepSeek <noreply@deepseek.com>
@cheerc

cheerc commented Jun 29, 2026

Copy link
Copy Markdown
Owner Author

Verdict

REJECTED

Stage 1 - correctness

CRITICAL: src/api.py clears skip_version indiscriminately in check_update().
The backend comment says Manual check (user clicked "Check for Updates") = active user intent → auto-clear skip_version. However, the backend has no way to distinguish manual checks from automatic mount checks. check_update() is also called automatically on app mount (in frontend/src/App.tsx). This means skip_version will be wiped out on every startup, completely breaking the "Skip this version" feature.
Recommendation: Add a manual argument to check_update(self, manual: bool = False) and have the frontend explicitly pass true when the user clicks the check button. Only clear skip_version if manual is true.

Evidence

  • ran: gh pr checks 187 --repo cheerc/simple-edge-tts → all green
  • cited: src/api.py:438 — self._config.set("skip_version", None)
  • cited: frontend/src/App.tsx:53 — const update = await api.checkUpdate();

@cheerc

cheerc commented Jun 29, 2026

Copy link
Copy Markdown
Owner Author

Review Verdict: REJECTED

Reviewed HEAD: e2ac1e4c405eab83ec70e028d013627debbcd6cb
Reviewer: set-team-reviewer2
Depth: D2 (diff + 1-hop import chain)
Lenses: correctness

Finding F1（CRITICAL）：auto-check 意外清除 skip_version

檔案： src/api.py:438-440

check_update() 現在總是清除 skip_version，但 mount-time auto-check（App.tsx:53 的 useEffect）和 manual check（SettingsModal.tsx:142）共用此方法。這導致：

  1. 使用者跳過版本 1.5.0
  2. 重啟 app → auto-check 清除 skip → 顯示 1.5.0 更新通知
  3. 使用者再次跳過 → 下次重啟又出現

違反 skip_version 的核心用途（永久跳過直到使用者手動檢查）。

建議修復： 將 check_update() 加上 clear_skip: bool = False 參數，auto-check 傳 False（保留既有 skip 行為），manual check 傳 True；或新增 manual_check_update() 方法。

def check_update(self, clear_skip: bool = False) -> str:
    ...
    current = self._get_app_version()
    if clear_skip:
        self._config.set("skip_version", None)
        self._config.save()
        checker = UpdateChecker(current)
    else:
        skip = self._config.get("skip_version")
        checker = UpdateChecker(current, skip_version=skip)
    ...

Finding F2（WARNING）：resolveMessage 使用 replace 而非 replaceAll

檔案： frontend/src/components/Toast.tsx:17

text = text.replace(`{${k}}`, v);

String.prototype.replace() 只取代第一個匹配。若 i18n 字串未來包含多個相同 placeholder（如 {version} → {version}），第二個不會被取代。建議改為 replaceAll 或 replace(/{${k}}/g, v)。

Finding F3（WARNING）：updateToast 的 message 在 download flow 中使用已解析字串

檔案： frontend/src/App.tsx:118-131

Download flow 的 updateToast 傳入 t("update_downloaded") 等已解析字串而非 { key } 物件。型別上合法，但若未來實作即時語言切換，這些訊息不會反應變化。低風險，可留待後續優化。

✅ 通過的檢查項目

Evidence

  • ran: gh pr checks 187 --repo cheerc/simple-edge-tts → Lint & Test: pass
  • ran: rg conflict markers → no output
  • cited: src/api.py:438-440 — check_update() always clears skip_version, but called from both auto-check (App.tsx:53) and manual check (SettingsModal.tsx:142)
  • cited: frontend/src/components/Toast.tsx:17 — String.prototype.replace() without global flag

@cheerc DeepSeek
fix: add manual flag to check_update, fix replaceAll in Toast
deca41d 
Reviewer findings F1+F2:
- F1 (CRITICAL): check_update() now accepts manual: bool = False.
  Auto-check (mount-time) respects skip_version; only manual check
  (user clicked button) clears skip_version. Prevents skip from
  being wiped on every app restart.
- F2 (WARNING): Use String.replaceAll() instead of replace() in
  resolveMessage() to handle duplicate placeholders correctly.

Co-Authored-By: DeepSeek <noreply@deepseek.com>
@cheerc

cheerc commented Jun 29, 2026

Copy link
Copy Markdown
Owner Author

Review Verdict: VERIFIED ✅

Reviewed HEAD: deca41dcc1f65f30905f6efa555490f8b77ee337
Reviewer: set-team-reviewer2
Round: 2 (re-review of fixup commit)

上一輪 finding 修復驗證

Finding 狀態 修復方式
F1 (CRITICAL) — auto-check 意外清除 skip_version ✅ Fixed check_update(manual: bool = False) 區分 auto/manual；manual 才清除 skip
F2 (WARNING) — replace 無 global flag ✅ Fixed 改為 replaceAll
F3 (WARNING) — download flow 使用已解析字串 ⏭️ 保留 合理設計取捨，不影響功能

變更摘要（Round 2 diff）

  • src/api.py — check_update(manual=False) 參數 + 條件分支
  • frontend/src/hooks/useApi.ts — 橋接層傳遞 manual 參數
  • frontend/src/components/SettingsModal.tsx — manual check 傳 true
  • frontend/src/types.ts — PyWebViewApi.check_update(manual?) 型別更新
  • frontend/src/components/Toast.tsx — replace → replaceAll
  • tests/test_api.py — 更新測試驗證 auto/manual 兩種路徑

通過的檢查項目

Evidence

  • ran: gh pr checks 187 --repo cheerc/simple-edge-tts → Lint & Test: pass
  • ran: rg conflict markers → no output
  • cited: src/api.py:438-446 — manual=True clears skip, manual=False (auto) respects it
  • cited: frontend/src/components/Toast.tsx:17 — replaceAll used instead of replace
  • cited: frontend/src/components/SettingsModal.tsx:142 — api.checkUpdate(true) for manual check
  • cited: tests/test_api.py:542-550 — auto-check retains skip_version
  • cited: tests/test_api.py:553-565 — manual check clears skip_version

@cheerc

cheerc commented Jun 29, 2026

Copy link
Copy Markdown
Owner Author

Verdict

VERIFIED

Stage 1 - correctness

The implementer has addressed the findings. The check_update method now distinguishes between automatic checks and manual user checks via the manual flag, correctly preserving the skip_version preference during automatic checks while allowing the manual check to bypass it.
Also, Toast.tsx was correctly updated to use replaceAll.

Evidence

  • ran: gh pr checks 187 --repo cheerc/simple-edge-tts → all green
  • cited: src/api.py:442 — if manual: self._config.set("skip_version", None)
  • cited: frontend/src/hooks/useApi.ts:202 — const result = await getApi().check_update(manual);

@cheerc
cheerc merged commit 50e5411 into main Jun 29, 2026
1 check passed
@cheerc
cheerc deleted the fix/update-flow-bugs-wave1 branch June 29, 2026 14:37
This was referenced Jun 29, 2026
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment