-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disable doc garbage collection when running in serve-doc
mode.
#282
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe pull request introduces several changes across multiple files in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant DocConnection
User->>Server: Start Document Server
Server->>Server: Initialize with doc_gc
Server->>DocConnection: Establish Connection
DocConnection-->>Server: Connection Established
User->>DocConnection: Fetch Document
DocConnection-->>User: Return Document
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
serve-doc
mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This pull request introduces changes to improve the functionality and usability of the y-sweet server, particularly in serve-doc
mode. Here's a concise summary of the key changes:
- Disabled document garbage collection in
serve-doc
mode to prevent docs from becoming inaccessible when there are no active connections - Renamed
Y_SWEET_PORT
environment variable toPORT
for better compatibility with standard HTTP container hosts - Added a new
doc_gc
parameter toServer::new()
function to control garbage collection behavior - Introduced a new test file
doc-server.test.ts
to validate theserve-doc
mode functionality - Refactored the SDK's main module (
js-pkg/sdk/src/main.ts
) to improve its API and export additional components
These changes address the issue of document inaccessibility in serve-doc
mode and enhance the overall flexibility and usability of the y-sweet server.
5 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- crates/y-sweet/src/main.rs (4 hunks)
- crates/y-sweet/src/server.rs (6 hunks)
- js-pkg/sdk/src/main.ts (1 hunks)
- tests/src/doc-server.test.ts (1 hunks)
- tests/src/server.ts (0 hunks)
Files not reviewed due to no reviewable changes (1)
- tests/src/server.ts
Additional context used
Biome
tests/src/doc-server.test.ts
[error] 5-6: Do not export from a test file.
(lint/suspicious/noExportsInTest)
Additional comments not posted (11)
tests/src/doc-server.test.ts (1)
1-95
: Great work on setting up the testing framework!The
DocServer
class provides a clean and robust way to manage the lifecycle of the document server during tests. The test cases cover important scenarios like fetching documents as updates.The use of
beforeAll
andafterAll
hooks ensures proper setup and teardown of the server.Overall, the implementation looks solid and well-structured.
Tools
Biome
[error] 5-6: Do not export from a test file.
(lint/suspicious/noExportsInTest)
js-pkg/sdk/src/main.ts (2)
2-2
: LGTM!Exporting the
DocConnection
class directly from theconnection
module improves clarity and streamlines the module's interface.
Line range hint
6-6
: Verify the removal ofYSweetError
import.Removing the import statement for an unused class helps keep the code clean and maintainable. However, please ensure that the removal of this import does not break any functionality in this module or other parts of the codebase that may still rely on it.
Run the following script to verify if
YSweetError
is used anywhere in the codebase:crates/y-sweet/src/main.rs (3)
40-40
: Standardizing the environment variable name for the port is a good practice.Changing the environment variable from
Y_SWEET_PORT
toPORT
aligns with the common practice used by most HTTP container hosts. This enhances compatibility and allows for greater flexibility in deployment scenarios where the environment variable name is standardized across different applications.
76-76
: Consistent change to standardize the environment variable name for the port.This change is consistent with the previous modification in the
Serve
variant. Standardizing the environment variable name toPORT
aligns with the common practice used by most HTTP container hosts, enhancing compatibility and allowing for greater flexibility in deployment scenarios.
182-182
: Please provide more context about the new boolean parameter.The
Server::new
function has been updated to include a new boolean parameter, which is set totrue
in theServe
subcommand andfalse
in theServeDoc
subcommand. Could you please provide more information about the purpose and implications of this parameter?Understanding the reasoning behind this change and how it influences the application's behavior would be helpful for a thorough review.
Also applies to: 266-266
crates/y-sweet/src/server.rs (5)
80-82
: LGTM!The new
doc_gc
field is well-named, typed, and documented. Disabling doc GC for single-doc mode is a sensible design choice.
92-92
: Looks good!The
doc_gc
parameter is correctly added to the constructor to initialize the new field.
102-102
: LGTM!The
doc_gc
field is correctly initialized using the constructor parameter.
159-170
: Conditional spawning ofdoc_gc_worker
looks good!The code correctly spawns the
doc_gc_worker
only whendoc_gc
is enabled. The worker is spawned with the right parameters and instrumented with a tracing span.
Line range hint
664-701
: Test cases updated correctly!The test cases now pass
true
for thedoc_gc
parameter when creating theServer
instance. This ensures that the document garbage collection functionality is covered in the tests.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
This pull request introduces significant changes to the y-sweet server implementation, focusing on improving document management in serve-doc
mode. Here's a concise summary of the key modifications:
- Modified
Server
struct incrates/y-sweet/src/server.rs
to includedoc_gc
parameter, controlling garbage collection behavior - Updated
Server::new()
function to incorporate the newdoc_gc
parameter - Adjusted garbage collection logic in
doc_gc_worker
function to respect thedoc_gc
flag - Implemented
serve_doc
method inServer
struct for single-document mode operation - Added
single_doc_routes
method toServer
for routing in single-document mode
These changes effectively prevent document garbage collection in serve-doc
mode, addressing the issue of document inaccessibility when there are no active connections.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
When running with
serve-doc
, if there is no active connection, the doc will be garbage collected and then accessing it will not work. This prevents the doc from being garbage collected inserve-doc
mode.This also renames the
Y_SWEET_PORT
env var toPORT
, which is used by convention by most HTTP container hosts.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests