-
Notifications
You must be signed in to change notification settings - Fork 62
Hazel in Patchwork #1959
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
Open
disconcision
wants to merge
118
commits into
dev
Choose a base branch
from
patchwork
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Hazel in Patchwork #1959
Conversation
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
- Remove dead code: HazelEmbed no longer sends init to iframe on load (the iframe ignored this message anyway) - Add message source verification in HazelEmbed to only accept messages from its own iframe, improving security with multiple embeds - Update documentation to clarify the one-way init handshake: iframe signals readiness, parent responds with state - Add comment in PatchworkComm.re explaining the vestigial init handler Co-Authored-By: Claude Opus 4.5 <[email protected]>
The init message is now strictly one-way (iframe → parent). Parent responds with state, not init. This removes the dead U_s1_init case from PatchworkComm.re and updates the generated OCaml types. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Hide mode menu and contents in Patchwork mode (Editors.re) - Skip documentation slides in Patchwork mode (Init.re) - Add iframe guard to sync_to_parent wrapper (Editor.re, SyncReplace.re) - Default zen mode on in iframe, off otherwise (Settings.re) - Gracefully handle unknown postMessage types (PatchworkComm.re) - Remove DocGraph component and unused dependencies from embed - Clean up perf logging in HazelEmbed.tsx Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add PerfLog.log() for simple messages (not just timing) - Convert all direct [PERF] console.log calls to use PerfLog - Set PerfLog.enabled = false by default - Remove unused pieces_in_doc to break dependency cycle Perf logging can be re-enabled with PerfLog.enable(). Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Update patchwork-integration.md: - Add Echo Loop Prevention section - Replace Future Work stub with consolidated content - Fix directory structure (remove deleted DocGraph.tsx) - Move patchwork-profiling.md from plan/ to docs/ - Remove plan/ directory and obsolete docs: - patchwork-future.md (merged into integration doc) - patchwork-automerge-patches-REVISED.md (implemented) - patchwork-automerge-patches-plan.md (implemented) - patchwork-caret-shard-fix.md (implemented) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Undo/redo are handled at the History level (swapping Page.Model snapshots), which bypasses Editor.Update where sync_to_parent is called. This adds a bridge to sync undo/redo state changes: - Add PatchworkUndoSync module to extract zippers from Page.Model and call sync after undo/redo - Refactor SyncReplace to factor out core sync logic: - send_state_delta: state sync without action check - send_caret_position: caret sync without action check - sync_for_undo: combines both for undo/redo use - Call PatchworkUndoSync.sync in History.re after undo/redo Known issue: Receiver can intermittently crash with "Piece not found" exception when the delta references pieces not in the merged doc. Documented in patchwork-integration.md for future investigation. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Hazel's tree structure means deleted pieces vanish, but Automerge's flat map keeps pieces until explicitly removed. Without explicit deletion, deleted pieces become orphans in Automerge, breaking undo/redo sync: restored pieces already exist unchanged and aren't forwarded to other clients, causing "Piece not found" crashes. Changes: - Add `deleted` field to EditorState message protocol - PatchworkComm.re now sends deleted IDs (already computed, just not sent) - tool.tsx deletes pieces from Automerge when received - Document the tree vs flat map mismatch in patchwork-integration.md Co-Authored-By: Claude Opus 4.5 <[email protected]>
Mark stale caret cleanup and optimized forwarding as done in Future Work section. Update Current Limitations to reflect that only anonymous users may see brief duplicate carets. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Check if window is defined before accessing window.parent. In Node.js there is no window object, so accessing parent would fail. Now safely returns false in non-browser environments. Co-Authored-By: Claude Opus 4.5 <[email protected]>
When the cursor's piece is deleted during sync, we fall back to ancestors. Previously we'd end up at the first child of the ancestor. Now we track which child the cursor was in and position there (or the closest available child if that one was deleted). Co-Authored-By: Claude Opus 4.5 <[email protected]>
When the cursor's piece is deleted during sync, before falling back to ancestor-based positioning, first try to find the rightmost same-segment predecessor that survives. These are pieces in fst(siblings) at each level of the zipper - pieces that were lexically before the cursor in the same segment scope. This gives better results when content before the cursor in the same scope survives (e.g., foo + bar + |baz becomes foo + bar + | when baz is deleted), while still falling back to structural positioning when the cursor was at the start of a child with no predecessors. Co-Authored-By: Claude Opus 4.5 <[email protected]>
When a selection fragments a multi-shard tile (e.g., selecting just "("
in "(1)"), both fragments share the same ID but have different shards.
The flat doc representation could only hold one entry per ID, causing
one fragment to be lost. This led to corrupted sync state like "1(1)"
on the receiver.
Fix: Use Zipper.unselect_and_zip instead of Zipper.zip when converting
zippers to segments for sync. This reassembles fragments before
flattening, ensuring the flat doc has proper tile structure.
Also adds:
- Tile structure validation logging in FlatConvert and PatchworkComm
- Detailed delta logging for debugging sync issues
- Warning comment on Zipper.zip recommending unselect_and_zip
- Future work note about preserving selections during sync
Co-Authored-By: Claude Opus 4.5 <[email protected]>
When receiving remote edits, the local user selection is now restored after cursor repositioning. The approach: - Save the leftmost piece of the selection (ID + shards) before sync - After cursor repositioning succeeds exactly, grow selection leftward until we find that piece - Restore the original focus direction so caret ends up on correct side - Only restore Normal mode selections, not Buffer selections - Use shards field matching to handle fragmented multi-delimiter forms Also fixes cursor repositioning when selection covers entire segment by using a piece from the selection edge as anchor when siblings are empty. Co-Authored-By: Claude Opus 4.5 <[email protected]>
… remove completed items - Add Remote Selection Rendering section with wire protocol and implementation approach - Remove completed future work items (stale caret cleanup, caret forwarding optimization, selection preservation) - Minor formatting cleanup Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add 'before' field to EditorState message for granular CRDT ops - Track changed_before in delta computation (PatchworkComm.re) - Add SegmentValidator module for post-sync invariant checking - Validate shards/children, segment shape, and UUID uniqueness See docs/automerge-granular-sync.md for design rationale. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Try it out:
automerge:27ShjXmwBQPAovhh9Sgd5L5ue1a1. This registers a HazelDoc Patchwork datatype and tool, which will enable you to create and view new HazelDocs....menu at the top right.Relevant external URLs:
patchwork pushif changes are made; see patchwork-integration.md in the docs directory for detailed instructions.automerge:27ShjXmwBQPAovhh9Sgd5L5ue1a1Hazel in Patchwork
This adds supports for collaborative multiplayer hazel programs via patchwork+automerge.
In particular:
Minor limitations:
Major limitation:
let x = 1 iwhere one user appends andnat the same time as another backspaces thet. The only other situation which definitely causes clobbering are when two users edit the same token at the same time. There's no intrinsic complexity here; we'd just need to switch the token type to the automerge text CRDT; it's just not totally clear to me if that's desirable behavior for all token classes. It might be reasonable for a comment, but when editing a variable name perhaps clobbering is better? Unsure.Appendix: Restructuring operations versus concurrent edits.
Note: The below is now outdated, after implementing the plan specified in automerge-granular-sync.md. However it is retained as an intuition pump for the impedance mismatch which has now been partially addressed. In particular, both of the examples at the end now work fine concurrently
For example, consider the following program:
This will have the following (simplified) flattened rep:
Our basic setup handles concurrent edits to individual entries in the above list, but concurrent edits to the same entry will result in clobbering. So, for example, any two atomic tokens can be concurrently edited. But, any addition to the root segment, for example adding a new let definition (say, for a new variable z) between the two existing one while another user adds a
+ zto the end of the program, will result in clobbering. Additionally, if a user tries to edit the pattern for the definition ofx, say changing it tox, w, while another user edits the definition similarly, say changing1 + (2 * 3)to1 + (2 * 3), 4, these will get clobbered because, even though they are not edits to the same segment, they are nonetheless both edits to the list of lists for node A.