feat(md): improve actions menu pattern, add task action#2691
Conversation
sedson
commented
Apr 20, 2026
- add deps to actions, add task action, reafactor task compooser
- adding more
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughFor task composition, an optional onSuccess callback is threaded through component registration into ComposeTask. Component params typing is widened. Lexical action/hotkey systems gain dependency-based gating, action composition/ignore options, and UI/behavior tweaks. Channel input gained disk-attach support. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
js/app/packages/block-md/component/ComposeTask.tsx (1)
465-494: 🧹 Nitpick | 🔵 TrivialExtract duplicated success-handling branch.
The
props.onSuccess ? ... : showTaskCreatedToast(...)/props.onCreateTask?.()block is repeated verbatim in both the "close immediately" and "create more" paths. Consolidating reduces drift risk if the success contract changes again.♻️ Suggested refactor
+ const notifySuccess = (documentId: string) => { + if (props.onSuccess) { + props.onSuccess({ documentId, title: taskTitle, content: taskContent }); + } else { + showTaskCreatedToast(documentId, taskTitle, taskContent); + } + props.onCreateTask?.(taskTitle, taskContent); + };Then replace both branches with
notifySuccess(documentId);.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-md/component/ComposeTask.tsx` around lines 465 - 494, Extract the duplicated success-handling into a single helper (e.g., notifySuccess) that accepts documentId, taskTitle, and taskContent and performs the shared logic: call props.onSuccess({ documentId, title: taskTitle, content: taskContent }) if present, otherwise call showTaskCreatedToast(documentId, taskTitle, taskContent), and then call props.onCreateTask?.(taskTitle, taskContent); replace the two verbatim branches around the "close immediately" path and after createTaskWithProperties with a call to notifySuccess(documentId, taskTitle, taskContent), making sure to keep the existing ordering relative to clearTaskComposerDraft() and setIsCreating(false) as in the original flows.js/app/packages/block-md/component/useMarkdownCommands.ts (1)
145-171:⚠️ Potential issue | 🟠 MajorBlock-level hotkeys drop the caller-provided
conditiongate.
shared.hideis built from theconditionparameter (line 122) so hotkeys are suppressed when the caller's gate is false (e.g., input not focused/visible). The newhideon line 161 completely overridesshared.hide, so block-level hotkeys will now appear/fire based purely on dependency presence even when the caller says the scope is inactive. Inline formats (lines 126-142) still honorshared.hide— this asymmetry is almost certainly unintended.The new
conditioncallback has the same problem: it ignores the outerconditionand only checks dependencies.🐛 Proposed fix — compose with the outer condition
const group = createHotkeyGroup(); - const hide = condition ? () => !condition() : undefined; - const shared = { scopeId, hide, runWithInputFocused: true } as const; + const hide = condition ? () => !condition() : undefined; + const shared = { scopeId, hide, runWithInputFocused: true } as const; @@ for (const action of ACTIONS) { const token = ACTION_ID_TO_TOKEN[action.id]; if (!token) continue; registerHotkey({ ...shared, hotkeyToken: token, description: action.name, icon: action.icon, keyDownHandler: () => { const editor = getEditor(); if (!editor) return false; action.action(editor); editor.focus(); return true; }, - hide: () => { - const editor = getEditor(); - if (!editor) return true; - return !editor.hasNodes(action.dependencies ?? []); - }, - condition: () => { - const editor = getEditor(); - if (!editor) return false; - return editor.hasNodes(action.dependencies ?? []); - }, + hide: () => { + if (condition && !condition()) return true; + const editor = getEditor(); + if (!editor) return true; + return !editor.hasNodes(action.dependencies ?? []); + }, + condition: () => { + if (condition && !condition()) return false; + const editor = getEditor(); + if (!editor) return false; + return editor.hasNodes(action.dependencies ?? []); + }, }).withGroup(group); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-md/component/useMarkdownCommands.ts` around lines 145 - 171, The block-level hotkey registration overrides the caller-provided guard by replacing shared.hide and shared.condition with functions that only check action.dependencies; update the hide and condition passed to registerHotkey so they combine (logical AND) the existing shared.hide/shared.condition with the dependency checks — e.g., in the registerHotkey call for each action, call the original shared.hide() and shared.condition() and only proceed if both the shared predicate and the editor.hasNodes(action.dependencies ?? []) are true; keep references to ACTIONS, ACTION_ID_TO_TOKEN, registerHotkey, shared, action and getEditor to locate and modify the hide and condition handlers.js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts (1)
208-219:⚠️ Potential issue | 🟡 Minor
videoaction missingdependencies— inconsistent with peers.Every other media/element action in this diff gained a
dependenciesentry (image→ImageNode,link→LinkNode,table→TableNode, etc.).videodispatchesTRY_INSERT_MEDIA_UPLOAD_COMMANDbut has no dependency declared, so bothActionsMenufiltering anduseMarkdownCommandshotkey gating will always show/enable it regardless of whether the required node is registered. If this is intentional (because the media upload path can create multiple node types), a brief comment would help; otherwise add the appropriate node(s).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts` around lines 208 - 219, The video action (id: 'video') dispatches TRY_INSERT_MEDIA_UPLOAD_COMMAND but lacks a dependencies entry, so update the action object to include the appropriate node dependency (e.g., VideoNode or a shared MediaNode) similar to other actions (ImageNode, LinkNode, TableNode) so ActionsMenu filtering and useMarkdownCommands gating work correctly; if omission was intentional because the upload flow can create multiple node types, add a brief comment above the action explaining why no dependencies are declared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/channel/Input/configured-markdown-editor.ts`:
- Around line 66-87: The new attachFromDiskAction uses id: 'image' which
unintentionally overrides the default image action from ACTIONS (per ActionsMenu
/ MarkdownConfigBuilder.withActions merge behavior) — change the action id to a
unique value such as 'attach-image' to preserve the default
TRY_INSERT_MEDIA_UPLOAD_COMMAND/ImageNode behavior; also make the file input a
one-shot element by removing its change listener (or using { once: true }) and
removing the input from the DOM after use so the handler is cleaned up
(reference attachFromDiskAction, onAttachFromDisk, ACTIONS, ActionsMenu,
MarkdownConfigBuilder.withActions).
- Line 59: The ignoreActionIds list passed to withActions currently contains the
incorrect action id 'equation', so the math/LaTeX item is not being hidden;
update the ignoreActionIds array in configured-markdown-editor.ts (the call to
withActions) to use the correct math action id 'latex' (matching the id defined
in plugins/actions/actions.ts) so that the math slash item is properly hidden
alongside 'hr' and 'table'.
In
`@js/app/packages/core/component/LexicalMarkdown/component/menu/ActionsMenu.tsx`:
- Around line 68-82: The nested defensive check using
props.editor.hasNodes(dependencies) duplicates the earlier validActions
filtering and is redundant; simplify the click handler in ActionsMenu.tsx by
removing the inner dependencies conditional and console.error branch and always
invoking action(props.editor) when action is present (keep the outer check for
action and the existing validActions-based filtering), referencing the symbols
action, dependencies, props.editor.hasNodes, and validActions to locate and
update the code.
In `@js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts`:
- Around line 168-194: The 'task' action silently returns when
globalSplitManager() is undefined and its onSuccess callback can call
editor.dispatchCommand after the editor has been unmounted; update the action
(id: 'task') to log or surface an error when globalSplitManager() is missing
(match existing console.error behavior), and protect the async onSuccess closure
by using a disposal guard/cancellation check before calling
editor.dispatchCommand (for example use editor.getRootElement() or a local
disposed flag tied to the editor lifetime, mirroring the pattern in
EmailMessageBody), and finally verify the peer 'video' action to add the missing
dependency (e.g., include VideoNode in the dependencies array) so media nodes
are declared consistently.
---
Outside diff comments:
In `@js/app/packages/block-md/component/ComposeTask.tsx`:
- Around line 465-494: Extract the duplicated success-handling into a single
helper (e.g., notifySuccess) that accepts documentId, taskTitle, and taskContent
and performs the shared logic: call props.onSuccess({ documentId, title:
taskTitle, content: taskContent }) if present, otherwise call
showTaskCreatedToast(documentId, taskTitle, taskContent), and then call
props.onCreateTask?.(taskTitle, taskContent); replace the two verbatim branches
around the "close immediately" path and after createTaskWithProperties with a
call to notifySuccess(documentId, taskTitle, taskContent), making sure to keep
the existing ordering relative to clearTaskComposerDraft() and
setIsCreating(false) as in the original flows.
In `@js/app/packages/block-md/component/useMarkdownCommands.ts`:
- Around line 145-171: The block-level hotkey registration overrides the
caller-provided guard by replacing shared.hide and shared.condition with
functions that only check action.dependencies; update the hide and condition
passed to registerHotkey so they combine (logical AND) the existing
shared.hide/shared.condition with the dependency checks — e.g., in the
registerHotkey call for each action, call the original shared.hide() and
shared.condition() and only proceed if both the shared predicate and the
editor.hasNodes(action.dependencies ?? []) are true; keep references to ACTIONS,
ACTION_ID_TO_TOKEN, registerHotkey, shared, action and getEditor to locate and
modify the hide and condition handlers.
In `@js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts`:
- Around line 208-219: The video action (id: 'video') dispatches
TRY_INSERT_MEDIA_UPLOAD_COMMAND but lacks a dependencies entry, so update the
action object to include the appropriate node dependency (e.g., VideoNode or a
shared MediaNode) similar to other actions (ImageNode, LinkNode, TableNode) so
ActionsMenu filtering and useMarkdownCommands gating work correctly; if omission
was intentional because the upload flow can create multiple node types, add a
brief comment above the action explaining why no dependencies are declared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9788c96f-62f9-4708-9a9b-4b0f4a4205f0
📒 Files selected for processing (11)
js/app/packages/app/component/split-layout/componentRegistry.tsxjs/app/packages/app/component/split-layout/layoutManager.tsjs/app/packages/block-md/component/ComposeTask.tsxjs/app/packages/block-md/component/useMarkdownCommands.tsjs/app/packages/channel/Input/ChannelInput.tsxjs/app/packages/channel/Input/configured-markdown-editor.tsjs/app/packages/core/component/LexicalMarkdown/builder/MarkdownConfigBuilder.tsjs/app/packages/core/component/LexicalMarkdown/builder/MarkdownShell.tsxjs/app/packages/core/component/LexicalMarkdown/builder/types.tsjs/app/packages/core/component/LexicalMarkdown/component/menu/ActionsMenu.tsxjs/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts
| if (options.onAttachFromDisk) { | ||
| const onAttachFromDisk = options.onAttachFromDisk; | ||
| const attachFromDiskAction: Action = { | ||
| id: 'image', | ||
| name: 'Image', | ||
| keywords: ['picture', 'photo', 'img', 'upload', 'attach'], | ||
| category: ActionCategory.MEDIA, | ||
| icon: ImageIcon, | ||
| action: () => { | ||
| const input = document.createElement('input'); | ||
| input.type = 'file'; | ||
| input.multiple = true; | ||
| input.accept = 'image/*'; | ||
| input.addEventListener('change', () => { | ||
| const files = Array.from(input.files ?? []); | ||
| if (files.length > 0) onAttachFromDisk(files); | ||
| }); | ||
| input.click(); | ||
| }, | ||
| }; | ||
| editor.withActions({ additionalActions: [attachFromDiskAction] }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify intended override of the default image slash action.
This additional action uses id: 'image', which collides with the default image action in ACTIONS. Per the merge logic in ActionsMenu (and MarkdownConfigBuilder.withActions), it will replace the default — so the default TRY_INSERT_MEDIA_UPLOAD_COMMAND behavior is lost in the channel input and there is no longer an ImageNode dependency on this entry. If that's intentional (channel input routes attachments outside Lexical), consider a distinct id like 'attach-image' to avoid surprising future maintainers who expect the default behavior.
Also note: the hidden <input> is never removed and its change listener is retained until the element is GC'd. Not a leak in practice, but onCleanup/one-shot listener removal would be tidier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/channel/Input/configured-markdown-editor.ts` around lines 66
- 87, The new attachFromDiskAction uses id: 'image' which unintentionally
overrides the default image action from ACTIONS (per ActionsMenu /
MarkdownConfigBuilder.withActions merge behavior) — change the action id to a
unique value such as 'attach-image' to preserve the default
TRY_INSERT_MEDIA_UPLOAD_COMMAND/ImageNode behavior; also make the file input a
one-shot element by removing its change listener (or using { once: true }) and
removing the input from the DOM after use so the handler is cleaned up
(reference attachFromDiskAction, onAttachFromDisk, ACTIONS, ActionsMenu,
MarkdownConfigBuilder.withActions).
| const { action, dependencies } = props.action; | ||
| if (action) { | ||
| action(props.editor); | ||
| if (dependencies !== undefined) { | ||
| if (props.editor.hasNodes(dependencies)) { | ||
| action(props.editor); | ||
| } else { | ||
| console.error( | ||
| 'Dispatched Action with missing dependencies:', | ||
| props.action | ||
| ); | ||
| } | ||
| } else { | ||
| action(props.editor); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Defensive editor.hasNodes check duplicates the validActions filter.
Since validActions already excludes actions whose dependencies aren't present, this branch can only log the console.error if the ACTIONS list is mutated or dependencies change between filter and click (effectively unreachable given current usage). Not a bug, but the nested conditional could be simplified to action(props.editor) with the filter as the single source of truth — or the filter could be dropped and this kept as the only gate. Keeping both is fine as a belt-and-braces guard; just calling out the redundancy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/app/packages/core/component/LexicalMarkdown/component/menu/ActionsMenu.tsx`
around lines 68 - 82, The nested defensive check using
props.editor.hasNodes(dependencies) duplicates the earlier validActions
filtering and is redundant; simplify the click handler in ActionsMenu.tsx by
removing the inner dependencies conditional and console.error branch and always
invoking action(props.editor) when action is present (keep the outer check for
action and the existing validActions-based filtering), referencing the symbols
action, dependencies, props.editor.hasNodes, and validActions to locate and
update the code.
| { | ||
| id: 'task', | ||
| name: 'Task', | ||
| keywords: ['task', 'todo', 'create'], | ||
| category: ActionCategory.ELEMENT, | ||
| icon: CheckSquare, | ||
| action: (editor: LexicalEditor) => { | ||
| const splitManager = globalSplitManager(); | ||
| if (!splitManager) return; | ||
| splitManager.createPopoverSplit({ | ||
| content: { | ||
| type: 'component', | ||
| id: 'task-compose', | ||
| params: { | ||
| onSuccess: (result: ComposeTaskSuccess) => { | ||
| editor.dispatchCommand(INSERT_DOCUMENT_MENTION_COMMAND, { | ||
| documentId: result.documentId, | ||
| documentName: result.title, | ||
| blockName: 'task', | ||
| }); | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| }, | ||
| dependencies: [DocumentMentionNode], | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm globalSplitManager()'s nullable behavior and whether createPopoverSplit params are typed.
rg -nP -C3 'export\s+(const|function)\s+globalSplitManager\b'
rg -nP -C5 'createPopoverSplit\s*\(' --type=ts
rg -nP -C3 "id:\s*'task-compose'" --type=tsRepository: macro-inc/macro
Length of output: 7629
🏁 Script executed:
# Check PopoverSplitOptions type definition
rg -nP 'type\s+PopoverSplitOptions|interface\s+PopoverSplitOptions' --type=ts
# Check ComposeTask.tsx for onSuccess callback handling
rg -nP -A20 "onSuccess:" js/app/packages/block-md/component/ComposeTask.tsx
# Check if editor has disposal/cleanup tracking
rg -nP "editor\.getRootElement|editor\.dispose|isEditorDisposed" --type=ts
# Check how other actions handle splitManager null case
rg -nP -B3 -A3 "globalSplitManager\(\)" js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.tsRepository: macro-inc/macro
Length of output: 3016
🏁 Script executed:
# Get the full PopoverSplitOptions type definition
sed -n '87,110p' js/app/packages/app/component/split-layout/layoutManager.ts
# Check all actions for splitManager null handling patterns
rg -nP -B5 -A8 "globalSplitManager\(\)" js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts
# Check ComposeTask onSuccess callback implementation
sed -n '450,475p' js/app/packages/block-md/component/ComposeTask.tsxRepository: macro-inc/macro
Length of output: 1783
🏁 Script executed:
# Check if SplitContent params are typed
rg -nP 'type\s+SplitContent|interface\s+SplitContent' --type=ts
# Look for editor disposal patterns in LexicalMarkdown
rg -nP "editor\.(dispose|onError)" --type=ts
# Check if other action callbacks also capture editor and dispatch commands
rg -nP -B10 -A10 "dispatchCommand" js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts | head -80
# Check how layout.ts handles the null splitManager vs actions.ts
sed -n '75,85p' js/app/packages/app/component/split-layout/layout.tsRepository: macro-inc/macro
Length of output: 3168
🏁 Script executed:
# Get full SplitContent type definition
sed -n '35,70p' js/app/packages/app/component/split-layout/layoutManager.ts
# Check if all simple actions use dispatchCommand directly without null checks
rg -nP -B3 "action:\s*\(editor" js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts | head -100
# Check if there are any other popover callbacks with async onSuccess
rg -nP -B3 -A8 "onSuccess.*editor\.dispatchCommand" --type=tsRepository: macro-inc/macro
Length of output: 3391
🏁 Script executed:
# Check the video and image actions for comparison (they also open popovers)
sed -n '196,230p' js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts
# Search for other async callbacks that capture editor to see if there's a disposal pattern
rg -nP "onSuccess.*editor" --type=ts | head -20
# Check if ComposeTask component handles params.onSuccess or if params are untyped
rg -nP -B5 "props\.onSuccess" js/app/packages/block-md/component/ComposeTask.tsx
# Look for editor cleanup/disposal patterns in LexicalMarkdown
rg -nP "createEffect.*editor|useEffect.*editor" js/app/packages/core/component/LexicalMarkdown/ --type=ts | head -20Repository: macro-inc/macro
Length of output: 1493
🏁 Script executed:
# Check video action - does it really lack dependencies?
sed -n '211,221p' js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts
# Verify image and link also use queueMicrotask pattern
sed -n '196,245p' js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts
# Check if there's any type definition for ComposeTask params
rg -nP "interface.*ComposeTaskProps|type.*ComposeTaskProps" --type=ts
# Check if any other async popover callbacks guard against disposed editors
rg -nP -B5 -A10 "popoverSplit.*onSuccess" --type=tsRepository: macro-inc/macro
Length of output: 1749
🏁 Script executed:
# Get full ComposeTaskProps interface
sed -n '163,185p' js/app/packages/block-md/component/ComposeTask.tsx
# Check if any LexicalEditor instance tracks disposal state
rg -nP "isEditorDisposed|editor\.isDisposed" --type=ts
# Verify video action definition one more time to be certain it lacks dependencies
rg -nP -A5 "id:\s*'video'" js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts
# Check if ComposeTask is ever unmounted while compose flow is pending
rg -nP "unmount|dispose|cleanup.*ComposeTask" --type=tsRepository: macro-inc/macro
Length of output: 42673
task action: silent null check and unguarded editor reference in async onSuccess callback.
-
When
globalSplitManager()returns undefined, the action silently returns with no user feedback. Contrast withjs/app/packages/app/component/split-layout/layout.tswhich logsconsole.error('no split manager found')in the same scenario. Consider adding error logging or a toast so this state is diagnosable. -
The
onSuccesscallback captureseditorand dispatches into it asynchronously from a popover callback that may resolve after the editor unmounts. There's no guard against the editor being disposed. Pattern exists in the codebase (e.g.,js/app/packages/block-email/component/EmailMessageBody.tsxlines 167–177) using adisposedflag to prevent stale callbacks. Consider adding similar guards beforedispatchCommand(e.g., checkeditor.getRootElement()or a cancellation token tied to editor lifetime). -
Minor: The
videoaction (lines 211–220) lacks adependenciesfield while peer media actions (image,link) include it. Verify whetherVideoNodedependency should be added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/component/LexicalMarkdown/plugins/actions/actions.ts`
around lines 168 - 194, The 'task' action silently returns when
globalSplitManager() is undefined and its onSuccess callback can call
editor.dispatchCommand after the editor has been unmounted; update the action
(id: 'task') to log or surface an error when globalSplitManager() is missing
(match existing console.error behavior), and protect the async onSuccess closure
by using a disposal guard/cancellation check before calling
editor.dispatchCommand (for example use editor.getRootElement() or a local
disposed flag tied to the editor lifetime, mirroring the pattern in
EmailMessageBody), and finally verify the peer 'video' action to add the missing
dependency (e.g., include VideoNode in the dependencies array) so media nodes
are declared consistently.