Repository navigation
fix(id): sanitize collaborative cursor names - #4052
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 36 minutes. View limit details
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. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbfb52a641
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| State(state): State<super::AppState>, | ||
| ) -> impl IntoResponse { | ||
| ws.on_upgrade(move |socket| { | ||
| ws.max_message_size(crate::world_session::MAX_FRAME_BYTES) |
There was a problem hiding this comment.
When a user pastes more than 1 MiB of text, this new limit rejects the entire edit and disconnects the socket. The browser's handleEditorChange in web/src/collab.ts sends all pending ProseMirror steps in one message without size validation or splitting, and the server's receive loop exits on the resulting WebSocket error. On reconnect, the client's MSG.INIT handler destroys the existing editor and rebuilds it from server state, losing the rejected paste and any other unconfirmed edits. Add compatible client-side handling, including splitting oversized individual steps or visibly rejecting the edit without discarding local state, before applying this cap to collaborative editing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 72a3897 (queue task 5053): send() in pkgs/id/web/src/collab.ts now measures the packed MessagePack payload and never sends a message over the 1 MiB MAX_FRAME_BYTES cap. An oversized STEPS message (e.g. a >1 MiB paste) is dropped with a console error and the connection error status instead of being sent — the socket is never closed by the cap, no reconnect/INIT rebuild happens, and the local document with all unconfirmed edits is preserved; the edit simply cannot sync until it is made smaller (e.g. undo), which is the "visibly rejecting without discarding local state" outcome. Splitting oversized step batches so they actually sync (ACK-ordered chunking, plus the single-oversized-step case) is filed as #4324. Web checks on the branch: vitest run src/collab.test.ts 3/3 pass, bun run build:js succeeds.
The 1 MiB max_message_size added for #1833 closes the socket when exceeded, and the reconnect INIT rebuild discards every unconfirmed local edit — a >1 MiB paste would be lost. Guard send() in web/src/collab.ts: a packed message over MAX_FRAME_BYTES is dropped with an error status instead of sent, keeping the socket and the local document alive; the edit simply cannot sync until it is smaller. True chunked sending of oversized step batches is filed as #4324.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72a3897018
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| #[allow(clippy::unwrap_used, clippy::panic)] | ||
| #[tokio::test] | ||
| async fn cursor_ingest_sanitizes_before_storage_and_broadcast() { |
There was a problem hiding this comment.
test_
The naming guideline in pkgs/id/AGENTS.md explicitly requires tests to use the test_ prefix. Rename this function to test_cursor_ingest_sanitizes_before_storage_and_broadcast to comply with that repository convention.
AGENTS.md reference: pkgs/id/AGENTS.md:L140-L140
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f1d8f90 (task 5053): the regression test is renamed to test_cursor_ingest_sanitizes_before_storage_and_broadcast per the pkgs/id/AGENTS.md test_ prefix convention. The same commit merges origin/main (conflict in collab.rs resolved by keeping main's crate::frames::MAX_FRAME_BYTES handler form plus this PR's store_and_broadcast_cursor helper).
…regression test to test_ prefix (Codex P1 on PR #4052)
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
MAX_FRAME_BYTES(1 MiB).Verification
cargo fmt --manifest-path pkgs/id/Cargo.toml -- --check— passed.git diff --check— passed.cargo test --features webinpkgs/id— running locally; CI remains authoritative.Closes #1833
Need help on this PR? Tag
@codesmith-botwith what you need.