Skip to content

fix(storage): use IndexedDB in frontend mode with seamless migration#48

Merged
AmintaCCCP merged 4 commits intomainfrom
fix/issue-15-indexeddb-storage-migration
Mar 7, 2026
Merged

fix(storage): use IndexedDB in frontend mode with seamless migration#48
AmintaCCCP merged 4 commits intomainfrom
fix/issue-15-indexeddb-storage-migration

Conversation

@AmintaCCCP
Copy link
Copy Markdown
Owner

@AmintaCCCP AmintaCCCP commented Mar 6, 2026

Problem

Users with large starred repository datasets can hit localStorage quota limits in pure frontend mode, causing sync/persistence failures.

Solution

This PR migrates Zustand persisted state from localStorage to IndexedDB (large-capacity storage), with seamless backward compatibility:

  1. Added src/services/indexedDbStorage.ts custom persist storage.
  2. Switched Zustand persist storage to createJSONStorage(() => indexedDBStorage).
  3. Added automatic migration path:
    • On read: try IndexedDB first.
    • If empty: read legacy localStorage snapshot and copy it into IndexedDB.
  4. Added dual-write compatibility behavior:
    • Primary write goes to IndexedDB.
    • Best-effort backup write to localStorage (quota errors are safely ignored).

Migration Safety (No Data Loss)

  • Existing users upgrade with no manual steps.
  • Legacy localStorage data is auto-imported to IndexedDB on first load.
  • If IndexedDB is unavailable, app falls back to localStorage behavior.
  • localStorage quota failures no longer break persistence path for large users.

Validation

  • Frontend build passes (npm run build).
  • Storage path is backward-compatible and resilient for high-volume datasets.

Closes #15

Summary by CodeRabbit

  • New Features
    • App data now persists to IndexedDB with JSON storage for larger, more reliable state persistence.
    • Automatic migration from legacy localStorage on first access, keeping backward compatibility.
    • New public helper to combine default and custom categories with optional translation.
  • Reliability
    • Safer rehydration with SSR-safe guards, operation timeouts, merge/migrate handling, and improved error resilience.
  • Chores
    • Desktop build: DevTools menu and global shortcut added for toggling developer tools.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 6, 2026

📝 Walkthrough

Walkthrough

Adds an IndexedDB-backed StateStorage (indexedDBStorage) with SSR-safe guards, localStorage migration/fallback and timeouts; switches the Zustand store to use createJSONStorage(() => indexedDBStorage) with migrate/merge rehydration, normalization helpers, and exports getAllCategories.

Changes

Cohort / File(s) Summary
IndexedDB Persistence Provider
src/services/indexedDbStorage.ts
New indexedDBStorage: StateStorage implementing `getItem(name): Promise<string
Store Configuration & Rehydration
src/store/useAppStore.ts
Switched persistence to createJSONStorage(() => indexedDBStorage) with version: 1; replaced onRehydrateStorage with migrate and merge handlers; added normalizeNumberSet and normalizePersistedState to restore Sets and derived fields; added PersistedAppState type and exported getAllCategories(customCategories, language).
Desktop Build Workflow
.github/workflows/build-desktop.yml
Enabled globalShortcut and persistent DevTools menu; registers a global shortcut (Ctrl/Cmd+Shift+I) to toggle DevTools, ensures shortcuts are unregistered on quit, and enforces devTools enabled in production.

Sequence Diagram(s)

sequenceDiagram
participant Store as Store (Zustand)
participant IDB as IndexedDB
participant Local as localStorage
Store->>IDB: getItem(key)
alt IDB available & returns value
  IDB-->>Store: value
else IDB available but returns null
  IDB-->>Store: null
  Store->>Local: getItem(key)
  Local-->>Store: value or null
  alt Local has value
    Store->>IDB: setItem(key, value)  %% migrate to IDB
    IDB-->>Store: ok
  end
else IDB unavailable or error
  IDB-->>Store: error
  Store->>Local: getItem(key)
  Local-->>Store: value or null
end
Note right of Store: On writes, attempt both IDB and Local storage
Store->>IDB: setItem(key, value)
Store->>Local: setItem(key, value)
Note right of Store: On deletes, remove from localStorage first
Store->>Local: removeItem(key)
Store->>IDB: removeItem(key)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through bytes and tiny threads,
Migrated crumbs from old cupboards' beds,
Indexed the loot with careful paws,
Kept a copy just in case of laws,
Hooray—sync hums softly, no more dreads.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning DevTools shortcut changes in build-desktop.yml are unrelated to the storage migration objective stated in linked issue #15. Remove the DevTools shortcut changes from build-desktop.yml or clarify their necessity in a separate PR focused on desktop improvements.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: migrating storage from localStorage to IndexedDB with automatic migration support.
Linked Issues check ✅ Passed The PR fully addresses issue #15 by implementing IndexedDB storage with fallback, automatic migration from localStorage, and quota-error resilience.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/issue-15-indexeddb-storage-migration

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/services/indexedDbStorage.ts (3)

9-23: Consider connection pooling for improved efficiency.

Each helper function (idbGet, idbSet, idbDelete) opens a new database connection. For Zustand persist's typical usage pattern (writes on state changes), this is acceptable, but for high-frequency operations it could cause overhead.

If performance becomes a concern, consider caching the database connection with a lazy singleton pattern.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/services/indexedDbStorage.ts` around lines 9 - 23, openDb currently opens
a new IndexedDB connection on every call (used by idbGet, idbSet, idbDelete),
which can be costly under high-frequency usage; change openDb to cache a single
IDBDatabase Promise (lazy singleton) that is returned on subsequent calls (e.g.,
store the Promise<IDBDatabase> in a module-scoped variable like
cachedDbPromise), ensure onupgradeneeded logic still runs for first-open, and
handle errors by clearing the cache on rejection so retries can recreate the
connection; update callers (idbGet/idbSet/idbDelete) to keep calling openDb but
rely on the cached Promise.

120-126: Frequent quota warnings may spam the console.

For users with large datasets (5,000+ stars), every state change will attempt a localStorage write and log a warning when it fails. This could flood the console with [storage] localStorage backup set failed (ignored) messages.

Consider suppressing repeated warnings or logging at debug level after the first occurrence.

🔇 Suggested improvement with warning throttle
+let localStorageWarningShown = false;
+
 export const indexedDBStorage: StateStorage = {
   // ...
   setItem: async (name: string, value: string): Promise<void> => {
     // ... IndexedDB write ...

     // Secondary compatibility backup (best effort only)
     try {
       window.localStorage.setItem(name, value);
+      localStorageWarningShown = false; // Reset on success
     } catch (error) {
       // Expected for large users (QuotaExceededError). Do not fail persistence.
-      console.warn('[storage] localStorage backup set failed (ignored):', error);
+      if (!localStorageWarningShown) {
+        console.warn('[storage] localStorage backup set failed (ignored):', error);
+        localStorageWarningShown = true;
+      }
     }
   },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/services/indexedDbStorage.ts` around lines 120 - 126, The catch block
that warns on localStorage failures (around window.localStorage.setItem(name,
value)) can spam the console; add a module-scoped flag (e.g.,
hasLoggedLocalStorageQuotaError) or a simple throttle so the warning is only
printed once (or print at console.debug after the first occurrence). Update the
catch clause to check/set that flag and emit the original console.warn only the
first time, and use console.debug or no-op for subsequent failures to suppress
repeated messages.

91-101: Migration is not atomic across concurrent calls.

If multiple tabs or concurrent getItem calls occur before IndexedDB contains data, each may independently read from localStorage and attempt migration. While functionally idempotent (same value), this could result in redundant writes and console noise.

For a single-tab app this is unlikely to be an issue, but if you expect multi-tab usage, consider using a migration flag in IndexedDB to coordinate.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/services/indexedDbStorage.ts` around lines 91 - 101, The migration can
race across concurrent calls reading localStorage then writing to IndexedDB; to
fix, implement a migration coordination flag in IndexedDB so only the first
caller performs the copy: before reading legacy localStorage, check a migration
marker via idbGet (e.g., `${name}__migrated` or a per-store marker), if marker
absent then attempt to set the marker atomically (use an IndexedDB transaction
or an idbSet for the marker) and only the caller that succeeds copies
legacyValue via idbSet and sets the marker to true; other callers should
re-check idbGet(name) after seeing the marker (or wait briefly) and return the
newly-stored value, referencing idbGet, idbSet and the legacy
window.localStorage.getItem(name) in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/services/indexedDbStorage.ts`:
- Around line 113-117: The try/catch around idbSet in indexedDbStorage.ts
currently swallows errors; change it to log the error with
console.warn('[storage] IndexedDB set failed:', error) and then rethrow the
caught error (throw error) so callers (e.g., Zustand persist) can observe write
failures; update the idbSet call's catch block to propagate the error instead of
silently returning.

---

Nitpick comments:
In `@src/services/indexedDbStorage.ts`:
- Around line 9-23: openDb currently opens a new IndexedDB connection on every
call (used by idbGet, idbSet, idbDelete), which can be costly under
high-frequency usage; change openDb to cache a single IDBDatabase Promise (lazy
singleton) that is returned on subsequent calls (e.g., store the
Promise<IDBDatabase> in a module-scoped variable like cachedDbPromise), ensure
onupgradeneeded logic still runs for first-open, and handle errors by clearing
the cache on rejection so retries can recreate the connection; update callers
(idbGet/idbSet/idbDelete) to keep calling openDb but rely on the cached Promise.
- Around line 120-126: The catch block that warns on localStorage failures
(around window.localStorage.setItem(name, value)) can spam the console; add a
module-scoped flag (e.g., hasLoggedLocalStorageQuotaError) or a simple throttle
so the warning is only printed once (or print at console.debug after the first
occurrence). Update the catch clause to check/set that flag and emit the
original console.warn only the first time, and use console.debug or no-op for
subsequent failures to suppress repeated messages.
- Around line 91-101: The migration can race across concurrent calls reading
localStorage then writing to IndexedDB; to fix, implement a migration
coordination flag in IndexedDB so only the first caller performs the copy:
before reading legacy localStorage, check a migration marker via idbGet (e.g.,
`${name}__migrated` or a per-store marker), if marker absent then attempt to set
the marker atomically (use an IndexedDB transaction or an idbSet for the marker)
and only the caller that succeeds copies legacyValue via idbSet and sets the
marker to true; other callers should re-check idbGet(name) after seeing the
marker (or wait briefly) and return the newly-stored value, referencing idbGet,
idbSet and the legacy window.localStorage.getItem(name) in your changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 683ddf5d-b4b6-4fd8-9171-15dcc0641485

📥 Commits

Reviewing files that changed from the base of the PR and between 69fc999 and deaa9f0.

📒 Files selected for processing (2)
  • src/services/indexedDbStorage.ts
  • src/store/useAppStore.ts

Comment on lines +113 to +117
try {
await idbSet(name, value);
} catch (error) {
console.warn('[storage] IndexedDB set failed:', error);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Consider propagating IndexedDB write failures.

If IndexedDB is the primary storage and its write fails, the current code silently swallows the error. For users exceeding localStorage quota (the target use case), if IndexedDB also fails, data is lost with no indication to the caller.

Zustand persist may handle errors gracefully, but consider whether failures in the primary storage path should be propagated so the application can respond appropriately.

🔧 Suggested improvement
     if (canUseIndexedDB()) {
       try {
         await idbSet(name, value);
       } catch (error) {
         console.warn('[storage] IndexedDB set failed:', error);
+        // Re-throw if this was our only viable storage option
+        // (localStorage will likely fail for large data users)
+        throw error;
       }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
await idbSet(name, value);
} catch (error) {
console.warn('[storage] IndexedDB set failed:', error);
}
try {
await idbSet(name, value);
} catch (error) {
console.warn('[storage] IndexedDB set failed:', error);
// Re-throw if this was our only viable storage option
// (localStorage will likely fail for large data users)
throw error;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/services/indexedDbStorage.ts` around lines 113 - 117, The try/catch
around idbSet in indexedDbStorage.ts currently swallows errors; change it to log
the error with console.warn('[storage] IndexedDB set failed:', error) and then
rethrow the caught error (throw error) so callers (e.g., Zustand persist) can
observe write failures; update the idbSet call's catch block to propagate the
error instead of silently returning.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/services/indexedDbStorage.ts`:
- Around line 139-153: The setItem implementation may write to IndexedDB via
idbSet but safeLocalStorageSet can fail (quota) leaving callers that
synchronously read localStorage stale; modify setItem to dispatch a post-save
notification (e.g., a custom window event) after a successful idbSet so other
modules can refresh, keep idbSet and safeLocalStorageSet behavior but emit the
event only on success; then implement a simple read-through in the synchronous
getters in backendAdapter and backendErrors (cache the first read from
localStorage into a module-scoped variable, return the cached value on
subsequent calls) and add a comment documenting this known quota/async
limitation and that listeners should update cache on the post-save event.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: dc760ee8-c8a5-4e33-98a7-867c5e7bd76a

📥 Commits

Reviewing files that changed from the base of the PR and between deaa9f0 and 1c743a5.

📒 Files selected for processing (1)
  • src/services/indexedDbStorage.ts

Comment on lines +139 to +153
setItem: async (name: string, value: string): Promise<void> => {
if (typeof window === 'undefined') return;

// Primary path: IndexedDB first (large data friendly)
if (canUseIndexedDB()) {
try {
await withTimeout(idbSet(name, value));
} catch (error) {
console.warn('[storage] IndexedDB set failed:', error);
}
}

// Secondary compatibility backup (best effort only)
safeLocalStorageSet(name, value);
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find all direct localStorage reads of the store key to assess scope of inconsistency
rg -n "localStorage\.getItem\s*\(\s*['\"]github-stars-manager['\"]" --type ts

Repository: AmintaCCCP/GithubStarsManager

Length of output: 272


🏁 Script executed:

#!/bin/bash
# Also search more broadly for localStorage.getItem patterns in the codebase
rg -n "localStorage\.getItem" --type ts -A 2 | head -100

Repository: AmintaCCCP/GithubStarsManager

Length of output: 908


🏁 Script executed:

#!/bin/bash
# Let's check the structure and locate the files mentioned in the review
fd -t f "backendAdapter" --type ts
fd -t f "backendErrors" --type ts

Repository: AmintaCCCP/GithubStarsManager

Length of output: 412


🏁 Script executed:

#!/bin/bash
# Get an overview of the indexedDbStorage.ts file to understand the context
wc -l src/services/indexedDbStorage.ts

Repository: AmintaCCCP/GithubStarsManager

Length of output: 109


🏁 Script executed:

#!/bin/bash
# Read backendAdapter.ts to understand the context of the localStorage read
sed -n '50,75p' src/services/backendAdapter.ts

Repository: AmintaCCCP/GithubStarsManager

Length of output: 801


🏁 Script executed:

#!/bin/bash
# Read backendErrors.ts to understand the context of the localStorage read
sed -n '55,75p' src/utils/backendErrors.ts

Repository: AmintaCCCP/GithubStarsManager

Length of output: 797


🏁 Script executed:

#!/bin/bash
# Read the full indexedDbStorage.ts to understand the storage implementation
cat -n src/services/indexedDbStorage.ts

Repository: AmintaCCCP/GithubStarsManager

Length of output: 6001


Address data consistency when localStorage quota is exceeded.

The concern is valid: when localStorage quota is exceeded, setItem successfully writes to IndexedDB but safeLocalStorageSet fails silently. However, backendAdapter.ts:60 and backendErrors.ts:65 read directly from localStorage afterward, returning stale or missing values.

The direct reads exist intentionally to avoid circular dependencies (as noted in backendAdapter's comment), but this creates eventual consistency issues:

  • If backendApiSecret is missing, authorization headers won't include the Bearer token (partial outage)
  • If language is missing, it defaults to 'zh' (incorrect for en users)

While both functions have fallback logic, the data loss during quota exceeded scenarios is a real functional issue. Given the async nature of IndexedDB, a synchronous getter won't work. Instead, consider:

  1. Inline a read-through cache in these two files: on first call, read from localStorage; on subsequent calls within the same page load, use the cached value (tolerates missed updates during quota exceeded)
  2. Document the limitation explicitly with a comment explaining this is a known issue during IndexedDB migration and quota exceeded scenarios
  3. Use a post-save hook in setItem to notify listeners (e.g., a window event) so these two files can refresh their internal state after writes succeed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/services/indexedDbStorage.ts` around lines 139 - 153, The setItem
implementation may write to IndexedDB via idbSet but safeLocalStorageSet can
fail (quota) leaving callers that synchronously read localStorage stale; modify
setItem to dispatch a post-save notification (e.g., a custom window event) after
a successful idbSet so other modules can refresh, keep idbSet and
safeLocalStorageSet behavior but emit the event only on success; then implement
a simple read-through in the synchronous getters in backendAdapter and
backendErrors (cache the first read from localStorage into a module-scoped
variable, return the cached value on subsequent calls) and add a comment
documenting this known quota/async limitation and that listeners should update
cache on the post-save event.

@AmintaCCCP
Copy link
Copy Markdown
Owner Author

已修复 #48 的白屏根因并保持平滑迁移/可回退不丢数据:\n\n- 把持久化数据类型修复从 前移到 ,确保首屏渲染前就完成结构标准化\n- 新增 :对 做数组兜底,对 做 规范化\n- 保留 IndexedDB 主读写 + localStorage 兼容写回策略,不影响旧版本回退读取\n- 保留原有排序/语言/认证状态恢复逻辑,避免迁移后行为变化\n\n这次改动针对你在 mac 上复现到的 和 。\n\n请在你的机器上用原复现路径验证一次:\n1) 使用当前 PR 最新 commit 安装包启动\n2) 带历史用户数据启动(不清理 userData)\n3) 确认无白屏,数据正常\n4) 回退到旧版本确认数据仍可读取\n\n如果你愿意,我下一步可以再补一个 e2e/单测专门覆盖“localStorage 旧快照 -> IDB 迁移 -> 回退读取”链路。

@AmintaCCCP
Copy link
Copy Markdown
Owner Author

已修复 #48 的白屏根因并保持平滑迁移/可回退不丢数据:

  • 把持久化数据类型修复从 onRehydrateStorage 前移到 persist.merge,确保首屏渲染前就完成结构标准化
  • 新增 normalizePersistedState:对 repositories/releases 做数组兜底,对 releaseSubscriptions/readReleases 做 Set 规范化
  • 保留 IndexedDB 主读写 + localStorage 兼容写回策略,不影响旧版本回退读取
  • 保留原有排序/语言/认证状态恢复逻辑,避免迁移后行为变化

这次改动针对你在 mac 上复现到的 r.has is not a function 和 Cannot read properties of undefined (reading 'length').

请在你的机器上用原复现路径验证一次:

  1. 使用当前 PR 最新 commit 安装包启动
  2. 带历史用户数据启动(不清理 userData)
  3. 确认无白屏,数据正常
  4. 回退到旧版本确认数据仍可读取

如果你愿意,我下一步可以再补一个 e2e/单测专门覆盖“localStorage 旧快照 -> IDB 迁移 -> 回退读取”链路。

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/store/useAppStore.ts (1)

451-471: Consider accepting the version parameter in migrate for future migrations.

The current migrate implementation ignores the version number. While this works for version 1, accepting the version parameter prepares the codebase for future schema migrations.

♻️ Suggested improvement
-      migrate: (persistedState) => persistedState as PersistedAppState,
+      migrate: (persistedState, version) => {
+        // Future migrations can be handled here based on version
+        // e.g., if (version === 0) { /* transform old schema */ }
+        return persistedState as PersistedAppState;
+      },

Additionally, lines 467-470 have minor redundancy since normalized already includes ...currentState from normalizePersistedState (line 131). The merge still functions correctly, but you could simplify to just return normalized as AppState & AppActions; if desired.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/store/useAppStore.ts` around lines 451 - 471, Change the migrate handler
to accept the version parameter (migrate: (persistedState, version) => ...) so
future schema migrations can branch on version; inside migrate, use the version
when needed (or pass it to normalizePersistedState) instead of ignoring it. For
the merge function, avoid redundant spreading of currentState by returning the
already-normalized object (return normalized as AppState & AppActions) if
normalizePersistedState guarantees it includes currentState; otherwise keep the
spread as a safe fallback—update merge to rely on normalizePersistedState or
explicitly merge only missing pieces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/store/useAppStore.ts`:
- Around line 451-471: Change the migrate handler to accept the version
parameter (migrate: (persistedState, version) => ...) so future schema
migrations can branch on version; inside migrate, use the version when needed
(or pass it to normalizePersistedState) instead of ignoring it. For the merge
function, avoid redundant spreading of currentState by returning the
already-normalized object (return normalized as AppState & AppActions) if
normalizePersistedState guarantees it includes currentState; otherwise keep the
spread as a safe fallback—update merge to rely on normalizePersistedState or
explicitly merge only missing pieces.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9276c7c3-9282-4c95-9af1-b71c6ec37b33

📥 Commits

Reviewing files that changed from the base of the PR and between 1c743a5 and 6d68297.

📒 Files selected for processing (1)
  • src/store/useAppStore.ts

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
.github/workflows/build-desktop.yml (1)

207-207: Enabling DevTools in production is a security trade-off.

Setting devTools: true in production allows end users to inspect application internals, execute arbitrary JavaScript, and access persisted storage (localStorage/IndexedDB). While helpful for debugging the storage migration issues this PR addresses, consider whether this should remain permanently or be reverted once the migration is stable.

If DevTools must stay enabled for troubleshooting, this is acceptable—just ensure it's a conscious decision rather than debug code left behind.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/build-desktop.yml at line 207, The workflow currently
forces devTools: true in production; change this to not ship DevTools enabled by
default by either reverting devTools to false or gating it behind an environment
flag (e.g., check NODE_ENV !== 'production' or an explicit ENABLE_DEVTOOLS
variable) where the default for production is false; update the 'devTools' entry
in the desktop build step to read the env toggle and add a brief comment noting
it's intentionally gated for troubleshooting so it isn't left enabled
inadvertently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/build-desktop.yml:
- Around line 310-318: The globalShortcut.register call inside app.whenReady()
(the globalShortcut.register('CommandOrControl+Shift+I' handler) intercepts
keystrokes system-wide and is redundant because your menu already provides {
role: 'toggleDevTools' } scoped to the focused window; remove the
globalShortcut.register block (and the associated will-quit cleanup) from the
app.whenReady() logic and drop globalShortcut from the imports so DevTools
behavior remains app-scoped via the menu and you avoid OS-level key
interception; keep createWindow() and the menu role intact.

---

Nitpick comments:
In @.github/workflows/build-desktop.yml:
- Line 207: The workflow currently forces devTools: true in production; change
this to not ship DevTools enabled by default by either reverting devTools to
false or gating it behind an environment flag (e.g., check NODE_ENV !==
'production' or an explicit ENABLE_DEVTOOLS variable) where the default for
production is false; update the 'devTools' entry in the desktop build step to
read the env toggle and add a brief comment noting it's intentionally gated for
troubleshooting so it isn't left enabled inadvertently.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 243779c3-2355-4842-a152-f830509c12b7

📥 Commits

Reviewing files that changed from the base of the PR and between 6d68297 and cc37cc1.

📒 Files selected for processing (1)
  • .github/workflows/build-desktop.yml

Comment on lines +310 to +318
'app.whenReady().then(() => {\\n' +
' createWindow();\\n' +
' globalShortcut.register(\\'CommandOrControl+Shift+I\\', () => {\\n' +
' const focused = BrowserWindow.getFocusedWindow();\\n' +
' if (focused && !focused.isDestroyed()) {\\n' +
' focused.webContents.toggleDevTools();\\n' +
' }\\n' +
' });\\n' +
'});\\n\\n' +
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Global shortcut intercepts system-wide, not just when app is focused.

globalShortcut.register() captures the keystroke at the OS level, meaning Ctrl+Shift+I will be intercepted even when this app isn't focused—potentially breaking DevTools in browsers or other Electron apps running simultaneously.

Additionally, this is redundant: line 292 already includes { role: 'toggleDevTools' }, which automatically provides the standard Ctrl+Shift+I / Cmd+Opt+I accelerator scoped to the app window.

Consider removing the global shortcut registration entirely, or use a non-conflicting key combination if a global shortcut is truly needed.

🔧 Proposed fix: Remove redundant global shortcut
 'app.whenReady().then(() => {\\n' +
-'  createWindow();\\n' +
-'  globalShortcut.register(\\'CommandOrControl+Shift+I\\', () => {\\n' +
-'    const focused = BrowserWindow.getFocusedWindow();\\n' +
-'    if (focused && !focused.isDestroyed()) {\\n' +
-'      focused.webContents.toggleDevTools();\\n' +
-'    }\\n' +
-'  });\\n' +
+'  createWindow();\\n' +
 '});\\n\\n' +

And remove the will-quit cleanup since it's no longer needed:

-'app.on(\\'will-quit\\', () => {\\n' +
-'  globalShortcut.unregisterAll();\\n' +
-'});\\n\\n' +

Also remove globalShortcut from the import on line 190:

-const mainJsContent = 'const { app, BrowserWindow, Menu, shell, globalShortcut } = require(\\'electron\\');\\n' +
+const mainJsContent = 'const { app, BrowserWindow, Menu, shell } = require(\\'electron\\');\\n' +
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
'app.whenReady().then(() => {\\n' +
' createWindow();\\n' +
' globalShortcut.register(\\'CommandOrControl+Shift+I\\', () => {\\n' +
' const focused = BrowserWindow.getFocusedWindow();\\n' +
' if (focused && !focused.isDestroyed()) {\\n' +
' focused.webContents.toggleDevTools();\\n' +
' }\\n' +
' });\\n' +
'});\\n\\n' +
'app.whenReady().then(() => {\\n' +
' createWindow();\\n' +
'});\\n\\n' +
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/build-desktop.yml around lines 310 - 318, The
globalShortcut.register call inside app.whenReady() (the
globalShortcut.register('CommandOrControl+Shift+I' handler) intercepts
keystrokes system-wide and is redundant because your menu already provides {
role: 'toggleDevTools' } scoped to the focused window; remove the
globalShortcut.register block (and the associated will-quit cleanup) from the
app.whenReady() logic and drop globalShortcut from the imports so DevTools
behavior remains app-scoped via the menu and you avoid OS-level key
interception; keep createWindow() and the menu role intact.

@AmintaCCCP AmintaCCCP merged commit 0bd96a2 into main Mar 7, 2026
5 checks passed
@AmintaCCCP AmintaCCCP deleted the fix/issue-15-indexeddb-storage-migration branch March 7, 2026 08:57
@AmintaCCCP AmintaCCCP mentioned this pull request Mar 7, 2026
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.

同步失败

1 participant