Fix/app clean up on logout - #408
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change introduces serialized logout and session-resource cleanup, separates authentication helpers, synchronizes deeplink handling with logout, moves product synchronization earlier in login, and updates antivirus availability handling. Supporting tests, mocks, configuration APIs, and runtime helpers are also added or revised. ChangesAuthentication and session lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Deeplink
participant Logout
participant SessionResources
participant DriveServer
Deeplink->>Logout: waitForLogoutToFinish()
Logout->>SessionResources: complete guarded cleanup
Deeplink->>Deeplink: process deeplink
Deeplink->>DriveServer: refresh current user
Deeplink-->>Deeplink: update credentials and login state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 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
| 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.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apps/main/auth/deeplink/handle-deeplink.ts (1)
18-21: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPotential indefinite block if logout cleanup hangs.
waitForLogoutToFinish()awaitslogoutInFlightwith no timeout. If any guarded cleanup step incloseUserSessionResources(e.g., antivirus IPC, virtual-drive teardown) never settles rather than rejecting, this await blocks forever, permanently stalling new deeplink logins.🔍 Verify cleanup-step timeout guarantees
#!/bin/bash ast-grep run --pattern 'function runCleanupStep($$$) { $$$ }' --lang typescript src/apps/main/auth/close-user-session-resources.ts cat -n src/apps/main/auth/logout.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/deeplink/handle-deeplink.ts` around lines 18 - 21, Update handleDeeplink and the waitForLogoutToFinish flow so a logout cleanup that never settles cannot block deeplink login indefinitely. Add a bounded timeout around waiting for logoutInFlight, while preserving normal completion and rejection handling and allowing the deeplink flow to proceed after the timeout.
🧹 Nitpick comments (1)
src/apps/main/auth/close-user-session-resources.test.ts (1)
98-164: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for guarded-cleanup resilience.
executeCloseUserSessionResourceswraps each step inrunCleanupStep, implying failures in one step shouldn't block subsequent steps (e.g., antivirus shutdown failing shouldn't prevent opening the auth window). No test here exercises that guarantee — consider adding a case where one step's mock rejects/throws and asserting later steps still execute.🤖 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.test.ts` around lines 98 - 164, Add a test for executeCloseUserSessionResources where an individual cleanup mock, such as shutdownMock, rejects or throws; assert that subsequent cleanup steps, including createAuthWindowMock and final resource cleanup, still execute. Keep the existing successful and concurrent-deduplication coverage unchanged.
🤖 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.test.ts`:
- Around line 109-163: Ensure shared Vitest mocks are reset between tests in
closeUserSessionResources tests, either by enabling the project’s existing
global clear-mocks configuration or adding explicit mock clearing in the test
setup. Preserve the existing assertions, especially the mid-flight
createAuthWindowMock check in “deduplicates concurrent cleanup requests,” so it
starts with no calls from the preceding test.
In `@src/apps/main/auth/deeplink/initialize-current-user.test.ts`:
- Around line 4-16: Fix the Prettier formatting in the hoisted mock declaration
at the top of initialize-current-user.test.ts, including the destructuring and
object literal layout around refreshMock, updateCredentialsMock, configGetMock,
and updateUserMock. Apply the repository’s standard formatter without changing
behavior.
In `@src/apps/main/remote-sync/service.test.ts`:
- Line 8: Reformat the long `vi.hoisted` mock declaration and the corresponding
line 56 declaration to comply with the 120-character max-len rule. Break the
destructured names and object properties across multiple lines while preserving
the existing mocks and behavior.
- Around line 71-112: Update the cancelPendingRemoteSync test to retrieve the
latest debounceMock result rather than index 0, ensuring it asserts against the
debounced function created by the current serviceModule import. Keep the
existing cancel invocation and expectation unchanged.
In `@src/core/bootstrap/register-session-event-handlers.test.ts`:
- Line 89: Remove the unused setBoundsOfWidgetByPathSpy declaration from the
test setup in register-session-event-handlers.test.ts; do not add an assertion
unless the test explicitly needs to verify that call.
---
Outside diff comments:
In `@src/apps/main/auth/deeplink/handle-deeplink.ts`:
- Around line 18-21: Update handleDeeplink and the waitForLogoutToFinish flow so
a logout cleanup that never settles cannot block deeplink login indefinitely.
Add a bounded timeout around waiting for logoutInFlight, while preserving normal
completion and rejection handling and allowing the deeplink flow to proceed
after the timeout.
---
Nitpick comments:
In `@src/apps/main/auth/close-user-session-resources.test.ts`:
- Around line 98-164: Add a test for executeCloseUserSessionResources where an
individual cleanup mock, such as shutdownMock, rejects or throws; assert that
subsequent cleanup steps, including createAuthWindowMock and final resource
cleanup, still execute. Keep the existing successful and
concurrent-deduplication coverage unchanged.
🪄 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: d4cca27b-d805-4c4b-a39b-166f2b9dae20
📒 Files selected for processing (7)
src/apps/main/auth/close-user-session-resources.test.tssrc/apps/main/auth/deeplink/handle-deeplink.test.tssrc/apps/main/auth/deeplink/handle-deeplink.tssrc/apps/main/auth/deeplink/initialize-current-user.test.tssrc/apps/main/auth/deeplink/initialize-current-user.tssrc/apps/main/remote-sync/service.test.tssrc/core/bootstrap/register-session-event-handlers.test.ts
| beforeEach(async () => { | ||
| vi.resetModules(); | ||
| startRemoteSyncMock.mockResolvedValue(undefined); | ||
| isInitialSyncReadyMock.mockReturnValue(true); | ||
| debounceMock.mockImplementation((fn: () => Promise<void>) => { | ||
| const debounced = vi.fn(async () => fn()) as unknown as DebouncedFunc<() => Promise<void>>; | ||
| debounced.cancel = vi.fn(); | ||
| return debounced; | ||
| }); | ||
|
|
||
| debounceSpy.mockImplementation((fn: (...args: unknown[]) => unknown) => { | ||
| const debounced = vi.fn(async () => fn()) as unknown as DebouncedFunc<() => Promise<unknown>>; | ||
| debounced.cancel = vi.fn(); | ||
| return debounced; | ||
| }); | ||
| eventBusEmitSpy.mockImplementation(() => true); | ||
| broadcastToWindowsSpy.mockImplementation(() => undefined); | ||
| initialSyncReadySpy.mockReturnValue(true); | ||
| setInitialSyncStateSpy.mockImplementation(() => true); | ||
| serviceModule = await import('./service'); | ||
| }); | ||
|
|
||
| it('should start the remote sync through the manager', async () => { | ||
| await serviceModule.startRemoteSync(); | ||
|
|
||
| expect(startRemoteSyncMock).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('should resynchronize remote changes and emit the completion event', async () => { | ||
| await serviceModule.resyncRemoteSync(); | ||
|
|
||
| call(debounceSpy).toStrictEqual([expect.any(Function), 3_000]); | ||
| expect(startRemoteSyncMock).toHaveBeenCalledTimes(1); | ||
| expect(eventBusEmitSpy).toHaveBeenCalledWith('REMOTE_CHANGES_SYNCHED'); | ||
| }); | ||
|
|
||
| it('should cancel the pending remote sync', () => { | ||
| serviceModule.cancelPendingRemoteSync(); | ||
|
|
||
| const debouncedFunction = debounceMock.mock.results[0]?.value as { cancel?: () => void }; | ||
| expect(debouncedFunction.cancel).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)service\.test\.ts$|(^|/)service\.ts$|package\.json$|vitest|vite\.config' || true
echo "== target file excerpt =="
if [ -f src/apps/main/remote-sync/service.test.ts ]; then
nl -ba src/apps/main/remote-sync/service.test.ts | sed -n '1,180p'
fi
echo "== related service excerpt =="
if [ -f src/apps/main/remote-sync/service.ts ]; then
ast-grep outline src/apps/main/remote-sync/service.ts || true
nl -ba src/apps/main/remote-sync/service.ts | sed -n '1,220p'
fi
echo "== vi mock / spies definitions =="
rg -n "vi\.mock\('lodash'|debounceModule|debounceMock|debounceSpy|initialSyncReadySpy|setInitialSyncStateSpy|resyncRemoteSync|cancelPendingRemoteSync|startRemoteSync" src/apps/main/remote-sync/service.test.ts src/apps/main/remote-sync/service.ts 2>/dev/null || trueRepository: internxt/drive-desktop-linux
Length of output: 1945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository presence =="
pwd
git rev-parse --show-toplevel 2>/dev/null || true
git status --short 2>/dev/null | head -50 || true
echo "== files =="
git ls-files | rg '(^|/)service\.test\.ts$|(^|/)service\.ts$|package\.json$|vitest|vite\.config' || true
echo "== target file excerpt =="
if [ -f src/apps/main/remote-sync/service.test.ts ]; then
nl -ba src/apps/main/remote-sync/service.test.ts | sed -n '1,180p'
fi
echo "== related service excerpt =="
if [ -f src/apps/main/remote-sync/service.ts ]; then
ast-grep outline src/apps/main/remote-sync/service.ts || true
nl -ba src/apps/main/remote-sync/service.ts | sed -n '1,220p'
fi
echo "== vi mock / spies definitions =="
rg -n "vi\.mock\('lodash'|debounceModule|debounceMock|debounceSpy|initialSyncReadySpy|setInitialSyncStateSpy|resyncRemoteSync|cancelPendingRemoteSync|startRemoteSync" src/apps/main/remote-sync/service.test.ts src/apps/main/remote-sync/service.ts 2>/dev/null || trueRepository: internxt/drive-desktop-linux
Length of output: 1945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository presence =="
pwd
git rev-parse --show-toplevel 2>/dev/null || true
git status --short 2>/dev/null | head -50 || true
echo "== files =="
git ls-files | rg '(^|/)service\.test\.ts$|(^|/)service\.ts$|package\.json$|vitest|vite\.config' || true
echo "== target file excerpt =="
if [ -f src/apps/main/remote-sync/service.test.ts ]; then
nl -ba src/apps/main/remote-sync/service.test.ts | sed -n '1,180p'
fi
echo "== related service excerpt =="
if [ -f src/apps/main/remote-sync/service.ts ]; then
ast-grep outline src/apps/main/remote-sync/service.ts || true
nl -ba src/apps/main/remote-sync/service.ts | sed -n '1,220p'
fi
echo "== vi mock / spies definitions =="
rg -n "vi\.mock\('lodash'|debounceModule|debounceMock|debounceSpy|initialSyncReadySpy|setInitialSyncStateSpy|resyncRemoteSync|cancelPendingRemoteSync|startRemoteSync" src/apps/main/remote-sync/service.test.ts src/apps/main/remote-sync/service.ts 2>/dev/null || trueRepository: internxt/drive-desktop-linux
Length of output: 1945
Fix the stale result index in the cancel test.
beforeEach re-imports the service each test, but debounceMock.mock.results keeps the previous test’s debounce(...) call result. debounceMock.mock.results[0] can therefore assert on the wrong debounced function instance. Use the latest result for this call, e.g. debounceMock.mock.results.at(-1)?.value as { cancel?: () => void }.
🤖 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/remote-sync/service.test.ts` around lines 71 - 112, Update the
cancelPendingRemoteSync test to retrieve the latest debounceMock result rather
than index 0, ensuring it asserts against the debounced function created by the
current serviceModule import. Keep the existing cancel invocation and
expectation unchanged.
|
1 participant
What is Changed / Added
Why
Summary by CodeRabbit
Bug Fixes
User Experience