feat(notebooks): show conflict modal on 410 error#58213
Merged
Conversation
Replace the force-save PATCH path with the standard 409 rebase. Modal opens only when PM-collab actually throws (parse / illegal transition) or on a 410 stream-trim. Recovery actions are discard-and-reload or copy-unsaved-to-new-notebook — no more PATCH-clobber that silently overwrites concurrent edits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Long unbroken strings overflowed horizontally because grid columns default to min-width: auto. Add min-w-0, switch overflow to y-only, break-words for unbroken text, and bump max-h to 30rem. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix(notebooks): use (duplicate) suffix for copied unsaved notebook Match the convention used by duplicateNotebook elsewhere in the file instead of the bespoke (unsaved changes) suffix.
setContent on a collab editor adds a 'replace whole doc' step to PM- collab's sendable buffer, which then triggers another save against the same trimmed stream and re-opens the modal. A page reload is the only way to drop the plugin's pending steps alongside localContent. Also drop the redundant loadNotebook in the copy-to-new-notebook path since we immediately navigate away.
Contributor
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
Contributor
|
Size Change: +12.9 kB (+0.01%) Total Size: 118 MB 📦 View Changed
ℹ️ View Unchanged
|
…eads
NotebookNodeQuery was calling updateAttributes({ ...attributes, x: y })
in three places. Tiptap merges, so the spread was redundant. It also
dragged the existing query object through useSyncedAttributes' string-
ifier, which then disagreed with the un-stringified previousNodeAttrs
and dispatched a no-op transaction — prosemirror-collab caught that as
a local edit and tried to save, looping on 410 after a Discard+reload.
Discard now just clearLocalContent + window.location.reload to remount
the editor with server content (initialContent path, no setContent
transaction).
a-lider
force-pushed
the
fix/notebook-stale-conflict-modal
branch
from
b22dbb1 to
1e94554
Compare
The modal handles both 410 stale and rebase failures, not just stale — rename for accuracy. NotebookStaleConflictModal -> NotebookCollabConflictModal, staleConflict -> collabConflict, showStaleConflict -> showCollabConflict, dismissStaleConflict -> dismissCollabConflict.
Collab notebooks update via SSE, not setContent. Restricting the polling-refresh setContent to !collabEnabled avoids dirtying PM-collab's sendable buffer when scheduleNotebookRefresh fires.
a-lider
commented
May 10, 2026
|
|
||
| if (isDataTableNode(modifiedQuery) && isEventsQuery(modifiedQuery.source)) { | ||
| modifiedQuery.source.fixedProperties = canvasFiltersOverride | ||
| updateAttributes({ ...attributes, isDefaultFilterApplied: true }) |
Contributor
Author
There was a problem hiding this comment.
I cleaned it up here. Spreading ..attributes was causing a no-op transaction save after reload, which triggered the modal again.
Tiptap’s updateAttributes(partial) always merges into existing attrs, so passing the full set is redundant.
useSyncedAttributes' hasChanges compared prosemirror state directly to stringifiedAttrs. When notebook content was loaded from JSON with the value as an object, the wrapper's stringification falsely registered as a change and dispatched a no-op transaction that prosemirror-collab caught as a local edit — looping on 410 after a Discard+reload.
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/scenes/notebooks/Notebook/notebookCollabLogic.ts:152-155
**`serverContent` is identical to `localContent` when the step throws**
`applyRemoteStep` throws before `editor.view.dispatch(tr)` is called (e.g. during `Step.fromJSON` or `receiveTransaction`), so the editor state is never mutated. `editor.getJSON()` in the catch block therefore returns the same document as `localContent` — both columns in the conflict modal render the same content, defeating the side-by-side preview.
The 410 path handles this correctly by using `error.data.content` (the actual server payload). The `rebaseFailed` path should do a similar lookup — the `notebookLogic` listener for `rebaseFailed` is already async and could fetch the latest server content there before calling `showCollabConflict`. The same issue exists in the SSE `onMessage` catch block at line 203.
### Issue 2 of 2
frontend/src/scenes/notebooks/Notebook/notebookLogic.ts:472-474
**`collabConflict` check is unreliable here — listeners are async**
`actions.applyRemoteSteps(steps)` only dispatches the action; the `applyRemoteSteps` listener (and the downstream `rebaseFailed` → `showCollabConflict` chain that sets `collabConflict`) is queued as an async task and won't have run yet when execution reaches this guard. In practice `values.collabConflict` will always be `null` at this point, so `actions.saveNotebook(...)` is unconditionally called even when the rebase failed. The more durable guard is the one at line 427, but it is subject to the same race during the retry (the listener chain may finish in the microtask gap around `await api.create`). Consider using a local flag or restructuring so the rebase result is known synchronously before deciding whether to retry.
Reviews (1): Last reviewed commit: "Simplify comments" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+152
to
+155
| } catch (e) { | ||
| posthog.captureException(e as Error, { action: 'notebook collab apply remote step' }) | ||
| actions.rebaseFailed({ serverContent: editor.getJSON(), localContent, localText }) | ||
| return |
Contributor
There was a problem hiding this comment.
serverContent is identical to localContent when the step throws
applyRemoteStep throws before editor.view.dispatch(tr) is called (e.g. during Step.fromJSON or receiveTransaction), so the editor state is never mutated. editor.getJSON() in the catch block therefore returns the same document as localContent — both columns in the conflict modal render the same content, defeating the side-by-side preview.
The 410 path handles this correctly by using error.data.content (the actual server payload). The rebaseFailed path should do a similar lookup — the notebookLogic listener for rebaseFailed is already async and could fetch the latest server content there before calling showCollabConflict. The same issue exists in the SSE onMessage catch block at line 203.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/notebooks/Notebook/notebookCollabLogic.ts
Line: 152-155
Comment:
**`serverContent` is identical to `localContent` when the step throws**
`applyRemoteStep` throws before `editor.view.dispatch(tr)` is called (e.g. during `Step.fromJSON` or `receiveTransaction`), so the editor state is never mutated. `editor.getJSON()` in the catch block therefore returns the same document as `localContent` — both columns in the conflict modal render the same content, defeating the side-by-side preview.
The 410 path handles this correctly by using `error.data.content` (the actual server payload). The `rebaseFailed` path should do a similar lookup — the `notebookLogic` listener for `rebaseFailed` is already async and could fetch the latest server content there before calling `showCollabConflict`. The same issue exists in the SSE `onMessage` catch block at line 203.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
There was a problem hiding this comment.
Changed that to fetch fresh notebook content when the conflict modal is opened
Comment on lines
+472
to
+474
| if (values.collabConflict) { | ||
| return values.notebook | ||
| } |
Contributor
There was a problem hiding this comment.
collabConflict check is unreliable here — listeners are async
actions.applyRemoteSteps(steps) only dispatches the action; the applyRemoteSteps listener (and the downstream rebaseFailed → showCollabConflict chain that sets collabConflict) is queued as an async task and won't have run yet when execution reaches this guard. In practice values.collabConflict will always be null at this point, so actions.saveNotebook(...) is unconditionally called even when the rebase failed. The more durable guard is the one at line 427, but it is subject to the same race during the retry (the listener chain may finish in the microtask gap around await api.create). Consider using a local flag or restructuring so the rebase result is known synchronously before deciding whether to retry.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/notebooks/Notebook/notebookLogic.ts
Line: 472-474
Comment:
**`collabConflict` check is unreliable here — listeners are async**
`actions.applyRemoteSteps(steps)` only dispatches the action; the `applyRemoteSteps` listener (and the downstream `rebaseFailed` → `showCollabConflict` chain that sets `collabConflict`) is queued as an async task and won't have run yet when execution reaches this guard. In practice `values.collabConflict` will always be `null` at this point, so `actions.saveNotebook(...)` is unconditionally called even when the rebase failed. The more durable guard is the one at line 427, but it is subject to the same race during the retry (the listener chain may finish in the microtask gap around `await api.create`). Consider using a local flag or restructuring so the rebase result is known synchronously before deciding whether to retry.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
There was a problem hiding this comment.
Not critical, worst case is 1 extra collab/save retry and showCollabConflict firing twice (modal re-renders)
Contributor
|
Reviews (2): Last reviewed commit: "fix(playwright): satisfy ty for setup he..." | Re-trigger Greptile |
a-lider
force-pushed
the
fix/notebook-stale-conflict-modal
branch
from
1ff080d to
ba867cf
Compare
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
frontend/src/scenes/notebooks/Notebook/notebookLogic.ts:1120-1126
**Conflicting toasts when navigation fails after successful copy**
`lemonToast.success`, `actions.dismissCollabConflict()`, and `actions.clearLocalContent()` all run before `await openNotebook(...)`. If `openNotebook` rejects (e.g. a routing error), the single `catch` block fires and shows "Could not copy your changes to a new notebook" — but the notebook *was* already created and the success toast already displayed. The user sees two simultaneous toasts with opposite meanings, with the conflict modal gone and no clear path to the new notebook.
```suggestion
actions.dismissCollabConflict()
actions.clearLocalContent()
try {
await openNotebook(created.short_id, NotebookTarget.Scene)
} catch {
lemonToast.success('Saved your unsaved changes to a new notebook. Navigate there manually if needed.')
return
}
lemonToast.success('Saved your unsaved changes to a new notebook.')
} catch {
lemonToast.error('Could not copy your changes to a new notebook.')
}
```
Reviews (3): Last reviewed commit: "Simplify comments" | Re-trigger Greptile |
ReeceJones
approved these changes
May 18, 2026
|
|
||
| return ( | ||
| <LemonModal | ||
| isOpen |
Contributor
There was a problem hiding this comment.
I think its probably better to pass the open state directly to the modal component instead of conditionally rendering this entire component which will cause mount & remount every time its visibility is toggled.
Contributor
Author
There was a problem hiding this comment.
Makes sense, fixed that
|
|
||
| // Icons for the most common embed types. | ||
| // Anything not listed here gets the generic IconNotebook fallback. | ||
| const NODE_ICONS: Partial<Record<NotebookNodeType, JSX.Element>> = { |
Contributor
There was a problem hiding this comment.
Do we already have this defined somewhere else? For example in the insert node menu? If yes, then is this necessary. If no, then we should update those other places to use this map.
Contributor
Author
There was a problem hiding this comment.
Yeah, thought about that. Ideally this would live on KNOWN_NODES next to titlePlaceholder, but it means touching 30+ NotebookNode*.tsx files just to add an icon: field, and this preview is the only place that needs a node-type → icon map.
The existing slash command icons map to specific subtypes (Trend, Funnel, Retention, Python query…) rather than the parent node type (e.g. ph-query covers a bunch of those), so reusing them didn't quite fit here.
Passing isOpen={!!collabConflict} lets LemonModal play its close
animation and keeps the component's kea subscriptions alive across
open/close, instead of remounting on every toggle.
Contributor
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
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
2 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.
Problem
When PM-collab can't rebase a user's local edits onto the latest server state, those edits silently disappear. Two failure modes:
MAXLENor TTL expired, so the missed steps from the client'slast_seen_versionare gone and can't be reappliedNeither has been hit during internal testing, but it's better to surface a fallback so the user can recover their work without digging through notebook history. Also, the modal suggests opening support ticket, it would helps us catch unexpected errors.
Changes
NotebookPreviewcomponent - lightweight read-only React renderer for a notebook ProseMirror JSON document. Used by the modal to show both versions without spinning up a full Tiptap editor.initialContent(nosetContenttransaction that would dirty PM-collab's sendable buffer).rebaseFailedaction innotebookCollabLogicbubbles up tonotebookLogicto open the modal whenapplyRemoteStepthrows during a 409 rebase.How did you test?
Manually broke the Redis stream to force a 410 on the next save:
Then typed a few characters in the notebook to trigger collab/save → got the modal → tested both Discard and Copy-to-new-notebook paths, plus closing the modal and re-typing.
Discard unsaved changes and reload:
Copy unsaved version to new Notebook:
Publish to changelog?
no
Docs update
🤖 Agent context