Fix/factory reset scope - #369
Open
Nuri1977 wants to merge 3 commits into
Open
Conversation
- clear browser storage, caches, databases, notebooks, and DuckLake state - remove AI settings, MCP configuration, Agent Skills, and managed runtimes - stop active agents, tasks, Flowfile, MCP, DuckDB, and database connections - safely validate project and managed Rosetta paths before deletion - delete and verify DBT Studio credentials without repeated macOS prompts - prevent late database writes from recreating state during reset - restart the app only after successful cleanup and verification - update reset warnings, tests, Plan 54, and factory reset documentation
📝 WalkthroughWalkthroughFactory reset now receives the Electron session, cancels active work, shuts down databases, clears credentials and application data with verification, relaunches after success, and prevents reset-dialog dismissal or renderer refetches during completion. ChangesFactory reset lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Renderer
participant SettingsIPCHandler
participant SettingsService
participant AgentService
participant DuckDBBootstrap
participant MainDatabaseService
participant SecureStorageService
participant ElectronApp
Renderer->>SettingsIPCHandler: invoke factory reset with session
SettingsIPCHandler->>SettingsService: resetFactorySettings(session)
SettingsService->>AgentService: cancel active agents and bridge requests
SettingsService->>DuckDBBootstrap: beginFactoryReset()
SettingsService->>MainDatabaseService: beginFactoryReset()
SettingsService->>SecureStorageService: clearAllCredentials()
SettingsService->>ElectronApp: relaunch and exit
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/services/duckdb.service.ts (1)
242-247: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject DuckDB reinitialization while factory reset is active.
reinitialize()unconditionally clearsDuckDBBootstrap.factoryResetInProgress, whilebeginFactoryReset()sets the same flag and waits for shutdown. Asettings:duckdb:reinitializecall mid-reset can make the reset stop rejectinginitialize(), allowing DuckDB to be reopened while reset cleanup is still in progress.🔒 Possible guard
static async reinitialize(options?: { dropExisting?: boolean; }): Promise<DuckDBMetadataPayload> { + if (this.factoryResetInProgress) { + throw new Error('DuckDB is unavailable during factory reset'); + } // Close all connections await this.shutdown(); - this.factoryResetInProgress = false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/services/duckdb.service.ts` around lines 242 - 247, Update DuckDBBootstrap.reinitialize to reject immediately when factoryResetInProgress is already true, before calling shutdown or modifying the flag. Preserve the existing reinitialization flow for non-reset calls, and ensure beginFactoryReset remains the owner of clearing the reset state after cleanup completes.
🧹 Nitpick comments (3)
tests/unit/main/services/secureStorage.service.test.ts (1)
36-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the untested failure branches.
The suite covers success, per-account failure, serialization, and macOS not-found handling, but doesn't exercise the non-macOS "remaining credentials after deletion" verification throw or the macOS generic-deletion-error throw. Since this code runs during an irreversible factory reset, covering these paths would increase confidence in the failure messaging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/main/services/secureStorage.service.test.ts` around lines 36 - 139, Add tests for the uncovered failure branches of SecureStorageService.clearAllCredentials: verify that on non-macOS it throws when credentials remain after deletion, and that on macOS it throws when generic service deletion fails with an error other than the handled not-found case. Assert the expected failure messages while preserving the existing success and handled-error coverage.src/main/services/secureStorage.service.ts (1)
65-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOriginal error context is discarded when re-throwing.
Both the non-macOS aggregate-failure throw (Line 90-92) and the macOS
catch(Line 118) replace the real underlying error with a generic message, losing the original message/stack. Since this only runs during factory reset — a destructive, hard-to-retry operation — the generic message surfaced toperformFactoryReset'sgetSanitizedResetDetailmakes production failures much harder to diagnose.♻️ Preserve original error as `cause`
} catch (error) { if (isMacKeychainItemNotFound(error)) return; - throw new Error('Failed to delete secure credential accounts'); + throw new Error('Failed to delete secure credential accounts', { + cause: error, + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/services/secureStorage.service.ts` around lines 65 - 124, Preserve underlying error context in both failure paths: update the aggregate failure thrown by clearAllCredentials and the non-not-found error rethrown by clearMacKeychainCredentials to include the original error as its cause. Track or retain the relevant deletion error while preserving the existing failure counts and generic messages, and keep isMacKeychainItemNotFound handling unchanged.tests/unit/main/services/settings.service.test.ts (1)
144-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the full cleanup fan-out, not just DuckDB/session/credentials/fs.
The success test mocks
TaskManagerService.cancelAll,AgentEditorBridgeService.resetForFactoryReset,FlowfileService.stop,MCPClientManager.disconnectAll,ConnectorsService.cleanupBigQueryKeyFiles, and the duckLake connection/adapter disconnects, but never asserts they were invoked. As currently written, a regression that silently drops one of these cleanup calls fromresetFactorySettingswould not be caught by this suite.✅ Example additions to the success-path test
expect(SecureStorageService.clearAllCredentials).toHaveBeenCalled(); + expect(TaskManagerService.cancelAll).toHaveBeenCalled(); + expect(AgentEditorBridgeService.resetForFactoryReset).toHaveBeenCalled(); + expect(FlowfileService.stop).toHaveBeenCalled(); + expect(MCPClientManager.disconnectAll).toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/main/services/settings.service.test.ts` around lines 144 - 188, Expand the success-path test for SettingsService.resetFactorySettings to assert every mocked cleanup dependency is invoked, including TaskManagerService.cancelAll, AgentEditorBridgeService.resetForFactoryReset, FlowfileService.stop, MCPClientManager.disconnectAll, ConnectorsService.cleanupBigQueryKeyFiles, and the duckLake connection and adapter disconnect methods. Keep the existing assertions and verify each call with the expected arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/services/settings.service.ts`:
- Around line 606-610: Update the factory-reset failure handling in the catch
block around resumeAfterFailedFactoryReset() so the thrown error explicitly
tells the user that the application must be restarted after teardown has begun.
Preserve the sanitized stage detail and existing cleanup call, and ensure the
UI-facing message includes a clear restart-required instruction.
- Around line 752-762: The shutdown operations in performFactoryReset are
unbounded and can leave the reset modal stuck indefinitely. Add or reuse a
shared withTimeout/Promise.race helper to bound FlowfileService.stop,
MCPClientManager.disconnectAll, DuckLakeConnectionManager.disconnectAll,
CatalogAdapterFactory.disconnectAll, DuckDBBootstrap.beginFactoryReset, and
MainDatabaseService.beginFactoryReset, propagating a descriptive timeout failure
through the existing reset error handling.
- Around line 625-639: Update getSanitizedResetDetail to allow the macOS
secure-storage verification message prefix “Failed to verify secure credential
account removal” in its safe-prefix list, preserving the existing sanitization
behavior and the current “Failed to delete ” match.
In `@src/main/services/taskManager.service.ts`:
- Around line 110-132: Update TaskManager.cancelAll() to avoid marking or
discarding running tasks that have no registered canceller; preserve those tasks
for completion tracking and surface the inability to cancel so the factory-reset
flow can wait or fail. Keep cancellation, status updates, broadcasts, and
cleanup for tasks with a registered canceller, and align the behavior with
cancel(id).
In `@tests/unit/main/services/secureStorage.service.test.ts`:
- Around line 1-22: Move the SecureStorageService import above all jest.mock
declarations in this test file to satisfy the import/first ESLint rule. Keep the
existing mock definitions and test behavior unchanged.
In `@tests/unit/main/services/settings.service.test.ts`:
- Around line 98-102: Move the SecureStorageService, DuckDBBootstrap, and
MainDatabaseService imports to the top import block in
tests/unit/main/services/settings.service.test.ts, before all non-import
statements, preserving their existing paths and usage.
---
Outside diff comments:
In `@src/main/services/duckdb.service.ts`:
- Around line 242-247: Update DuckDBBootstrap.reinitialize to reject immediately
when factoryResetInProgress is already true, before calling shutdown or
modifying the flag. Preserve the existing reinitialization flow for non-reset
calls, and ensure beginFactoryReset remains the owner of clearing the reset
state after cleanup completes.
---
Nitpick comments:
In `@src/main/services/secureStorage.service.ts`:
- Around line 65-124: Preserve underlying error context in both failure paths:
update the aggregate failure thrown by clearAllCredentials and the non-not-found
error rethrown by clearMacKeychainCredentials to include the original error as
its cause. Track or retain the relevant deletion error while preserving the
existing failure counts and generic messages, and keep isMacKeychainItemNotFound
handling unchanged.
In `@tests/unit/main/services/secureStorage.service.test.ts`:
- Around line 36-139: Add tests for the uncovered failure branches of
SecureStorageService.clearAllCredentials: verify that on non-macOS it throws
when credentials remain after deletion, and that on macOS it throws when generic
service deletion fails with an error other than the handled not-found case.
Assert the expected failure messages while preserving the existing success and
handled-error coverage.
In `@tests/unit/main/services/settings.service.test.ts`:
- Around line 144-188: Expand the success-path test for
SettingsService.resetFactorySettings to assert every mocked cleanup dependency
is invoked, including TaskManagerService.cancelAll,
AgentEditorBridgeService.resetForFactoryReset, FlowfileService.stop,
MCPClientManager.disconnectAll, ConnectorsService.cleanupBigQueryKeyFiles, and
the duckLake connection and adapter disconnect methods. Keep the existing
assertions and verify each call with the expected arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0440fd45-51e8-42a3-85e7-3f3a360b4065
📒 Files selected for processing (15)
src/main/ipcHandlers/settings.ipcHandlers.tssrc/main/services/agent.service.tssrc/main/services/ai/agentEditorBridge.service.tssrc/main/services/duckdb.service.tssrc/main/services/mainDatabase.service.tssrc/main/services/secureStorage.service.tssrc/main/services/settings.service.tssrc/main/services/taskManager.service.tssrc/renderer/components/modals/resetFactoryModal/index.tsxsrc/renderer/components/settings/AboutSettings.tsxsrc/renderer/controllers/settings.controller.tstests/unit/main/ipcHandlers/settings.ipcHandlers.test.tstests/unit/main/services/secureStorage.service.test.tstests/unit/main/services/settings.service.test.tstests/unit/renderer/components/ResetFactoryModal.test.tsx
- require an app restart after partial teardown failures - sanitize macOS credential verification failure details - add timeouts for factory-reset shutdown operations - preserve running tasks without registered cancellers - fail reset safely when background tasks cannot be cancelled - fix import-first lint violations in service tests - add regression tests for timeout and cancellation behavior
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/main/services/taskManager.service.test.ts (1)
1-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the "canceller throws" branch too.
The new test covers the no-canceller case but not the case where a registered canceller throws (Lines 123-128 in
taskManager.service.ts), which also incrementsuncancelledTaskCountand is relied on by the factory-reset abort path.➕ Suggested addition
it('counts a task whose canceller throws as uncancelled', () => { TaskManagerService.create({ id: 'throwing', type: 'test', label: 'Throwing task', cancellable: true, }); TaskManagerService.registerCanceller('throwing', () => { throw new Error('boom'); }); expect(TaskManagerService.cancelAll()).toBe(1); expect( TaskManagerService.list().find(({ id }) => id === 'throwing')?.status, ).toBe('running'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/main/services/taskManager.service.test.ts` around lines 1 - 46, Add a test alongside the existing TaskManagerService.cancelAll coverage that registers a canceller which throws, then assert cancelAll returns 1 and the task remains in running status. Use the existing create, registerCanceller, cancelAll, and list APIs, and retain the current cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/main/services/taskManager.service.test.ts`:
- Around line 1-46: Add a test alongside the existing
TaskManagerService.cancelAll coverage that registers a canceller which throws,
then assert cancelAll returns 1 and the task remains in running status. Use the
existing create, registerCanceller, cancelAll, and list APIs, and retain the
current cleanup behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 847201ae-a5e7-4f25-b808-423ce09d3fc8
📒 Files selected for processing (5)
src/main/services/settings.service.tssrc/main/services/taskManager.service.tstests/unit/main/services/secureStorage.service.test.tstests/unit/main/services/settings.service.test.tstests/unit/main/services/taskManager.service.test.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
Bug Fixes
Tests