feat: mesh collaboration via iroh and loro#38
Open
adhi-thirumala wants to merge 47 commits into
Open
Conversation
… into fable-collab
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds a new collaboration crate, session/runtime plumbing, editor collaboration patching, workspace collaboration UI, and supporting tests/docs. ChangesP2P collaboration
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/gpui-flowtext/src/rich_text/editor/mouse.rs (1)
278-304:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCapture the drag/drop canonical span from the pre-edit document and include the source-side deletion.
The
ReplaceParagraphSpanhere is built after the delete+insert has already run, so itsbeforepayload is reading post-move content. It also only spans the drop-side paragraph range, which misses the source-side deletion when the drag crosses paragraphs. That means the collaboration op no longer matches the local edit and can replay incorrectly on peers.🤖 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 `@crates/gpui-flowtext/src/rich_text/editor/mouse.rs` around lines 278 - 304, Build the `ReplaceParagraphSpan` canonical op from the pre-edit state in `mouse.rs` so it reflects the drag/drop before any mutation is applied; capture the source-side deleted paragraph span as part of the same canonical operation, not just the inserted drop range. Update the `EditRecord`/`mark_document_changed_with_ops` path around `canonical_operations` so the payload uses the pre-delete document contents and includes both source and destination paragraph ranges when the drag crosses paragraphs.crates/gpui-flowtext/src/rich_text/editor/object_selection.rs (1)
492-500:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't emit a fake
BlockId(0)for deleted-block collaboration ops.The deleted block id is resolved after the block/id vectors have already been mutated, and a miss is silently rewritten to
BlockId(0). In collaboration mode that can delete the wrong remote block instead of describing the local edit. Capture the block id before removal, and if it is unavailable, fall back to a broader op likeReplaceDocumentrather than fabricating an id.Also applies to: 513-513
🤖 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 `@crates/gpui-flowtext/src/rich_text/editor/object_selection.rs` around lines 492 - 500, The block identifier is being resolved after the block and id vectors have been mutated, causing the lookup to fail and fall back to a fabricated BlockId(0), which can delete the wrong remote block in collaboration mode. Capture the block identifier by calling self.identity_map.block_id(block_ix) BEFORE removing the block from the vectors using blocks.remove() and remove_block_ids(). Use the captured block id in the CanonicalOperation for DeleteBlock. If the block id cannot be captured (when the lookup fails), replace the DeleteBlock operation with a broader operation like ReplaceDocument instead of using a fake BlockId(0). Apply this same fix to all locations where blocks are deleted and canonical operations are created.
🧹 Nitpick comments (11)
crates/flowstate-collab/src/proto_direct.rs (1)
31-38: ⚡ Quick winSimplify redundant
try_fromafter length check.Line 33 already ensures
payload.len() <= MAX_FRAME_LEN(2 MiB), which is well belowu32::MAX. Thetry_fromon line 34 can never fail, making the?unreachable.♻️ Suggested simplification
let payload = postcard::to_stdvec(&(PROTOCOL_VERSION, value))?; ensure!(payload.len() <= MAX_FRAME_LEN, "direct frame exceeds {MAX_FRAME_LEN} bytes"); - let len = u32::try_from(payload.len())?; + let len = payload.len() as u32; let mut frame = len.to_le_bytes().to_vec();🤖 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 `@crates/flowstate-collab/src/proto_direct.rs` around lines 31 - 38, Simplify encode_frame in proto_direct.rs by removing the redundant u32::try_from(payload.len())? conversion after the MAX_FRAME_LEN check; since payload.len() is already bounded below u32::MAX, compute the length directly as a u32 and keep the rest of the frame assembly unchanged.crates/flowstate-collab/src/net/swarm.rs (1)
137-162: ⚡ Quick winConsider structured logging instead of
eprintln!for production.Line 149 uses
eprintln!to report gossip decode failures. For production deployments, replace this with a proper logging framework (e.g.,tracing::warn!) to enable configurable log levels and structured output.🤖 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 `@crates/flowstate-collab/src/net/swarm.rs` around lines 137 - 162, Replace the eprintln! call in the handle_event function's error handling branch (where proto_gossip::decode fails) with a structured logging call using tracing::warn!. This will allow production deployments to configure log levels and get structured output instead of relying on stderr output, enabling better observability and log aggregation.crates/flowstate/src/collab/session.rs (1)
592-595: ⚡ Quick winExtract undo manager configuration to named constants or configuration.
The hardcoded values
merge_interval(500)andmax_undo_steps(300)are magic numbers. Consider extracting them to named constants or making them configurable for better maintainability.♻️ Proposed refactor with named constants
+const COLLAB_UNDO_MERGE_INTERVAL_MS: u64 = 500; +const COLLAB_MAX_UNDO_STEPS: usize = 300; + fn attach_undo_manager(&mut self) { if self.undo_manager.is_some() { return; } let Some(doc) = &self.doc else { return; }; let mut undo_manager = UndoManager::new(doc); - undo_manager.set_merge_interval(500); - undo_manager.set_max_undo_steps(300); + undo_manager.set_merge_interval(COLLAB_UNDO_MERGE_INTERVAL_MS); + undo_manager.set_max_undo_steps(COLLAB_MAX_UNDO_STEPS); self.undo_manager = Some(undo_manager); }🤖 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 `@crates/flowstate/src/collab/session.rs` around lines 592 - 595, The UndoManager setup in session.rs uses magic numbers for merge_interval and max_undo_steps; extract the 500 and 300 values into named constants or a session/undo configuration near UndoManager::new, set_merge_interval, and set_max_undo_steps so the defaults are easy to find and adjust.crates/flowstate-collab/src/ticket.rs (1)
46-48: 💤 Low valueConsider handling serialization failure gracefully instead of panicking.
While
postcardserialization of this simple struct is unlikely to fail, usingexpect()introduces a potential panic point. Consider returning an error or documenting why this can never fail.♻️ Alternative approach returning an error
If the
Tickettrait allows it, consider changing the signature to returnResult:fn encode_bytes(&self) -> Result<Vec<u8>, Box<dyn std::error::Error>> { Ok(postcard::to_stdvec(self)?) }However, if the trait requires
Vec<u8>, the current approach with a clear panic message is acceptable.🤖 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 `@crates/flowstate-collab/src/ticket.rs` around lines 46 - 48, The encode_bytes method in the Ticket trait implementation uses expect() which can panic on serialization failure. First, check the Ticket trait definition to see if the return type can be changed from Vec<u8> to a Result type. If the trait signature is flexible, update encode_bytes to return Result<Vec<u8>, Box<dyn std::error::Error>> and use the ? operator instead of expect(). If the trait signature is fixed and requires Vec<u8>, then keep the current approach but add a doc comment above the method explaining why postcard serialization of SessionTicket cannot fail and the panic is justified as a last resort safeguard.crates/flowstate/src/collab/session_presence.rs (1)
9-11: 💤 Low valueConsider using a structured logging framework instead of
eprintln!.The code uses
eprintln!for error messages (lines 10, 28). For better observability and consistency, consider using a structured logging framework liketracingorlog. This would allow filtering, formatting, and routing of log messages in production.Also applies to: 27-29
🤖 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 `@crates/flowstate/src/collab/session_presence.rs` around lines 9 - 11, The code currently uses eprintln! macro for logging error messages when presence updates fail. Replace the eprintln! calls with a structured logging framework such as tracing or log to enable better observability, filtering, and production-grade log management. Update all occurrences where eprintln! is used for the flowstate collab presence update error message (in the error handling block for presence.apply() and any similar error logging) to use the appropriate logging macro from your chosen framework instead, ensuring consistent structured logging throughout the module.crates/flowstate-collab/src/proto_gossip.rs (2)
7-7: MoveDIRECT_ALPNtoproto_direct.rsmodule.The
DIRECT_ALPNconstant is used exclusively in direct protocol code (net/direct.rs and net/runtime.rs) and semantically belongs with other direct protocol constants. Currently it's defined inproto_gossip.rsalongsidePROTOCOL_VERSIONandGOSSIP_INLINE_LIMIT, but it has no relation to the gossip protocol. Move it toproto_direct.rswhere it should be grouped withMAX_FRAME_LENandMAX_PAYLOAD_CHUNK_LENfor better code organization.🤖 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 `@crates/flowstate-collab/src/proto_gossip.rs` at line 7, Move the DIRECT_ALPN constant out of proto_gossip.rs and into proto_direct.rs so it lives with the other direct-protocol constants like MAX_FRAME_LEN and MAX_PAYLOAD_CHUNK_LEN. Update any direct-protocol code that references DIRECT_ALPN, especially net/direct.rs and net/runtime.rs, to import it from proto_direct.rs instead of proto_gossip.rs, and leave proto_gossip.rs containing only gossip-related constants such as PROTOCOL_VERSION and GOSSIP_INLINE_LIMIT.
30-32: Consider optimizingencoded_lento avoid allocating the full message buffer.The function calls
encode(msg)?which allocates and serializes the entire message using postcard just to measure its length. Since there's only one call site and the message size is bounded byGOSSIP_INLINE_LIMIT(2KB), the practical impact is minimal. However, if this function is called frequently in future code paths, consider implementing a size-counting approach or accepting the allocation as acceptable for this use case.🤖 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 `@crates/flowstate-collab/src/proto_gossip.rs` around lines 30 - 32, The encoded_len function allocates the full message buffer by calling encode(msg)? just to measure its length, which is inefficient even though the practical impact is minimal with the current 2KB size bound. Either implement a size-counting approach that avoids the full buffer allocation (such as using a custom serializer that only counts bytes without storing them), or if accepting the allocation as reasonable, add a comment in the function explaining why the current approach is acceptable given the bounded message size and single call site.crates/flowstate/src/collab/share_dialog.rs (1)
146-151: 💤 Low valueConsider logging clipboard write failures.
The
let _ =pattern silently discards any error fromwindow_handle.update(). While this is likely intentional (the window may have closed while the async ticket-minting task was in flight, and clipboard copying is best-effort), logging the failure would help debugging clipboard issues.Optional: log clipboard errors
if let Ok(text) = &encoded && let Some(window_handle) = copy_to_clipboard { let text = text.clone(); - let _ = window_handle.update(cx, |_, _, cx| cx.write_to_clipboard(ClipboardItem::new_string(text))); + if let Err(e) = window_handle.update(cx, |_, _, cx| cx.write_to_clipboard(ClipboardItem::new_string(text))) { + eprintln!("Failed to copy invite to clipboard: {e}"); + } }🤖 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 `@crates/flowstate/src/collab/share_dialog.rs` around lines 146 - 151, The `window_handle.update()` call that writes to the clipboard is silently discarding any errors with the `let _ =` pattern. Replace this pattern by capturing the Result returned from the update call and logging any failures using an appropriate logger (such as error or warn level). Keep the operation as best-effort by not propagating or panicking on the error, but ensure that clipboard write failures are logged to help with debugging clipboard-related issues.crates/flowstate-collab/src/binding.rs (1)
148-167: ⚡ Quick winRemove unnecessary clone in
rebuild_indexes.Line 153 clones each
BindingRowbefore passing it toindex_row, butindex_rowonly needs a reference. This adds O(n) allocations every time indexes are rebuilt (after insert/remove/move operations).♻️ Proposed fix
pub fn rebuild_indexes(&mut self) { self.by_paragraph.clear(); self.by_block.clear(); self.by_container.clear(); for ix in 0..self.rows.len() { - let row = self.rows[ix].clone(); - self.index_row(ix, &row); + self.index_row(ix, &self.rows[ix]); } }🤖 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 `@crates/flowstate-collab/src/binding.rs` around lines 148 - 167, In the rebuild_indexes method, remove the unnecessary clone operation on line 153 where each BindingRow is being cloned. Since the index_row method only requires a reference to the BindingRow (as seen in its signature), pass a direct reference to self.rows[ix] to index_row instead of creating a clone first. This eliminates wasteful O(n) allocations that occur every time indexes are rebuilt after insert/remove/move operations.crates/flowstate-collab/tests/convergence.rs (1)
14-55: ⚡ Quick winConsider adding convergence tests for structural blocks.
The current convergence test comprehensively validates paragraph text and style convergence, but doesn't test structural block operations (insert/move/replace/delete of images, equations, tables). Adding convergence tests for these operations would increase confidence in the collaboration implementation.
🤖 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 `@crates/flowstate-collab/tests/convergence.rs` around lines 14 - 55, The test `two_peers_converge_with_reordered_and_duplicated_update_imports` currently validates convergence for paragraph text and style operations but does not cover structural block operations. Add new convergence test functions that exercise insert, move, replace, and delete operations on structural blocks (such as images, equations, and tables) between two peers with reordered and duplicated imports, following the same test pattern used for the existing paragraph-focused test to ensure these operations also converge correctly.crates/gpui-flowtext/src/rich_text/editor/lifecycle.rs (1)
265-268: ⚡ Quick winMethod name misleading: clears all undo/redo history, not just collaboration history.
The method
clear_collab_historyclears bothundo_stackandredo_stackentirely, which affects all editor history, not just collaboration-specific history. Consider renaming toclear_undo_redo_stacksorclear_all_historyto accurately reflect the scope of the operation.🤖 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 `@crates/gpui-flowtext/src/rich_text/editor/lifecycle.rs` around lines 265 - 268, The method `clear_collab_history` has a misleading name since it clears both the `undo_stack` and `redo_stack` entirely, affecting all editor history rather than just collaboration-specific history. Rename this method to `clear_undo_redo_stacks` or `clear_all_history` to accurately reflect that it clears all undo and redo history, not just collaboration-related history. Be sure to update all call sites that reference this method name.
🤖 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 `@crates/flowstate-collab/src/ids.rs`:
- Around line 18-20: `SessionId::new` currently uses `rand::random()`, but
session IDs need a stronger entropy source; update the constructor in
`crates/flowstate-collab/src/ids.rs` to generate the ID with `rand::rngs::OsRng`
(or `SysRng`) instead of `rand::random()`, and keep the change localized to the
`SessionId`/ID generation path so all session IDs come from a cryptographically
secure source.
In `@crates/flowstate-collab/src/local_apply.rs`:
- Around line 148-164: In local_apply.rs inside the split/row insertion logic,
validate the new row metadata first before mutating the source LoroText: fetch
the inserted block id and version (the lookups around block_ids.get(insert_ix)
and blocks.get(insert_ix)) before calling delete_utf8 on the original paragraph,
and only truncate text once those checks succeed so a failed split cannot leave
the document partially edited.
In `@crates/flowstate-collab/src/net/blobs.rs`:
- Around line 45-57: In `BlobStore::insert_with_id`, reinserting an existing
`BlobId` currently leaves the old payload in `entries`, and `BlobStore::get`
returns that stale first match. Update `insert_with_id` to remove or overwrite
any existing entry with the same `BlobId` before pushing the new bytes, while
keeping `total_bytes` consistent with the replacement logic.
In `@crates/flowstate-collab/src/net/direct.rs`:
- Around line 138-146: The semaphore permit is being acquired after reading the
request frame, which allows unbounded task spawning and request buffering before
the concurrency limit is checked. Move the
self.permits.clone().try_acquire_owned() check to occur before the read_frame
call in handle_stream() to ensure the concurrency limit gates network work. This
issue applies at two locations: the primary site in handle_stream (lines
138-146) and a sibling location (lines 152-160). Additionally, consider gating
the permit acquisition even earlier, before tokio::spawn invokes handle_stream
in the accept() method, to limit task creation itself.
In `@crates/flowstate-collab/src/net/runtime.rs`:
- Around line 18-40: `start()` leaves `RUNTIME` pointing at dead channels after
`net_main()` shuts down, so future calls reuse a stopped bridge; change
`RuntimeBridge`/`start()` so a failed or exited runtime can be detected and
replaced with a fresh `spawn_runtime()` result instead of returning the cached
pair. Also update the `direct.rs` endpoint cache (the `OnceLock` around the
client endpoint) so it is invalidated or refreshed when the runtime restarts,
otherwise the new bridge will still use a stale endpoint.
- Around line 101-107: Contain per-command failures inside net_main so a bad
join/publish does not exit the runtime or skip responses. In
NetCommand::CreateSession, catch SwarmHandle::spawn errors locally, reply to the
caller with the failure, and keep the loop running; in the publish path around
handle.publish(payload).await, handle the error locally instead of propagating
it. Apply the same error containment at
crates/flowstate-collab/src/net/runtime.rs:101-107,
crates/flowstate-collab/src/net/runtime.rs:116-120, and
crates/flowstate-collab/src/net/runtime.rs:128-130, using the existing net_main
/ replace_swarm / handle.publish flow as the anchor points.
In `@crates/flowstate-collab/src/projection.rs`:
- Around line 169-175: `verify_lineage` in
`crates/flowstate-collab/src/projection.rs` only validates `META_SESSION`, so
add a `META_SCHEMA` check against `SCHEMA_VERSION` before returning success.
Update the `verify_lineage` path to fail fast with an error when the stored
schema is missing or does not match the current version, alongside the existing
session check, so snapshot projection never reaches `document_from_loro` with an
incompatible layout.
In `@crates/flowstate-collab/src/remote_apply.rs`:
- Around line 34-37: In remote_apply.rs, the Diff::Map handling in
remote_apply::apply should emit only one object-replacement patch per map diff
instead of calling apply_map_diff once per updated key; update the logic so
ReplaceObjectBlock is generated once per affected row even when replace_block_at
changes KIND, DATA, and REV together. Use the apply_map_diff path and its
related map-diff dispatch to dedupe by row/target before applying patches, and
ensure the same fix covers the other map-diff site referenced by the comment.
In `@crates/flowstate/src/collab/asset_transfer.rs`:
- Around line 142-147: In record_from_bytes, replace the use of DefaultHasher
with a stable, deterministic wire hash for cross-peer verification so identical
bytes always produce the same content_hash across platforms and Rust versions.
Update the content_hash computation to use a portable algorithm such as SHA-256
or xxHash, and keep the existing byte_len and content_hash validation logic in
AssetRecord unchanged except for comparing against the new stable hash value.
In `@crates/flowstate/src/collab/mod.rs`:
- Around line 119-146: The `session_by_panel` mapping is being inserted on line
137 before two fallible operations execute: the `try_send` call for
RegisterDirectHandler and the `establish_joined_peer` call. If either operation
fails, the function returns an error but leaves the mapping in place, causing
subsequent attach attempts to incorrectly think the panel is already attached.
Move the `self.session_by_panel.insert(panel_id, session_id)` statement to after
the `Self::establish_joined_peer(session, commands, cx)?` call succeeds,
ensuring the mapping is only recorded when the entire attachment operation
completes successfully.
- Around line 53-86: The `start_session_for_panel` function registers a session
at line 75 via `self.register_session(entity.clone(), cx)`, but if either the
`RegisterDirectHandler` command send (line 77) or the `CreateSession` command
send (line 82) fails, the function returns an error without cleaning up the
registered session. This creates a resource leak where the session ID remains
registered but the network layer never received the commands. To fix this, when
either `try_send` call fails, detach the session from its context and unregister
it before returning the error. Reference the correct cleanup pattern used in the
`join_session` method at lines 106-110, which properly calls detach and
unregister before returning errors, and apply the same approach to both error
paths in `start_session_for_panel`.
In `@crates/flowstate/src/collab/presence_view.rs`:
- Around line 33-39: The directory creation in collaboration_recovery_path
currently ignores failures from create_dir_all, so update this function to
handle that error instead of discarding it. Either change
collaboration_recovery_path to return a Result<PathBuf> and propagate the
std::fs::create_dir_all error to callers, or at minimum log/return a failure
when the temp recovery directory cannot be created; keep the rest of the
path-building logic with SessionId and sanitized_recovery_title unchanged.
In `@crates/flowstate/src/collab/session_timers.rs`:
- Around line 238-249: When `rebuild_from_projection` rebuilds the document
after a self-check by calling `replace_document_from_collaboration()`, any
deferred remote patches stored in `session_io.rs` become stale since they
describe edits already present in the rebuilt projection. These stale patches
would then be incorrectly re-applied on the next flush. Clear the
`pending_remote_patches` from `session_io.rs` within the
`rebuild_from_projection` method after the document has been rebuilt to prevent
this re-application of already-synchronized edits.
In `@crates/flowstate/src/collab/session.rs`:
- Around line 131-132: Switch the session request channels in `Session` setup
from unbounded to bounded to apply backpressure and avoid memory growth; update
the `direct_tx/direct_rx` and `undo_tx/undo_rx` creation in `collab/session.rs`
to use `async_channel::bounded` with a justified capacity, and make sure the
downstream direct request pump and undo request pump code paths still handle
send/receive behavior correctly. If keeping unbounded is intentional, document
that decision near the channel setup.
In `@crates/flowstate/src/workspace/workspace/collab.rs`:
- Around line 204-208: In join_collaboration_from_clipboard’s synchronous error
path around crate::collab::join_session, don’t just log and return None; surface
a user-visible error prompt before exiting the branch. Update the error handling
in workspace/collab.rs so the failure is reported through the existing UI/error
notification mechanism used by this flow, while still returning None after the
prompt is shown.
In `@crates/flowstate/src/workspace/workspace/top_bar.rs`:
- Around line 109-143: Update collaboration_top_bar_button in
workspace/top_bar.rs so the “Leave Shared Session” menu item is disabled unless
the active document actually has an attached collaboration session, not just
when has_document is true. Use the same workspace/session state used by
confirm_leave_collaboration_on_active_document to compute the enabled flag, and
pass that into file_menu_item for the “Leave Shared Session” entry so local tabs
don’t show a no-op action.
In `@crates/flowstate/src/workspace/workspace/window.rs`:
- Around line 84-87: `window.rs` is shutting down the global collaboration
runtime too early, which breaks other open windows; change the
`window_handle.update` close path so
`workspace.leave_all_collaboration_sessions(cx)` only affects the current
workspace and `crate::collab::shutdown(cx)` is deferred until the last
workspace/window is gone or behind a shared reference count. Also audit the
collaboration close/leave flow in `collab_prompts.rs` so it does not trigger a
global shutdown from a single workspace path, and route any such teardown
through the same last-window guard.
In `@crates/gpui-flowtext/src/rich_text/editor/block_insertion.rs`:
- Around line 79-80: Update prepare_block_insertion_index and
insert_ordered_block_fragment_after_caret so the placeholder caret paragraph is
only removed when the insertion does not include InputBlock::Paragraph; preserve
the lone empty paragraph for paragraph-block inserts to keep
self.document.paragraphs and self.document.blocks aligned. Use the existing
block-rebuild logic in insert_ordered_block_fragment_after_caret and the caret
cleanup in prepare_block_insertion_index to gate the removal instead of
unconditionally stripping the paragraph.
---
Outside diff comments:
In `@crates/gpui-flowtext/src/rich_text/editor/mouse.rs`:
- Around line 278-304: Build the `ReplaceParagraphSpan` canonical op from the
pre-edit state in `mouse.rs` so it reflects the drag/drop before any mutation is
applied; capture the source-side deleted paragraph span as part of the same
canonical operation, not just the inserted drop range. Update the
`EditRecord`/`mark_document_changed_with_ops` path around `canonical_operations`
so the payload uses the pre-delete document contents and includes both source
and destination paragraph ranges when the drag crosses paragraphs.
In `@crates/gpui-flowtext/src/rich_text/editor/object_selection.rs`:
- Around line 492-500: The block identifier is being resolved after the block
and id vectors have been mutated, causing the lookup to fail and fall back to a
fabricated BlockId(0), which can delete the wrong remote block in collaboration
mode. Capture the block identifier by calling
self.identity_map.block_id(block_ix) BEFORE removing the block from the vectors
using blocks.remove() and remove_block_ids(). Use the captured block id in the
CanonicalOperation for DeleteBlock. If the block id cannot be captured (when the
lookup fails), replace the DeleteBlock operation with a broader operation like
ReplaceDocument instead of using a fake BlockId(0). Apply this same fix to all
locations where blocks are deleted and canonical operations are created.
---
Nitpick comments:
In `@crates/flowstate-collab/src/binding.rs`:
- Around line 148-167: In the rebuild_indexes method, remove the unnecessary
clone operation on line 153 where each BindingRow is being cloned. Since the
index_row method only requires a reference to the BindingRow (as seen in its
signature), pass a direct reference to self.rows[ix] to index_row instead of
creating a clone first. This eliminates wasteful O(n) allocations that occur
every time indexes are rebuilt after insert/remove/move operations.
In `@crates/flowstate-collab/src/net/swarm.rs`:
- Around line 137-162: Replace the eprintln! call in the handle_event function's
error handling branch (where proto_gossip::decode fails) with a structured
logging call using tracing::warn!. This will allow production deployments to
configure log levels and get structured output instead of relying on stderr
output, enabling better observability and log aggregation.
In `@crates/flowstate-collab/src/proto_direct.rs`:
- Around line 31-38: Simplify encode_frame in proto_direct.rs by removing the
redundant u32::try_from(payload.len())? conversion after the MAX_FRAME_LEN
check; since payload.len() is already bounded below u32::MAX, compute the length
directly as a u32 and keep the rest of the frame assembly unchanged.
In `@crates/flowstate-collab/src/proto_gossip.rs`:
- Line 7: Move the DIRECT_ALPN constant out of proto_gossip.rs and into
proto_direct.rs so it lives with the other direct-protocol constants like
MAX_FRAME_LEN and MAX_PAYLOAD_CHUNK_LEN. Update any direct-protocol code that
references DIRECT_ALPN, especially net/direct.rs and net/runtime.rs, to import
it from proto_direct.rs instead of proto_gossip.rs, and leave proto_gossip.rs
containing only gossip-related constants such as PROTOCOL_VERSION and
GOSSIP_INLINE_LIMIT.
- Around line 30-32: The encoded_len function allocates the full message buffer
by calling encode(msg)? just to measure its length, which is inefficient even
though the practical impact is minimal with the current 2KB size bound. Either
implement a size-counting approach that avoids the full buffer allocation (such
as using a custom serializer that only counts bytes without storing them), or if
accepting the allocation as reasonable, add a comment in the function explaining
why the current approach is acceptable given the bounded message size and single
call site.
In `@crates/flowstate-collab/src/ticket.rs`:
- Around line 46-48: The encode_bytes method in the Ticket trait implementation
uses expect() which can panic on serialization failure. First, check the Ticket
trait definition to see if the return type can be changed from Vec<u8> to a
Result type. If the trait signature is flexible, update encode_bytes to return
Result<Vec<u8>, Box<dyn std::error::Error>> and use the ? operator instead of
expect(). If the trait signature is fixed and requires Vec<u8>, then keep the
current approach but add a doc comment above the method explaining why postcard
serialization of SessionTicket cannot fail and the panic is justified as a last
resort safeguard.
In `@crates/flowstate-collab/tests/convergence.rs`:
- Around line 14-55: The test
`two_peers_converge_with_reordered_and_duplicated_update_imports` currently
validates convergence for paragraph text and style operations but does not cover
structural block operations. Add new convergence test functions that exercise
insert, move, replace, and delete operations on structural blocks (such as
images, equations, and tables) between two peers with reordered and duplicated
imports, following the same test pattern used for the existing paragraph-focused
test to ensure these operations also converge correctly.
In `@crates/flowstate/src/collab/session_presence.rs`:
- Around line 9-11: The code currently uses eprintln! macro for logging error
messages when presence updates fail. Replace the eprintln! calls with a
structured logging framework such as tracing or log to enable better
observability, filtering, and production-grade log management. Update all
occurrences where eprintln! is used for the flowstate collab presence update
error message (in the error handling block for presence.apply() and any similar
error logging) to use the appropriate logging macro from your chosen framework
instead, ensuring consistent structured logging throughout the module.
In `@crates/flowstate/src/collab/session.rs`:
- Around line 592-595: The UndoManager setup in session.rs uses magic numbers
for merge_interval and max_undo_steps; extract the 500 and 300 values into named
constants or a session/undo configuration near UndoManager::new,
set_merge_interval, and set_max_undo_steps so the defaults are easy to find and
adjust.
In `@crates/flowstate/src/collab/share_dialog.rs`:
- Around line 146-151: The `window_handle.update()` call that writes to the
clipboard is silently discarding any errors with the `let _ =` pattern. Replace
this pattern by capturing the Result returned from the update call and logging
any failures using an appropriate logger (such as error or warn level). Keep the
operation as best-effort by not propagating or panicking on the error, but
ensure that clipboard write failures are logged to help with debugging
clipboard-related issues.
In `@crates/gpui-flowtext/src/rich_text/editor/lifecycle.rs`:
- Around line 265-268: The method `clear_collab_history` has a misleading name
since it clears both the `undo_stack` and `redo_stack` entirely, affecting all
editor history rather than just collaboration-specific history. Rename this
method to `clear_undo_redo_stacks` or `clear_all_history` to accurately reflect
that it clears all undo and redo history, not just collaboration-related
history. Be sure to update all call sites that reference this method name.
🪄 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: 0a0195cb-c104-4a46-a26a-5ff0efdfaa62
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (77)
CLAUDE.mdCargo.tomlcrates/flowstate-collab/Cargo.tomlcrates/flowstate-collab/src/binding.rscrates/flowstate-collab/src/ids.rscrates/flowstate-collab/src/lib.rscrates/flowstate-collab/src/local_apply.rscrates/flowstate-collab/src/net/anti_entropy.rscrates/flowstate-collab/src/net/blobs.rscrates/flowstate-collab/src/net/direct.rscrates/flowstate-collab/src/net/mod.rscrates/flowstate-collab/src/net/runtime.rscrates/flowstate-collab/src/net/swarm.rscrates/flowstate-collab/src/presence.rscrates/flowstate-collab/src/projection.rscrates/flowstate-collab/src/proto_direct.rscrates/flowstate-collab/src/proto_gossip.rscrates/flowstate-collab/src/remote_apply.rscrates/flowstate-collab/src/schema.rscrates/flowstate-collab/src/self_check.rscrates/flowstate-collab/src/ticket.rscrates/flowstate-collab/tests/anti_entropy.rscrates/flowstate-collab/tests/convergence.rscrates/flowstate-collab/tests/local_apply.rscrates/flowstate-collab/tests/projection.rscrates/flowstate-collab/tests/translation.rscrates/flowstate/Cargo.tomlcrates/flowstate/src/collab/asset_transfer.rscrates/flowstate/src/collab/mod.rscrates/flowstate/src/collab/presence_view.rscrates/flowstate/src/collab/session.rscrates/flowstate/src/collab/session_io.rscrates/flowstate/src/collab/session_presence.rscrates/flowstate/src/collab/session_timers.rscrates/flowstate/src/collab/share_dialog.rscrates/flowstate/src/collab/shutdown.rscrates/flowstate/src/collab/status.rscrates/flowstate/src/commands.rscrates/flowstate/src/commands/keymap.rscrates/flowstate/src/lib.rscrates/flowstate/src/rich_text_element/mod.rscrates/flowstate/src/workspace/workspace/collab.rscrates/flowstate/src/workspace/workspace/collab_prompts.rscrates/flowstate/src/workspace/workspace/documents.rscrates/flowstate/src/workspace/workspace/keybindings.rscrates/flowstate/src/workspace/workspace/mod.rscrates/flowstate/src/workspace/workspace/render_documents.rscrates/flowstate/src/workspace/workspace/render_status.rscrates/flowstate/src/workspace/workspace/render_top_bar.rscrates/flowstate/src/workspace/workspace/top_bar.rscrates/flowstate/src/workspace/workspace/traits.rscrates/flowstate/src/workspace/workspace/window.rscrates/gpui-flowtext/Cargo.tomlcrates/gpui-flowtext/src/collaboration.rscrates/gpui-flowtext/src/document/blocks.rscrates/gpui-flowtext/src/rich_text/editor/block_insertion.rscrates/gpui-flowtext/src/rich_text/editor/caret_movement.rscrates/gpui-flowtext/src/rich_text/editor/collab_apply.rscrates/gpui-flowtext/src/rich_text/editor/commands.rscrates/gpui-flowtext/src/rich_text/editor/edit_pipeline.rscrates/gpui-flowtext/src/rich_text/editor/lifecycle.rscrates/gpui-flowtext/src/rich_text/editor/media.rscrates/gpui-flowtext/src/rich_text/editor/mod.rscrates/gpui-flowtext/src/rich_text/editor/mouse.rscrates/gpui-flowtext/src/rich_text/editor/movement_core.rscrates/gpui-flowtext/src/rich_text/editor/object_assets.rscrates/gpui-flowtext/src/rich_text/editor/object_selection.rscrates/gpui-flowtext/src/rich_text/editor/paste.rscrates/gpui-flowtext/src/rich_text/editor/render_blocks.rscrates/gpui-flowtext/src/rich_text/editor/serialization.rscrates/gpui-flowtext/src/rich_text/editor/table_equation_editing.rscrates/gpui-flowtext/src/rich_text/editor/tables.rscrates/gpui-flowtext/src/rich_text/layout/block_layout.rscrates/gpui-flowtext/src/rich_text/tests/collab_capture.rscrates/gpui-flowtext/src/rich_text/tests/mod.rshelpers/docs/collab_qa.mdplan.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@fix.md`:
- Around line 298-300: The markdown heading hierarchy in fix.md has a gap
between the level-2 heading WAVE 3 and the level-4 heading V1, which violates
the MD001 rule for contiguous heading levels. Change the V1 heading from four
hash marks (####) to three hash marks (###) to maintain proper heading hierarchy
immediately under WAVE 3.
- Around line 21-40: The markdown code fence for the WAVE plan block is missing
a language tag, which violates MD040 and causes inconsistent rendering. Add the
language tag "text" immediately after the opening triple backticks on line 21
(before the newline), so the fence opens with ```text instead of just ```.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
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 `@crates/flowstate-collab/src/net/runtime.rs`:
- Around line 212-224: The attach_session call on the direct_state object
executes before SwarmHandle::spawn which can fail, leaving a stale attached
session if spawn errors. Move the direct_state.attach_session(session) call to
execute only after the SwarmHandle::spawn completes successfully and
replace_swarm finishes. Apply this same fix at both locations where this pattern
occurs (the one shown in the diff and the second occurrence mentioned in the
comment).
🪄 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: 4a34d9d2-bd30-4286-899d-533a2c1ec439
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (69)
Cargo.tomlcrates/flowstate-collab/Cargo.tomlcrates/flowstate-collab/src/binding.rscrates/flowstate-collab/src/ids.rscrates/flowstate-collab/src/lib.rscrates/flowstate-collab/src/local_apply.rscrates/flowstate-collab/src/net/blobs.rscrates/flowstate-collab/src/net/direct.rscrates/flowstate-collab/src/net/mod.rscrates/flowstate-collab/src/net/runtime.rscrates/flowstate-collab/src/net/swarm.rscrates/flowstate-collab/src/patch_apply.rscrates/flowstate-collab/src/presence.rscrates/flowstate-collab/src/projection.rscrates/flowstate-collab/src/proto_direct.rscrates/flowstate-collab/src/proto_gossip.rscrates/flowstate-collab/src/remote_apply.rscrates/flowstate-collab/src/schema.rscrates/flowstate-collab/src/self_check.rscrates/flowstate-collab/src/ticket.rscrates/flowstate-collab/tests/convergence.rscrates/flowstate-collab/tests/remote_apply.rscrates/flowstate-collab/tests/swarm_loopback.rscrates/flowstate-collab/tests/translation.rscrates/flowstate/Cargo.tomlcrates/flowstate/src/collab/asset_transfer.rscrates/flowstate/src/collab/manager.rscrates/flowstate/src/collab/mod.rscrates/flowstate/src/collab/notify.rscrates/flowstate/src/collab/presence_view.rscrates/flowstate/src/collab/pump.rscrates/flowstate/src/collab/session.rscrates/flowstate/src/collab/session_io.rscrates/flowstate/src/collab/session_presence.rscrates/flowstate/src/collab/session_timers.rscrates/flowstate/src/collab/share_dialog.rscrates/flowstate/src/collab/share_dialog_view.rscrates/flowstate/src/collab/shutdown.rscrates/flowstate/src/collab/status.rscrates/flowstate/src/workspace/icons.rscrates/flowstate/src/workspace/workspace/collab.rscrates/flowstate/src/workspace/workspace/collab_prompts.rscrates/flowstate/src/workspace/workspace/documents.rscrates/flowstate/src/workspace/workspace/mod.rscrates/flowstate/src/workspace/workspace/render_status.rscrates/flowstate/src/workspace/workspace/render_top_bar.rscrates/flowstate/src/workspace/workspace/top_bar.rscrates/flowstate/src/workspace/workspace/traits.rscrates/flowstate/src/workspace/workspace/window.rscrates/gpui-flowtext/src/document/blocks.rscrates/gpui-flowtext/src/persistence/records.rscrates/gpui-flowtext/src/persistence/validation.rscrates/gpui-flowtext/src/rich_text/editor/block_insertion.rscrates/gpui-flowtext/src/rich_text/editor/collab_apply.rscrates/gpui-flowtext/src/rich_text/editor/commands.rscrates/gpui-flowtext/src/rich_text/editor/hit_testing.rscrates/gpui-flowtext/src/rich_text/editor/lifecycle.rscrates/gpui-flowtext/src/rich_text/editor/mod.rscrates/gpui-flowtext/src/rich_text/editor/mouse.rscrates/gpui-flowtext/src/rich_text/editor/object_assets.rscrates/gpui-flowtext/src/rich_text/editor/object_selection.rscrates/gpui-flowtext/src/rich_text/editor/platform.rscrates/gpui-flowtext/src/rich_text/editor/search_highlights.rscrates/gpui-flowtext/src/rich_text/element.rscrates/gpui-flowtext/src/rich_text/tests/collab_capture.rscrates/gpui-flowtext/src/rich_text/tests/edit_layout.rscrates/gpui-flowtext/src/rich_text/tests/persistence_blocks.rshelpers/docs/collab_qa.mdhelpers/docs/collaboration.md
💤 Files with no reviewable changes (4)
- crates/flowstate/src/collab/shutdown.rs
- crates/flowstate/src/workspace/workspace/window.rs
- crates/gpui-flowtext/src/rich_text/tests/edit_layout.rs
- crates/flowstate/src/workspace/workspace/collab_prompts.rs
✅ Files skipped from review due to trivial changes (2)
- helpers/docs/collaboration.md
- helpers/docs/collab_qa.md
🚧 Files skipped from review as they are similar to previous changes (26)
- crates/flowstate/Cargo.toml
- crates/flowstate-collab/src/self_check.rs
- crates/flowstate-collab/Cargo.toml
- crates/flowstate-collab/src/proto_gossip.rs
- crates/flowstate/src/workspace/workspace/render_top_bar.rs
- crates/flowstate/src/collab/status.rs
- crates/flowstate-collab/src/binding.rs
- Cargo.toml
- crates/gpui-flowtext/src/rich_text/editor/object_selection.rs
- crates/gpui-flowtext/src/rich_text/editor/lifecycle.rs
- crates/gpui-flowtext/src/rich_text/editor/mod.rs
- crates/flowstate/src/collab/asset_transfer.rs
- crates/flowstate-collab/src/ticket.rs
- crates/flowstate-collab/src/ids.rs
- crates/flowstate-collab/src/lib.rs
- crates/flowstate-collab/src/proto_direct.rs
- crates/flowstate-collab/src/presence.rs
- crates/flowstate/src/workspace/workspace/documents.rs
- crates/flowstate-collab/src/net/mod.rs
- crates/gpui-flowtext/src/rich_text/editor/collab_apply.rs
- crates/flowstate-collab/src/local_apply.rs
- crates/gpui-flowtext/src/rich_text/editor/block_insertion.rs
- crates/flowstate/src/workspace/workspace/mod.rs
- crates/flowstate-collab/src/remote_apply.rs
- crates/flowstate/src/workspace/workspace/collab.rs
- crates/flowstate/src/collab/session_io.rs
Implemented collaboration-only tracing with non-blocking file logging. What changed: - Added tracing, tracing-subscriber, and tracing-appender. - Added crates/flowstate/src/logging.rs using tracing_appender::non_blocking, so log writing runs on its own worker thread. - Logging is initialized from main and the WorkerGuard is held for app lifetime. - Added structured collaboration logs for: - session create, join, attach, leave, shutdown - gossip subscribe/publish/receive/lag/neighbors - direct snapshot/update/blob/asset pulls and serves - text sync import/export, local edit capture, remote patches - presence publish/apply and peer counts - anti-entropy digest decisions and recovery pulls - asset transfer and validation - offline/reconnect/recovery/self-check drift - relevant join/share workspace errors Defaults: - Logs go to flowstate_data_dir()/logs as daily rolling flowstate.log... files. - Override with FLOWSTATE_LOG_DIR=/path/to/logs. - Default filter is collaboration-only: - flowstate::collab=trace - flowstate::workspace::workspace=trace - flowstate_collab=trace - Override with FLOWSTATE_LOG or RUST_LOG. Validation: - cargo clippy -p flowstate -p flowstate-collab passes. - git diff --check passes.
… into fable-collab
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/flowstate/src/collab/session_io.rs`:
- Around line 105-110: The malformed digest payload handling in the
VersionVector::decode section is currently returning an Err that propagates up
to the caller in session.rs at line 724, causing fatal session detachment.
Instead of returning Err when the decode fails, handle the malformed digest
gracefully by logging the warning (already done) and returning a success value
that indicates the digest was invalid or should be skipped, preventing the
session detachment. This treats the malformed digest as invalid input to be
ignored rather than as a fatal local error.
- Around line 24-35: The pending_remote_updates buffer in the session attachment
code path has no size constraints and will grow unbounded when updates are
received before the session is fully attached. Add a check before pushing bytes
to self.pending_remote_updates to enforce a maximum cap on either the number of
pending updates or total byte count. If the cap is exceeded, either drop the
update, reject it with an error, or implement appropriate backpressure to
prevent memory exhaustion from bursty or malicious traffic during session join.
In `@crates/flowstate/src/logging.rs`:
- Line 9: The logging configuration uses daily log rotation but does not
implement any cleanup or retention policy for old log files, causing unbounded
disk growth over time. Configure log file retention and cleanup in the logger
setup (around the DEFAULT_LOG_FILTER constant and logger initialization at lines
26-27) by adding a retention policy that automatically deletes old log files
after a configurable period (e.g., 7-30 days) or limits the total number of
retained log files. This ensures disk space is managed responsibly while still
maintaining useful logs for debugging.
🪄 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: 3c954e8d-3dd0-483a-a2a1-7ed353499dcf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlcrates/flowstate-collab/Cargo.tomlcrates/flowstate-collab/src/net/anti_entropy.rscrates/flowstate-collab/src/net/direct.rscrates/flowstate-collab/src/net/mod.rscrates/flowstate-collab/src/net/runtime.rscrates/flowstate-collab/src/net/swarm.rscrates/flowstate-collab/src/proto_gossip.rscrates/flowstate/Cargo.tomlcrates/flowstate/src/collab/asset_transfer.rscrates/flowstate/src/collab/manager.rscrates/flowstate/src/collab/pump.rscrates/flowstate/src/collab/session.rscrates/flowstate/src/collab/session_io.rscrates/flowstate/src/collab/session_presence.rscrates/flowstate/src/collab/session_timers.rscrates/flowstate/src/lib.rscrates/flowstate/src/logging.rscrates/flowstate/src/main.rscrates/flowstate/src/workspace/workspace/collab.rs
✅ Files skipped from review due to trivial changes (1)
- crates/flowstate/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/flowstate-collab/src/proto_gossip.rs
- Cargo.toml
- crates/flowstate-collab/src/net/mod.rs
- crates/flowstate-collab/src/net/runtime.rs
- crates/flowstate/src/collab/asset_transfer.rs
- crates/flowstate-collab/src/net/swarm.rs
- crates/flowstate-collab/src/net/direct.rs
- crates/flowstate/src/workspace/workspace/collab.rs
- crates/flowstate/src/collab/pump.rs
- crates/flowstate/src/collab/session_timers.rs
- crates/flowstate/src/collab/manager.rs
- crates/flowstate/src/collab/session_presence.rs
Comment on lines
+24
to
+35
| if self.doc.is_none() || self.binding.is_none() || self.editor.is_none() { | ||
| tracing::debug!( | ||
| session = %self.session, | ||
| bytes = bytes.len(), | ||
| queued_updates = self.pending_remote_updates.len() + 1, | ||
| has_doc = self.doc.is_some(), | ||
| has_binding = self.binding.is_some(), | ||
| has_editor = self.editor.is_some(), | ||
| "queueing remote collaboration update until session is attached", | ||
| ); | ||
| self.pending_remote_updates.push(bytes.to_vec()); | ||
| return Ok(()); |
Contributor
There was a problem hiding this comment.
Bound pre-attach remote update buffering to avoid memory blowups.
When the session is not fully attached, every inbound update is appended to pending_remote_updates with no byte/count cap. Under bursty or malicious traffic during join, this can grow without bound and exhaust memory before attach completes.
💡 Suggested direction
+// Example guardrails (tune values):
+const MAX_PENDING_REMOTE_UPDATES: usize = 512;
+const MAX_PENDING_REMOTE_UPDATE_BYTES: usize = 8 * 1024 * 1024;
pub fn import_update_bytes(&mut self, bytes: &[u8], cx: &mut Context<Self>) -> Result<()> {
if self.doc.is_none() || self.binding.is_none() || self.editor.is_none() {
+ let queued_bytes: usize = self.pending_remote_updates.iter().map(Vec::len).sum();
+ if self.pending_remote_updates.len() >= MAX_PENDING_REMOTE_UPDATES
+ || queued_bytes.saturating_add(bytes.len()) > MAX_PENDING_REMOTE_UPDATE_BYTES
+ {
+ tracing::warn!(session = %self.session, queued = self.pending_remote_updates.len(), queued_bytes, incoming = bytes.len(), "remote update queue limit exceeded during attach/join");
+ // choose one policy: drop oldest/newest, request resync, or fail join explicitly
+ return Ok(());
+ }
self.pending_remote_updates.push(bytes.to_vec());
return Ok(());
}🤖 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 `@crates/flowstate/src/collab/session_io.rs` around lines 24 - 35, The
pending_remote_updates buffer in the session attachment code path has no size
constraints and will grow unbounded when updates are received before the session
is fully attached. Add a check before pushing bytes to
self.pending_remote_updates to enforce a maximum cap on either the number of
pending updates or total byte count. If the cap is exceeded, either drop the
update, reject it with an error, or implement appropriate backpressure to
prevent memory exhaustion from bursty or malicious traffic during session join.
Eliminates the whole-document rewrite/reprojection on every non-trivial edit while preserving the single-root LoroText design and CRDT convergence. - T1: maintained paragraph->byte Fenwick offset index in DocBinding (new body_index.rs); range helpers no longer stringify the whole body. - T2: incremental ReplaceParagraphSpan — span-scoped minimal body splice, mark carry-over, targeted block-row reconcile; replace_document fallback. - T4: delta-driven remote reconcile — single-paragraph fast path via the Loro TextDelta + slice_delta; falls back to full reprojection on mark changes / newline changes / multi-region edits. - T6-T9: editor keystroke-path incrementalization (skip section rebuild on content edits, in-place block update, O(log n)+shift offset update). - T10: O(1) DocumentIdentityMap::paragraph_index via FxHashMap. - T12: remove dead pre-CRDT op-replay path + postcard wire format. - T15: cheap self-check via cached version-vector/projection hash. - T14: mark plan.md/fix.md superseded by FIX_LORO_ROOT.md. Adds a deterministic convergence regression and tests/perf_smoke.rs proving edit-proportional local deltas and O(1) remote paragraph reconcile. T3/T5 (dead-in-prod/optional) and T11/T13 (breaking/higher-risk) deferred.
- logging: default to `error` only; `FLOWSTATE_LOG_LEVEL` (error|warn|info| debug|trace) raises Flowstate's own crates, `FLOWSTATE_LOG`/`RUST_LOG` give full EnvFilter control, `FLOWSTATE_LOG_STDOUT` also mirrors logs to stdout. - README: document the logging environment variables and precedence. - clippy: add missing trailing `;` in session_io asset-serve warn arm (semicolon_if_nothing_returned); `cargo clippy --workspace --all-targets` is now clean.
remove_subtree was called but never defined in b459052 (broken build); implement it cycle-safely. Clippy/build fixes: collapsible-if and bool-to-int (flowstate-document), renamed fn param + unused AssetId import (flowstate-collab), Iterator::last -> next_back and box large DocumentRuntimeSource variant (flowstate), private EditRecord visibility + pt->px test helper (gpui-flowtext). flowstate-flow: log corrupt .fl0 loads via tracing instead of silently discarding them. collab: after an external save/checkpoint records a named revision into Loro, refresh the session version-vector and publish an anti-entropy digest so peers converge on the new revision op promptly.
Implements every outstanding item from adjustmentplan.md surfaced by a full section-by-section audit, verified by cargo test (190 pass) + cargo clippy (0 errors/0 warnings in-tree). flowstate-document: - §11 section page structure (page size/margins/columns/orientation/numbering + header/footer flows) in schema, import (read+write), and projection. - §19 integrity index chunk; §27 schema-migration records (infra). - §28 container-id resolution used in the projector (not just stored). - §29 native LoroMap::clear/LoroMovableList::clear. - §31 AssetMap image dimensions; §15 register_user helper. gpui-flowtext: - §16 real affinity/gravity: EditorSelection carries+consumes affinity and VisualGravity drives caret placement at soft-wrap seams (no longer derived). - §11 DocumentSection.page payload. flowstate-collab: - §16 affinity sourced from the editor selection (endpoint_intent removed). - §22 missing_dependency_request wired into pending-import tracing. - §23 permanent subscription now filters by origin, trigger, per-event frontier and a runtime epoch. - §24 full projection index set (paragraph/block/table/style/section/asset/ search/cursor-cache) materialized and used for O(1) lookups + invalidation. - §28 container-id fast-path resolution; §29 native clear; §15 set_author_identity. flowstate-docx: - §26 structured DOCX import: tables, images (OPC media), and equations (OMML->LaTeX via new interpreter/omml.rs + interpreter/structured.rs). - §26 export embeds JPEG as well as PNG. flowstate: - §11/§15 wiring: stable local user identity in app settings, threaded through the runtime actor (SetAuthorIdentity) so revisions carry durable authorship; collaborating saves refresh anti-entropy. Also fixes a redo-selection regression (removed an over-aggressive clear_pending_undo_selection) and updates the segment-count test for Loro-native revision recording.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI/UX Enhancements
Bug Fixes
Tests & Documentation