feat: Handle notebook conflicts

frontend/src/scenes/notebooks/Notebook/Notebook.tsx

@@ -23,6 +23,7 @@ import posthog from 'posthog-js'
 import { LemonBanner } from 'lib/lemon-ui/LemonBanner'
 import { SCRATCHPAD_NOTEBOOK } from './notebooksListLogic'
 import { FloatingSlashCommands, SlashCommandsExtension } from './SlashCommands'
+import { NotebookConflictWarning } from './NotebookConflictWarning'
 
 export type NotebookProps = {
     shortId: string
@@ -37,7 +38,7 @@ const PLACEHOLDER_TITLES = ['Release notes', 'Product roadmap', 'Meeting notes',
 
 export function Notebook({ shortId, editable = false }: NotebookProps): JSX.Element {
     const logic = notebookLogic({ shortId })
-    const { notebook, content, notebookLoading, isEmpty } = useValues(logic)
+    const { notebook, content, notebookLoading, isEmpty, conflictWarningVisible } = useValues(logic)
     const { setEditorRef, onEditorUpdate, duplicateNotebook, loadNotebook } = useActions(logic)
     const { isExpanded } = useValues(notebookSettingsLogic)
 
@@ -169,15 +170,17 @@ export function Notebook({ shortId, editable = false }: NotebookProps): JSX.Elem
         <BindLogic logic={notebookLogic} props={{ shortId }}>
             <div className={clsx('Notebook', !isExpanded && 'Notebook--compact')}>
                 <FloatingSlashCommands />
-                {!notebook && notebookLoading ? (
+                {conflictWarningVisible ? (
+                    <NotebookConflictWarning />
+                ) : !notebook && notebookLoading ? (
                     <div className="space-y-4 px-8 py-4">
                         <LemonSkeleton className="w-1/2 h-8" />
                         <LemonSkeleton className="w-1/3 h-4" />
                         <LemonSkeleton className="h-4" />
                         <LemonSkeleton className="h-4" />
                     </div>
                 ) : !notebook ? (
-                    <NotFound object={'notebook'} />
+                    <NotFound object="notebook" />
                 ) : isEmpty && !editable ? (
                     <div className="NotebookEditor">
                         <h1>

frontend/src/scenes/notebooks/Notebook/NotebookConflictWarning.tsx

@@ -0,0 +1,22 @@
+import { useActions } from 'kea'
+import { LemonButton } from 'lib/lemon-ui/LemonButton'
+import { notebookLogic } from './notebookLogic'
+
+export function NotebookConflictWarning(): JSX.Element {
+    const { loadNotebook } = useActions(notebookLogic)
+
+    return (
+        <div className="flex flex-col items-center text-muted-alt m-10">
+            <h2 className="text-muted-alt">This Notebook has been edited elsewhere</h2>
+
+            <p>
+                It looks like someone else has been editing this content too. You will need to reload the notebook to
+                see the latest version.
+            </p>
+
+            <LemonButton type="primary" onClick={loadNotebook}>
+                Reload to see the latest content
+            </LemonButton>
+        </div>
+    )
+}

frontend/src/scenes/notebooks/Notebook/notebookLogic.ts

@@ -34,6 +34,7 @@ export const notebookLogic = kea<notebookLogicType>([
         loadNotebook: true,
         saveNotebook: (notebook: Pick<NotebookType, 'content' | 'title'>) => ({ notebook }),
         exportJSON: true,
+        showConflictWarning: true,
     }),
     reducers({
         localContent: [
@@ -50,21 +51,27 @@ export const notebookLogic = kea<notebookLogicType>([
                 setEditorRef: (_, { editor }) => editor,
             },
         ],
-
         ready: [
             false,
             {
                 setReady: () => true,
             },
         ],
+        conflictWarningVisible: [
+            false,
+            {
+                showConflictWarning: () => true,
+                loadNotebookSuccess: () => false,
+            },
+        ],
     }),
     loaders(({ values, props, actions }) => ({
         notebook: [
-            undefined as NotebookType | undefined,
+            null as NotebookType | null,
             {
                 loadNotebook: async () => {
                     // NOTE: This is all hacky and temporary until we have a backend
-                    let response: NotebookType | undefined
+                    let response: NotebookType | null
 
                     if (props.shortId === SCRATCHPAD_NOTEBOOK.short_id) {
                         response = {
@@ -73,7 +80,8 @@ export const notebookLogic = kea<notebookLogicType>([
                             version: 0,
                         }
                     } else if (props.shortId.startsWith('template-')) {
-                        response = values.notebookTemplates.find((template) => template.short_id === props.shortId)
+                        response =
+                            values.notebookTemplates.find((template) => template.short_id === props.shortId) || null
                     } else {
                         response = await api.notebooks.get(props.shortId)
                     }
@@ -95,28 +103,37 @@ export const notebookLogic = kea<notebookLogicType>([
                         return values.notebook
                     }
 
-                    const response = await api.notebooks.update(values.notebook.short_id, {
-                        version: values.notebook.version,
-                        content: notebook.content,
-                        title: notebook.title,
-                    })
+                    try {
+                        const response = await api.notebooks.update(values.notebook.short_id, {
+                            version: values.notebook.version,
+                            content: notebook.content,
+                            title: notebook.title,
+                        })
 
-                    // If the object is identical then no edits were made, so we can safely clear the local changes
-                    if (notebook.content === values.localContent) {
-                        actions.clearLocalContent()
-                    }
+                        // If the object is identical then no edits were made, so we can safely clear the local changes
+                        if (notebook.content === values.localContent) {
+                            actions.clearLocalContent()
+                        }
 
-                    return response
+                        return response
+                    } catch (error: any) {
+                        if (error.code === 'conflict') {
+                            actions.showConflictWarning()
+                            return null
+                        } else {
+                            throw error
+                        }
+                    }
                 },
             },
         ],
 
         newNotebook: [
-            undefined as NotebookType | undefined,
+            null as NotebookType | null,
             {
                 duplicateNotebook: async () => {
                     if (!values.notebook) {
-                        return
+                        return null
                     }
 
                     // We use the local content if set otherwise the notebook content. That way it supports templates, scratchpad etc.

frontend/src/scenes/notebooks/NotebookScene.tsx

@@ -31,12 +31,12 @@ export const scene: SceneExport = {
 export function NotebookScene(): JSX.Element {
     const { notebookId, mode } = useValues(notebookSceneLogic)
     const { setNotebookMode } = useActions(notebookSceneLogic)
-    const { notebook, notebookLoading } = useValues(notebookLogic({ shortId: notebookId }))
+    const { notebook, notebookLoading, conflictWarningVisible } = useValues(notebookLogic({ shortId: notebookId }))
     const { exportJSON } = useActions(notebookLogic({ shortId: notebookId }))
     const { selectNotebook, setNotebookSideBarShown } = useActions(notebookSidebarLogic)
     const { selectedNotebook, notebookSideBarShown } = useValues(notebookSidebarLogic)
 
-    if (!notebook && !notebookLoading) {
+    if (!notebook && !notebookLoading && !conflictWarningVisible) {
         return <NotFound object="notebook" />
     }
 

posthog/api/notebook.py

@@ -2,6 +2,7 @@
 
 import structlog
 from django.db.models import QuerySet
+from django.db import transaction
 from django.utils.timezone import now
 from django_filters.rest_framework import DjangoFilterBackend
 from rest_framework import request, serializers, viewsets
@@ -11,6 +12,7 @@
 from posthog.api.forbid_destroy_model import ForbidDestroyModel
 from posthog.api.routing import StructuredViewSetMixin
 from posthog.api.shared import UserBasicSerializer
+from posthog.exceptions import Conflict
 from posthog.models import User
 from posthog.models.activity_logging.activity_log import Change, Detail, changes_between, log_activity, load_activity
 from posthog.models.activity_logging.activity_page import activity_page_response
@@ -97,20 +99,22 @@ def update(self, instance: Notebook, validated_data: Dict, **kwargs) -> Notebook
         except Notebook.DoesNotExist:
             before_update = None
 
-        if validated_data.keys():
-            instance.last_modified_at = now()
-            instance.last_modified_by = self.context["request"].user
+        with transaction.atomic():
+            # select_for_update locks the database row so we ensure version updates are atomic
+            locked_instance = Notebook.objects.select_for_update().get(pk=instance.pk)
 
-        # TODO: This is not atomic meaning we could still end up with race conditions
-        if validated_data.get("content"):
-            if validated_data.get("version") != instance.version:
-                raise serializers.ValidationError(
-                    "Notebook was modified by someone else. Please refresh and try again."
-                )
+            if validated_data.keys():
+                locked_instance.last_modified_at = now()
+                locked_instance.last_modified_by = self.context["request"].user
 
-            validated_data["version"] = instance.version + 1
+                if validated_data.get("content"):
+                    if validated_data.get("version") != locked_instance.version:
+                        raise Conflict("Someone else edited the Notebook")
+
+                    validated_data["version"] = locked_instance.version + 1
+
+                updated_notebook = super().update(locked_instance, validated_data)
 
-        updated_notebook = super().update(instance, validated_data)
         changes = changes_between("Notebook", previous=before_update, current=updated_notebook)
 
         log_notebook_activity(