Improve editor reliability and app workflows#21
Conversation
|
Too many files changed for review. ( Bypass the limit by tagging |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 184 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/NativeEditorInlineCommentComposerView.swift">
<violation number="1" location="docmostly/Features/Editor/NativeEditorInlineCommentComposerView.swift:26">
P2: Dismissing while an inline comment is posting now remains possible, leaving its request running after the sheet closes; reopening and submitting can create a duplicate before the first request establishes its pending draft. Keep cancellation unavailable while `isSubmitting`, as in the previous toolbar.</violation>
</file>
<file name="docmostly/Features/Favorites/FavoritesViewModel.swift">
<violation number="1" location="docmostly/Features/Favorites/FavoritesViewModel.swift:38">
P2: A favorites revision arriving during an in-flight load can leave the screen showing the prior snapshot. The replacement task exits at this guard after cancelling the old task, so track a load generation/request ID (or queue a reload) rather than dropping the new request.</violation>
</file>
<file name="docmostly/Features/Editor/NativeEditorNestedDocumentView.swift">
<violation number="1" location="docmostly/Features/Editor/NativeEditorNestedDocumentView.swift:23">
P2: A subpages block inside editable callout/details/column content loads against synthetic `nested-<UUID>` rather than its containing page, so it shows an error or no subpages. Carry the parent page ID into this nested model instead of generating an ID.</violation>
</file>
<file name="docmostly/Features/Comments/CommentBody.swift">
<violation number="1" location="docmostly/Features/Comments/CommentBody.swift:99">
P2: Editing a comment containing a ProseMirror `hardBreak` rewrites that inline break as separate paragraphs on save. Mark `hardBreak` comments non-editable until their block boundaries can round-trip independently.</violation>
</file>
<file name="docmostly/Features/Editor/NativeEditorRichBlockPreviewView.swift">
<violation number="1" location="docmostly/Features/Editor/NativeEditorRichBlockPreviewView.swift:125">
P2: Synced-source blocks without a raw ProseMirror node render/edit as empty. Use the existing factory fallback, as the callout/details/columns branches do, so legacy or programmatically constructed blocks retain `previewText`.</violation>
</file>
<file name="docmostly/Features/Editor/NativeEditorTextInputView+iOS.swift">
<violation number="1" location="docmostly/Features/Editor/NativeEditorTextInputView+iOS.swift:107">
P2: Changing a focused block from paragraph to heading, or changing its alignment, leaves existing iOS text rendered with the old font/alignment because the unchanged-text path only updates selection. Re-render `attributedText` when `block.kind` or `block.alignment` changes while preserving the current selection.</violation>
</file>
<file name="docmostly/Networking/DocmostComment.swift">
<violation number="1" location="docmostly/Networking/DocmostComment.swift:122">
P1: A comment response with a huge `attrs` payload can now be fully decoded and retained, bypassing ProseMirror's attribute budget; this can cause excessive memory use from remote comment data. Decode comments with an initialized `ProseMirrorDecodingBudget` (or validate attributes before retaining the document) on this path.</violation>
</file>
<file name="docmostly/Features/Editor/NativeRichEditorViewModel+Collaboration.swift">
<violation number="1" location="docmostly/Features/Editor/NativeRichEditorViewModel+Collaboration.swift:103">
P2: Applying a deferred remote snapshot clears all rendered collaborator cursors without resolving the still-present `remoteCursors` again. Refresh them after installing the snapshot so collaborator presence returns immediately after conflict resolution.</violation>
</file>
<file name="docmostly/Features/Notifications/NotificationListViewModel.swift">
<violation number="1" location="docmostly/Features/Notifications/NotificationListViewModel.swift:164">
P2: Concurrent mark-as-read requests can leave the shared unread badge too high when one request fails after another succeeds, because rollback restores a stale whole-count snapshot. Track pending optimistic deltas (or serialize these mutations) so one rollback cannot undo another request's successful decrement.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return node.text != nil | ||
| && node.attrs?.isEmpty != false | ||
| && node.content == nil | ||
| case "hardBreak": |
There was a problem hiding this comment.
P2: Editing a comment containing a ProseMirror hardBreak rewrites that inline break as separate paragraphs on save. Mark hardBreak comments non-editable until their block boundaries can round-trip independently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/Comments/CommentBody.swift, line 99:
<comment>Editing a comment containing a ProseMirror `hardBreak` rewrites that inline break as separate paragraphs on save. Mark `hardBreak` comments non-editable until their block boundaries can round-trip independently.</comment>
<file context>
@@ -0,0 +1,125 @@
+ return node.text != nil
+ && node.attrs?.isEmpty != false
+ && node.content == nil
+ case "hardBreak":
+ return node.attrs?.isEmpty != false
+ && node.content == nil
</file context>
| try await operation() | ||
| } catch is CancellationError { | ||
| locallyReadIDs.remove(notification.id) | ||
| store.restoreUnreadCount(previousUnreadCount) |
There was a problem hiding this comment.
P2: Concurrent mark-as-read requests can leave the shared unread badge too high when one request fails after another succeeds, because rollback restores a stale whole-count snapshot. Track pending optimistic deltas (or serialize these mutations) so one rollback cannot undo another request's successful decrement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/Notifications/NotificationListViewModel.swift, line 164:
<comment>Concurrent mark-as-read requests can leave the shared unread badge too high when one request fails after another succeeds, because rollback restores a stale whole-count snapshot. Track pending optimistic deltas (or serialize these mutations) so one rollback cannot undo another request's successful decrement.</comment>
<file context>
@@ -4,67 +4,192 @@ import Observation
+ try await operation()
+ } catch is CancellationError {
+ locallyReadIDs.remove(notification.id)
+ store.restoreUnreadCount(previousUnreadCount)
} catch {
+ locallyReadIDs.remove(notification.id)
</file context>
| NativeEditorTransclusionSourceBlockView( | ||
| blockID: block.id, | ||
| source: source, | ||
| content: block.rawNode?.content ?? [], |
There was a problem hiding this comment.
P2: Synced-source blocks without a raw ProseMirror node render/edit as empty. Use the existing factory fallback, as the callout/details/columns branches do, so legacy or programmatically constructed blocks retain previewText.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/Editor/NativeEditorRichBlockPreviewView.swift, line 125:
<comment>Synced-source blocks without a raw ProseMirror node render/edit as empty. Use the existing factory fallback, as the callout/details/columns branches do, so legacy or programmatically constructed blocks retain `previewText`.</comment>
<file context>
@@ -74,39 +78,65 @@ struct NativeEditorRichBlockPreviewView: View {
+ NativeEditorTransclusionSourceBlockView(
+ blockID: block.id,
+ source: source,
+ content: block.rawNode?.content ?? [],
+ actions: richBlockActions,
+ pageID: pageID,
</file context>
| content: block.rawNode?.content ?? [], | |
| content: block.rawNode?.content ?? NativeEditorRichBlockNodeFactory.transclusionSourceNode(from: source).content ?? [], |
| authoritativeText: sourceText | ||
| ) { | ||
| case .current: | ||
| applyBoundSelectionIfNeeded(to: textView) |
There was a problem hiding this comment.
P2: Changing a focused block from paragraph to heading, or changing its alignment, leaves existing iOS text rendered with the old font/alignment because the unchanged-text path only updates selection. Re-render attributedText when block.kind or block.alignment changes while preserving the current selection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/Editor/NativeEditorTextInputView+iOS.swift, line 107:
<comment>Changing a focused block from paragraph to heading, or changing its alignment, leaves existing iOS text rendered with the old font/alignment because the unchanged-text path only updates selection. Re-render `attributedText` when `block.kind` or `block.alignment` changes while preserving the current selection.</comment>
<file context>
@@ -0,0 +1,503 @@
+ authoritativeText: sourceText
+ ) {
+ case .current:
+ applyBoundSelectionIfNeeded(to: textView)
+ case .staleLocalEcho:
+ return
</file context>
| remoteCursors = [] | ||
| } | ||
| if resolvedRemoteCursors.isEmpty == false { | ||
| resolvedRemoteCursors = [] |
There was a problem hiding this comment.
P2: Applying a deferred remote snapshot clears all rendered collaborator cursors without resolving the still-present remoteCursors again. Refresh them after installing the snapshot so collaborator presence returns immediately after conflict resolution.
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 103:
<comment>Applying a deferred remote snapshot clears all rendered collaborator cursors without resolving the still-present `remoteCursors` again. Refresh them after installing the snapshot so collaborator presence returns immediately after conflict resolution.</comment>
<file context>
@@ -58,15 +65,43 @@ extension NativeRichEditorViewModel {
+ remoteCursors = []
+ }
+ if resolvedRemoteCursors.isEmpty == false {
+ resolvedRemoteCursors = []
+ }
}
</file context>
![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: dd512be6f1
ℹ️ 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".
| let replayDecision = OfflinePageUpdateReplayDecision.resolve( | ||
| serverPage: page, | ||
| queuedTitle: title, | ||
| queuedDocument: documentSnapshot, | ||
| baseDocument: baseDocument |
There was a problem hiding this comment.
When the title-only update response does not include content, passing that response into OfflinePageUpdateReplayDecision.resolve makes serverPage.content nil, so the resolver treats an otherwise synchronized collaborative document as .conflict. In that case a normal collaborative title save is retained as an offline/conflicted draft instead of being acknowledged; fetch a JSON page snapshot or otherwise use a contentful server document before making this decision.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func load(appState: AppState) async { | ||
| guard isLoading == false else { return } |
There was a problem hiding this comment.
When a favorites refresh is already running and favoriteRevision changes, for example after starring or unstarring a page while the list is still loading, the new .task(id:) call into load exits at this guard instead of recording a follow-up refresh. The in-flight request may have been issued before the mutation and can then apply stale results, leaving the Favorites screen unchanged until a manual refresh; allow the latest request to supersede the old one or queue a reload after it finishes.
Useful? React with 👍 / 👎.
| VStack(alignment: .leading, spacing: 10) { | ||
| Group { | ||
| if let sourceURL { | ||
| AsyncImage(url: sourceURL) { phase in |
There was a problem hiding this comment.
Docmost resource URLs are same-origin and often require the session cookie, but the app keeps cookies in SessionCookieJar and disables automatic cookie handling for API calls; AsyncImage(url:) cannot attach that cookie header. Auth-protected image blocks will therefore show the unavailable state even for signed-in users; load with a request that includes activeSessionCookies instead.
Useful? React with 👍 / 👎.
| PageReaderView(pageID: target.slugId, initialCommentID: target.commentId) | ||
| .task(id: target.id) { | ||
| appState.openPage(target) |
There was a problem hiding this comment.
On iOS, notification taps use navigationDestination(item:) and this destination now depends on target.commentId, but the destination task is still keyed only by target.id. If the user opens another notification for the same page with a different comment, SwiftUI keeps the same item identity and this task does not rerun, so appState.selectedCommentID remains on the previous comment and the new thread is not focused; include the comment id in the navigation/task identity or explicitly react to it.
Useful? React with 👍 / 👎.
| guard let document = try? container.decodeIfPresent(ProseMirrorDocument.self, forKey: .content), | ||
| Self.isWithinCommentContentBudget(document) else { |
There was a problem hiding this comment.
With this change, an object content payload is decoded into the full recursive ProseMirrorDocument before any of the comment-specific depth/node/text limits run. A malformed or hostile workspace comment with excessive nesting can therefore consume memory or stack before being rejected, whereas the old decoder enforced depth during decode; use a bounded decoder or validate as nodes are decoded.
Useful? React with 👍 / 👎.
| @ObservationIgnored private let socketClient = NotificationSocketClient() | ||
|
|
||
| func refreshUnreadCount(appState: AppState) async { | ||
| guard isRefreshingUnreadCount == false else { return } |
There was a problem hiding this comment.
If a realtime notification arrives while the periodic or scene-phase unread-count request is already in flight, this guard drops the event-triggered refresh. That in-flight request may have been sent before the new notification was counted, so it can write the old unreadCount and leave the badge stale until the next 60-second poll; remember that another refresh is needed instead of returning immediately.
Useful? React with 👍 / 👎.
| if comment.isResolved == false, canComment { | ||
| CommentReplyComposerView( | ||
| commentID: comment.id, | ||
| draft: viewModel.replyDraft(for: comment.id), |
There was a problem hiding this comment.
For every unresolved comment without an existing draft, evaluating this row calls replyDraft(for:), which inserts into the observable replyDraftsByCommentID dictionary while SwiftUI is computing body. That update-during-render path can trigger undefined SwiftUI behavior and extra invalidations just by opening a comments panel with unresolved threads; create the draft before presenting the composer or use a non-mutating lookup from the view.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
All reported issues were addressed across 27 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
Summary
Why
This consolidates the current reliability and product-parity work into one reviewable change. The main themes are preventing editor data loss during collaboration or page transitions, making offline recovery deterministic, filling native interaction gaps, and removing navigation states where a row could highlight without opening its destination.
Developer and user impact
Editing, collaboration, comments, notifications, favorites, and offline recovery should behave more consistently across iOS, iPadOS, and macOS. Settings destinations now open reliably from both supported entry paths.
Validation
git diff --cached --checktestSettingsRowsOpenTheirDestinationstestSpaceSettingsEntrySupportsNestedNavigationA full Xcode build/test suite was not run as part of this publication pass.