Conversation
- add `switchTheme` to desktop RPC schema/client and backend handler - introduce `useAppConfig` query hook and prefetch app config at startup - apply active theme globally in `AppShell` and update Settings with optimistic theme switching + error state
- Introduce `AppRuntime` to own startup/services, downloads, and migration state - Add library migration flow with progress events, path validation, rollback, and DB path rewrites - Move persisted config and SQLite DB to app-data, with legacy config/DB fallback and migration - Wire new RPC methods/events for library migration and remove Theme nav entry
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR centralizes desktop service lifecycle into a new AppRuntime, moves service and migration logic out of module-level RPC, introduces a LibraryMigrationManager with rollback, changes config persistence to APP_DATA_DIR, makes DB initialization explicit, and adds UI/theme/migration integration and RPC endpoints. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Settings UI
participant Runtime as AppRuntime
participant Manager as LibraryMigrationManager
participant DB as Database
participant FS as File System
participant Kiwix as Kiwix Server
User->>Client: Click "Migrate Library"
Client->>Runtime: startLibraryMigration(nextPath)
Runtime->>Manager: startLibraryMigration(nextPath)
Manager->>Manager: validatePaths()
alt validation fails
Manager-->>Client: return error state
else validation passes
Manager->>Runtime: stopServices()
Runtime->>Kiwix: stop()
Manager->>FS: collect .zim files, compute totalBytes
loop each artifact
Manager->>FS: rename (or copy-with-progress on EXDEV)
Manager->>Manager: update movedBytes -> emit progress via runtime
end
Manager->>DB: bulkRewriteLocalPaths(old→new)
Manager->>Manager: rebuildLibraryXmlFromDisk()
Manager->>Runtime: configManager.setLibraryPath(nextPath)
Manager->>Runtime: startServices()
Runtime->>Kiwix: start()
Manager-->>Client: state="completed"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/bun/utils/download-manager.ts (1)
76-86:⚠️ Potential issue | 🟠 MajorIncorrect speed calculation: accumulation bug and wrong multiplier.
Two issues in the download speed calculation:
- Line 77:
bytesSinceLastReport = value.lengthoverwrites instead of accumulates. Only the last chunk before the 500ms threshold is counted.- Line 86: Multiplying by
100instead of1000. SincetimeSinceLastReportis in milliseconds, converting to bytes/second requires* 1000.🐛 Proposed fix
if (value) { fileWriter.write(value); downloaded += value.length; - bytesSinceLastReport = value.length; + bytesSinceLastReport += value.length; const now = performance.now(); const timeSinceLastReport = now - lastReportTime; // 500ms throttle if (timeSinceLastReport > 500) { info.downloadedBytes = downloaded; info.progress = contentLength > 0 ? (downloaded / contentLength) * 100 : 0; - info.bytesPerSec = (bytesSinceLastReport / timeSinceLastReport) * 100; + info.bytesPerSec = (bytesSinceLastReport / timeSinceLastReport) * 1000; this.notifyProgress(info); lastReportTime = now; bytesSinceLastReport = 0; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/bun/utils/download-manager.ts` around lines 76 - 86, The speed calculation wrongly resets bytesSinceLastReport and uses the wrong multiplier: in download-manager.ts inside the chunk handling (variables downloaded, bytesSinceLastReport, lastReportTime, info.bytesPerSec), change bytesSinceLastReport so it accumulates the chunk sizes instead of overwriting (e.g., add value.length to bytesSinceLastReport) and compute bytes per second by multiplying by 1000 (not 100) when converting from milliseconds: info.bytesPerSec = (bytesSinceLastReport / timeSinceLastReport) * 1000; ensure bytesSinceLastReport is reset after reporting.
🧹 Nitpick comments (4)
apps/desktop/src/db/migrate.ts (1)
27-29: Consider whetherloadPersistedConfig()is needed here.The return value from
loadPersistedConfig()is not used, andgetDatabasePath()doesn't depend on it being called first (it derives the path fromgetConfigDir()which usesos.homedir()and env variables).If
loadPersistedConfig()is kept for future-proofing or consistency with the runtime startup, that's fine. Otherwise, it could be removed to simplify the migration CLI.🔧 Optional simplification if loadPersistedConfig is not needed
async function main() { - await loadPersistedConfig(); const dbPath = getDatabasePath(); ensureDir(getConfigDir());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/db/migrate.ts` around lines 27 - 29, The call to loadPersistedConfig() is redundant because its return value is unused and getDatabasePath()/getConfigDir() do not depend on it; remove the loadPersistedConfig() invocation from migrate.ts to simplify the CLI, leaving the calls to getDatabasePath() and ensureDir(getConfigDir()), and run tests or search for any hidden side-effects of loadPersistedConfig() to confirm nothing else relies on its execution before migration.apps/desktop/src/bun/utils/library-migration-manager.ts (2)
408-421: Potential unhandled rejection in stream copy error handling.If the reader emits an error, the promise rejects immediately, but the writer may still emit an error afterward (e.g., if it's mid-write when the reader fails). Since the promise is already rejected, the second
rejectcall is a no-op, but the writer error event handler is still attached and could potentially cause issues in edge cases.Consider destroying the writer explicitly on reader error:
♻️ Proposed defensive cleanup
await new Promise<void>((resolve, reject) => { const reader = createReadStream(src); const writer = createWriteStream(dest); reader.on("data", (chunk: Buffer) => { onProgress?.(chunk.length); }); - reader.on("error", reject); - writer.on("error", reject); + reader.on("error", (err) => { + writer.destroy(); + reject(err); + }); + writer.on("error", (err) => { + reader.destroy(); + reject(err); + }); writer.on("close", resolve); reader.pipe(writer); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/bun/utils/library-migration-manager.ts` around lines 408 - 421, The stream copy Promise using createReadStream/createWriteStream attaches both reader.on("error", reject) and writer.on("error", reject) which can lead to race conditions and stray handlers; update the Promise inside library-migration-manager.ts to, on reader error, call writer.destroy(err) (or end/destroy) and remove the other event listeners before rejecting, and similarly on writer error destroy the reader and cleanup listeners, ensuring both streams are explicitly destroyed and listeners removed so only one rejection occurs; reference the existing reader and writer variables and the Promise wrapper around the pipe operation.
358-362: Consider typing the error more specifically thanany.The
error?.codecheck works but usesanytype. A more type-safe approach would use a type guard or NodeJS.ErrnoException.♻️ Type-safe error handling
- } catch (error: any) { - if (error?.code !== "EXDEV") { + } catch (error) { + if (!(error instanceof Error) || (error as NodeJS.ErrnoException).code !== "EXDEV") { throw error; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/bun/utils/library-migration-manager.ts` around lines 358 - 362, The catch currently types error as any; change it to unknown and narrow it safely (e.g., create a type guard isErrnoException(err): err is NodeJS.ErrnoException or check typeof err === "object" && err !== null && "code" in err) before accessing error.code in the library-migration-manager.ts catch block so the EXDEV branch uses a properly typed NodeJS.ErrnoException (or guarded) value instead of any.apps/desktop/src/bun/utils/config-manager.ts (1)
9-11: Consider removing unused constant if bootstrap.json write is no longer needed.
BOOTSTRAP_PATHis still used for reading (line 52), but the summary mentions bootstrap saving was removed. Ifbootstrap.jsonis only read for legacy migration and never written by this code, consider adding a comment clarifying this is read-only for migration purposes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/bun/utils/config-manager.ts` around lines 9 - 11, The BOOTSTRAP_PATH constant appears to refer to a bootstrap.json that is no longer written by this module and is only read for legacy migration (see code that reads BOOTSTRAP_PATH); either remove BOOTSTRAP_PATH if it's truly unused, or clearly mark it as a read-only legacy migration artifact by adding a brief comment above the constant (or rename to LEGACY_BOOTSTRAP_PATH) explaining it's kept only for backward-compatibility/one-time migration and not written by this code; update any references to the new name if you rename it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/db/queries.ts`:
- Around line 81-92: bulkRewriteLocalPaths builds a LIKE pattern from
oldLibraryPath which can misinterpret '%' and '_' as wildcards; update
bulkRewriteLocalPaths to escape backslashes, '%' and '_' in oldLibraryPath when
constructing likePattern (e.g., replace "\" -> "\\", "%" -> "\%", "_" -> "\_")
and use a SQL LIKE with an explicit ESCAPE '\\' clause so the pattern matches a
literal prefix, referencing the function bulkRewriteLocalPaths and variables
oldLibraryPath/likePattern and the UPDATE query.
In `@apps/desktop/src/mainview/screens/Settings.tsx`:
- Around line 125-144: Both useEffect hooks only handle migrationState.status
=== "completed" and neglect the "error" terminal state; update the guard in the
effects that reference migrationState.status (the block that calls
queryClient.invalidateQueries(["app-config"], ["local-library"]) and the block
that sets the 3s timeout to call resetMigrationState) to also treat "error" as a
terminal state (e.g., check for status === "completed" || status === "error" or
use a helper isTerminal status check) so the timeout will clear the error UI via
resetMigrationState and any desired cache handling can run for error if
intended; modify the conditions around migrationState in those useEffect hooks
(referencing migrationState, queryClient.invalidateQueries, and
resetMigrationState) accordingly.
In `@apps/desktop/src/mainview/store/index.ts`:
- Around line 59-63: The hydration call to api.getLibraryMigrationState() can
overwrite live progress events; change the promise handler that calls
useLibraryMigrationStore.getState().setState(state) to first read the current
store state (via useLibraryMigrationStore.getState()) and only apply the
hydrated state if the current status is still "idle" (or otherwise appropriate
non-active sentinel), so onLibraryMigrationProgress events that arrived during
the async fetch are not clobbered; reference api.getLibraryMigrationState,
useLibraryMigrationStore.getState().setState, and onLibraryMigrationProgress to
locate and update the logic.
---
Outside diff comments:
In `@apps/desktop/src/bun/utils/download-manager.ts`:
- Around line 76-86: The speed calculation wrongly resets bytesSinceLastReport
and uses the wrong multiplier: in download-manager.ts inside the chunk handling
(variables downloaded, bytesSinceLastReport, lastReportTime, info.bytesPerSec),
change bytesSinceLastReport so it accumulates the chunk sizes instead of
overwriting (e.g., add value.length to bytesSinceLastReport) and compute bytes
per second by multiplying by 1000 (not 100) when converting from milliseconds:
info.bytesPerSec = (bytesSinceLastReport / timeSinceLastReport) * 1000; ensure
bytesSinceLastReport is reset after reporting.
---
Nitpick comments:
In `@apps/desktop/src/bun/utils/config-manager.ts`:
- Around line 9-11: The BOOTSTRAP_PATH constant appears to refer to a
bootstrap.json that is no longer written by this module and is only read for
legacy migration (see code that reads BOOTSTRAP_PATH); either remove
BOOTSTRAP_PATH if it's truly unused, or clearly mark it as a read-only legacy
migration artifact by adding a brief comment above the constant (or rename to
LEGACY_BOOTSTRAP_PATH) explaining it's kept only for
backward-compatibility/one-time migration and not written by this code; update
any references to the new name if you rename it.
In `@apps/desktop/src/bun/utils/library-migration-manager.ts`:
- Around line 408-421: The stream copy Promise using
createReadStream/createWriteStream attaches both reader.on("error", reject) and
writer.on("error", reject) which can lead to race conditions and stray handlers;
update the Promise inside library-migration-manager.ts to, on reader error, call
writer.destroy(err) (or end/destroy) and remove the other event listeners before
rejecting, and similarly on writer error destroy the reader and cleanup
listeners, ensuring both streams are explicitly destroyed and listeners removed
so only one rejection occurs; reference the existing reader and writer variables
and the Promise wrapper around the pipe operation.
- Around line 358-362: The catch currently types error as any; change it to
unknown and narrow it safely (e.g., create a type guard isErrnoException(err):
err is NodeJS.ErrnoException or check typeof err === "object" && err !== null &&
"code" in err) before accessing error.code in the library-migration-manager.ts
catch block so the EXDEV branch uses a properly typed NodeJS.ErrnoException (or
guarded) value instead of any.
In `@apps/desktop/src/db/migrate.ts`:
- Around line 27-29: The call to loadPersistedConfig() is redundant because its
return value is unused and getDatabasePath()/getConfigDir() do not depend on it;
remove the loadPersistedConfig() invocation from migrate.ts to simplify the CLI,
leaving the calls to getDatabasePath() and ensureDir(getConfigDir()), and run
tests or search for any hidden side-effects of loadPersistedConfig() to confirm
nothing else relies on its execution before migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f70d0ec-3726-4073-a67b-9c74a70406eb
📒 Files selected for processing (22)
apps/desktop/.generated/catalog/catalog.jsonapps/desktop/src/bun/index.tsapps/desktop/src/bun/rpc.tsapps/desktop/src/bun/runtime.tsapps/desktop/src/bun/utils/config-manager.tsapps/desktop/src/bun/utils/download-manager.tsapps/desktop/src/bun/utils/library-migration-manager.tsapps/desktop/src/bun/utils/paths.tsapps/desktop/src/bun/utils/zim-manager.tsapps/desktop/src/db/index.tsapps/desktop/src/db/migrate.tsapps/desktop/src/db/queries.tsapps/desktop/src/mainview/App.tsxapps/desktop/src/mainview/components/Sidebar.tsxapps/desktop/src/mainview/hooks/useAppConfig.tsapps/desktop/src/mainview/lib/electroview.tsapps/desktop/src/mainview/lib/rpcClient.tsapps/desktop/src/mainview/lib/theme.tsapps/desktop/src/mainview/screens/Settings.tsxapps/desktop/src/mainview/store/index.tsapps/desktop/src/shared/rpc.tsapps/desktop/src/shared/types.ts
# Conflicts: # apps/desktop/src/bun/index.ts # apps/desktop/src/bun/rpc.ts # apps/desktop/src/bun/utils/paths.ts # apps/desktop/src/db/index.ts # apps/desktop/src/mainview/App.tsx # apps/desktop/src/mainview/lib/electroview.ts # apps/desktop/src/shared/rpc.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements