Fix/app clean up on logout - #408
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes extract authentication helpers, add coordinated logout resource cleanup, update deeplink and user-session handling, move product synchronization in the login flow, and add tests for the new behavior and runtime mocks. ChangesAuthentication and session lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 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
| try { | ||
| logger.debug({ tag: 'ANTIVIRUS', msg: '[Main] Setting up antivirus IPC handlers' }); | ||
| setupAntivirusIpc(); | ||
| const antivirusIpc = setupAntivirusIpc(); | ||
| removeAntivirusIpcHandlers = antivirusIpc.removeMessagesHandlers ?? null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent memory and IPC listener leaks on successive logins.
If this function is invoked multiple times across login sessions without a process restart, setupAntivirusIpc() will attach duplicate IPC handlers. Call cleanupAntivirusIpc() before setting up new handlers to clean up any existing ones.
🔧 Proposed fix
try {
logger.debug({ tag: 'ANTIVIRUS', msg: '[Main] Setting up antivirus IPC handlers' });
+ cleanupAntivirusIpc();
const antivirusIpc = setupAntivirusIpc();
removeAntivirusIpcHandlers = antivirusIpc.removeMessagesHandlers ?? null;📝 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.
| try { | |
| logger.debug({ tag: 'ANTIVIRUS', msg: '[Main] Setting up antivirus IPC handlers' }); | |
| setupAntivirusIpc(); | |
| const antivirusIpc = setupAntivirusIpc(); | |
| removeAntivirusIpcHandlers = antivirusIpc.removeMessagesHandlers ?? null; | |
| try { | |
| logger.debug({ tag: 'ANTIVIRUS', msg: '[Main] Setting up antivirus IPC handlers' }); | |
| cleanupAntivirusIpc(); | |
| const antivirusIpc = setupAntivirusIpc(); | |
| removeAntivirusIpcHandlers = antivirusIpc.removeMessagesHandlers ?? null; |
🤖 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/background-processes/antivirus/try-setup-antivirus-ipc-and-initialize.ts`
around lines 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.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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
AlexisMora
left a comment
AlexisMora
left a comment
There was a problem hiding this comment.
Please, stick to one relate set of changes per pr, this pr introduces not only "clean up on logout change" but rather product availability and so on
|
|
||
| export async function handleDeeplink({ url }: Props) { | ||
| try { | ||
| await waitForLogoutToFinish(); |
There was a problem hiding this comment.
This is not this function responsability, but rather on the callers, this way we have have the necessary logic separated and the function only does what it is described.
Also, why this scenario could happen? why the deeplink feature (which is for login) could be happening when logging out?
| @@ -23,7 +24,7 @@ export async function initializeCurrentUser() { | |||
| mnemonic: currentUser.mnemonic, | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
Why not just ConfigStore.set('userData', updatedUser); ?
There was a problem hiding this comment.
The app has some important classes that cache configuration values like this one. If only the configuration is updated, the cached values remain, and the user may be shown the previous user's information when logging in. This function updates both the configuration and the cache to prevent this proble
|
|
||
| let closeUserSessionResourcesInFlight: Promise<void> | undefined; | ||
|
|
||
| async function runCleanupStep({ step, task }: CleanupStepProps) { |
There was a problem hiding this comment.
| @@ -0,0 +1,20 @@ | |||
| import packageConfig from '../../../../package.json'; | |||
There was a problem hiding this comment.
This could be moved into backend/features/auth no? since you are just creaqting it (And generate tests for it)
| eventBus.emit('USER_AVAILABLE_PRODUCTS_UPDATED', userProducts); | ||
| } catch (error) { | ||
| logger.error({ | ||
| tag: 'PRODUCTS', | ||
| msg: 'Found difference in user products, storing and emitting update', | ||
| msg: 'Failed to resolve available user products', | ||
| error, |
There was a problem hiding this comment.
Why? the reason i did this originally was because we want to emit and propagate the changes only when the products change
There was a problem hiding this comment.
I agree. You are right, the intended behavior is to propagate only real product changes. I have updated the service to emit USER_AVAILABLE_PRODUCTS_UPDATED only when fetched products differ from stored products, and adjusted the tests accordingly.
| import { ValueObject } from './ValueObject'; | ||
|
|
||
| export class BucketEntry extends ValueObject<number> { | ||
| static readonly MAX_SIZE = ABSOLUTE_UPLOAD_FILE_SIZE_LIMIT; |
There was a problem hiding this comment.
Why not delete the property whatsoever and use ABSOLUTE_UPLOAD_FILE_SIZE_LIMIT
There was a problem hiding this comment.
Sorry about that—it was something the AI did that didn't make sense, and I forgot to remove it.
| return left(new Error('logout request was not successful')); | ||
| } | ||
| return right(response.data); | ||
| return right(true); |
There was a problem hiding this comment.
This function returns a Boolean value, and previously it was returning data that wasn't needed, which caused a type error.
| } | ||
|
|
||
| // Keep product flags in sync even if later UI steps fail. | ||
| await getUserAvailableProductsAndStore(); |
There was a problem hiding this comment.
Woulndt it be better to leave it where originally was? so if changes have happened there the renderer already can act accordingly
There was a problem hiding this comment.
I moved it earlier on purpose so product flags are refreshed even if a later login/UI step fails.
The renderer won’t miss this, because on subscribe it also receives the current stored products immediately, so it can still react correctly even if the event fired before mount.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/apps/main/auth/deeplink/handle-deeplink.test.ts (1)
89-96: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAwait completed logout before deeplink login.
handleDeeplink()starts withprocessDeeplink()andupdateCredentials()whilelogout()runs on the unmockeduser-logged-outpath and later callsresetConfig(), which resetsmnemonic,newToken, anduserData. Await a completion signal such aswaitForLogoutToFinish()before processing the deeplink, or update this test to call that dependency throughhandleDeeplink().🤖 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.test.ts` around lines 89 - 96, Update the handleDeeplink flow to await waitForLogoutToFinish before invoking processDeeplink or updateCredentials, ensuring logout’s resetConfig cannot overwrite the new credentials. Keep the existing dependency ordering and verify the test exercises this completion signal through handleDeeplink rather than only mocking it unused.src/apps/main/auth/handlers.ts (1)
36-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAttach error handling to
logout()before running it fire-and-forget.
closeUserSession()callsvoid logout(), which discards failures fromcloseUserSessionResources()or the server logout request. This can produce unhandled rejections during teardown. Uselogout().catch(...)or await it if this path should report logout failures.🤖 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/handlers.ts` at line 36, Update closeUserSession’s logout invocation to handle the promise rejection before running it fire-and-forget. Attach a catch handler to logout() that reports failures appropriately, or await logout() if the teardown path supports awaiting, while preserving the existing closeUserSession 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.
Inline comments:
In `@src/backend/features/auth/logout.ts`:
- Around line 9-28: Make logout single-flight by adding a module-level in-flight
promise covering the entire operation in logout(), including configuration save,
resource cleanup, event emission, server logout, and resetConfig. Return the
existing promise to concurrent callers, clear the stored promise in finally, and
add a test verifying concurrent logout calls share one operation and do not
duplicate side effects.
In `@src/backend/features/config/reset-config.test.ts`:
- Line 62: Update the mock call assertions in
src/backend/features/config/reset-config.test.ts#L62-L62 and
src/backend/features/auth/logout.test.ts#L82-L82 to compare calls against
one-element argument arrays: ['availableUserProducts'] and ['USER_LOGGED_OUT'],
respectively.
---
Outside diff comments:
In `@src/apps/main/auth/deeplink/handle-deeplink.test.ts`:
- Around line 89-96: Update the handleDeeplink flow to await
waitForLogoutToFinish before invoking processDeeplink or updateCredentials,
ensuring logout’s resetConfig cannot overwrite the new credentials. Keep the
existing dependency ordering and verify the test exercises this completion
signal through handleDeeplink rather than only mocking it unused.
In `@src/apps/main/auth/handlers.ts`:
- Line 36: Update closeUserSession’s logout invocation to handle the promise
rejection before running it fire-and-forget. Attach a catch handler to logout()
that reports failures appropriately, or await logout() if the teardown path
supports awaiting, while preserving the existing closeUserSession behavior.
🪄 Autofix
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: fb83d3bd-a842-4deb-81b0-c7eb9f027b58
📒 Files selected for processing (41)
src/apps/main/auth/close-user-session-resources.test.tssrc/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.test.tssrc/apps/main/auth/handlers.tssrc/apps/main/auth/service.test.tssrc/apps/main/interface.d.tssrc/apps/main/preload.d.tssrc/apps/main/realtime.tssrc/apps/main/remote-sync/service.test.tssrc/backend/features/auth/headers.test.tssrc/backend/features/auth/headers.tssrc/backend/features/auth/index.tssrc/backend/features/auth/logout.test.tssrc/backend/features/auth/logout.tssrc/backend/features/auth/user-session.test.tssrc/backend/features/auth/user-session.tssrc/backend/features/backup/download-backup.test.tssrc/backend/features/backup/download-backup.tssrc/backend/features/config/can-his-config-be-restore.test.tssrc/backend/features/config/can-his-config-be-restore.tssrc/backend/features/config/reset-config.test.tssrc/backend/features/config/reset-config.tssrc/backend/features/payments/services/get-user-available-products-and-store.test.tssrc/backend/features/payments/services/get-user-available-products-and-store.tssrc/context/shared/domain/value-objects/BucketEntry.tssrc/core/bootstrap/register-session-event-handlers.test.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.tssrc/infra/drive-server/services/backup/backup.service.test.tssrc/infra/drive-server/services/files/services/add-file-to-trash.test.tssrc/infra/drive-server/services/files/services/create-file.test.tssrc/infra/drive-server/services/files/services/create-thumbnail.test.tssrc/infra/drive-server/services/files/services/delete-file-content-from-bucket.test.tssrc/infra/drive-server/services/files/services/move-file.test.tssrc/infra/drive-server/services/files/services/override-file.test.tssrc/infra/drive-server/services/files/services/rename-file.test.tsvitest.setup.main.ts
💤 Files with no reviewable changes (3)
- src/apps/main/auth/service.test.ts
- src/apps/main/auth/close-user-session-resources.test.ts
- src/core/bootstrap/register-session-event-handlers.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/infra/drive-server/services/auth/auth.service.ts
- src/infra/drive-server/services/auth/auth.service.test.ts
- src/infra/drive-server/client/drive-server.client.instance.test.ts
- vitest.setup.main.ts
- src/apps/main/auth/close-user-session-resources.ts
- src/backend/features/payments/services/get-user-available-products-and-store.ts
- src/infra/drive-server/client/drive-server.client.instance.ts
- src/apps/main/auth/deeplink/initialize-current-user.test.ts
- src/apps/main/remote-sync/service.test.ts
| export async function logout() { | ||
| logger.debug({ msg: 'Logging out' }); | ||
|
|
||
| const user = getUser(); | ||
|
|
||
| if (user) { | ||
| const { uuid } = user; | ||
|
|
||
| saveConfig({ uuid }); | ||
| } | ||
|
|
||
| await closeUserSessionResources(); | ||
| eventBus.emit('USER_LOGGED_OUT'); | ||
|
|
||
| if (user) { | ||
| void driveServerModule.auth.logout(); | ||
| } | ||
|
|
||
| resetConfig(); | ||
| logger.debug({ msg: '[AUTH] User logged out' }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make logout() single-flight.
closeUserSessionResources() only shares cleanup at Line 20. A concurrent caller still saves configuration, emits USER_LOGGED_OUT, starts another server logout, and resets configuration at Lines 12-28. A later reset can affect a session that starts after the first logout completes.
Store one in-flight promise for the complete logout operation. Clear it in finally. Add a concurrent logout test.
Proposed fix
+let logoutInFlight: Promise<void> | undefined;
+
export async function logout() {
+ if (logoutInFlight) {
+ return logoutInFlight;
+ }
+
+ logoutInFlight = executeLogout();
+ try {
+ await logoutInFlight;
+ } finally {
+ logoutInFlight = undefined;
+ }
+}
+
+async function executeLogout() {
logger.debug({ msg: 'Logging out' });
// existing logout steps
}📝 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.
| export async function logout() { | |
| logger.debug({ msg: 'Logging out' }); | |
| const user = getUser(); | |
| if (user) { | |
| const { uuid } = user; | |
| saveConfig({ uuid }); | |
| } | |
| await closeUserSessionResources(); | |
| eventBus.emit('USER_LOGGED_OUT'); | |
| if (user) { | |
| void driveServerModule.auth.logout(); | |
| } | |
| resetConfig(); | |
| logger.debug({ msg: '[AUTH] User logged out' }); | |
| let logoutInFlight: Promise<void> | undefined; | |
| export async function logout() { | |
| if (logoutInFlight) { | |
| return logoutInFlight; | |
| } | |
| logoutInFlight = executeLogout(); | |
| try { | |
| await logoutInFlight; | |
| } finally { | |
| logoutInFlight = undefined; | |
| } | |
| } | |
| async function executeLogout() { | |
| logger.debug({ msg: 'Logging out' }); | |
| const user = getUser(); | |
| if (user) { | |
| const { uuid } = user; | |
| saveConfig({ uuid }); | |
| } | |
| await closeUserSessionResources(); | |
| eventBus.emit('USER_LOGGED_OUT'); | |
| if (user) { | |
| void driveServerModule.auth.logout(); | |
| } | |
| resetConfig(); | |
| logger.debug({ msg: '[AUTH] User logged out' }); | |
| } |
🤖 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/backend/features/auth/logout.ts` around lines 9 - 28, Make logout
single-flight by adding a module-level in-flight promise covering the entire
operation in logout(), including configuration save, resource cleanup, event
emission, server logout, and resetConfig. Return the existing promise to
concurrent callers, clear the stored promise in finally, and add a test
verifying concurrent logout calls share one operation and do not duplicate side
effects.
| resetConfig(); | ||
|
|
||
| // Then | ||
| calls(configDeleteMock).toContainEqual('availableUserProducts'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert one-argument mock call tuples.
calls(mock) returns argument arrays. Both assertions compare a bare string, so they cannot match the recorded calls.
src/backend/features/config/reset-config.test.ts#L62-L62: compare against['availableUserProducts'].src/backend/features/auth/logout.test.ts#L82-L82: compare against['USER_LOGGED_OUT'].
📍 Affects 2 files
src/backend/features/config/reset-config.test.ts#L62-L62(this comment)src/backend/features/auth/logout.test.ts#L82-L82
🤖 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/backend/features/config/reset-config.test.ts` at line 62, Update the mock
call assertions in src/backend/features/config/reset-config.test.ts#L62-L62 and
src/backend/features/auth/logout.test.ts#L82-L82 to compare calls against
one-element argument arrays: ['availableUserProducts'] and ['USER_LOGGED_OUT'],
respectively.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
081319e to
1b1e6c5
Compare
|
|
||
| await Promise.all([ | ||
| tryCatch( | ||
| () => DependencyInjectionUserProvider.clearUser(), |
There was a problem hiding this comment.
you could just do:
DependencyInjectionUserProvider.clearUser,as well for the other methods
| @@ -27,4 +27,8 @@ export class DependencyInjectionUserProvider { | |||
| DependencyInjectionUserProvider._user = user; | |||
| ConfigStore.set('userData', user); | |||
| } | |||
|
|
|||
| static clearUser() { | |||
| DependencyInjectionUserProvider._user = null; | |||
| } | |||
There was a problem hiding this comment.
Add a todo comment as well that references that ticket
| const userProducts = await PaymentsModule.getUserAvailableProducts({ | ||
| paymentsClientConfig, | ||
| }); | ||
| try { |
There was a problem hiding this comment.
Why these changes? i dont recall these steps throwing an Exception
There was a problem hiding this comment.
You're right, it's not necessary; I've removed it.
|
2 participants
What is Changed / Added
Why
Summary by CodeRabbit
New Features
Bug Fixes
Tests