feat(prompt-input): collapse /<command> into inline pill#785
Conversation
Export a pure `(icon: string) => string` helper from command-icon.tsx. Refactor `<CommandIcon>` to call it internally so the fallback logic (REGISTRY[icon] ?? REGISTRY.command) lives in one place. The DOM-side input pill serializer (upcoming task) will import this without touching SolidJS reactivity.
Add cmd-mark pill awareness to getNodeLength, setCursorPosition, and setRangeEdge. A slash-command pill uses data-cmd-mark="true" and data-name=<name>; its logical offset length is 1 + name.length (slash trigger + name), while textContent is only name. The cmd-mark branch in getNodeLength is placed before the textContent fallback so the asymmetry is preserved. Both isPill predicates in setCursorPosition and setRangeEdge now include the data-cmd-mark check alongside data-type file/agent. Tests: 3 new cases covering getNodeLength value, branch-order asymmetry, and setCursorPosition snap behavior; all 1312 tests pass.
- Add createCommandMark() that builds the pill DOM: outer span with data-cmd-mark/name/source/icon + contenteditable=false, icon child (data-cmd-icon, class=command-icon, innerHTML from resolveCommandIconSvg), label child (data-cmd-label, textContent = name, no slash). - renderPartsToEditor: index-0 TextPart with .command → pill + tail fragment; tail = content.slice(1 + name.length) preserving separator + args verbatim. - parseEditorToParts: before main visit loop, detect leading data-cmd-mark child. Walk siblings collecting followingTextRun (text nodes, BR as \n); stop at file/agent/cmd-mark boundary. Push marked TextPart content = "/" + name + run. Skip consumed nodes by slicing children from startIndex = 1 + consumedSiblings. - Mid-stream data-cmd-mark (not at index 0) falls through to visit()'s recursive-children branch: label textContent surfaced as plain text, no command metadata emitted. - Import command-pill.css via side-effect import. - Add editor-serialize.test.ts: pill DOM attrs, round-trips for "/cmd ", "/cmd args", "/cmd args" (double-space), self-heal mid-stream pill.
Replace the plain TextPart in the command branch of extractPromptFromParts
with createCommandTextPart so the restored Prompt carries the command field.
The editor can then re-render the pill instead of falling back to raw text.
Plain-prompt and image-attachment branches are unchanged byte-for-byte.
Tests now assert command: { name, source } on all three command-mode cases.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThis PR implements visual collapsing of slash commands into inline pills within the prompt input. Users can trigger pills by typing a command then space, pasting command text into an empty prompt, or selecting from the slash popover. Pills render atomically (icon + brand-name), are deletable as a unit via backspace, and route through dedicated submit paths while maintaining backend compatibility. ChangesSlash Command Pill Input Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Suggested priority: P2 (includes user-path files (packages/app/src/components/prompt-input.tsx, packages/app/src/components/prompt-input/attachments.ts, packages/app/src/components/prompt-input/command-backspace.test.ts, packages/app/src/components/prompt-input/command-backspace.ts, packages/app/src/components/prompt-input/command-copy.test.ts, packages/app/src/components/prompt-input/command-copy.ts, packages/app/src/components/prompt-input/command-paste.test.ts, packages/app/src/components/prompt-input/command-paste.ts, packages/app/src/components/prompt-input/command-pill.css, packages/app/src/components/prompt-input/command-prepend.test.ts, packages/app/src/components/prompt-input/command-prepend.ts, packages/app/src/components/prompt-input/command-space-trigger.test.ts, packages/app/src/components/prompt-input/command-space-trigger.ts, packages/app/src/components/prompt-input/command-text-part.test.ts, packages/app/src/components/prompt-input/command-text-part.ts, packages/app/src/components/prompt-input/editor-dom.test.ts, packages/app/src/components/prompt-input/editor-dom.ts, packages/app/src/components/prompt-input/editor-input.ts, packages/app/src/components/prompt-input/editor-serialize.test.ts, packages/app/src/components/prompt-input/editor-serialize.ts, packages/app/src/components/prompt-input/invariant.test.ts, packages/app/src/components/prompt-input/invariant.ts, packages/app/src/components/prompt-input/keydown.ts, packages/app/src/components/prompt-input/popover-controllers.ts, packages/app/src/components/prompt-input/submit.test.ts, packages/app/src/components/prompt-input/submit.ts, packages/app/src/context/prompt.test.ts, packages/app/src/context/prompt.tsx, packages/app/src/utils/prompt.test.ts, packages/app/src/utils/prompt.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive 'command pill' system for the prompt input, enabling slash commands to be rendered as interactive UI elements. The changes introduce four primary interaction paths: selection from popovers (Path A), conversion of typed commands upon pressing space (Path B), conversion of command-shaped pastes into pills (Path C), and specialized submission routing for command-marked parts (Path D). Key technical additions include clipboard rewriting to ensure the leading slash is preserved during copy operations, a custom backspace fallback ladder for pill deconstruction, and updated DOM serialization logic to maintain command metadata across state changes. Extensive unit tests accompany the new logic, and I have no feedback to provide as no review comments were included.
…ction Helper module + tests. The editor-input.ts/prompt-input.tsx wiring landed in the preceding Path B commit since they share the same file.
When the user opens the slash popover by typing `/<query>` and then
selects a command from the popover, the previous code prepended the
marked TextPart without removing the typed slash literal, producing
`[marked, Text("/foo")]` with a leftover slash. Detect the slash-query
shape ([Text("/<query>")]) and replace instead of prepend.
Empty prompt and slash-query state both go through the replace branch;
the E1 branch keeps the prepend-with-original-content semantics for
non-empty unrelated prompts.
getTextLength and isNormalizedEditor both walked the DOM without recognising the command pill. getCursorPosition feeds cloned ranges through getTextLength, so caret math after a pill was off by one (visible label has only `name.length` chars but the logical length is `1 + name.length` for the implicit slash). isNormalizedEditor returning false on a normal pill child forced the reconcile pass to rebuild the entire editor on every keystroke, moving the caret and breaking IME composition. Both primitives now treat a `data-cmd-mark="true"` element the same way getNodeLength already did: as an opaque pill whose logical length is `1 + dataset.name.length`.
2372019 to
876d6ff
Compare
Perf delta summaryComparator: pass
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/components/prompt-input/command-copy.ts`:
- Around line 36-38: The current check only tests range.startContainer and
range.endContainer and misses ranges that intersect the editor with both
endpoints outside; update the condition in command-copy.ts to also consider the
range.commonAncestorContainer (or otherwise test intersection) so you return
false only when none of startContainer, endContainer or commonAncestorContainer
are inside the editor—for example, replace the single contains check with a
combined check using editor.contains(range.startContainer) ||
editor.contains(range.endContainer) ||
editor.contains(range.commonAncestorContainer) before returning false.
In `@packages/app/src/components/prompt-input/editor-serialize.ts`:
- Around line 71-75: The normalization currently treats any element with
el.dataset.cmdMark === "true" as normalized; change this so a cmd-mark is only
considered normalized when it is the first child (index 0) of its parent. In the
function containing this check in editor-serialize.ts, replace the unconditional
cmdMark return with a test that confirms el.dataset.cmdMark === "true" AND that
el is the parent's first child (e.g., parentElement?.children[0] === el or
computed index === 0); otherwise continue with normal checks (e.g., allow BR
only).
🪄 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: CHILL
Plan: Pro Plus
Run ID: ac496bb7-596f-42a1-a602-a677a5b1d977
📒 Files selected for processing (32)
packages/app/src/components/prompt-input.tsxpackages/app/src/components/prompt-input/attachments.tspackages/app/src/components/prompt-input/command-backspace.test.tspackages/app/src/components/prompt-input/command-backspace.tspackages/app/src/components/prompt-input/command-copy.test.tspackages/app/src/components/prompt-input/command-copy.tspackages/app/src/components/prompt-input/command-paste.test.tspackages/app/src/components/prompt-input/command-paste.tspackages/app/src/components/prompt-input/command-pill.csspackages/app/src/components/prompt-input/command-prepend.test.tspackages/app/src/components/prompt-input/command-prepend.tspackages/app/src/components/prompt-input/command-space-trigger.test.tspackages/app/src/components/prompt-input/command-space-trigger.tspackages/app/src/components/prompt-input/command-text-part.test.tspackages/app/src/components/prompt-input/command-text-part.tspackages/app/src/components/prompt-input/editor-dom.test.tspackages/app/src/components/prompt-input/editor-dom.tspackages/app/src/components/prompt-input/editor-input.tspackages/app/src/components/prompt-input/editor-serialize.test.tspackages/app/src/components/prompt-input/editor-serialize.tspackages/app/src/components/prompt-input/invariant.test.tspackages/app/src/components/prompt-input/invariant.tspackages/app/src/components/prompt-input/keydown.tspackages/app/src/components/prompt-input/popover-controllers.tspackages/app/src/components/prompt-input/submit.test.tspackages/app/src/components/prompt-input/submit.tspackages/app/src/context/prompt.test.tspackages/app/src/context/prompt.tsxpackages/app/src/utils/prompt.test.tspackages/app/src/utils/prompt.tspackages/ui/src/components/command-icon.test.tspackages/ui/src/components/command-icon.tsx
isNormalizedEditor previously accepted a `data-cmd-mark` element at any position, but spec invariant requires the marked TextPart to be at index 0 only. A mid-stream pill should signal that some path breached the invariant; letting the normalizer report it as normal blocks the reconcile pass from rebuilding the editor from the canonical Prompt and prevents self-healing. Tighten the predicate to require `index === 0` and add coverage for both shapes.
Path B's `mirror.input=true` short-circuited reconcile because the raw `/<name> ` text node passed isNormalizedEditor(): the prompt store gained a marked TextPart but the editor kept showing raw slash text, breaking #778's main acceptance path. Drop the mirror flag in the Path B branch so reconcile falls through to the non-mirror branch, which compares store vs DOM via isPromptEqual and forces renderEditorWithCursor when they diverge. Same call site also passed no target cursor to renderEditorWithCursor, which then read currentCursor() off the pre-repaint DOM. For Path C paste this meant caret 0 on an empty editor even though the store said "end of /<name> args"; for Backspace-into-pill it kept the stale caret near the deleted pill. Add an optional cursor parameter and have reconcile pass prompt.cursor() so store-originated repaints honor the target position the path computed. Regression covered by path-b-integration.test.ts: pins the chain from raw DOM + insertText space through to repainted [data-cmd-mark] DOM.
Re-opening the slash popover while the prompt already started with a marked TextPart and picking another command used to produce `[marked_new, marked_old, ...rest]`, violating the spec invariant that a Prompt has at most one marked TextPart and it lives at index 0. Detect the leading-marked case in prependCommandMark and replace the old marked TextPart with the new one instead of prepending alongside, preserving any tail args/files/agents/images by identity. Treat the user picking a second command as changing their mind about the command, not stacking two commands. Update command-prepend.test.ts to assert the at-most-one invariant and identity preservation of tail parts.
Switching to a second command via prependCommandMark drops the old args along with the old marked TextPart. The previous commit already behaved this way via `current.slice(1)`, but the intent was only implicit. Add a test that asserts `content === "/new "` after switching from `/old hello world`, plus a comment explaining the product reasoning: a pill is an atomic unit, and args carry the old command's semantics that may not map to the new command (e.g. /summarize "the diff" → /translate would silently feed mismatched arguments). This kills any ambiguity about whether args-drop was incidental or intended.
The previous commit dropped old args on command switch out of an overcautious "args might not fit the new command" reasoning. That reasoning does not survive contact with the actual command system: built-in commands like /review and /init both use \$ARGUMENTS as a plain string substitution (e.g. `git show \$ARGUMENTS`, `git diff \$ARGUMENTS...HEAD`), and most plugin commands do the same. There is no schema enforcing what args mean, so any text the user kept is still legitimate input. Silently deleting args the user already typed costs them real work and is not recoverable by re-selecting the original command. The user can see the args in the editor; if they no longer fit the new command, they remove them themselves. Extract args from the leading marked TextPart's content (everything after the `/<name> ` prefix) and feed them into createCommandTextPart for the new command. Tail parts (files, agents, images) still survive by identity. Test reversed to assert args carry over, plus an empty-args case covering the trailing-space invariant.
Path A/B/C all wrote the new marked Prompt straight into the prompt store and returned, bypassing the homepage owner mirror logic at the end of handleInput. Result: a user who turned a slash query into a pill on the homepage, then navigated away and came back, lost the pill — the owner still held the pre-pill raw text and replayed that on hydration. Same shape as the args-drop bug — the system silently discarded work the user did. Move the portable/pinned record() call into its own createEffect on prompt.current() + prompt.context.items() + imageAttachments(). Every mutation of the prompt store now flows through the same mirror, regardless of which path produced it. handleInput's tail-only duplicate is removed, as is the shouldReset branch's explicit owner clear — both are subsumed by the effect (portable.record() treats an empty payload as "clear snapshot", so reset still works). No loop risk: portable.record() and pinned.recordEdit() both dedupe by deep-equal payload, so hydration paths that set the prompt to the owner's own snapshot are a no-op on re-record. Test added to portable-draft.test.ts: a marked TextPart survives a record + restore + consumeForHomepage round-trip with its command field intact, pinning the contract the owner has to honor.
The mirror effect introduced last commit fires at mount with the
prompt store's default empty state. Both owner implementations treat
an empty payload as destructive: portable.record() clears the
snapshot via setSnapshot(null), and pinned.recordEdit() overwrites
the existing slot's content in place. Because the mirror effect is
declared before the hydration effect in createEditorInput, the
mount-time fire ran first and destroyed any pending portable
snapshot or pinned prefill before hydration had a chance to
consume them. Net effect: a homepage A→B navigation lost A's
draft, and a pinned prefill on mount got wiped to empty.
Add {defer: true} to the mirror effect's `on()` so the effect
skips its mount-time invocation and waits for the first real
prompt change. That change is whichever comes first: hydration
replaying a snapshot into the prompt (in which case the resulting
record() call is a deep-equal no-op against the owner's own
snapshot, no destructive overwrite), or the user typing
(legitimate record). A fresh homepage with no owner data simply
stays quiet until the user does something.
Two regression guards added to pin the hazard the defer:true
protects against — empty payload through portable.record()
destroys the snapshot, and empty payload through
pinned.recordEdit() overwrites the prefill in place. Anyone
reorganizing the mirror effect will see exactly what they're
defending against.
prompt.current() is session-keyed by {dir, id} in the prompt binding, so
homepage A -> homepage B navigation flips prompt.current() to the new
session's DEFAULT_PROMPT. defer:true only protects the initial mount fire;
the subsequent scope-swap fire would record an empty payload against the
new directory and destroy the snapshot held for the old one before the
hydration effect can move it.
Track lastSeenDir / lastSeenSessionID in the mirror effect and skip the
first fire after either changes. Hydration then runs and either applies a
pinned prefill or consumes a portable carry; the resulting prompt change
re-wakes the effect with refs updated, and recording proceeds normally
(deep-equal dedupe against the snapshot's own contents is a no-op).
Browsers usually fire an additional input event after compositionend with inputType="insertCompositionText" and composing=false, which would wake the mirror effect via handleInput. Not all browsers/input methods do; on those, the final committed prompt sits in the store but never reaches portable.record() / pinned.recordEdit(), so a draft committed via IME alone is lost on next homepage navigation. Add composing() to the mirror effect's tracked deps. The false->true transition fires the effect (skipped immediately by the compose guard); the true->false transition fires the effect with the post-commit prompt and records it. Scope-change refs are unaffected because dir/sessionID don't change across a compositionend.
…in tests
The mirror effect is the single source of truth for portable/pinned draft
recording. Pull it into owner-mirror.ts so the decision branches (composing
skip, session-route skip, scope-change skip, pinned vs portable routing)
can be exercised directly without going through createEffect.
Tests drive applyOwnerMirrorTick with synthetic state. They cover:
- IME compositionend records the committed prompt without relying on a
post-compose input event
- session-route ticks return skip-session and leave both owners alone
- homepage A -> homepage B navigation returns skip-scope and preserves
the snapshot held for the old directory
- pinned slot bound to the current dir routes to pinned (portable
untouched); slot bound elsewhere routes to portable
`{defer: true}` on the wrapping createEffect is a SolidJS framework primitive
and is not exercised here; bun's test runner resolves solid-js to its
server build, where createEffect is a no-op. The pinned-draft.test.ts /
portable-draft.test.ts hazard-pin tests at the owner layer remain the
canonical proof of why defer matters.
Summary
Collapse
/<command> argsin the prompt input into the same inline pill (icon + brand-colored name) that PR #768 already renders in the sent bubble. Selecting from the slash popover, typing the command followed by Space, or pasting/<known-name> argsinto an empty input all materialise the leading/<name>into adata-cmd-markelement while the args remain plain text.Why
Fixes #778. The sent bubble shows the command name as a brand-colored pill, but until now the same text in the prompt input rendered as raw
/<name>. Two surfaces, two visual treatments for the same concept. This PR aligns the editor side to the send-side model: a single optionalTextPart.commandfield carries the metadata, content stays/<name> <args>end-to-end, andmetadata.commandInvocationon the wire is unchanged.Related Issue
Fixes #778
Human Review Status
Pending
Review Focus
The marked-TextPart invariants. There is at most one per Prompt, always at index 0, and
contentalways equals/<name> <args>with a single trailing space after the name. Most of the new code is the helpers (command-text-part.ts,command-prepend.ts,command-space-trigger.ts,command-paste.ts,command-backspace.ts) that enforce these invariants on the way into the store, plus the editor-side primitives (editor-dom.ts,editor-serialize.ts) that treat the pill as a unit of logical length1 + name.length. The contenteditable round-trip is the most fragile surface and has the largest test coverage.Other things worth double-checking:
command-text-part.ts:16-45. Non-ASCII names fall back to exact byte match so caret math stays length-preserving.inputType === "insertText" && data === " ", which is naturally false on paste, Backspace, IME commit, and programmatic mutations. No flag state machine.command-backspace.ts: caret inside the prefix region of a marked TextPart degrades the pill back to plain text without dropping attachments.Risk Notes
imageAttachments()through to the new prompt, so the existing image flow is unchanged. Same forprompt.context.items().FileAttachmentPart/AgentPart/ImagePart.text.split(" ")[0] === "/<known-name>"codepath insubmit.tsis kept as a safety net; the new path only fires whenpart.commandmetadata is present.editor-dom.test.tsandeditor-serialize.test.tscover the DOM contract; a visual diff for the pill itself can be added in a small follow-up.dev:desktopwalkthrough of the IME + paste + Backspace paths on macOS/Windows. The unit and integration coverage exercises the same code paths.How To Verify
The 4 pre-existing icon-button test failures on
dev(CSS 30x30 vs test expectation 24x24) are unrelated to this PR.Screenshots or Recordings
(To be attached by reviewer; pill visuals match PR #768 sent-bubble exactly, sharing
--brand-primaryand--font-weight-bodyviacommand-pill.css.)Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit