Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 16 additions & 20 deletions lib/components/HashMenu/HashWpMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ import type { HashMenuItem } from "./types";
import type { AnyEditor } from "./editorUtils";
import {
getSizeFromCurrentBlock,
clearTriggerText,
insertWpChip,
insertWpChipIntoBlock,
} from "./editorUtils";

const Menu = styled.div.attrs({ className: "op-bn-hash-menu" })`
Expand Down Expand Up @@ -54,27 +52,31 @@ export function createHashWpMenuComponent(
const searchQuery = items[0]?.title ?? "";
const visibleResults = (resultsRef.current ?? []).slice(0, MAX_RESULTS);

const currentSize = getSizeFromCurrentBlock(editor);
const currentBlockId = editor.getTextCursorPosition()?.block?.id;
Comment thread
judithroth marked this conversation as resolved.

// Mutate each item's onItemClick so BlockNote's keyboard handler
// (Enter / PgUp / PgDn) calls the correct insertion for that result.
visibleResults.forEach((wp, index) => {
if (!items[index]) return;

const size = getSizeFromCurrentBlock(editor);
const blockId = editor.getTextCursorPosition()?.block?.id;

items[index].onItemClick = () => {
requestAnimationFrame(() => {
if (!blockId) return;
editor.focus();
if (!currentBlockId) return;

// BlockNote splits the block on Enter - remove the new empty block it created.
const currentBlock = editor.getTextCursorPosition()?.block;
if (currentBlock && currentBlock.id !== blockId) {

// BlockNote's default "Enter" behavior splits the block.
// If the block ID changed, it was a keyboard Enter, so we remove the new empty block.
const isKeyboardEnter = currentBlock && currentBlock.id !== currentBlockId;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we simplify this by using something like event.preventDefault and event.stopPropagation?


if (isKeyboardEnter) {
editor.removeBlocks([currentBlock.id]);
}

clearTriggerText(editor);
insertWpChipIntoBlock(editor, blockId, wp, size);
editor.focus();

insertWpChip(editor, wp, currentSize);
});
};
});
Expand All @@ -101,17 +103,11 @@ export function createHashWpMenuComponent(
<MenuItem
key={wp.id}
$selected={selectedIndex === index}
// Mouse path: e.preventDefault() stops BlockNote from doing its own
// cleanup, so we clear the trigger text manually before inserting.
// Mouse path: e.preventDefault() prevents the editor from losing focus.
// This allows us to safely insert the chip without needing TipTap to restore the cursor.
onMouseDown={(e) => {
e.preventDefault();
const size = getSizeFromCurrentBlock(editor);
const blockId = clearTriggerText(editor);
if (blockId) {
editor.focus();
editor.setTextCursorPosition(blockId, "end");
}
insertWpChip(editor, wp, size);
items[index]?.onItemClick();
}}
>
<BlockCard workPackage={wp} inDropdown />
Expand Down
86 changes: 25 additions & 61 deletions lib/components/HashMenu/editorUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function getSizeFromCurrentBlock(editor: AnyEditor): InlineWpSize {
for (const node of content) {
if (node.type !== "text") continue;
const text = node.text as string;
const match = text.match(/(#+)[^#]/);
const match = text.match(/(#+)/);
if (match) {
const hashCount = match[1].length;
if (hashCount >= 3) return "s";
Expand All @@ -34,43 +34,30 @@ export function getSizeFromCurrentBlock(editor: AnyEditor): InlineWpSize {
return "xxs";
}

/**
* Removes the # trigger text (and any extra # symbols) from the current block.
* BlockNote removes #query itself on Enter, but may leave extra # characters
* (e.g. ## from ###query). Returns the block id, or null if nothing was found.
*/
export function clearTriggerText(editor: AnyEditor): string | null {
const block = editor.getTextCursorPosition()?.block;
if (!block) return null;

const content = (block.content ?? []) as any[];

const triggerNodeIndex = content.findIndex((n) => {
if (n.type !== "text") return false;
return /#+/.test(n.text as string);
});
const tiptap = (editor as any)._tiptapEditor;
if (!tiptap) return null;

if (triggerNodeIndex === -1) return null;
const { state, view } = tiptap;
const { selection } = state;
const { $from, from } = selection;

const triggerNode = content[triggerNodeIndex] as { type: string; text: string; styles: any };
const text = triggerNode.text;
const hashIndex = text.search(/#/);
const textBefore = hashIndex > 0 ? text.slice(0, hashIndex) : null;
const textBefore = $from.parent.textBetween(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of using TipTap here, would it be possible to use BlockNote's setTextCursorPosition to set the position right before the newly inserted block (which I assume is inserted right after the search text the user entered), then you can delete backwards until the "#" sign, without having to worry about having a BlockNote block messing with the counting - since the search via "#" only is text, there can be no other block than the one that was just created.

If that is not possible I am okay with keeping TipTap, but at the moment I don't see why this approach should cause problems and why we necessarily need TipTap here 😅

Math.max(0, $from.parentOffset - 50),
$from.parentOffset,
undefined,
"\n"
);

const cleanedContent = [
...content.slice(0, triggerNodeIndex),
...(textBefore ? [{ type: "text", text: textBefore, styles: triggerNode.styles }] : []),
];
const match = textBefore.match(/(#+\S*)$/);
if (!match) return null;

editor.updateBlock(block.id, { content: cleanedContent } as any);
return block.id;
}
const triggerLength = match[1].length;
const tr = state.tr.delete(from - triggerLength, from);
view.dispatch(tr);

function focusAndMoveToEnd(editor: AnyEditor, blockId: string): void {
requestAnimationFrame(() => {
editor.focus();
editor.setTextCursorPosition(blockId, "end");
});
const block = (editor as any).getTextCursorPosition()?.block;
return block?.id ?? null;
}

/**
Expand All @@ -80,43 +67,20 @@ function focusAndMoveToEnd(editor: AnyEditor, blockId: string): void {
export function insertWpChip(editor: AnyEditor, wp: WorkPackage, size: InlineWpSize): void {
const instanceId = makeInstanceId();

clearTriggerText(editor);

(editor.insertInlineContent as (content: unknown[]) => void)([
{ type: "openProjectWorkPackageInline", props: { wpid: String(wp.id), instanceId, size } },
{ type: "text", text: " ", styles: {} },
]);

requestAnimationFrame(() => {
editor.focus();
const cursor = editor.getTextCursorPosition();
if (cursor?.block?.id) {
editor.setTextCursorPosition(cursor.block.id, "end");
}
});
editor.focus();
}
/**
* Keyboard (Enter) path: inserts chip directly into block content by ID,
* bypassing cursor position entirely to avoid race conditions with
* BlockNote's Enter handling which moves the cursor to a new block.
*/
export function insertWpChipIntoBlock(
editor: AnyEditor,
blockId: string,
_blockId: string,
wp: WorkPackage,
size: InlineWpSize,
size: InlineWpSize
): void {
const instanceId = makeInstanceId();
const block = editor.getBlock(blockId);
if (!block) return;

const content = (block.content ?? []) as any[];

editor.updateBlock(blockId, {
content: [
...content,
{ type: "openProjectWorkPackageInline", props: { wpid: String(wp.id), instanceId, size } },
{ type: "text", text: " ", styles: {} },
],
} as any);

focusAndMoveToEnd(editor, blockId);
insertWpChip(editor, wp, size);
}
105 changes: 49 additions & 56 deletions test/lib/components/hashMenu.test.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding all those tests!
Please add at least one real integration test that interacts with a browser. I see value in these unit tests here since they cover edge cases like having multiple texts with "#" in a line and seeing that it is deleted only until the first # (from right) 👍
However, integration tests often uncover problems the developer or AI hasn't thought of yet. And those are typically very hard to test in unit tests :)

Original file line number Diff line number Diff line change
@@ -1,114 +1,107 @@
// @vitest-environment jsdom
import { describe, it, expect, vi } from "vitest";
import { BlockNoteEditor } from "@blocknote/core";
import {
getSizeFromCurrentBlock,
insertWpChipIntoBlock,
clearTriggerText,
} from "../../../lib/components/HashMenu/editorUtils";

type FakeContent = { type: string; text?: string; styles?: any; props?: any }[];

function makeFakeEditor(content: FakeContent = []) {
let block = { id: "block-1", content };
const inserted: FakeContent = [];

return {
block,
inserted,
getTextCursorPosition: () => ({ block }),
getBlock: (id: string) => (id === block.id ? block : null),
updateBlock: (id: string, update: { content: FakeContent }) => {
if (id === block.id) {
block.content = update.content;
inserted.push(...update.content);
}
},
insertInlineContent: (content: FakeContent) => {
inserted.push(...content);
},
focus: vi.fn(),
setTextCursorPosition: vi.fn(),
};
function createTestEditor(text: string) {
const editor = BlockNoteEditor.create({
initialContent: [{ type: "paragraph", content: text }],
});

const block = editor.document[0];
editor.setTextCursorPosition(block, "end");

return editor;
}

describe("getSizeFromCurrentBlock", () => {
it("returns xxs for #", () => {
const editor = makeFakeEditor([{ type: "text", text: "#foo" }]);
const editor = createTestEditor("#foo");
expect(getSizeFromCurrentBlock(editor as any)).toBe("xxs");
});

it("returns xs for ##", () => {
const editor = makeFakeEditor([{ type: "text", text: "##foo" }]);
const editor = createTestEditor("##foo");
expect(getSizeFromCurrentBlock(editor as any)).toBe("xs");
});

it("returns s for ### or more", () => {
const editor = makeFakeEditor([{ type: "text", text: "###foo" }]);
const editor = createTestEditor("###foo");
expect(getSizeFromCurrentBlock(editor as any)).toBe("s");

const editor2 = makeFakeEditor([{ type: "text", text: "####foo" }]);
const editor2 = createTestEditor("####foo");
expect(getSizeFromCurrentBlock(editor2 as any)).toBe("s");
});

it("returns xxs if no hashes", () => {
const editor = makeFakeEditor([{ type: "text", text: "foo" }]);
const editor = createTestEditor("foo");
expect(getSizeFromCurrentBlock(editor as any)).toBe("xxs");
});
});

describe("clearTriggerText", () => {
it("removes # text and returns block id", () => {
const editor = makeFakeEditor([{ type: "text", text: "#foo" }]);
const editor = createTestEditor("#foo");
const blockId = clearTriggerText(editor as any);
expect(blockId).toBe("block-1");
expect(editor.block.content).toEqual([]);

expect(blockId).toBe(editor.document[0].id);
const block = editor.getBlock(editor.document[0].id);
expect(block?.content).toEqual([]);
});

it("does nothing if no block", () => {
const editor = { getTextCursorPosition: () => null, updateBlock: vi.fn() };
const editor = {
_tiptapEditor: null,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of using TipTap, please try to create an empty BlockNote editor or just remove the contents from the one you create with createTestEditor instead.

getTextCursorPosition: () => null,
updateBlock: vi.fn(),
};
expect(clearTriggerText(editor as any)).toBeNull();
});

it("keeps text before # and removes trigger", () => {
const editor = makeFakeEditor([{ type: "text", text: "Hello #foo" }]);
const editor = createTestEditor("Hello #foo");
const blockId = clearTriggerText(editor as any);
expect(blockId).toBe("block-1");
expect(editor.block.content).toEqual([{ type: "text", text: "Hello " }]);

expect(blockId).toBe(editor.document[0].id);
const block = editor.getBlock(editor.document[0].id);
expect((block?.content as any)[0].text).toBe("Hello ");
});

it("works with multiple # in the text", () => {
const editor = makeFakeEditor([{ type: "text", text: "Pre #one #two #three" }]);
const editor = createTestEditor("Pre #one #two #three");
const blockId = clearTriggerText(editor as any);
expect(blockId).toBe("block-1");
expect(editor.block.content).toEqual([{ type: "text", text: "Pre " }]);

expect(blockId).toBe(editor.document[0].id);
const block = editor.getBlock(editor.document[0].id);

expect((block?.content as any)[0].text).toBe("Pre #one #two ");
});
});

describe("insertWpChipIntoBlock", () => {
it("adds a chip to the block content", () => {
const editor = makeFakeEditor([]);
const editor = createTestEditor("test");

const insertSpy = vi.spyOn(editor, "insertInlineContent").mockImplementation(() => {});
vi.spyOn(editor, "focus").mockImplementation(() => {});

insertWpChipIntoBlock(
editor as any,
"block-1",
editor.document[0].id,
{ id: 1, subject: "Fix bug" } as any,
"xxs"
);

const inserted = editor.inserted;
expect(inserted.length).toBe(2);

const chip = inserted.find(
(c): c is { type: string; props: { size: string } } =>
c.type === "openProjectWorkPackageInline" && c.props?.size
);
expect(chip).toBeDefined();
expect(chip?.props.size).toBe("xxs");
});

it("does nothing if block not found", () => {
const editor = makeFakeEditor([]);
expect(() =>
insertWpChipIntoBlock(editor as any, "wrong-id", { id: 1 } as any, "xxs")
).not.toThrow();
expect(insertSpy).toHaveBeenCalledWith([
{
type: "openProjectWorkPackageInline",
props: { wpid: "1", instanceId: expect.any(String), size: "xxs" },
},
{ type: "text", text: " ", styles: {} },
]);
});
});
Loading