Add keepalive option in client.deactivate #928
Conversation
WalkthroughThe changes in this pull request modify the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC
Client->>Client: deactivate(options)
alt keepalive is true
Client->>RPC: deactivateClient()
else keepalive is false
Client->>Client: enqueue deactivation
end
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
packages/sdk/src/client/client.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/sdk".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "packages/sdk/.eslintrc.js » ../../.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/sdk/src/client/client.ts (1)
290-303: Consider implementing a dedicated cleanup handler.To make the deactivation process more robust, consider:
- Implementing a dedicated cleanup handler that manages both immediate and queued deactivation paths
- Using
window.addEventListener('unload', ...)to automatically trigger immediate deactivation- Ensuring consistent state management and logging across all cleanup scenarios
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk/src/client/client.ts(2 hunks)
🔇 Additional comments (1)
packages/sdk/src/client/client.ts (1)
238-245: LGTM! Well-chosen solution for handling page unload scenarios.
The addition of keepalive: true to the fetch configuration is an excellent choice that ensures deactivation requests can complete even when the page is being unloaded, addressing the core issue of incomplete deactivation processes.
hackerwins
changed the title
hackerwins
force-pushed
the
deactivate-beforeunload
branch
from
69e21c5 to
7aef309
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/sdk/src/client/client.ts (2)
239-246: Consider adding type safety for fetch configurationWhile the implementation is correct, we can improve type safety by explicitly typing the init parameter.
- fetch: (input, init) => { + fetch: (input: RequestInfo | URL, init?: RequestInit) => { const newInit = { ...init, keepalive: this.keepalive, - }; + } as RequestInit;
316-321: Consider enhancing keepalive path with loggingThe keepalive path could benefit from additional logging to aid debugging.
if (options.keepalive) { this.keepalive = true; + logger.debug(`[DC] c:"${this.getKey()}" deactivating with keepalive enabled`); const resp = task(); this.keepalive = false; + logger.debug(`[DC] c:"${this.getKey()}" keepalive flag reset`); return resp; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk/src/client/client.ts(3 hunks)
🔇 Additional comments (3)
packages/sdk/src/client/client.ts (3)
202-202: LGTM: Well-placed keepalive property
The private keepalive property is appropriately placed and follows TypeScript conventions.
290-294: LGTM: Clear and informative documentation
The documentation clearly explains the purpose and use case of the keepalive option.
301-314: LGTM: Well-structured task implementation
The task implementation is well-organized and handles errors appropriately.
What this PR does / why we need it?
Since introducing version vectors, we have been using them for GC.
However, if the client deactivate process is not properly completed when a browser in the middle of editing is closed, the version vector table in the database retains version vectors for users whose deactivate process was incomplete.
As a result, unnecessary values remain in the minimum version vector, causing GC to fail to operate.
Therefore, we needed to enable functionality that allows the client to request the user's deactivate process when the browser is refreshed or closed. To address this, we made
rpcClient.DeactivateClientcallable.Since the execution of the browser's
beforeunloadevent handler is not guaranteed, I conducted a hit rate test with three comparison methods:navigator.sendBeaconfetchrpcClient.DeactivateClientBelow are the hit rate comparison results. In summary,
rpcClient.DeactivateClientdemonstrated the highest hit rate.As a result, without adding extra code, we configured the RPC creation process to use the
keepaliveoption infetch.Test Results
navigator.sendBeacon
fetch + keepalive
rpc.DeactivateClient (client.deactivate({fireImmediately: true}))
Any background context you want to provide?
What are the relevant tickets?
Fixes #
Checklist
Summary by CodeRabbit
New Features
deactivatefunctionality to allow immediate execution based on optional parameters.Bug Fixes
deactivatemethod to ensure correct execution with new parameters.