From 763f652b24f96877ae8db3cd95645aa6e5f782c3 Mon Sep 17 00:00:00 2001 From: Ben White Date: Mon, 4 Dec 2023 15:00:37 +0100 Subject: [PATCH] feat: Better Notebook refreshing (#18980) --- frontend/src/lib/api.ts | 16 ++- .../scenes/notebooks/Notebook/Notebook.tsx | 108 +++++++++--------- .../notebooks/Notebook/notebookLogic.ts | 63 +++++----- posthog/api/notebook.py | 19 ++- posthog/api/test/notebooks/test_notebook.py | 14 +++ 5 files changed, 130 insertions(+), 90 deletions(-) diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index c847c079f3013..591140cae3ce4 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -2,7 +2,7 @@ import { decompressSync, strFromU8 } from 'fflate' import { encodeParams } from 'kea-router' import { ActivityLogProps } from 'lib/components/ActivityLog/ActivityLog' import { ActivityLogItem, ActivityScope } from 'lib/components/ActivityLog/humanizeActivity' -import { toParams } from 'lib/utils' +import { objectClean, toParams } from 'lib/utils' import posthog from 'posthog-js' import { SavedSessionRecordingPlaylistsResult } from 'scenes/session-recordings/saved-playlists/savedSessionRecordingPlaylistsLogic' @@ -102,6 +102,7 @@ export interface ActivityLogPaginatedResponse extends PaginatedResponse { export interface ApiMethodOptions { signal?: AbortSignal + headers?: Record } const CSRF_COOKIE_NAME = 'posthog_csrftoken' @@ -1519,8 +1520,14 @@ const api = { }, notebooks: { - async get(notebookId: NotebookType['short_id']): Promise { - return await new ApiRequest().notebook(notebookId).get() + async get( + notebookId: NotebookType['short_id'], + params: Record = {}, + headers: Record = {} + ): Promise { + return await new ApiRequest().notebook(notebookId).withQueryString(toParams(params)).get({ + headers, + }) }, async update( notebookId: NotebookType['short_id'], @@ -1842,6 +1849,7 @@ const api = { response = await fetch(url, { signal: options?.signal, headers: { + ...objectClean(options?.headers ?? {}), ...(getSessionId() ? { 'X-POSTHOG-SESSION-ID': getSessionId() } : {}), }, }) @@ -1865,6 +1873,7 @@ const api = { const response = await fetch(url, { method: 'PATCH', headers: { + ...objectClean(options?.headers ?? {}), ...(isFormData ? {} : { 'Content-Type': 'application/json' }), 'X-CSRFToken': getCookie(CSRF_COOKIE_NAME) || '', ...(getSessionId() ? { 'X-POSTHOG-SESSION-ID': getSessionId() } : {}), @@ -1897,6 +1906,7 @@ const api = { const response = await fetch(url, { method: 'POST', headers: { + ...objectClean(options?.headers ?? {}), ...(isFormData ? {} : { 'Content-Type': 'application/json' }), 'X-CSRFToken': getCookie(CSRF_COOKIE_NAME) || '', ...(getSessionId() ? { 'X-POSTHOG-SESSION-ID': getSessionId() } : {}), diff --git a/frontend/src/scenes/notebooks/Notebook/Notebook.tsx b/frontend/src/scenes/notebooks/Notebook/Notebook.tsx index 4b896c1cdb989..60340f4e0819c 100644 --- a/frontend/src/scenes/notebooks/Notebook/Notebook.tsx +++ b/frontend/src/scenes/notebooks/Notebook/Notebook.tsx @@ -86,64 +86,62 @@ export function Notebook({ setContainerSize(size as 'small' | 'medium') }, [size]) - // TODO - Render a special state if the notebook is empty - - if (conflictWarningVisible) { - return - } else if (!notebook && notebookLoading) { - return - } else if (notebookMissing) { - return - } - return ( -
- {isTemplate && ( - - This is a template. You can create a copy of it to edit and use as your own. - - )} - - - {shortId === SCRATCHPAD_NOTEBOOK.short_id ? ( - - This is your scratchpad. It is only visible to you and is persisted only in this browser. It's a - great place to gather ideas before turning into a saved Notebook! - - ) : null} - -
- - - - - + {conflictWarningVisible ? ( + + ) : !notebook && notebookLoading ? ( + + ) : notebookMissing ? ( + + ) : ( +
+ {isTemplate && ( + + This is a template. You can create a copy of it to edit and use as your own. + + )} + + + {shortId === SCRATCHPAD_NOTEBOOK.short_id ? ( + + This is your scratchpad. It is only visible to you and is persisted only in this browser. + It's a great place to gather ideas before turning into a saved Notebook! + + ) : null} + +
+ + + + + +
-
+ )} ) } diff --git a/frontend/src/scenes/notebooks/Notebook/notebookLogic.ts b/frontend/src/scenes/notebooks/Notebook/notebookLogic.ts index c76a7adb6657d..0ace283e8b22c 100644 --- a/frontend/src/scenes/notebooks/Notebook/notebookLogic.ts +++ b/frontend/src/scenes/notebooks/Notebook/notebookLogic.ts @@ -1,20 +1,8 @@ import { lemonToast } from '@posthog/lemon-ui' -import { - actions, - beforeUnmount, - BuiltLogic, - connect, - kea, - key, - listeners, - path, - props, - reducers, - selectors, - sharedListeners, -} from 'kea' +import { actions, beforeUnmount, BuiltLogic, connect, kea, key, listeners, path, props, reducers, selectors } from 'kea' import { loaders } from 'kea-loaders' import { router, urlToAction } from 'kea-router' +import { subscriptions } from 'kea-subscriptions' import api from 'lib/api' import { downloadFile, slugify } from 'lib/utils' import posthog from 'posthog-js' @@ -34,6 +22,7 @@ import type { notebookLogicType } from './notebookLogicType' import { EditorRange, JSONContent, NotebookEditor } from './utils' const SYNC_DELAY = 1000 +const NOTEBOOK_REFRESH_MS = window.location.origin === 'http://localhost:8000' ? 5000 : 30000 export type NotebookLogicMode = 'notebook' | 'canvas' @@ -81,6 +70,7 @@ export const notebookLogic = kea([ setPreviewContent: (jsonContent: JSONContent) => ({ jsonContent }), clearPreviewContent: true, loadNotebook: true, + scheduleNotebookRefresh: true, saveNotebook: (notebook: Pick) => ({ notebook }), renameNotebook: (title: string) => ({ title }), setEditingNodeId: (editingNodeId: string | null) => ({ editingNodeId }), @@ -221,8 +211,14 @@ export const notebookLogic = kea([ } } else { try { - response = await api.notebooks.get(props.shortId) + response = await api.notebooks.get(props.shortId, undefined, { + 'If-None-Match': values.notebook?.version, + }) } catch (e: any) { + if (e.status === 304) { + // Indicates nothing has changed + return values.notebook + } if (e.status === 404) { return null } @@ -232,7 +228,7 @@ export const notebookLogic = kea([ const notebook = migrate(response) - if (!values.notebook && notebook.content) { + if (notebook.content && (!values.notebook || values.notebook.version !== notebook.version)) { // If this is the first load we need to override the content fully values.editor?.setContent(notebook.content) } @@ -421,15 +417,7 @@ export const notebookLogic = kea([ (shouldBeEditable, previewContent) => shouldBeEditable && !previewContent, ], }), - sharedListeners(({ values, actions }) => ({ - onNotebookChange: () => { - // Keep the list logic up to date with any changes - if (values.notebook && values.notebook.short_id !== SCRATCHPAD_NOTEBOOK.short_id) { - actions.receiveNotebookUpdate(values.notebook) - } - }, - })), - listeners(({ values, actions, sharedListeners, cache }) => ({ + listeners(({ values, actions, cache }) => ({ insertAfterLastNode: async ({ content }) => { await runWhenEditorIsReady( () => !!values.editor, @@ -560,8 +548,8 @@ export const notebookLogic = kea([ values.editor?.setContent(values.content) }, - saveNotebookSuccess: sharedListeners.onNotebookChange, - loadNotebookSuccess: sharedListeners.onNotebookChange, + saveNotebookSuccess: actions.scheduleNotebookRefresh, + loadNotebookSuccess: actions.scheduleNotebookRefresh, exportJSON: () => { const file = new File( @@ -592,6 +580,24 @@ export const notebookLogic = kea([ values.editor?.setTextSelection(selection) }) }, + + scheduleNotebookRefresh: () => { + clearTimeout(cache.refreshTimeout) + cache.refreshTimeout = setTimeout(() => { + actions.loadNotebook() + }, NOTEBOOK_REFRESH_MS) + }, + })), + + subscriptions(({ actions }) => ({ + notebook: (notebook?: NotebookType) => { + // Keep the list logic up to date with any changes + if (notebook && notebook.short_id !== SCRATCHPAD_NOTEBOOK.short_id) { + actions.receiveNotebookUpdate(notebook) + } + // If the notebook ever changes, we want to reset the scheduled refresh + actions.scheduleNotebookRefresh() + }, })), urlToAction(({ values, actions, cache }) => ({ @@ -606,7 +612,8 @@ export const notebookLogic = kea([ }, })), - beforeUnmount(() => { + beforeUnmount(({ cache }) => { + clearTimeout(cache.refreshTimeout) const hashParams = router.values.currentLocation.hashParams delete hashParams['🦔'] router.actions.replace( diff --git a/posthog/api/notebook.py b/posthog/api/notebook.py index 4781523e059f2..dd122a8c0dd7c 100644 --- a/posthog/api/notebook.py +++ b/posthog/api/notebook.py @@ -12,7 +12,9 @@ extend_schema_view, OpenApiExample, ) -from rest_framework import request, serializers, viewsets +from rest_framework import serializers, viewsets +from rest_framework.request import Request +from rest_framework.response import Response from rest_framework.decorators import action from rest_framework.permissions import IsAuthenticated @@ -253,7 +255,7 @@ def get_queryset(self) -> QuerySet: return queryset - def _filter_request(self, request: request.Request, queryset: QuerySet) -> QuerySet: + def _filter_request(self, request: Request, queryset: QuerySet) -> QuerySet: filters = request.GET.dict() for key in filters: @@ -329,8 +331,17 @@ def _filter_request(self, request: request.Request, queryset: QuerySet) -> Query return queryset + def retrieve(self, request: Request, *args: Any, **kwargs: Any) -> Response: + instance = self.get_object() + serializer = self.get_serializer(instance) + + if str(request.headers.get("If-None-Match")) == str(instance.version): + return Response(None, 304) + + return Response(serializer.data) + @action(methods=["GET"], url_path="activity", detail=False) - def all_activity(self, request: request.Request, **kwargs): + def all_activity(self, request: Request, **kwargs): limit = int(request.query_params.get("limit", "10")) page = int(request.query_params.get("page", "1")) @@ -338,7 +349,7 @@ def all_activity(self, request: request.Request, **kwargs): return activity_page_response(activity_page, limit, page, request) @action(methods=["GET"], url_path="activity", detail=True) - def activity(self, request: request.Request, **kwargs): + def activity(self, request: Request, **kwargs): notebook = self.get_object() limit = int(request.query_params.get("limit", "10")) page = int(request.query_params.get("page", "1")) diff --git a/posthog/api/test/notebooks/test_notebook.py b/posthog/api/test/notebooks/test_notebook.py index a82b8aef4062d..0325765001be0 100644 --- a/posthog/api/test/notebooks/test_notebook.py +++ b/posthog/api/test/notebooks/test_notebook.py @@ -229,3 +229,17 @@ def test_patching_does_not_leak_between_teams(self) -> None: data={"content": {"something": "here"}}, ) assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_responds_not_modified_if_versions_match(self) -> None: + response = self.client.post( + f"/api/projects/{self.team.id}/notebooks", + data={"content": {}, "text_content": ""}, + ) + assert response.status_code == status.HTTP_201_CREATED + + response = self.client.get( + f"/api/projects/{self.team.id}/notebooks/{response.json()['short_id']}", + HTTP_IF_NONE_MATCH=response.json()["version"], + ) + + assert response.status_code == status.HTTP_304_NOT_MODIFIED