feat: add durable document sessions and workspace polish#25
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
15 issues found across 82 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docmostly/Features/Editor/NativeEditorJavaScriptCRDTDocumentEngine.swift">
<violation number="1" location="docmostly/Features/Editor/NativeEditorJavaScriptCRDTDocumentEngine.swift:217">
P3: Local CRDT updates are committed through two delivery paths when collaboration is active: the returned array is committed directly and the same update is still forwarded through `localUpdates()`. Although the digest check usually suppresses a second insert, this adds a racing persistence call for every edit; the commit path should drain updates without also publishing them to the raw stream, or use only one delivery path.</violation>
</file>
<file name="docmostly/Features/PageTree/PageTreeNodeArray.swift">
<violation number="1" location="docmostly/Features/PageTree/PageTreeNodeArray.swift:27">
P2: A parent that became empty locally can permanently hide children added later: this line marks the preserved empty array as loaded, so `PageTreeViewModel.toggle` returns before fetching when a refresh reports that the parent has children again. Preserving only when `previousNode.hasChildren == node.hasChildren` keeps newly added children discoverable.</violation>
</file>
<file name="docmostly/Features/PageTree/PageTreeView.swift">
<violation number="1" location="docmostly/Features/PageTree/PageTreeView.swift:120">
P2: The recent-pages rail can remain stale when a page/favorite revision changes during the initial load: the restarted task hits this guard and exits, then no task is triggered when initialization later completes. The initialization gate would be safer if a pending revision-triggered refresh were preserved or the initialization state were included in the task identity.</violation>
</file>
<file name="docmostly/Features/Editor/NativeEditorCollaborationPresenceClient.swift">
<violation number="1" location="docmostly/Features/Editor/NativeEditorCollaborationPresenceClient.swift:295">
P2: A failure while recording the sent update now permanently stops forwarding later local updates for the current WebSocket session, even though the frame itself was already sent. Keeping the sender alive and leaving the update pending for reconnect would prevent subsequent edits from being stranded until the next reconnect.</violation>
</file>
<file name="docmostly/Features/PageTree/PageTreeViewModel.swift">
<violation number="1" location="docmostly/Features/PageTree/PageTreeViewModel.swift:36">
P2: Refreshing the page tree can show moved descendants twice or retain deleted/stale nested pages. This call preserves the entire previously loaded subtree after only the root records are refreshed, and `isChildrenLoaded` then prevents a later expand from fetching the current children; preserving loaded subtrees would need reconciliation or invalidation when the root refreshes.</violation>
</file>
<file name="docmostly/Features/Favorites/FavoritesViewModel.swift">
<violation number="1" location="docmostly/Features/Favorites/FavoritesViewModel.swift:47">
P2: Refreshing favorites while a next-page request is in flight can append that request's stale results to the newly refreshed page set, producing mixed or incorrect pagination. The initial-load path could remain blocked while `isLoadingNextPage` is true (or otherwise invalidate the next-page request before replacing `pages`).</violation>
</file>
<file name="docmostly/Features/Editor/DocumentSessionRegistry.swift">
<violation number="1" location="docmostly/Features/Editor/DocumentSessionRegistry.swift:58">
P2: A session can be reinserted after `removeAll()` has cleared the registry. When logout or server switching races with session creation, the canceled creation may still finish and this assignment caches the old user/workspace session, allowing a later lookup to reuse it; guarding the result with a generation/token or verifying the task is still registered would prevent stale resurrection.</violation>
</file>
<file name="docmostly/DocmostlyCRDTRuntime.js">
<violation number="1" location="docmostly/DocmostlyCRDTRuntime.js:13332">
P2: A subsequent remote body update can restore a stale page title because this snapshot always includes the runtime's last local/seed title, even though remote title mutations are handled outside the Yjs document. Keeping title metadata out of this body snapshot, or synchronizing the runtime title when the remote page-title event is applied, would prevent title reversion and false conflicts.</violation>
</file>
<file name="docmostly/App/AppState+Management.swift">
<violation number="1" location="docmostly/App/AppState+Management.swift:30">
P2: After changing an emoji, the page tree can continue showing the old icon because this writes only the editable-page cache; `CachedPageTreeItem` is not updated, and the tree view is not reloaded by `pageDiscoveryRevision`. Updating the tree projection as part of the icon mutation would keep the sidebar consistent online and offline.</violation>
<violation number="2" location="docmostly/App/AppState+Management.swift:30">
P1: An icon-only response can erase the cached document when the server omits `content`: `saveEditablePage` treats a missing document as empty before updating the cache. Preserving the existing cached document (or updating only the cached icon) avoids losing the page body for subsequent offline/native loads.</violation>
</file>
<file name="docmostly/Features/Editor/DocumentSession.swift">
<violation number="1" location="docmostly/Features/Editor/DocumentSession.swift:68">
P2: A window opened after this session has already processed an update can remain on its stale initial document because the stream replays only the immutable `initialSnapshot`; the session does not retain or replay its latest published snapshot. Replaying the latest projection to new subscribers (or making this subscription fetch the current kernel snapshot) would keep shared windows synchronized.</violation>
</file>
<file name="docmostly/Features/Editor/NativeRichEditorViewModel+Collaboration.swift">
<violation number="1" location="docmostly/Features/Editor/NativeRichEditorViewModel+Collaboration.swift:340">
P1: Reopened read-only drafts can lose or corrupt content when editing resumes: the UI is restored from `initialSnapshot`, but the CRDT engine is still seeded with the original page and is marked ready for local changes. Retained drafts should be promoted/seeding the engine before this snapshot is treated as an editable CRDT baseline, or kept non-editable until that happens.</violation>
</file>
<file name="docmostly/Features/Editor/NativeEditorCRDTSyncCoordinator.swift">
<violation number="1" location="docmostly/Features/Editor/NativeEditorCRDTSyncCoordinator.swift:29">
P2: Reconnects can resend local updates that were already included in the durable pending-update replay: this stream retains elements after its sender is cancelled, while `outboundFramesAfterAuthentication()` separately reloads and sends those same updates. A per-connection subscription or ack-aware queue that removes/reconciles buffered entries during reconnect would avoid duplicate frames and unbounded buffering.</violation>
</file>
<file name="docmostly/Features/Editor/NativeEditorCollaborationSyncDriver.swift">
<violation number="1" location="docmostly/Features/Editor/NativeEditorCollaborationSyncDriver.swift:16">
P1: Receive-only sessions now upload and acknowledge all persisted local CRDT updates during authentication, even though they are explicitly not allowed to send local document updates. This can push edits from a viewer/read-only connection and remove them from the pending queue; gate pending replay/acknowledgement on `context.allowsLocalDocumentUpdates(for:)` while retaining the initial state-vector sync for read-only sessions.</violation>
</file>
<file name="docmostly/Persistence/DocumentLocalPersistencePeer.swift">
<violation number="1" location="docmostly/Persistence/DocumentLocalPersistencePeer.swift:162">
P3: A local update compacted before it is sent keeps its full payload after `markPushed`, causing acknowledged document bodies to accumulate in storage and be excluded from future compaction metrics. Clearing the payload when acknowledging an update already covered by `snapshotSequence` would preserve replay behavior while preventing this storage leak.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let page: DocmostEditablePage = try await apiClient.send(.updatePage(pageId: pageId, icon: icon)) | ||
| isOffline = false | ||
| if let cacheScope { | ||
| scheduleCacheWrite(.saveEditablePage(page, scope: cacheScope)) |
There was a problem hiding this comment.
P1: An icon-only response can erase the cached document when the server omits content: saveEditablePage treats a missing document as empty before updating the cache. Preserving the existing cached document (or updating only the cached icon) avoids losing the page body for subsequent offline/native loads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/App/AppState+Management.swift, line 30:
<comment>An icon-only response can erase the cached document when the server omits `content`: `saveEditablePage` treats a missing document as empty before updating the cache. Preserving the existing cached document (or updating only the cached icon) avoids losing the page body for subsequent offline/native loads.</comment>
<file context>
@@ -19,6 +19,20 @@ extension AppState {
+ let page: DocmostEditablePage = try await apiClient.send(.updatePage(pageId: pageId, icon: icon))
+ isOffline = false
+ if let cacheScope {
+ scheduleCacheWrite(.saveEditablePage(page, scope: cacheScope))
+ }
+ markPageDiscoveryChanged()
</file context>
| crdtSyncCoordinator = session.syncCoordinator | ||
| isCRDTEngineReadyForLocalChanges = restoredLocalState || | ||
| session.documentEngine.requiresInitialRemoteSnapshot == false | ||
| if let initialSnapshot = session.initialSnapshot { |
There was a problem hiding this comment.
P1: Reopened read-only drafts can lose or corrupt content when editing resumes: the UI is restored from initialSnapshot, but the CRDT engine is still seeded with the original page and is marked ready for local changes. Retained drafts should be promoted/seeding the engine before this snapshot is treated as an editable CRDT baseline, or kept non-editable until that happens.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/Editor/NativeRichEditorViewModel+Collaboration.swift, line 340:
<comment>Reopened read-only drafts can lose or corrupt content when editing resumes: the UI is restored from `initialSnapshot`, but the CRDT engine is still seeded with the original page and is marked ready for local changes. Retained drafts should be promoted/seeding the engine before this snapshot is treated as an editable CRDT baseline, or kept non-editable until that happens.</comment>
<file context>
@@ -325,17 +328,17 @@ extension NativeRichEditorViewModel {
+ crdtSyncCoordinator = session.syncCoordinator
+ isCRDTEngineReadyForLocalChanges = restoredLocalState ||
+ session.documentEngine.requiresInitialRemoteSnapshot == false
+ if let initialSnapshot = session.initialSnapshot {
+ applyInitialCRDTDocumentSnapshot(initialSnapshot)
}
</file context>
| [try await frame(for: coordinator.makeInitialSyncMessage())] | ||
| let initialMessage = try await coordinator.makeInitialSyncMessage() | ||
| let initial = frame(for: initialMessage) | ||
| initialPendingUpdates = try await coordinator.pendingLocalUpdates() |
There was a problem hiding this comment.
P1: Receive-only sessions now upload and acknowledge all persisted local CRDT updates during authentication, even though they are explicitly not allowed to send local document updates. This can push edits from a viewer/read-only connection and remove them from the pending queue; gate pending replay/acknowledgement on context.allowsLocalDocumentUpdates(for:) while retaining the initial state-vector sync for read-only sessions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/Editor/NativeEditorCollaborationSyncDriver.swift, line 16:
<comment>Receive-only sessions now upload and acknowledge all persisted local CRDT updates during authentication, even though they are explicitly not allowed to send local document updates. This can push edits from a viewer/read-only connection and remove them from the pending queue; gate pending replay/acknowledgement on `context.allowsLocalDocumentUpdates(for:)` while retaining the initial state-vector sync for read-only sessions.</comment>
<file context>
@@ -3,14 +3,32 @@ import Foundation
- [try await frame(for: coordinator.makeInitialSyncMessage())]
+ let initialMessage = try await coordinator.makeInitialSyncMessage()
+ let initial = frame(for: initialMessage)
+ initialPendingUpdates = try await coordinator.pendingLocalUpdates()
+ var pending: [Data] = []
+ pending.reserveCapacity(initialPendingUpdates.count)
</file context>
|
|
||
| var refreshedNode = node | ||
| refreshedNode.children = previousNode.children | ||
| refreshedNode.isChildrenLoaded = true |
There was a problem hiding this comment.
P2: A parent that became empty locally can permanently hide children added later: this line marks the preserved empty array as loaded, so PageTreeViewModel.toggle returns before fetching when a refresh reports that the parent has children again. Preserving only when previousNode.hasChildren == node.hasChildren keeps newly added children discoverable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/PageTree/PageTreeNodeArray.swift, line 27:
<comment>A parent that became empty locally can permanently hide children added later: this line marks the preserved empty array as loaded, so `PageTreeViewModel.toggle` returns before fetching when a refresh reports that the parent has children again. Preserving only when `previousNode.hasChildren == node.hasChildren` keeps newly added children discoverable.</comment>
<file context>
@@ -7,6 +7,28 @@ nonisolated extension Array where Element == PageTreeNode {
+
+ var refreshedNode = node
+ refreshedNode.children = previousNode.children
+ refreshedNode.isChildrenLoaded = true
+ return refreshedNode
+ }
</file context>
| await loadInitialSpaceState() | ||
| } | ||
| .task(id: pageBrowserTaskKey) { | ||
| guard initializedBrowserSpaceID == space.id else { return } |
There was a problem hiding this comment.
P2: The recent-pages rail can remain stale when a page/favorite revision changes during the initial load: the restarted task hits this guard and exits, then no task is triggered when initialization later completes. The initialization gate would be safer if a pending revision-triggered refresh were preserved or the initialization state were included in the task identity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/PageTree/PageTreeView.swift, line 120:
<comment>The recent-pages rail can remain stale when a page/favorite revision changes during the initial load: the restarted task hits this guard and exits, then no task is triggered when initialization later completes. The initialization gate would be safer if a pending revision-triggered refresh were preserved or the initialization state were included in the task identity.</comment>
<file context>
@@ -113,9 +114,10 @@ struct PageTreeView: View {
+ await loadInitialSpaceState()
}
.task(id: pageBrowserTaskKey) {
+ guard initializedBrowserSpaceID == space.id else { return }
await refreshBrowser()
}
</file context>
| let page: DocmostEditablePage = try await apiClient.send(.updatePage(pageId: pageId, icon: icon)) | ||
| isOffline = false | ||
| if let cacheScope { | ||
| scheduleCacheWrite(.saveEditablePage(page, scope: cacheScope)) |
There was a problem hiding this comment.
P2: After changing an emoji, the page tree can continue showing the old icon because this writes only the editable-page cache; CachedPageTreeItem is not updated, and the tree view is not reloaded by pageDiscoveryRevision. Updating the tree projection as part of the icon mutation would keep the sidebar consistent online and offline.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/App/AppState+Management.swift, line 30:
<comment>After changing an emoji, the page tree can continue showing the old icon because this writes only the editable-page cache; `CachedPageTreeItem` is not updated, and the tree view is not reloaded by `pageDiscoveryRevision`. Updating the tree projection as part of the icon mutation would keep the sidebar consistent online and offline.</comment>
<file context>
@@ -19,6 +19,20 @@ extension AppState {
+ let page: DocmostEditablePage = try await apiClient.send(.updatePage(pageId: pageId, icon: icon))
+ isOffline = false
+ if let cacheScope {
+ scheduleCacheWrite(.saveEditablePage(page, scope: cacheScope))
+ }
+ markPageDiscoveryChanged()
</file context>
| let id = UUID() | ||
| let pair = AsyncStream.makeStream(of: NativeEditorCRDTDocumentSnapshot.self) | ||
| snapshotContinuations[id] = pair.continuation | ||
| if let initialSnapshot { |
There was a problem hiding this comment.
P2: A window opened after this session has already processed an update can remain on its stale initial document because the stream replays only the immutable initialSnapshot; the session does not retain or replay its latest published snapshot. Replaying the latest projection to new subscribers (or making this subscription fetch the current kernel snapshot) would keep shared windows synchronized.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/Editor/DocumentSession.swift, line 68:
<comment>A window opened after this session has already processed an update can remain on its stale initial document because the stream replays only the immutable `initialSnapshot`; the session does not retain or replay its latest published snapshot. Replaying the latest projection to new subscribers (or making this subscription fetch the current kernel snapshot) would keep shared windows synchronized.</comment>
<file context>
@@ -0,0 +1,251 @@
+ let id = UUID()
+ let pair = AsyncStream.makeStream(of: NativeEditorCRDTDocumentSnapshot.self)
+ snapshotContinuations[id] = pair.continuation
+ if let initialSnapshot {
+ pair.continuation.yield(initialSnapshot)
+ }
</file context>
| pendingLocalUpdatesProvider: @escaping @Sendable () async throws -> [Data] = { [] }, | ||
| localUpdateDidSend: @escaping @Sendable (Data) async throws -> Void = { _ in } | ||
| ) { | ||
| let localUpdates = AsyncStream.makeStream(of: Data.self) |
There was a problem hiding this comment.
P2: Reconnects can resend local updates that were already included in the durable pending-update replay: this stream retains elements after its sender is cancelled, while outboundFramesAfterAuthentication() separately reloads and sends those same updates. A per-connection subscription or ack-aware queue that removes/reconciles buffered entries during reconnect would avoid duplicate frames and unbounded buffering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/Editor/NativeEditorCRDTSyncCoordinator.swift, line 29:
<comment>Reconnects can resend local updates that were already included in the durable pending-update replay: this stream retains elements after its sender is cancelled, while `outboundFramesAfterAuthentication()` separately reloads and sends those same updates. A per-connection subscription or ack-aware queue that removes/reconciles buffered entries during reconnect would avoid duplicate frames and unbounded buffering.</comment>
<file context>
@@ -10,15 +10,35 @@ actor NativeEditorCRDTSyncCoordinator {
+ pendingLocalUpdatesProvider: @escaping @Sendable () async throws -> [Data] = { [] },
+ localUpdateDidSend: @escaping @Sendable (Data) async throws -> Void = { _ in }
) {
+ let localUpdates = AsyncStream.makeStream(of: Data.self)
self.documentEngine = documentEngine
self.remoteUpdateHandler = remoteUpdateHandler
</file context>
| private func drainRuntimeOutputs() throws { | ||
| try drainLocalUpdates() | ||
| private func drainRuntimeOutputs() throws -> [Data] { | ||
| let updates = try drainLocalUpdates() |
There was a problem hiding this comment.
P3: Local CRDT updates are committed through two delivery paths when collaboration is active: the returned array is committed directly and the same update is still forwarded through localUpdates(). Although the digest check usually suppresses a second insert, this adds a racing persistence call for every edit; the commit path should drain updates without also publishing them to the raw stream, or use only one delivery path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/Editor/NativeEditorJavaScriptCRDTDocumentEngine.swift, line 217:
<comment>Local CRDT updates are committed through two delivery paths when collaboration is active: the returned array is committed directly and the same update is still forwarded through `localUpdates()`. Although the digest check usually suppresses a second insert, this adds a racing persistence call for every edit; the commit path should drain updates without also publishing them to the raw stream, or use only one delivery path.</comment>
<file context>
@@ -168,31 +213,33 @@ final class NativeEditorJSCRDTDocumentEngine: NativeEditorCRDTDocumentEngine {
- private func drainRuntimeOutputs() throws {
- try drainLocalUpdates()
+ private func drainRuntimeOutputs() throws -> [Data] {
+ let updates = try drainLocalUpdates()
for snapshot in try takeDocumentSnapshots() {
snapshotContinuation.yield(snapshot)
</file context>
| guard matches.isEmpty == false else { return } | ||
|
|
||
| let maximumSequence = matches.reduce(Int64(0)) { result, update in | ||
| update.isPushed = true |
There was a problem hiding this comment.
P3: A local update compacted before it is sent keeps its full payload after markPushed, causing acknowledged document bodies to accumulate in storage and be excluded from future compaction metrics. Clearing the payload when acknowledging an update already covered by snapshotSequence would preserve replay behavior while preventing this storage leak.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Persistence/DocumentLocalPersistencePeer.swift, line 162:
<comment>A local update compacted before it is sent keeps its full payload after `markPushed`, causing acknowledged document bodies to accumulate in storage and be excluded from future compaction metrics. Clearing the payload when acknowledging an update already covered by `snapshotSequence` would preserve replay behavior while preventing this storage leak.</comment>
<file context>
@@ -0,0 +1,523 @@
+ guard matches.isEmpty == false else { return }
+
+ let maximumSequence = matches.reduce(Int64(0)) { result, update in
+ update.isPushed = true
+ return max(result, update.sequence)
+ }
</file context>
Greptile SummaryThis PR introduces durable per-document collaboration sessions backed by a new SwiftData persistence layer (
Confidence Score: 4/5The core session and persistence architecture is well-structured and thread-safe; the main items needing follow-up are two dead parameters, a compaction recovery design concern, and some redundant availability guards — none of which affect correctness of the happy path. The new persistence layer correctly uses actor isolation throughout, the reconnect replay uses an at-least-once delivery pattern with explicit sent-acknowledgement, and the retained-draft promotion logic handles the two main offline cases (no prior CRDT state vs. existing local updates). The identified issues are all code-quality or defensive-hardening concerns rather than functional defects in the changed paths. DocumentLocalPersistencePeer.swift (compaction writes both snapshots to identical data), DocumentSession.swift (unused Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant VM as NativeRichEditorViewModel
participant AS as AppState
participant Reg as DocumentSessionRegistry
participant Sess as DocumentSession
participant Peer as DocumentLocalPersistencePeer
participant Coord as NativeEditorCRDTSyncCoordinator
participant Driver as NativeEditorCollaborationSyncDriver
VM->>AS: makeDocumentSession(pageID:title:document:)
AS->>Reg: session(for:title:document:)
Reg->>Peer: load(key) — legacy migration check
Reg->>Sess: open(title:document:)
Sess->>Peer: load(key) — restore stored state
Sess->>Sess: restore(snapshot + updates)
Sess->>Sess: promoteRetainedDraft() if draft exists
Sess-->>Reg: DocumentSession (cached)
Reg-->>AS: DocumentSession
AS-->>VM: NativeEditorPreparedDocumentSession
VM->>VM: configureDocumentSession(_:restoredLocalState:)
Note over VM: applies initialSnapshot to editor
VM->>Driver: outboundFramesAfterAuthentication()
Driver->>Coord: pendingLocalUpdates()
Coord->>Peer: pendingLocalUpdates(key)
Peer-->>Coord: [Data]
Driver-->>VM: [syncFrame] + [pendingUpdateFrames]
VM->>Driver: didSendOutboundFramesAfterAuthentication()
Driver->>Coord: recordLocalUpdateSent(_:) x N
loop local edits
VM->>Coord: integrateLocalChange(_:)
Coord->>Peer: append(update, origin: .local)
Coord->>Coord: yield update to committedLocalUpdateStream
end
loop remote updates
VM->>Sess: commitRemoteUpdate(_:)
Sess->>Peer: append(update, origin: .remote)
Sess->>Sess: compactIfNeeded()
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant VM as NativeRichEditorViewModel
participant AS as AppState
participant Reg as DocumentSessionRegistry
participant Sess as DocumentSession
participant Peer as DocumentLocalPersistencePeer
participant Coord as NativeEditorCRDTSyncCoordinator
participant Driver as NativeEditorCollaborationSyncDriver
VM->>AS: makeDocumentSession(pageID:title:document:)
AS->>Reg: session(for:title:document:)
Reg->>Peer: load(key) — legacy migration check
Reg->>Sess: open(title:document:)
Sess->>Peer: load(key) — restore stored state
Sess->>Sess: restore(snapshot + updates)
Sess->>Sess: promoteRetainedDraft() if draft exists
Sess-->>Reg: DocumentSession (cached)
Reg-->>AS: DocumentSession
AS-->>VM: NativeEditorPreparedDocumentSession
VM->>VM: configureDocumentSession(_:restoredLocalState:)
Note over VM: applies initialSnapshot to editor
VM->>Driver: outboundFramesAfterAuthentication()
Driver->>Coord: pendingLocalUpdates()
Coord->>Peer: pendingLocalUpdates(key)
Peer-->>Coord: [Data]
Driver-->>VM: [syncFrame] + [pendingUpdateFrames]
VM->>Driver: didSendOutboundFramesAfterAuthentication()
Driver->>Coord: recordLocalUpdateSent(_:) x N
loop local edits
VM->>Coord: integrateLocalChange(_:)
Coord->>Peer: append(update, origin: .local)
Coord->>Coord: yield update to committedLocalUpdateStream
end
loop remote updates
VM->>Sess: commitRemoteUpdate(_:)
Sess->>Peer: append(update, origin: .remote)
Sess->>Sess: compactIfNeeded()
end
Reviews (1): Last reviewed commit: "feat: add durable document sessions and ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| kernel.documentEngine | ||
| } | ||
|
|
||
| func open(title: String, document: NativeEditorDocument) async throws { |
There was a problem hiding this comment.
document parameter in open(title:document:)
The document: NativeEditorDocument argument is accepted but never referenced anywhere in the method body — only title is used (as a fallback for the retained-draft title). DocumentSessionRegistry passes the same document to both the engine factory and this method, so it is already baked into the kernel before open() is called. The dead parameter misleads future readers into thinking the initial document content can be changed via this call.
Consider either removing the parameter and updating the callers, or adding an underscore to signal intent: open(title: String, document _: NativeEditorDocument).
Context Used: AGENTS.md (source)
| ) | ||
| } else if let offlineQueue { | ||
| result = try offlineQueue.resolvePendingPageUpdateKeepingLocal( | ||
| _ = remoteBaseDocument |
There was a problem hiding this comment.
remoteBaseDocument parameter acknowledged but not removed
_ = remoteBaseDocument is a code-smell: the parameter was intentionally silenced rather than removed from the public signature. Every caller is still forced to supply a ProseMirrorDocument value that is immediately discarded, and the suppress-line gives no indication whether this is intentional API stability or an oversight. If the parameter is no longer needed, remove it from the signature and update callers; if it is intentionally preserved for a planned future use, a comment explaining that would prevent confusion.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| document.snapshot = snapshot | ||
| document.recoverySnapshot = snapshot | ||
| document.snapshotSequence = sequence |
There was a problem hiding this comment.
After every successful compact() call both document.snapshot and document.recoverySnapshot are set to the exact same bytes in the same SwiftData transaction. This means restore() in DocumentSession can only fall back to recoverySnapshot when the two diverge — which never happens here. If the compaction-produced state itself is invalid (e.g. a Yjs encoding regression), both fields will carry the same bad data and the fallback branch in restore() will also throw, making the session permanently un-openable.
Consider retaining the previous snapshot as recoverySnapshot before overwriting the primary, so the two fields genuinely differ after a compaction cycle.
| document.snapshot = snapshot | |
| document.recoverySnapshot = snapshot | |
| document.snapshotSequence = sequence | |
| document.recoverySnapshot = document.snapshot ?? snapshot | |
| document.snapshot = snapshot | |
| document.snapshotSequence = sequence |
| Group { | ||
| if #available(iOS 26.0, macOS 26.0, *) { | ||
| GlassEffectContainer(spacing: 8) { | ||
| PageBrowserScopePills(viewModel: viewModel) | ||
| } | ||
| .buttonStyle(.plain) | ||
| .accessibilityAddTraits(viewModel.selectedScope == scope ? .isSelected : []) | ||
| } else { | ||
| PageBrowserScopePills(viewModel: viewModel) | ||
| } | ||
| } |
There was a problem hiding this comment.
#available(iOS 26.0, macOS 26.0, *) checks are redundant given the declared minimum deployment targets
Per the project guidelines, both the iOS and macOS targets require iOS/macOS 26.0 as their minimum version. When the deployment target already guarantees availability, the #available condition is always true at runtime, making the else branches dead code. The same pattern appears in PageBrowserScopePill. Remove the availability guards and the else branches to keep the call site clean and avoid the impression that a pre-26 fallback path is actually exercised.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18b76e15ea
ℹ️ 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".
| _ = try await documentSessionRegistry.session( | ||
| for: key, | ||
| title: title, | ||
| document: NativeEditorDocument(proseMirrorDocument: document) | ||
| ) |
There was a problem hiding this comment.
When replaying a legacy .updatePage / .updatePageCRDT record in the offline replay loop, this now only creates/opens a local DocumentSession. The caller removes the offline record immediately after replay(...) returns, but no collaboration socket is opened here and no pending CRDT updates are sent to the server; that means a queued offline body edit can be marked replayed after only the metadata update, and the document content will not reach Docmost until the user happens to open that page later. The removed replayCRDTPageUpdate path explicitly synchronized through the collab websocket before the record was deleted.
Useful? React with 👍 / 👎.
| if let initialSnapshot = session.initialSnapshot { | ||
| applyInitialCRDTDocumentSnapshot(initialSnapshot) |
There was a problem hiding this comment.
In the legacy-ProseMirror-draft case, DocumentSession.open can expose a retained draft as initialSnapshot while restoredLocalState is still false because there is no CRDT base yet. Passing that snapshot into applyInitialCRDTDocumentSnapshot marks isCRDTEngineReadyForLocalChanges = true and rebases the saved document even though the engine has not received the remote Yjs state; if the user edits or autosaves before the first collaboration sync, the local update is encoded against an empty CRDT document instead of the server base.
Useful? React with 👍 / 👎.
| do { | ||
| try await self?.send(frame) | ||
| try await syncDriver.didSendLocalUpdate(update) |
There was a problem hiding this comment.
When a local CRDT update is sent, this immediately calls didSendLocalUpdate, which marks the stored update as pushed and removes it from future pendingLocalUpdates(). If the websocket send completes locally but the connection drops before the Hocuspocus server applies and persists that frame, a relaunch/reconnect will no longer resend the update, so an offline or in-flight edit can be lost. Please only clear the pending flag after an application-level sync acknowledgement, such as the collaboration sync status indicating the document is synced.
Useful? React with 👍 / 👎.
| snapshotCapturedAt: snapshotCapturedAt | ||
| ) | ||
| _ = snapshotCapturedAt | ||
| try await queuePageMetadataUpdate(pageId: pageId, title: title, baseTitle: baseTitle) |
There was a problem hiding this comment.
For offline/current CRDT saves this now queues only a title metadata mutation, and queues nothing at all when the title did not change. The actual body update lives only in StoredDocumentUpdate, which is not counted by pendingOfflineMutationCount or replayed by scheduleOfflineQueueReconciliation; if the user leaves the page before the collaboration socket reconnects, the body edit will sit locally and will not be pushed until that page is opened again.
Useful? React with 👍 / 👎.
1 participant
What changed
Checks