-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(cli): add interactive UI for artifact experimental setup #560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add animated welcome screen and searchable multi-select prompt when running `openspec artifact-experimental-setup` without the --tool flag in interactive mode. Users can now browse and select multiple tools for setup instead of requiring the --tool flag. - Add welcome screen with ASCII art animation - Add searchable multi-select prompt component - Support multi-tool setup in single command invocation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughReplaces the single-tool experimental setup with an interactive multi-tool batch setup, adds a searchable multi-select prompt and an animated welcome screen, centralizes skill/command template generation across tools, optionally creates a default project config, and adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as artifact-workflow
participant Prompt as searchableMultiSelect
participant Validator as AI_TOOLS
participant Templates as TemplateLoader
participant Adapter as CommandAdapter
participant FS as FileSystem
User->>CLI: Run `artifact-experimental-setup`
alt --tool provided
CLI->>Validator: Validate single tool
else --tool missing
alt interactive
CLI->>Prompt: Show selectable tools
Prompt->>User: Display searchable list
User->>Prompt: Select one+ tools
Prompt->>CLI: Return selectedTools[]
else non-interactive
CLI->>User: Error with valid tool list
end
end
loop for each selected tool
CLI->>Validator: Ensure tool exists & skillsDir configured
Validator->>CLI: Return tool metadata
CLI->>Templates: Preload shared templates
Templates->>CLI: Return templates
CLI->>FS: Write SKILL.md into tool's skillsDir
CLI->>Adapter: Check for command adapter
alt adapter exists
Adapter->>FS: Write slash-command files
FS-->>CLI: Confirm files created
else no adapter
Adapter-->>CLI: Skip command generation
end
CLI->>CLI: Record per-tool results
end
CLI->>FS: Optionally create openspec/config.yaml (interactive)
CLI->>User: Print consolidated summary (tools, skills, commands)
sequenceDiagram
participant User
participant showWelcome as showWelcomeScreen()
participant Terminal as Terminal
participant Renderer as FrameRenderer
participant Input as InputHandler
User->>showWelcome: start
showWelcome->>Terminal: Check TTY, NO_COLOR, width
alt canAnimate
loop every interval
showWelcome->>Renderer: Get next WELCOME_ANIMATION frame
Renderer->>Terminal: Render art + text side-by-side
end
else
showWelcome->>Renderer: Render static frame
Renderer->>Terminal: Display once
end
showWelcome->>Input: Wait for Enter or Ctrl+C
User->>Input: Press Enter
Input->>showWelcome: proceed
showWelcome->>Terminal: Clear screen
showWelcome-->>User: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Greptile SummaryThis PR enhances the
The changes maintain backward compatibility - the Key implementation details:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as artifact-workflow.ts
participant Welcome as welcome-screen.ts
participant MultiSelect as searchable-multi-select.ts
participant FileSystem as FileSystemUtils
User->>CLI: openspec artifact-experimental-setup
alt No --tool flag provided
CLI->>CLI: Check isInteractive()
alt Interactive mode
CLI->>Welcome: showWelcomeScreen()
Welcome->>Welcome: Render animated ASCII art
Welcome-->>User: Display welcome animation
User->>Welcome: Press Enter
Welcome-->>CLI: Continue
CLI->>MultiSelect: searchableMultiSelect(choices)
MultiSelect-->>User: Display searchable list
User->>MultiSelect: Type to filter, Enter to select
User->>MultiSelect: Tab to confirm
MultiSelect-->>CLI: Return selectedTools[]
CLI->>CLI: Set options.selectedTools
else Non-interactive mode
CLI-->>User: Error: Missing --tool flag
end
end
CLI->>CLI: Validate all selected tools
loop For each tool in toolsToSetup
CLI->>CLI: Start spinner
loop For each skill template
CLI->>FileSystem: writeFile(skillFile, content)
FileSystem-->>CLI: Success
end
CLI->>CLI: Get command adapter
alt Adapter exists
loop For each command template
CLI->>FileSystem: writeFile(commandFile, content)
FileSystem-->>CLI: Success
end
else No adapter
CLI->>CLI: Mark commands as skipped
end
CLI->>CLI: Stop spinner (success)
end
CLI-->>User: Display summary with created files
|
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/commands/artifact-workflow.ts`:
- Around line 1056-1062: The non-interactive branch is checking
process.stdin.isTTY directly (process.stdin.isTTY) but the codebase provides an
isInteractive() utility (already imported) that accounts for CI and
OPEN_SPEC_INTERACTIVE; replace the direct TTY check with a call to
isInteractive() (i.e., use !isInteractive()) in the artifact workflow branch so
the interactive detection is consistent with the rest of the code; update any
log comments if needed to reflect the utility-driven decision.
- Around line 1003-1006: The current catch block in the tool setup loop (which
calls spinner.fail(`Failed for ${tool.name}`) and then re-throws the error)
aborts the entire run; instead, change the logic in the tool processing loop
(where each tool is set up—look for the try/catch around the per-tool setup,
e.g., references to spinner and tool.name) to collect failures into a local
array (e.g., failures or failedTools) with entries containing tool.name and the
caught error, do not re-throw inside the per-tool catch so the loop continues to
the next tool, and after the loop finishes check the failures array and surface
a concise summary (log or throw a single aggregated error) so callers get a
complete report of which tools failed.
In `@src/prompts/searchable-multi-select.ts`:
- Around line 174-179: The page indicator is computed from Math.floor(cursor /
pageSize) which mismatches the sliding/centered window logic; instead determine
the window start index used by the sliding window (use the existing windowStart
variable if present or compute startIndex = Math.max(0, cursor -
Math.floor(pageSize/2))) and base currentPage on that startIndex (currentPage =
Math.floor(startIndex / pageSize) + 1) while keeping totalPages =
Math.ceil(filteredChoices.length / pageSize), then use those values when pushing
the pagination line; reference variables: filteredChoices, pageSize, cursor, and
windowStart/startIndex.
In `@src/ui/welcome-screen.ts`:
- Around line 131-140: The computed totalHeight and its explanatory comment are
stale/misleading: either remove the unused totalHeight variable and its outdated
comment, or if totalHeight is intended to be used, correct the arithmetic and
comment to match the actual calculation (ensure frameHeight = numContentLines +
1 and that the comment reflects those values) and keep numContentLines,
frameHeight and totalHeight consistent with WELCOME_ANIMATION.frames and
textLines; update or remove the line with totalHeight accordingly.
🧹 Nitpick comments (6)
src/prompts/searchable-multi-select.ts (2)
112-116: Character input handling may miss some printable characters.The condition
key.name && key.name.length === 1may not capture all printable input. Some terminals report the actual character inkey.sequencerather thankey.name, and special characters or Unicode input might be missed.Consider also checking
key.sequencefor more robust character capture:♻️ Suggested improvement
// Character input - handle printable characters - if (key.name && key.name.length === 1 && !key.ctrl) { - setSearchText(searchText + key.name); + const char = key.sequence ?? key.name; + if (char && char.length === 1 && !key.ctrl && !key.meta) { + setSearchText(searchText + char); setCursor(0); }
80-88: Allow toggling selection with Enter for already-selected items.Currently, pressing Enter on an already-selected choice does nothing (line 83 checks
!selectedSet.has(choice.value)). Users may expect Enter to toggle selection (deselect if already selected), which is a common UX pattern in multi-select prompts.♻️ Suggested toggle behavior
// Enter to add item if (isEnterKey(key)) { const choice = filteredChoices[cursor]; - if (choice && !selectedSet.has(choice.value)) { - setSelectedValues([...selectedValues, choice.value]); + if (choice) { + if (selectedSet.has(choice.value)) { + // Deselect if already selected + setSelectedValues(selectedValues.filter(v => v !== choice.value)); + } else { + setSelectedValues([...selectedValues, choice.value]); + } setSearchText(''); setCursor(0); } return; }src/ui/welcome-screen.ts (2)
90-101: Raw mode restoration should use try-finally for robustness.If an error occurs between
setRawMode(true)and the cleanup inonData, the terminal could be left in raw mode. While unlikely in this simple case, wrapping in try-finally or ensuring cleanup on error would be safer.
146-162: Potential race condition: interval callback may run afterrunningis set to false.The
runningflag is checked inside the interval callback (line 147), butclearIntervalis called after settingrunning = false(lines 168-169). Due to JavaScript's event loop, the interval callback could theoretically execute one more time afterrunningis set to false but beforeclearIntervalcompletes. The early return handles this, but swapping the order would be cleaner.♻️ Suggested order swap
// Wait for Enter await waitForEnter(); // Stop animation - running = false; clearInterval(interval); + running = false;src/commands/artifact-workflow.ts (2)
873-878: Settingoptions.toolafter multi-select is unnecessary.After multi-select,
options.toolis set toselectedTools[0](line 877), buttoolsToSetupon line 887 usesoptions.selectedTools || [options.tool!]. This works but the assignment tooptions.toolis confusing since it's not used meaningfully whenselectedToolsis populated. Consider simplifying.♻️ Suggested simplification
if (selectedTools.length === 0) { throw new Error('At least one tool must be selected'); } - options.tool = selectedTools[0]; options.selectedTools = selectedTools; } else {Then update line 887:
// Determine tools to set up - const toolsToSetup = options.selectedTools || [options.tool!]; + const toolsToSetup = options.selectedTools ?? [options.tool!];
1078-1083: Git commit suggestion includes trailing slashes that may cause issues.Line 1079 appends
/to eachskillsDir(t.skillsDir + '/'), which is unconventional forgit addand could behave unexpectedly if the directory doesn't exist. Standard practice is to use paths without trailing slashes.♻️ Suggested fix
// Git commit suggestion with all tool directories - const toolDirs = validatedTools.map(t => t.skillsDir + '/').join(' '); + const toolDirs = validatedTools.map(t => t.skillsDir).join(' '); console.log(chalk.bold('To share with team:'));
| // Show pagination indicator if needed | ||
| if (filteredChoices.length > pageSize) { | ||
| const currentPage = Math.floor(cursor / pageSize) + 1; | ||
| const totalPages = Math.ceil(filteredChoices.length / pageSize); | ||
| lines.push(chalk.dim(` (${currentPage}/${totalPages})`)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pagination indicator calculation may be inaccurate with the sliding window approach.
The pagination uses a sliding/centered window (lines 155-160), but the page indicator calculation (lines 176-177) uses simple Math.floor(cursor / pageSize), which doesn't match the sliding window logic. This could show misleading page numbers.
🔧 Suggested fix
// Show pagination indicator if needed
if (filteredChoices.length > pageSize) {
- const currentPage = Math.floor(cursor / pageSize) + 1;
- const totalPages = Math.ceil(filteredChoices.length / pageSize);
- lines.push(chalk.dim(` (${currentPage}/${totalPages})`));
+ // Show position indicator instead of page numbers (more accurate with sliding window)
+ lines.push(chalk.dim(` (${cursor + 1}/${filteredChoices.length})`));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Show pagination indicator if needed | |
| if (filteredChoices.length > pageSize) { | |
| const currentPage = Math.floor(cursor / pageSize) + 1; | |
| const totalPages = Math.ceil(filteredChoices.length / pageSize); | |
| lines.push(chalk.dim(` (${currentPage}/${totalPages})`)); | |
| } | |
| // Show pagination indicator if needed | |
| if (filteredChoices.length > pageSize) { | |
| // Show position indicator instead of page numbers (more accurate with sliding window) | |
| lines.push(chalk.dim(` (${cursor + 1}/${filteredChoices.length})`)); | |
| } |
🤖 Prompt for AI Agents
In `@src/prompts/searchable-multi-select.ts` around lines 174 - 179, The page
indicator is computed from Math.floor(cursor / pageSize) which mismatches the
sliding/centered window logic; instead determine the window start index used by
the sliding window (use the existing windowStart variable if present or compute
startIndex = Math.max(0, cursor - Math.floor(pageSize/2))) and base currentPage
on that startIndex (currentPage = Math.floor(startIndex / pageSize) + 1) while
keeping totalPages = Math.ceil(filteredChoices.length / pageSize), then use
those values when pushing the pagination line; reference variables:
filteredChoices, pageSize, cursor, and windowStart/startIndex.
| let frameIndex = 0; | ||
| let running = true; | ||
| let isFirstRender = true; | ||
|
|
||
| // Content height for cursor movement between frames | ||
| const numContentLines = Math.max(WELCOME_ANIMATION.frames[0].length, textLines.length); | ||
| const frameHeight = numContentLines + 1; // internal newlines (11) + trailing newlines (2) = 13 | ||
|
|
||
| // Total height including initial newline (for cleanup) | ||
| const totalHeight = frameHeight + 1; // 14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable totalHeight calculation contains misleading comment.
Line 137 comment says // internal newlines (11) + trailing newlines (2) = 13 but frameHeight = numContentLines + 1 doesn't match this arithmetic. The comment appears stale or incorrect relative to the actual calculation.
🔧 Suggested fix
// Content height for cursor movement between frames
const numContentLines = Math.max(WELCOME_ANIMATION.frames[0].length, textLines.length);
- const frameHeight = numContentLines + 1; // internal newlines (11) + trailing newlines (2) = 13
-
- // Total height including initial newline (for cleanup)
- const totalHeight = frameHeight + 1; // 14
+ const frameHeight = numContentLines + 2; // content lines + 2 trailing newlines from render
+
+ // Total height including initial newline (for cleanup)
+ const totalHeight = frameHeight + 1; // +1 for initial newline📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let frameIndex = 0; | |
| let running = true; | |
| let isFirstRender = true; | |
| // Content height for cursor movement between frames | |
| const numContentLines = Math.max(WELCOME_ANIMATION.frames[0].length, textLines.length); | |
| const frameHeight = numContentLines + 1; // internal newlines (11) + trailing newlines (2) = 13 | |
| // Total height including initial newline (for cleanup) | |
| const totalHeight = frameHeight + 1; // 14 | |
| let frameIndex = 0; | |
| let running = true; | |
| let isFirstRender = true; | |
| // Content height for cursor movement between frames | |
| const numContentLines = Math.max(WELCOME_ANIMATION.frames[0].length, textLines.length); | |
| const frameHeight = numContentLines + 2; // content lines + 2 trailing newlines from render | |
| // Total height including initial newline (for cleanup) | |
| const totalHeight = frameHeight + 1; // +1 for initial newline |
🤖 Prompt for AI Agents
In `@src/ui/welcome-screen.ts` around lines 131 - 140, The computed totalHeight
and its explanatory comment are stale/misleading: either remove the unused
totalHeight variable and its outdated comment, or if totalHeight is intended to
be used, correct the arithmetic and comment to match the actual calculation
(ensure frameHeight = numContentLines + 1 and that the comment reflects those
values) and keep numContentLines, frameHeight and totalHeight consistent with
WELCOME_ANIMATION.frames and textLines; update or remove the line with
totalHeight accordingly.
- Update version from 0.20.0 to 0.23.0 to match package.json - Set pnpmDeps hash to empty string to trigger rebuild - Fix update-flake.sh to work on macOS (use portable grep/sed) CI will fail with correct hash which we'll then apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@flake.nix`:
- Around line 26-31: The pnpmDeps block uses pkgs.fetchPnpmDeps and currently
has an empty hash which breaks nix builds; run the repository's pnpm deps/hash
update script to regenerate the correct integrity/hash for pnpmDeps, replace the
empty string in the hash attribute under pnpmDeps (the pkgs.fetchPnpmDeps block)
with the computed value, and commit the updated flake.nix so pnpm = pkgs.pnpm_9
and fetcherVersion remain unchanged.
- Continue setup for remaining tools when one fails - Collect and report all failures at the end - Only throw if all tools fail - Show partial success summary (configured vs failed)
Summary
openspec artifact-experimental-setupinteractively--toolflagTest plan
openspec artifact-experimental-setupwithout--toolflag in interactive terminal--toolflag still works for non-interactive usage🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.