Skip to content

feat: [Game] 遊戲設定記住上次腔調/級別,並雲端同步 - #229

Merged
Aiuanyu merged 3 commits into
mainfrom
claude/game-settings-localstorage-sync-niyzjb
Jul 14, 2026
Merged

feat: [Game] 遊戲設定記住上次腔調/級別,並雲端同步#229
Aiuanyu merged 3 commits into
mainfrom
claude/game-settings-localstorage-sync-niyzjb

Conversation

@GJRobert

Copy link
Copy Markdown
Collaborator

開遊戲 modal 時優先套用上一次實際玩過的腔調/級別(存於 localStorage
hakkaGameLastDataVarName,隨遊戲進度一起雲端同步);若從未玩過遊戲,才
退回網頁本身(或其他功能)目前設定的腔調/級別,行為與既有 fallback 邏輯
一致。

開遊戲 modal 時優先套用上一次實際玩過的腔調/級別(存於 localStorage
hakkaGameLastDataVarName,隨遊戲進度一起雲端同步);若從未玩過遊戲,才
退回網頁本身(或其他功能)目前設定的腔調/級別,行為與既有 fallback 邏輯
一致。

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a feature to remember and sync the last played game dialect and level (hakkaGameLastDataVarName) across devices. It updates the game UI to restore this preference upon opening the game modal and integrates it into the cloud synchronization logic. However, the current sync implementation has two critical issues: first, a data loss bug where local preference changes can be overwritten by older cloud data during sync; second, a redundant upload bug where pulling new settings from the cloud triggers an unnecessary push back to the cloud. Introducing a synced preferences snapshot (hakkaPrefsSynced) to track local changes and compare merged values is recommended to resolve these issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread js/cloud-sync.js Outdated
Comment thread js/cloud-sync.js Outdated
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review 摘要

這個 PR 讓遊戲 modal 開啟時優先套用「上一次實際玩過」的腔調/級別(存於 localStorage['hakkaGameLastDataVarName']),並透過既有的 preferences 雲端同步機制帶著走。整體方向合理,AGENTS.md 的資料分類表也同步補上一列,index.html 的快取版本號也有依慣例遞增,try/catch 保護 localStorage 存取、以及「值沒變就不寫入也不觸發同步」的短路判斷都做得不錯。

主要想請作者確認一個可能會讓功能實質失效的同步 race,其餘是程式碼品質層級的建議。


🐛 潛在的正確性問題(建議修正):cloud-wins 覆寫發生在 push 讀值之前,導致本地最新選擇被吃掉

js/cloud-sync.jssyncFromCloud() 中:

// 空字串代表雲端「從未玩過遊戲」,此時保留本地值(可能本地剛玩過、還沒推上去)
if (cloudPrefs.gameLastDataVarName) {
  localStorage.setItem('hakkaGameLastDataVarName', cloudPrefs.gameLastDataVarName);
}

這段會在雲端已有「非空」值時,無條件用雲端值覆寫本地 localStorage——即使本地剛剛才寫入了一個更新的值。緊接著 Smart Push 判斷 prefsChanged 時用的是覆寫前捕捉到的 localPrefs.gameLastDataVarName,但真正執行上傳的 syncToCloud()重新讀取當下 localStorage(此時已被上面那行覆寫成舊的雲端值)。

實際重現路徑(單一裝置、不需要多裝置併發):

  1. 雲端已同步過「四基」(上次玩的紀錄)。
  2. 使用者這次玩「海中」→ saveLastPlayedGameVarName('海中') 把本地寫成 海中,並觸發 triggerCloudSync()
  3. debounce 後 syncFromCloud() 執行:讀到 cloudPrefs.gameLastDataVarName === '四基'(雲端還沒更新),因為是 truthy,直接把本地 hakkaGameLastDataVarName 覆寫回 四基
  4. prefsChanged 判斷用的是覆寫前擷取的 '海中' vs 雲端 '四基' → 不同 → 觸發 syncToCloud()
  5. syncToCloud() 是重新 localStorage.getItem('hakkaGameLastDataVarName'),這時已經被第 3 步改回 四基,於是上傳的還是 四基(等於沒真的上傳新選擇)。
  6. 結果:使用者剛選的「海中」不管是本地還是雲端都消失了,下次開 modal 又會回到「四基」。

也就是說,只要使用者切換到「跟目前雲端已同步值不同」的新關卡,這個新選擇幾乎都會在下一次同步後被悄悄吃掉——這剛好是這個 PR 想解決的核心情境(記住最近一次玩的關卡),影響會比較明顯。

這個「雲端非空就整包覆寫本地」的寫法其實是沿用既有 romanizerJoiningMode 的處理方式,並非這個 PR 新引入的模式,但 romanizerJoiningMode 變更頻率低、影響較不明顯,這裡因為每次切換關卡都會觸發,問題會更容易被使用者感知到。

建議方向(擇一即可):

  • push 時改用同步流程一開始擷取好的 localPrefs(覆寫前的本地值)來決定要送出的內容,而不是讓 syncToCloud() 重新讀被覆寫過的 localStorage;或
  • 依照 AGENTS.md 表格宣稱的「LWW」語意,改成真的比較 updated_at/某種版本戳記,而不是「雲端有值就贏」;或
  • 至少把這個 preferences 的合併邏輯抽成獨立的 mergePreferences 純函式(呼應文件中「每種同步資料都要有對應 mergeXxx 純函式」的慣例),方便日後補測試、也比較不會再犯這種「先覆寫、後讀取」的順序問題。

若這個 race 是已知且可接受的技術債,麻煩在 PR 說明或程式碼註解點一下,避免之後被誤認為新 bug。


🧹 程式碼品質:新增的「套用上次關卡」邏輯與既有兩段高度重複

js/game/game-ui.jshandleStartGameClick 目前有三段幾乎一模一樣的「決定 gameActiveDialect/gameActiveDataVarName、切換 ready/select block 顯示、設定 game-target-level 文字、showGameView('setup')」邏輯:

  • 新增的「上次玩過」分支(約 57–75 行)
  • 既有的「currentActiveDialectLevelFullName fallback」分支(約 77–90 行)
  • 既有的「從進度推論」分支(約 110–129 行)

這次新增等於是把重複次數從 2 次變成 3 次。建議抽出一個小 helper,例如:

function applyGameLevelSelection(dataVarName, displayName) {
  gameActiveDataVarName = dataVarName;
  const  = dataVarName.substring(0, 1);
  const  = dataVarName.substring(1);
  gameActiveDialect = getDialectInfo(, ).腔名 || '四縣';
  document.getElementById('game-target-level').textContent = displayName;
  if (readyBlock) readyBlock.style.display = 'block';
  if (selectBlock) selectBlock.style.display = 'none';
  const startSessionBtn = document.getElementById('gameStartSessionBtn');
  if (startSessionBtn) startSessionBtn.style.display = 'block';
  showGameView('setup');
  gameModal.style.display = 'flex';
}

三個分支都改呼叫這個函式即可,往後如果 ready-block 的 DOM 結構要調整,只需要改一處,不會有漏改其中一支分支的風險。


其他觀察(非阻擋項)

  • getDialectInfo/getFullLevelName 對非預期的 dataVarName(例如格式跑掉)都有 fallback,不會拋例外,這點沿用得不錯,新分支的行為也一致。
  • window[lastVarName] 是否已被 loadDataFromDB 載入才會有值,if (lastVarData) 有做防呆,找不到時會自然 fall through 到既有 fallback 邏輯,這個 nil check 是對的。
  • 測試覆蓋率:專案目前只有 js/game/srs.test.js 這種針對純函式的 ad hoc assert 腳本,UI/localStorage/cloud-sync 相關程式碼本來就沒有自動化測試,這個 PR 沒補測試算是符合現狀,不算扣分,但上面提到的合併順序問題如果之後真的要修,會蠻適合順便補一個針對 syncFromCloud preferences 合併邏輯的最小測試。
  • 沒有發現安全性疑慮:新增的文字都是用 textContent 賦值,沒有 innerHTML/HTML 拼接的注入風險;Supabase 回傳資料的信任層級與既有欄位(romanizerJoiningMode)一致,沒有引入新的攻擊面。

整體來說方向正確、有照著既有慣例(版本號、AGENTS.md 表格、try/catch)做,主要建議是確認一下上面那個同步覆寫的 race,以及考慮把三段重複邏輯合併成一個 helper。

開遊戲 modal 仍照舊優先用網頁本身(或其他功能)目前設定的腔調/級別;
換腔換級按鈕旁新增橘色醒目「搞上擺个腔級」按鈕,只有跟上次實際玩過的
腔調/級別不同時才會出現,按下去才代入 localStorage 記住的上次遊戲設定。
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 14, 2026

Copy link
Copy Markdown

Deploying hakspring with  Cloudflare Pages  Cloudflare Pages

Latest commit: 67281ea
Status: ✅  Deploy successful!
Preview URL: https://31943c2c.hakspring.pages.dev
Branch Preview URL: https://claude-game-settings-localst.hakspring.pages.dev

View logs

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review: feat: 遊戲設定記住上次腔調/級別,並雲端同步 (PR 229)

Overview
Adds a "搞上擺个腔級" button in the game setup modal that lets the user re-apply the tone/level combo from their last actually-played session (stored in localStorage.hakkaGameLastDataVarName), synced to Supabase preferences. The button only shows when the remembered value differs from the currently displayed one, and is recorded only when a session actually starts (startSession), not merely when a level is selected — a sensible distinction called out in the code comments.

Strengths

  • Cloud-sync governance followed correctly: the new AGENTS.md table row and the preferences merge logic in cloud-sync.js mirror the existing romanizerJoiningMode LWW pattern exactly, including the "empty string = never set, do not clobber local" guard and the smart-push diff check. This matches the repos documented merge-strategy rules.
  • Defensive try/catch around all new localStorage access, consistent with existing style.
  • Avoids redundant cloud-sync triggers by early-returning in saveLastPlayedGameVarName when the value has not changed.
  • Cache-busting query strings (?v=...) were bumped for every changed asset in index.html, per project convention.
  • Uses varData.name / textContent consistently with the existing three call sites that set gameActiveDataVarName/gameActiveDialect - no copy-paste drift introduced.

Issues

  1. Dark-mode style missing for the new highlight button (violates AGENTS.md UI convention)

style.css adds:

.game-btn-highlight { background-color: #ff9800; }
.game-btn-highlight:hover { background-color: #e68900; }

but there is no dark-mode override. The existing @media (prefers-color-scheme: dark) block (around line 4158) already overrides .game-btn { background-color: #357ABD; }. Since .game-btn and .game-btn-highlight have equal CSS specificity and the dark-mode .game-btn rule appears later in the stylesheet than .game-btn-highlight, in dark mode the buttons background resolves back to the plain blue - the orange highlight (and its hover state) is silently lost, defeating the purpose of the "highlight" affordance for dark-mode users. AGENTS.md explicitly requires new GUI components to include a dark-theme counterpart. Suggest adding a .game-btn-highlight rule inside the dark-mode media query (or increasing specificity) so the highlight survives in dark mode.

  1. PR description says the level is auto-applied, implementation is button-triggered (please confirm intended UX)

The PR description states the modal will prioritize/auto-apply the last-played tone/level when opening the game modal. Looking at handleStartGameClick in game-ui.js, the priority order when opening the modal is unchanged: (1) current site-wide dialect/level, (2) inferred from SRS due dates - hakkaGameLastDataVarName is never consulted there. It is only applied when the user explicitly clicks the new "搞上擺个腔級" button. That is a reasonable (arguably safer/more explicit) design, but it does not match what the PR description promises. Worth updating the PR description (or the code, whichever reflects the real intent) so reviewers/testers are not testing for the wrong behavior.

Minor notes

  • No automated test added, but that is consistent with this projects existing test posture (only srs.test.html/srs.test.js exist for pure SRS logic; UI wiring like this generally is not covered) - not blocking.
  • refreshUseLastPlayedBtns visibility check (window[lastVarName] truthy) safely no-ops if the referenced dataset has not loaded yet, rather than throwing - good defensive choice.

Security / Performance
No security concerns (no innerHTML/dynamic script injection, textContent used throughout). Performance impact is negligible - one extra localStorage read/write plus an existing debounced cloud-sync trigger.

Overall a small, well-scoped feature that follows the repos cloud-sync governance conventions closely. The main things worth addressing before merge are the dark-mode CSS gap and clarifying the actual vs. described auto-apply behavior.


Generated with Claude Code

- 回應 PR review:偏好設定同步比照 hakkaDailyStatsSynced 引入
  hakkaPrefsSynced 已同步快照,本地相對快照未變才套用雲端值,
  否則保留本地並用合併後的最終值判斷是否需要 push,避免剛玩的
  新關卡被雲端舊值覆寫、或把剛拉下來的舊值又原封不動推回去。
- .game-btn-highlight 補上暗色模式覆寫,避免橘色醒目按鈕在暗色
  模式下被 .game-btn 的藍色覆蓋掉。
- 結果頁「調整設定/換其他腔調/級別」按鈕改回選項畫面(沿用剛
  打完那局的腔調/級別),不再每次都先逼使用者重選腔調/級別;
  真要換的話畫面上仍有「換其他腔/其他級」可點。
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review: 遊戲設定記住上次腔調/級別,並雲端同步 (#229)

Overview
Adds a 搞上擺个腔級 (use last-played level) button to the game setup modal. The last actually-played dialect/level (hakkaGameLastDataVarName) is recorded only when a session actually starts (not just selected), synced to Supabase via the existing preferences column, and offered as a one-click shortcut when it differs from whatever the site's current lookup context would otherwise default to. Also reworks syncFromCloud's preference merge to use a synced snapshot (hakkaPrefsSynced) so genuinely-new local changes aren't clobbered by a stale cloud read, and tweaks the post-session return-to-setup flow to keep the just-played level instead of forcing a full reselect.

Code quality & conventions

  • Follows AGENTS.md conventions well: cache-busting query strings bumped for all three changed assets (style.css, game-ui.js, cloud-sync.js), dark-mode CSS added for the new .game-btn-highlight class, and the new sync data row was added to the data-classification table in AGENTS.md.
  • The new snapshot-based merge in js/cloud-sync.js (~lines 241-268) is a nice generalization of the pattern already used for hakkaDailyStatsSynced, with comments that explain why the snapshot check exists rather than just restating the code.
  • getLastPlayedGameVarName / saveLastPlayedGameVarName / refreshUseLastPlayedBtn are small, single-purpose, and consistent in style with the rest of game-ui.js.

Potential issues

  1. One-time migration edge case in the preference merge (js/cloud-sync.js lines 241-258): before this PR, syncFromCloud treated 'cloud has a value' as authoritative. Now, on the first sync after this ships, hakkaPrefsSynced doesn't exist yet, so syncedPrefs defaults to {}, compared against 'none'/''. For a user whose local romanizerJoiningMode (or a value written on another device) differs from 'none', the new code treats it as 'changed since last sync' and pushes local over cloud instead of pulling cloud down. Narrow one-time window for multi-device users mid-migration, and a subtle deviation from true timestamp-based LWW (AGENTS.md calls this merge 'LWW', but there's no updated_at comparison — it's closer to own-writes tracking). Not blocking, but worth clarifying the actual semantics in AGENTS.md.
  2. refreshUseLastPlayedBtn / gameActiveDataVarName coupling (js/game/game-ui.js line 46): the visibility check compares lastVarName (raw localStorage key) directly against gameActiveDataVarName, which is always assigned from window[dataVarName].name (e.g. lines 106/154/181). This only works if window[x].name === x always holds — an assumption already baked into pre-existing code elsewhere in the file, so not a new bug, but this PR adds a second place relying on it. Worth flagging in case a future data-loading change ever breaks it silently.
  3. Minor: saveLastPlayedGameVarName/getLastPlayedGameVarName wrap localStorage access in try/catch, but the newly-added syncFromCloud/syncToCloud paths touching hakkaGameLastDataVarName/hakkaPrefsSynced don't have the same guard — matches the existing (unguarded) style for the other keys there, so consistent rather than a regression.

Security
No user-controlled input is written to the DOM (uses textContent), and the new localStorage key only stores a short internal dataVarName code — no injection or data-exposure concerns.

Performance
Negligible — one extra localStorage read/write and one small extra JSON field synced alongside the existing preferences payload.

Test coverage
No automated tests exist in this project (vanilla JS, no test harness per AGENTS.md), consistent with existing practice. Since the sync-merge logic is the trickiest part of this change, it'd be worth manually verifying: (a) a fresh user with no prior hakkaPrefsSynced/hakkaGameLastDataVarName; (b) two devices, one just played a session — confirm the other picks up the highlight button after sync; (c) an offline/local change plus a stale cloud read — confirm local isn't clobbered.

Overall a well-scoped, carefully-commented change that follows the project's established sync-governance patterns. The migration nuance in point 1 is the only thing worth a second look before merging.

@Aiuanyu
Aiuanyu merged commit ed96d6d into main Jul 14, 2026
6 checks passed
@Aiuanyu
Aiuanyu deleted the claude/game-settings-localstorage-sync-niyzjb branch July 14, 2026 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants