Skip to content

refactor: commandline#46

Open
d1rshan wants to merge 2 commits into
mainfrom
refactor/cmdline
Open

refactor: commandline#46
d1rshan wants to merge 2 commits into
mainfrom
refactor/cmdline

Conversation

@d1rshan
Copy link
Copy Markdown
Owner

@d1rshan d1rshan commented Jun 2, 2026

No description provided.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 2, 2026

Greptile Summary

This PR refactors the commandline feature by extracting all stateful logic into a createCommandlineController factory function and inlining the CommandlineInput sub-component, converting the default export to a named export in the process.

  • The controller pattern cleanly separates reactive state, derived memos, and public actions, but cmd.selectCurrent (type () => void) is passed where CommandlineList expects onSelectItem: (item: CommandlineItemType) => void; the list calls the callback with the clicked item, which selectCurrent silently ignores in favour of visibleItems()[selectedIndex()], causing the wrong command to run when the cursor hasn't hovered over the target item since the palette was opened.
  • The inlined CommandlineInput drops bg-transparent and text-(--text) from the input's class list, which can produce a white browser-default background and theme-unaware text colour in some browsers.

Confidence Score: 3/5

Not safe to merge as-is: clicking a command palette item can silently execute the wrong command when the cursor hasn't moved since the palette opened.

The click-selection path now ignores the specific item passed by the list and relies entirely on selectedIndex, which is only updated via pointer-move events. A user who opens the palette and immediately clicks an item (cursor stationary) will trigger whatever command is at selectedIndex (defaulting to 0), not the one they clicked. Combined with other regressions carried over from previous review rounds, the commandline's core interaction loop is unreliable.

apps/frontend/src/features/commandline/components/commandline.tsx — the onSelectItem wiring and the inlined CommandlineInput styling both need attention.

Important Files Changed

Filename Overview
apps/frontend/src/features/commandline/components/commandline.tsx Major refactor extracts logic into createCommandlineController; introduces a click-selection bug where selectCurrent ignores the item passed by CommandlineList and uses selectedIndex instead, running the wrong command when the pointer hasn't hovered an item since the palette opened.
apps/frontend/src/features/commandline/components/commandline-input.tsx File deleted; its contents were inlined into commandline.tsx with minor styling differences (bg-transparent and text-(--text) removed from the input class).
apps/frontend/src/app/layout.tsx Import updated from default to named export — trivial, safe change.

Reviews (2): Last reviewed commit: "mobile" | Re-trigger Greptile

Comment thread apps/frontend/src/features/commandline/components/commandline.tsx
Comment thread apps/frontend/src/features/commandline/components/commandline.tsx
Comment thread apps/frontend/src/features/commandline/components/commandline.tsx
Comment on lines +22 to +36
createEffect(() => {
if (cmd.scope() === "themes" && cmd.isOpen()) {
const item = cmd.visibleItems()[cmd.selectedIndex()];

if (item && item.id.startsWith("theme-")) {
themeManager.preview(item.id.replace("theme-", "") as any);
}

return;
}

if (cmd.isOpen()) {
themeManager.reset();
}
});
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.

P1 themeManager.reset() not called when closing from the themes scope

The close() action calls setScope("root") and setIsOpen(false) synchronously in the same call frame. SolidJS defers createEffect callbacks until after all synchronous signal writes complete, so by the time the theme effect runs, both scope() = "root" and isOpen() = false. Neither branch of the effect fires (isOpen() is falsy), so themeManager.reset() is skipped. If a user previews a theme and then clicks the backdrop to close the palette, the previewed theme is never reverted.

Comment thread apps/frontend/src/features/commandline/components/commandline.tsx
Comment on lines +68 to +75
<CommandlineList
items={cmd.visibleItems()}
selectedIndex={cmd.selectedIndex()}
scope={cmd.scope()}
interactionType={cmd.interactionType()}
onHoverItem={cmd.hoverItem}
onSelectItem={cmd.selectCurrent}
/>
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.

P1 Wrong item executed on click when pointer hasn't moved

CommandlineList calls props.onSelectItem(item) passing the specific clicked item (line 75 of commandline-list.tsx), but cmd.selectCurrent ignores its argument and instead fetches visibleItems()[selectedIndex()]. If the user opens the palette and clicks an item without having moved the pointer after opening (so onPointerMove never ran to update selectedIndex), selectCurrent will execute the command at the default selectedIndex (typically 0) instead of the clicked one. TypeScript silently accepts a () => void where (item: CommandlineItemType) => void is expected, so there is no compile-time warning.

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