Fix/app clean up on logout#408
Open
egalvis27 wants to merge 5 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds serialized logout cleanup, separates authentication helpers, synchronizes deeplink handling with logout, updates product and antivirus state flows, and adjusts shared providers, configuration APIs, client wiring, domain constants, and test mocks. ChangesAuthentication and session lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Logout
participant SessionCleanup
participant ConfigStore
participant DriveServer
Caller->>Logout: call logout()
Logout->>ConfigStore: save user and reset session data
Logout->>SessionCleanup: close user session resources
SessionCleanup-->>Logout: cleanup completes
Logout->>DriveServer: auth.logout()
Logout-->>Caller: resolve logout promise
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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: 4
🧹 Nitpick comments (1)
src/infra/drive-server/services/auth/auth.service.test.ts (1)
26-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the
logoutmethod.While test coverage exists for
login,access, andrefresh, thelogoutmethod inAuthServicelacks a dedicated test block. Consider adding adescribe('logout', ...)block to test the success and error states of the logout request, particularly since its return value was recently updated toright(true).🤖 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/infra/drive-server/services/auth/auth.service.test.ts` around lines 26 - 27, Add a dedicated describe('logout', ...) block in the AuthService test suite covering both successful and failing logout requests. Assert that the success case returns right(true), and verify the error case preserves the expected failure result while reusing the existing test setup and mocking patterns.
🤖 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/apps/main/auth/close-user-session-resources.ts`:
- Around line 40-45: Wrap the tray reset and widget hiding operations near lines
40-45 in runCleanupStep so synchronous UI errors are caught without aborting
subsequent cleanup. Also wrap widget destruction near lines 100-102 in
runCleanupStep, retrieving getWidget() inside that callback as required; apply
both changes in close-user-session-resources.ts.
In `@src/apps/main/auth/logout.test.ts`:
- Around line 102-103: Update the USER_LOGGED_OUT assertion in the logout test
to match the mock call structure returned by calls(eventBusEmitMock), wrapping
the event name in an argument array like the USER_AVAILABLE_PRODUCTS_UPDATED
assertion. Keep the existing event value unchanged.
In
`@src/apps/main/background-processes/antivirus/try-setup-antivirus-ipc-and-initialize.ts`:
- Around line 13-16: Update the initialization flow around setupAntivirusIpc()
to call cleanupAntivirusIpc() before registering new handlers, ensuring existing
IPC listeners are removed on repeated invocations while preserving the
subsequent removeAntivirusIpcHandlers assignment.
In `@src/apps/main/token-scheduler/TokenScheduler.ts`:
- Around line 10-12: Update TokenScheduler to maintain a static collection of
Job instances created by its schedule method. Store each non-null result from
nodeSchedule.scheduleJob in that collection, and change cancelAllJobs to cancel
only those tracked jobs instead of iterating over nodeSchedule.scheduledJobs
globally.
---
Nitpick comments:
In `@src/infra/drive-server/services/auth/auth.service.test.ts`:
- Around line 26-27: Add a dedicated describe('logout', ...) block in the
AuthService test suite covering both successful and failing logout requests.
Assert that the success case returns right(true), and verify the error case
preserves the expected failure result while reusing the existing test setup and
mocking patterns.
🪄 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
Run ID: ae67c103-a741-412d-a529-01161a2635fd
📒 Files selected for processing (32)
src/apps/main/auth/close-user-session-resources.tssrc/apps/main/auth/deeplink/handle-deeplink.test.tssrc/apps/main/auth/deeplink/handle-deeplink.tssrc/apps/main/auth/deeplink/initialize_current_user.tssrc/apps/main/auth/handlers.tssrc/apps/main/auth/headers.tssrc/apps/main/auth/logout.test.tssrc/apps/main/auth/logout.tssrc/apps/main/auth/refresh-token/create-token-schedule-with-retry.test.tssrc/apps/main/auth/refresh-token/refresh-token.test.tssrc/apps/main/auth/service.test.tssrc/apps/main/auth/service.tssrc/apps/main/auth/user-session.tssrc/apps/main/background-processes/antivirus/try-setup-antivirus-ipc-and-initialize.tssrc/apps/main/config.tssrc/apps/main/event-bus.tssrc/apps/main/remote-sync/service.tssrc/apps/main/token-scheduler/TokenScheduler.tssrc/apps/renderer/hooks/antivirus/useAntivirus.test.tsxsrc/apps/renderer/hooks/antivirus/useAntivirus.tsxsrc/apps/shared/dependency-injection/DependencyInjectionUserProvider.test.tssrc/apps/shared/dependency-injection/DependencyInjectionUserProvider.tssrc/backend/features/payments/services/get-user-available-products-and-store.test.tssrc/backend/features/payments/services/get-user-available-products-and-store.tssrc/backend/features/virtual-drive/services/daemon.service.test.tssrc/context/shared/domain/value-objects/BucketEntry.tssrc/core/bootstrap/register-session-event-handlers.tssrc/infra/drive-server/client/drive-server.client.instance.test.tssrc/infra/drive-server/client/drive-server.client.instance.tssrc/infra/drive-server/services/auth/auth.service.test.tssrc/infra/drive-server/services/auth/auth.service.tsvitest.setup.main.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/apps/main/auth/close-user-session-resources.ts`:
- Around line 40-50: Update the widget destruction logic in
closeUserSessionResources by replacing the later outer-scope widget reference
with a separate runCleanupStep named “destroy-widget”. Inside its task, retrieve
the widget via getWidget(), guard against a missing or destroyed instance, and
call destroy() on the current widget; keep the existing hide-widget-and-tray
cleanup scoped to its own widget reference.
🪄 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
Run ID: 72ac2a37-9a36-4757-a938-1d48f0fc9235
📒 Files selected for processing (1)
src/apps/main/auth/close-user-session-resources.ts
Comment on lines
+40
to
+50
| await runCleanupStep({ | ||
| step: 'hide-widget-and-tray', | ||
| task: () => { | ||
| resetTrayStatus('IDLE'); | ||
| const widget = getWidget(); | ||
|
|
||
| if (widget && !widget.isDestroyed()) { | ||
| widget.hide(); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix scoping to prevent ReferenceError on widget destruction.
Moving const widget = getWidget(); into this closure correctly guards the hiding logic but removes widget from the outer scope. This will either cause a ReferenceError later in the file at line 105 (if (widget && !widget.isDestroyed())) or silently skip destruction if an outer variable was left undefined.
As noted in a previous review, to safely destroy the widget, its destruction logic at lines 105-107 must also be wrapped in a cleanup step that retrieves the widget reference again.
🔧 Proposed fix for lines 105-107
Please replace lines 105-107 with the following:
await runCleanupStep({
step: 'destroy-widget',
task: () => {
const currentWidget = getWidget();
if (currentWidget && !currentWidget.isDestroyed()) {
currentWidget.destroy();
}
},
});Run the following script to verify if widget is still declared in the outer scope:
#!/bin/bash
# Description: Check if `widget` is declared in the outer scope.
rg -n 'widget' src/apps/main/auth/close-user-session-resources.ts🤖 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/apps/main/auth/close-user-session-resources.ts` around lines 40 - 50,
Update the widget destruction logic in closeUserSessionResources by replacing
the later outer-scope widget reference with a separate runCleanupStep named
“destroy-widget”. Inside its task, retrieve the widget via getWidget(), guard
against a missing or destroyed instance, and call destroy() on the current
widget; keep the existing hide-widget-and-tray cleanup scoped to its own widget
reference.
|
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.
What is Changed / Added
Why
Summary by CodeRabbit