Add support for document re-attachment with same client #996
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant Document
participant Server
User->>Client: attach(Document)
Client->>Server: Attach request
Server-->>Client: Attach response
Client->>Document: Set status to Attached
User->>Document: Update content
User->>Client: sync()
Client->>Server: Sync changes
Server-->>Client: Sync response
User->>Client: detach(Document)
Client->>Document: Set status to Detached
Client->>Document: reset()
User->>Client: attach(Document)
Client->>Server: Attach request
Server-->>Client: Attach response
Client->>Document: Set status to Attached
Assessment against linked issues
Possibly related PRs
Poem
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/document/document.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. 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. packages/sdk/test/integration/document_test.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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (3)
packages/sdk/test/integration/document_test.ts (3)
949-953: Consider removing redundant sync operation.The sync operation on line 950 might be redundant since the document was already synced on line 945. Consider whether this additional sync is necessary for the test scenario.
// 2. Detach document and sync before detaching - await client.sync(); await client.detach(doc); assert.equal(doc.getStatus(), DocStatus.Detached);
970-975: Consider removing redundant sync operation in cleanup.The sync operation on line 971 might be redundant since the document was already synced on line 967. Consider whether this additional sync is necessary for cleanup.
// 6. Final cleanup - await client.sync(); await client.detach(doc); assert.equal(doc.getStatus(), DocStatus.Detached); await client.deactivate();
927-976: Consider adding explicit verification of the reset functionality.While this integration test effectively verifies the end-to-end re-attachment behavior, it doesn't explicitly test that the
reset()method is being called or verify its effects. Consider adding assertions or logging to confirm that the document's internal state is properly reset during detachment.This could help ensure that the test is specifically validating the new reset functionality rather than just testing that re-attachment works by coincidence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/sdk/src/client/client.ts(1 hunks)packages/sdk/src/document/document.ts(1 hunks)packages/sdk/test/integration/document_test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/sdk/test/integration/document_test.ts (2)
packages/sdk/test/integration/integration_helper.ts (2)
toDocKey(30-35)testRPCAddr(8-8)packages/sdk/src/yorkie.ts (2)
SyncMode(29-29)DocStatus(45-45)
packages/sdk/src/document/document.ts (4)
packages/sdk/src/document/change/change_id.ts (1)
InitialChangeID(260-265)packages/sdk/src/document/change/checkpoint.ts (1)
InitialCheckpoint(108-108)packages/sdk/src/document/crdt/root.ts (1)
CRDTRoot(78-393)packages/sdk/src/yorkie.ts (1)
DocStatus(45-45)
🔇 Additional comments (9)
packages/sdk/src/document/document.ts (4)
694-698: LGTM! Clean reset of change tracking state.The method correctly resets all change tracking state to initial values using the appropriate constants. This ensures a clean slate for document reattachment.
700-703: LGTM! Proper CRDT state reset.The method correctly resets the CRDT state by creating a fresh root using
CRDTRoot.create()and clearing any existing clone. This ensures the document's data structure is properly reinitialized.
705-708: LGTM! Comprehensive client state reset.The method correctly clears all client-related state including online clients and presence information. This is essential for clean document reattachment as presence data is session-specific.
710-723: LGTM! Well-designed comprehensive reset method.The method provides a complete reset by orchestrating all state clearing operations. The
@internalannotation correctly indicates this is not part of the public API, and the comprehensive JSDoc documentation clearly explains its purpose for document reattachment.packages/sdk/src/client/client.ts (1)
543-543: LGTM! Correct integration of document reset functionality.The
doc.reset()call is properly placed after setting the document status to detached and only for non-removed documents. This ensures the document's internal state is cleared and ready for potential reattachment, which addresses the original issue described in the PR objectives.packages/sdk/test/integration/document_test.ts (4)
927-939: LGTM: Test setup follows established patterns.The test setup is well-structured with proper type definitions, unique document key generation, and client initialization.
940-948: LGTM: First attachment cycle is comprehensive.The initial attach-update-sync cycle properly verifies both document status and content, establishing a good baseline for the re-attachment test.
957-962: LGTM: Re-attachment verification is thorough.The re-attachment process properly verifies that the document can be attached again and maintains its previous content state.
963-969: LGTM: Post-reattachment functionality verification.The test correctly verifies that the document remains fully functional after re-attachment by performing updates and confirming the changes persist.
| // 3. Wait for server state to settle | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace arbitrary delay with more robust waiting mechanism.
The 500ms delay appears to be a workaround for server state synchronization. Consider using a more robust approach such as polling the server state or using explicit synchronization mechanisms rather than relying on an arbitrary timeout.
This approach could be brittle in different environments (slower CI systems, network latency, etc.) and may cause flaky tests.
- // 3. Wait for server state to settle
- await new Promise((resolve) => setTimeout(resolve, 500));
+ // 3. Ensure server state is consistent (if needed)
+ // Note: The reset() method should handle internal state cleanupCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/sdk/test/integration/document_test.ts around lines 954 to 956,
replace the fixed 500ms timeout used to wait for server state synchronization
with a more reliable method. Implement a polling loop or use explicit
synchronization APIs to repeatedly check the server state until it reaches the
expected condition or a timeout occurs, ensuring the test waits precisely for
readiness instead of an arbitrary delay. This will make the test more stable and
less prone to flakiness in varying environments.
|
Hello @minwoo1999! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #996 +/- ##
==========================================
+ Coverage 78.47% 78.59% +0.12%
==========================================
Files 65 65
Lines 5626 5639 +13
Branches 1025 1025
==========================================
+ Hits 4415 4432 +17
+ Misses 907 905 -2
+ Partials 304 302 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hello, I've signed the Contributor License Agreement (CLA). Please let me know if there's anything else needed! |
|
@hackerwins @chacha912 Why are we seeing this kind of behavior? I'm not sure about the context of the "document already detached" error when reattaching a document on the same client. |
|
@krapie @minwoo1999 Thanks for your contribution. The state of a document attached to a client( However, when a client detaches a document, the document may become stale and subject to garbage collection. GC in Yorkie removes CRDT tombstones based on the To prevent this, we currently disallow reattaching a document after it has been detached(yorkie-team/yorkie#904). To improve this, we're considering allowing clients to reattach previously detached documents—but only by resetting them to a clean, initial state before attachment. This would avoid resurrecting stale tombstone data while restoring flexibility to users. Since this behavior involves both SDK and server state handling, it would be best implemented in the Yorkie main repo (Go) first and reflect it to other SDKs. |
hackerwins
force-pushed
the
main
branch
4 times, most recently
from
3f1f084 to
0d6a2da
Compare
What this PR does / why we need it
This PR improves the developer experience when using
yorkie-js-sdkby allowing a previously detached document to be reattached using the same client instance.In some use cases—such as apps that need to switch between documents while maintaining a single active client—it’s natural to detach one document and later reattach it. However, this flow currently results in an error, as the internal state of the document isn't reset upon detachment.
To address this, we expose a public
reset()method that clears the document’s internal state, making it ready for reattachment. This allows developers to reattach previously detached documents safely and explicitly.Background context
Here's a typical scenario:
clientis created and activated.doc1is attached and then detached.doc2is attached and detached.doc1causes an error unless its internal state is reset.By calling
doc1.reset()before reattaching, this issue is resolved.Test
What are the relevant tickets?
Fixes yorkie-team/yorkie#1353
Checklist
Summary by CodeRabbit
New Features
Tests