feat: Notebooks links

[daibhin]

Problem

Towards #15680

• Links currently turn white when selected

Changes

1. Link pasting

When you paste a link into a notebook it should automatically unfurl

2. Link behaviour

• Clicking an external link will redirect you to a new tab
• Clicking a PostHog link will transition within the same tab but open the notebook in the sidebar view

How did you test this code?

Locally 👀

[pauldambra]

only one thing with useCallback (just because life tends to be easier the more we stick in kea land)

[pauldambra]

😍

[pauldambra]

Suggested change

	span::selection {
	// overriding ::selection is necessary here because
	// antd makes it invisible otherwise
	span::selection {

[pauldambra]

thinking its best to capture the context for the ::selection to make life easier for the future traveller

[pauldambra]

we should probably move some or all of this to the notebookSidebarLogic

A selector on that logic that returns a function that takes internal and shortId as args would probably be a step.

The selector would be equivalent to useCallback I think because it also only changes when its dependencies change (with the difference being that internal is provided as a simple first step

[daibhin]

Thanks for the suggestion! My Kea isn't brilliant but I think I implemented what you suggested and tested that everything still works.

One question I had around Kea as I went was whether there would be a difference between:

selectors(({ actions }) => ({
    notebookLinkClicked: [
        (s) => [s.notebookSideBarShown],
        (notebookSideBarShown): ((shortId: string, internal: boolean) => void) => {
            return (shortId: string, internal: boolean) => {
                if (!notebookSideBarShown && internal) {
                    actions.selectNotebook(shortId)
                    actions.setNotebookSideBarShown(true)
                }
            }
       },
    ],
})),

and

selectors(({ values, actions }) => ({
    notebookLinkClicked: [
        () => [],
        (): ((shortId: string, internal: boolean) => void) => {
            return (shortId: string, internal: boolean) => {
                if (!values.notebookSideBarShown && internal) {
                    actions.selectNotebook(shortId)
                    actions.setNotebookSideBarShown(true)
                }
            }
       },
    ],
})),

They both seem to work. I read that a selector is created for each value - is there a preference for referencing the value over the selector in any case?

Moved away from selectors because it was the wrong paradigm to begin with

[pauldambra]

ah wait... is this bad advice from me I wonder

@Twixes calling actions in Selectors... verboten?

[daibhin]

It seems to be done elsewhere

posthog/frontend/src/layout/navigation-3000/sidebars/insights.ts

Lines 45 to 138 in 1621ad9

	selectors(({ actions, values, cache }) => ({
	contents: [
	(s) => [s.insights, s.infiniteInsights, s.insightsLoading, teamLogic.selectors.currentTeamId],
	(insights, infiniteInsights, insightsLoading, currentTeamId) => [
	{
	key: 'insights',
	title: 'Insights',
	items: infiniteInsights.map(
	(insight) =>
	insight &&
	({
	key: insight.short_id,
	name: insight.name || insight.derived_name || 'Untitled',
	isNamePlaceholder: !insight.name,
	url: urls.insightView(insight.short_id),
	searchMatch: findSearchTermInItemName(
	insight.name || insight.derived_name || '',
	values.searchTerm
	),
	menuItems: (initiateRename) => [
	{
	items: [
	{
	to: urls.insightEdit(insight.short_id),
	label: 'Edit',
	},
	{
	onClick: () => {
	actions.duplicateInsight(insight)
	},
	label: 'Duplicate',
	},
	],
	},
	{
	items: [
	{
	onClick: initiateRename,
	label: 'Rename',
	keyboardShortcut: ['enter'],
	},
	{
	onClick: () => {
	deleteWithUndo({
	object: insight,
	endpoint: `projects/${currentTeamId}/insights`,
	callback: actions.loadInsights,
	})
	},
	status: 'danger',
	label: 'Delete insight',
	},
	],
	},
	],
	onRename: async (newName) => {
	const updatedItem = await api.update(
	`api/projects/${teamLogic.values.currentTeamId}/insights/${insight.id}`,
	{
	name: newName,
	}
	)
	insightsModel.actions.renameInsightSuccess(updatedItem)
	},
	} as BasicListItem)
	),
	loading: insightsLoading,
	remote: {
	isItemLoaded: (index) => !!(cache.requestedInsights[index] || infiniteInsights[index]),
	loadMoreItems: async (startIndex) => {
	for (let i = startIndex; i < startIndex + INSIGHTS_PER_PAGE; i++) {
	cache.requestedInsights[i] = true
	}
	await savedInsightsLogic.actions.setSavedInsightsFilters(
	{ page: Math.floor(startIndex / INSIGHTS_PER_PAGE) + 1 },
	true,
	false
	)
	},
	itemCount: insights.count,
	minimumBatchSize: INSIGHTS_PER_PAGE,
	},
	} as SidebarCategory,
	],
	],
	activeListItemKey: [
	(s) => [s.activeScene, s.sceneParams],
	(activeScene, sceneParams) => {
	return activeScene === Scene.Insight && sceneParams.params.shortId ? sceneParams.params.shortId : null
	},
	],
	// kea-typegen doesn't like selectors without deps, so searchTerm is just for appearances
	debounceSearch: [(s) => [s.searchTerm], () => true],
	})),

Whether that means it is right or not 🤷

[daibhin]

Perhaps there should be a notebookLinkClicked action with an associated listener that then calls the subsequent actions?

[Twixes]

I think in some cases that may work, but it is verboten. Selectors are part of the pure functions "ecosystem" in Kea, no side effects allowed (along with actions, and reducers). For side effects, reach for listeners and subscriptions

[Twixes]

@daibhin So, the code you linked doesn't actually call the action in the selector, it just returns a value with a callback to an action. That is is völlig erlaubt, since the selector is still pure that way

[pauldambra]

👍 sorry for the diversion through options 😊