Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: notebooks sharing step 1 #16415

Closed
wants to merge 41 commits into from
Closed

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Jul 6, 2023

Problem

Notebooks are the best, except for one thing, you can't use them as a simple blog...

Changes

  • sets up notebooks for sharing
  • puts button to trigger that behind a flag
  • set each component that can be added to a notebook and needs to make API calls so it shows a warning instead of failing API calls and showing an error state
  • generate an open graph image for the shared notebook
  • switches listing notebooks to use a basic serializer so we aren't loading all of the content to draw a table

This definitely isn't finished but is already a meaty PR

2023-07-07 11 38 54

How did you test this code?

some developer tests, some 👀 locally

./bin/tests -vv --lf posthog/models/test/test_exported_asset_model.py posthog/api/test/test_exports.py posthog/api/test/test_sharing.py posthog/tasks/exports/test/test_image_exporter.py posthog/api/test/notebooks/test_notebook.py

@pauldambra pauldambra requested a review from a team July 10, 2023 10:33
Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of comments. Main thing is a little confusion / conflation around when a notebook is being shared versus the state when the share modal is open

frontend/src/lib/constants.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/notebooks/Nodes/NotebookNodePerson.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/notebooks/Notebook/notebookLogic.ts Outdated Show resolved Hide resolved
!notebook?.is_template && {
label: 'Share',
onClick: () => {
push(urls.notebookShare(notebookId))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing we use the push method here to avoid actually setting the notebook into NotebookMode.Share via the setNotebookMode method as is done for View / Edit toggles:

{!editEnabled ? null : mode === NotebookMode.Edit ? (
<>
<LemonButton type="primary" onClick={() => setNotebookMode(NotebookMode.View)}>
Done
</LemonButton>
</>
) : (
<>
<LemonButton type="primary" onClick={() => setNotebookMode(NotebookMode.Edit)}>
Edit
</LemonButton>
</>
)}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the URL to open the modal is only to keep the mechanism the same between the different types of shareable thing. With the added bonus that it's nice for states to be addressable -> "You can click this link if you want to stop sharing that notebook"

frontend/src/lib/api.ts Outdated Show resolved Hide resolved
frontend/src/scenes/notebooks/Notebook/Notebook.tsx Outdated Show resolved Hide resolved
@pauldambra
Copy link
Member Author

have introduced a bug where when we render the notebooks sometimes we get an error... (reproducible by pinning to side or opening in full pane)

Error: flushSync was called from inside a lifecycle method. It cannot be called when React is already rendering.
    at flushSync3 (http://127.0.0.1:8234/static/chunk-P36WWPAF.js:17380:23)
    at PureEditorContent.maybeFlushSync (http://127.0.0.1:8234/static/chunk-FCWX6CS6.js:16646:38)
    at PureEditorContent.removeRenderer (http://127.0.0.1:8234/static/chunk-FCWX6CS6.js:16662:10)
    at ReactRenderer.destroy (http://127.0.0.1:8234/static/chunk-FCWX6CS6.js:16786:128)
    at ReactNodeView.destroy (http://127.0.0.1:8234/static/chunk-FCWX6CS6.js:16895:19)
    at CustomNodeViewDesc.destroy (http://127.0.0.1:8234/static/chunk-FCWX6CS6.js:6246:17)
    at NodeViewDesc.destroy (http://127.0.0.1:8234/static/chunk-FCWX6CS6.js:5534:24)
    at EditorView.destroy (http://127.0.0.1:8234/static/chunk-FCWX6CS6.js:9336:18)
    at Editor2.destroy (http://127.0.0.1:8234/static/chunk-FCWX6CS6.js:13154:17)
    at http://127.0.0.1:8234/static/chunk-FCWX6CS6.js:16982:16

2023-07-11 19 49 25

@pauldambra
Copy link
Member Author

maybe fixed here in next release :'( ueberdosis/tiptap#4000

@daibhin
Copy link
Contributor

daibhin commented Jul 12, 2023

I can reproduce this on master so I'd imagine it isn't related to your changes. Looks like ueberdosis/tiptap#3764 (comment) might fix it in the meantime

@pauldambra
Copy link
Member Author

I can reproduce this on master so I'd imagine it isn't related to your changes. Looks like ueberdosis/tiptap#3764 (comment) might fix it in the meantime

Ah, thanks for checking @daibhin I checked briefly and couldn't but maybe my machine is in a weird state 🤔

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@pauldambra
Copy link
Member Author

not so fast posthog-bot

@pauldambra pauldambra reopened this Jul 28, 2023
@posthog-bot posthog-bot removed the stale label Jul 31, 2023
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@pauldambra pauldambra reopened this Aug 15, 2023
@posthog-bot posthog-bot removed the stale label Aug 16, 2023
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@pauldambra pauldambra removed the stale label Aug 29, 2023
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@pauldambra
Copy link
Member Author

we'll come back to this

@pauldambra pauldambra closed this Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants