Skip to content

Note links: don't open on click while editing; add a link menu - #610

Open
fredrivett wants to merge 4 commits into
mainfrom
fredrivett/notes-app-link-ux-options
Open

fredrivett wants to merge 4 commits into
mainfrom
fredrivett/notes-app-link-ux-options

Conversation

@fredrivett

@fredrivett fredrivett commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

What & why

Clicking a link in an editable note opened it immediately, in the middle of editing. TipTap v3's StarterKit bundles the link extension with openOnClick: true, and we never overrode it.

Now:

  • Plain click/tap on a link just places the caret, as in any other editor.
  • ⌘/Ctrl-click opens the link in a new tab.
  • Link menu: while the caret is in a link (by click, tap or keyboard), a small card appears under it with the URL and Open · Copy · Edit · Remove. This is how links get opened on touch, where there's no modifier-click. Edit adds https:// to a bare domain and rejects unsafe schemes.
  • Read-only notes are unchanged: links still open natively.

Implementation notes:

  • Safe opening: stored note markdown isn't sanitised on parse, so [x](javascript:…) can exist in a note. Every open goes through getOpenableHref, which only allows http/https/mailto/tel. In the menu, an unsafe href shows with Open disabled; it can still be edited or removed.
  • Positioning: the menu is TipTap's BubbleMenu, appended to document.body so the composer card doesn't clip it. Inside the item dialog it's appended to the dialog instead, so Radix's focus trap still lets you reach the edit field.
  • Escape: pressing Escape in the edit field cancels the edit without closing the note. The key is caught on window in the capture phase, ahead of Radix's dismiss listener.
  • Dependency: @tiptap/extension-bubble-menu was already installed as an optional dependency of @tiptap/react. It's now declared in package.json.

Checklist

  • User-facing change → added/updated a PostHog event: note_link_opened with via: "menu" | "modifier_click"
  • User-facing or service change → docs updated, or N/A: N/A. This fixes link behaviour in an existing feature; no new flow, service or env var.
  • New error paths report via captureServerException / the error boundary, or N/A: N/A, client-only UI with no new error paths.
  • New behavior is covered by tests:
    • unit tests: link-href.test.ts
    • RTL tests for the panel, covering open, copy, unsafe href, edit save/invalid, Escape and blur-out
    • story play test: a plain click doesn't open the link, ⌘-click does
    • Storybook stories for the panel's variants
    • E2E (note-compose.spec.ts) covering the composer and detail dialog: a click doesn't open, the menu isn't clipped, Open from the menu, Escape keeps the dialog open, edit is saved, ⌘-click opens
  • If this fixes a recurring defect, considered a guardrail via the learn skill, or N/A: N/A, a one-off default.

Notes

  • Checked visually in light and dark mode, at a narrow viewport (the menu shifts to stay on-screen), in the composer card and in the detail dialog.
  • Adding a link to selected text (e.g. ⌘K) is out of scope. It would be a natural follow-up using the same menu.

🤖 Generated with Claude Code


Summary by cubic

Clicking a link in an editable note no longer opens it mid-edit. A plain click/tap places the caret and shows a link menu under the link with the URL plus Open, Copy, Edit, and Remove; ⌘/Ctrl-click opens the link in a new tab, and read-only notes still open links natively.

  • Stored note markdown isn't sanitized, so opening runs through a safe-href check allowing only http/https/mailto/tel; unsafe hrefs (e.g. javascript:) show with Open disabled but can still be edited or removed.
  • The menu follows the caret inside a link, appends to document.body so the composer card doesn't clip it (and to the dialog when open so the focus trap reaches the edit field), and Escape in the edit form cancels the edit without closing the note.
  • Adds a note_link_opened PostHog event with via: "menu" | "modifier_click" and declares @tiptap/extension-bubble-menu as a dependency.

Written for commit a3e18d9. Summary will update on new commits.

Review in cubic

fredrivett and others added 4 commits October 5, 2026 21:59
Stored note markdown isn't sanitised on parse, so anything that opens a
link checks it's an absolute http(s)/mailto/tel URL first. Also
normalises typed links (bare domains get https://) and formats a compact
display label.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
TipTap v3's StarterKit bundles the link extension with openOnClick on,
so any click on a link in an editable note opened it mid-edit. A plain
click now just places the caret; Cmd/Ctrl-click opens the link in a new
tab (via a safe-href check, tracked as note_link_opened). Read-only
notes still open links natively.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
With the caret in a link (click, tap or keyboard), a small card under
it shows the URL with Open, Copy, Edit and Remove — the way to open a
link on touch, where there's no modifier-click. Built on TipTap's
BubbleMenu (its extension, already installed via @tiptap/react, is now a
declared dependency). Appended to the dialog when inside one so the
focus trap reaches the edit field, and Escape cancels an edit without
closing the note.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
abode Ignored Ignored Preview Oct 5, 2026 9:00pm UTC

Request Review

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

4 issues found across 12 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="app/src/lib/link-href.ts">

<violation number="1" location="app/src/lib/link-href.ts:33">
P2: `mailto:123@example.com` is misclassified as `host:port` and saved as `https://mailto:123@example.com`, so editing this valid link opens `example.com` instead of composing mail. Recognize supported explicit schemes before applying the host:port exception.</violation>
</file>

<file name="app/src/components/note/note-link-menu-panel.test.tsx">

<violation number="1" location="app/src/components/note/note-link-menu-panel.test.tsx:63">
P2: `Copy link` is already visible before the click, so this assertion passes even if the click never invokes `onCopy`; the test does not verify the failed-copy path. Capture the mock, assert it was called, and check the label remains `Copy link` after its promise settles.</violation>
</file>

<file name="app/src/components/note/note-editor.stories.tsx">

<violation number="1" location="app/src/components/note/note-editor.stories.tsx:124">
P2: This assertion only detects calls through JavaScript `window.open`; the rendered link's native `_blank` navigation can open a tab without touching the spy. Assert that the click does not produce a browser popup/navigation instead.</violation>
</file>

<file name="app/src/components/note/note-link-menu-panel.tsx">

<violation number="1" location="app/src/components/note/note-link-menu-panel.tsx:133">
P2: Middle-clicking this anchor bypasses `onClick`, so the browser opens the link without `openNoteLink` recording `note_link_opened`. Handle middle-button `auxclick` and route it through `onOpen`.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread app/src/lib/link-href.ts
export function normalizeLinkInput(input: string): string | null {
const trimmed = input.trim();
if (!trimmed) return null;
if (HAS_SCHEME.test(trimmed) && !/^[^:/]+:\d/.test(trimmed)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: mailto:123@example.com is misclassified as host:port and saved as https://mailto:123@example.com, so editing this valid link opens example.com instead of composing mail. Recognize supported explicit schemes before applying the host:port exception.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/lib/link-href.ts, line 33:

<comment>`mailto:123@example.com` is misclassified as `host:port` and saved as `https://mailto:123@example.com`, so editing this valid link opens `example.com` instead of composing mail. Recognize supported explicit schemes before applying the host:port exception.</comment>

<file context>
@@ -0,0 +1,57 @@
+export function normalizeLinkInput(input: string): string | null {
+  const trimmed = input.trim();
+  if (!trimmed) return null;
+  if (HAS_SCHEME.test(trimmed) && !/^[^:/]+:\d/.test(trimmed)) {
+    return getOpenableHref(trimmed) ? trimmed : null;
+  }
</file context>
Suggested change
if (HAS_SCHEME.test(trimmed) && !/^[^:/]+:\d/.test(trimmed)) {
if (
HAS_SCHEME.test(trimmed) &&
(!/^[^:/]+:\d/.test(trimmed) ||
OPENABLE_PROTOCOLS.has(
`${trimmed.slice(0, trimmed.indexOf(":") + 1).toLowerCase()}`,
))
) {

const user = userEvent.setup();
renderPanel({ onCopy: vi.fn().mockResolvedValue(false) });
await user.click(screen.getByRole("button", { name: "Copy link" }));
expect(screen.getByRole("button", { name: "Copy link" })).toBeVisible();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Copy link is already visible before the click, so this assertion passes even if the click never invokes onCopy; the test does not verify the failed-copy path. Capture the mock, assert it was called, and check the label remains Copy link after its promise settles.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/components/note/note-link-menu-panel.test.tsx, line 63:

<comment>`Copy link` is already visible before the click, so this assertion passes even if the click never invokes `onCopy`; the test does not verify the failed-copy path. Capture the mock, assert it was called, and check the label remains `Copy link` after its promise settles.</comment>

<file context>
@@ -0,0 +1,143 @@
+    const user = userEvent.setup();
+    renderPanel({ onCopy: vi.fn().mockResolvedValue(false) });
+    await user.click(screen.getByRole("button", { name: "Copy link" }));
+    expect(screen.getByRole("button", { name: "Copy link" })).toBeVisible();
+  });
+
</file context>

return button;
});
await waitFor(() => expect(openButton).toBeVisible());
expect(open).not.toHaveBeenCalled();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: This assertion only detects calls through JavaScript window.open; the rendered link's native _blank navigation can open a tab without touching the spy. Assert that the click does not produce a browser popup/navigation instead.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/components/note/note-editor.stories.tsx, line 124:

<comment>This assertion only detects calls through JavaScript `window.open`; the rendered link's native `_blank` navigation can open a tab without touching the spy. Assert that the click does not produce a browser popup/navigation instead.</comment>

<file context>
@@ -85,3 +85,61 @@ export const ReportsEdits: Story = {
+        return button;
+      });
+      await waitFor(() => expect(openButton).toBeVisible());
+      expect(open).not.toHaveBeenCalled();
+
+      await user.keyboard("{Meta>}");
</file context>

href={href}
target="_blank"
rel="noopener noreferrer nofollow"
onClick={(event) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Middle-clicking this anchor bypasses onClick, so the browser opens the link without openNoteLink recording note_link_opened. Handle middle-button auxclick and route it through onOpen.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/components/note/note-link-menu-panel.tsx, line 133:

<comment>Middle-clicking this anchor bypasses `onClick`, so the browser opens the link without `openNoteLink` recording `note_link_opened`. Handle middle-button `auxclick` and route it through `onOpen`.</comment>

<file context>
@@ -0,0 +1,309 @@
+          href={href}
+          target="_blank"
+          rel="noopener noreferrer nofollow"
+          onClick={(event) => {
+            event.preventDefault();
+            onOpen();
</file context>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant