Uh oh!
There was an error while loading. Please reload this page.
feat(prompts): add scroll indicators and interactive listage module - #176
Conversation
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 9 seconds. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
📝 WalkthroughWalkthroughA new shared module packages/cli-core/src/lib/listage.ts implements custom interactive prompts ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
🦋 Changeset detectedLatest commit: 4f8e2e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
bde0a77 to
9480ddeCompare9480dde to
a2fd8f2Compare
wyattjoh
left a comment
There was a problem hiding this comment.
Code Review — PR #176
Reviewed with Opus + Codex second-opinion validation.
High
H1. TTY fallback does not handle /dev/tty or CONIN$ open failures(codex confirmed)packages/cli-core/src/lib/listage.ts:42-45 unconditionally calls createReadStream(TTY_PATH) when stdin is not a TTY, without attaching an error listener. In Docker without --tty, detached CI runners, or Windows sessions without CONIN$, the stream emits an async error event that is unhandled. Previously the same flaw existed in lib/prompts.ts but is now extended to search callsites (link, init/bootstrap).
H2. Scroll indicator math ignores rendered-line wrapping(codex confirmed)scrollBounds() (listage.ts:68-95) counts items, but @inquirer/core's usePagination paginates on rendered lines post-breakLines(). Long clerk api labels or narrow terminals produce wrapped choices, which means the "more above/below" counts and visible-range inference diverge from what is actually shown.
Medium
- M1. Prompt height jumps 1 line at edges (
listage.ts:104-107,115-117): only the non-empty indicator row renders at top/bottom. - M2.
Math.max(pageSize - 2, 3)plus indicator rows (listage.ts:294,508): callers passingpageSize: 3or4get 4-5 lines. - M3. Zero direct tests for
lib/listage.ts. Every command mocks it out;scrollBounds,withScrollIndicators,normalizeChoices,isSelectable, the TTY fallback, keypress handlers, and search race-cancellation are all unverified. Add at minimum alib/listage.test.tsfor the pure helpers. - M4.
ValidationErrorthrown insideuseMemo(listage.ts:216-223) is not guaranteed to be caught by@inquirer/core's validation path; prefer validating up-front in theselect()wrapper. - M5.
searchactivecan be-1with empty results (listage.ts:417-423,461): theuseState<number>()destructure default only applies when the value is strictlyundefined, not for thebounds.first = -1case. - M6.
cursorHideis appended on every select render (listage.ts:340) with no matchingcursorShowon the done branch (listage.ts:319-321). Commands that chain prompts (deploy,api/interactive) can render subsequent prompts with a hidden cursor. - M7. Duplicate
selectAPI surface: oldlib/prompts.ts:36-46still exportsselect, andcommands/init/skills.ts:17,106-113still uses it. Two implementations with different appearance and TTY-fallback paths. - M8.
packages/cli-core/src/commands/switch-env/README.mdis not updated despite the new interactive picker behavior (per.claude/rules/commands.md).
Missed on first pass (codex caught)
- All-disabled list deadlock.
listage.ts:217-220checksisNavigablerather thanisSelectable; an all-disabled list never throws and produces a prompt with no selectable item and no exit path. - Stuck loading state on search error.
listage.ts:427-453setsstatusto"loading"before fetch but the error path never resets it to"idle", leaving the prompt loading indefinitely and blocking arrow-key nav (listage.ts:485-494).
Nits
isSelectable<T>narrows to{ disabled?: false }but runtime accepts any falsydisabled(listage.ts:124-126).rl.clearLine(0)on arrow keys clobbers the type-ahead buffer (listage.ts:251-260).- Type-ahead
startsWithlacks Unicode normalization (listage.ts:275-278). SelectConfig/SearchConfigtypes not exported (listage.ts:185-191,378-390).•bullet inkeysHelpTipmay not render in all terminals (listage.ts:201,401).switch-env/index.ts:30reassigns the function parameter.@inquirer/ansiand@inquirer/figuresadded as direct deps though already transitive.
Positives
Clear motivation in the PR body (upstream removed helpMode / usePagination metadata). Abort-controller wiring in the search useEffect correctly cancels stale results. The defaultApplied.current ref is a subtle bug fix that is easy to miss. Deduplication of filterChoices from init/bootstrap.ts into the shared module is a net cleanup. Changeset scoped as minor matches the surface-visible change.
6cb7ab3 to
5e72371CompareThere was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/listage.ts`:
- Around line 495-504: The up/down key handler can enter a do…while loop when
searchResults is empty, causing NaN/index errors; update the condition that
currently reads "status !== 'loading' && (isUpKey(key) || isDownKey(key))" (and
the subsequent guard using bounds.first/last) to also require
searchResults.length > 0 (or equivalently bounds.first !== -1) before attempting
to compute next and call isSelectable; ensure functions/vars referenced are
status, isUpKey, isDownKey, bounds, active, searchResults, isSelectable, and
setActive so the handler early-returns when there are no results.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8291374-2d47-4d13-8d6e-816d4354a86b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.changeset/listage-improvements.mdpackages/cli-core/package.jsonpackages/cli-core/src/commands/api/interactive.test.tspackages/cli-core/src/commands/api/interactive.tspackages/cli-core/src/commands/deploy/index.test.tspackages/cli-core/src/commands/deploy/index.tspackages/cli-core/src/commands/init/bootstrap.tspackages/cli-core/src/commands/init/skills.tspackages/cli-core/src/commands/link/index.test.tspackages/cli-core/src/commands/link/index.tspackages/cli-core/src/commands/switch-env/README.mdpackages/cli-core/src/commands/switch-env/index.test.tspackages/cli-core/src/commands/switch-env/index.tspackages/cli-core/src/lib/listage.test.tspackages/cli-core/src/lib/listage.tspackages/cli-core/src/lib/prompts.tspackages/cli-core/src/test/integration/lib/harness.tspackages/cli-core/src/test/lib/stubs.ts
✅ Files skipped from review due to trivial changes (5)
- packages/cli-core/src/commands/switch-env/README.md
- .changeset/listage-improvements.md
- packages/cli-core/package.json
- packages/cli-core/src/commands/link/index.test.ts
- packages/cli-core/src/test/integration/lib/harness.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/cli-core/src/commands/api/interactive.ts
- packages/cli-core/src/commands/deploy/index.test.ts
- packages/cli-core/src/commands/deploy/index.ts
- packages/cli-core/src/commands/switch-env/index.ts
- packages/cli-core/src/commands/link/index.ts
- packages/cli-core/src/commands/switch-env/index.test.ts
Uh oh!
There was an error while loading. Please reload this page.
wyattjoh
left a comment
There was a problem hiding this comment.
Code review
Net positive change: a small custom select/search on top of @inquirer/core that adds scroll indicators, fills in the piped-stdin TTY fallback that search was missing, and ships an interactive env picker for switch-env. Implementation is careful and the scrollBounds test coverage is good. A few things worth tightening before merge.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
c5743ea to
997424aCompare
Railly
left a comment
There was a problem hiding this comment.
Arrow navigation lands on disabled items (lib/listage.ts:265). Uses isNavigable instead of isSelectable, upstream @inquirer/select uses isSelectable in the equivalent loop, and the search prompt below (:502) uses it too, so select is the odd one out. Coupled render quirk at :317-318 (cursor ❯ renders on active disabled rows) fixes itself once the loop predicate is corrected.
7393a4d to
091e9c9CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Custom select/search prompts built on @inquirer/core that show "↑ N more above" / "↓ N more below" when the list overflows the visible page. Also consolidates the piped-stdin TTY fallback and the duplicated filterChoices helper into a single listage module. Commands updated to use the new prompts: - clerk link (app picker) - clerk init (framework/PM pickers) - clerk api (category/endpoint pickers) - clerk deploy (domain/OAuth pickers) - clerk switch-env (new interactive env picker when no arg given)
- H1: Handle /dev/tty open failures gracefully (Docker, CI, Windows) - H2: Document single-line assumption in scrollBounds - M1: Stable indicator height — always render both lines when scrolling - M3: Add listage.test.ts with tests for scrollBounds, withScrollIndicators, filterChoices - M5: Guard search active against -1 when results are empty - M6: Add cursorShow on done branch to restore cursor after prompt - M7: Migrate init/skills to listage select (single API surface) - M8: Update switch-env README with interactive picker behavior - Codex: Fix all-disabled deadlock (check isSelectable not isNavigable) - Codex: Reset status to idle on search fetch error (prevent stuck loading) - Nit: Fix parameter reassignment in switch-env - Nit: Fix isSelectable type narrowing for falsy disabled values
…ompts.ts - Export SelectConfig and SearchConfig types for consumer use (M4 nit) - Remove select() from lib/prompts.ts — all consumers now use listage (M7)
- Guard arrow keys on empty search results to prevent NaN crash - Share ttyContext between listage.ts and prompts.ts (single impl) - Import real filterChoices/Separator in unit test stubs - Use figures.arrowUp/arrowDown for ASCII fallback on legacy terminals - Better UX for single-env switch-env (explicit "nothing to switch to") - Export ttyContext for reuse
…ecision - Use isSelectable (not isNavigable) in select arrow-key handler so disabled items are skipped, matching upstream @inquirer/select behavior - Remove unused isNavigable function - Expand scrollBounds doc comment to explicitly note ±1 drift for odd pageSize values
- Guard switch-env select() against non-interactive TTY (throws CliError with actionable message instead of hanging on EOF) - Remove unreachable indexMode:"number" branch and separatorCount from select renderItem (latent bug: separatorCount only counted visible items) - Migrate all confirm imports from @inquirer/prompts to lib/prompts.ts TTY-safe wrapper (link, deploy, unlink, api/interactive, init/bootstrap, init/preview) and update test mocks accordingly - Add ttyContext tests in listage.test.ts - Add scrollBounds pageSize=7 invariant test for odd pageSize coverage
091e9c9 to
4f8e2e6CompareUh oh!
There was an error while loading. Please reload this page.
Summary
select/searchprompts inlib/listage.tsbuilt on@inquirer/core'screatePromptthat show↑ N more above/↓ N more belowscroll indicators when choices overflow the visible pagelib/prompts.tsforselect/confirm, missing forsearch) and the duplicatedfilterChoiceshelper into the shared listage moduleclerk switch-envwhen no argument is given in human modeWhy custom prompts?
@inquirer/selectv5 and@inquirer/searchv4 removed the oldhelpModetheme option, andusePaginationreturns a plain string with no metadata about items above/below the viewport. There's no theme hook or callback to inject scroll indicators without reimplementing the render function viacreatePrompt.Commands updated
clerk linksearchclerk initsearchclerk api(interactive)selectclerk deployselectclerk switch-envselect(new)Other improvements
searchprompts now have the TTY fallback for piped stdin (previously missing —linkandinit/bootstrapimportedsearchdirectly from@inquirer/prompts)filterChoiceshelper frominit/bootstrap.ts(now shared fromlistage.ts)Test plan
clerk apiwith no args — verify scroll indicators appear on the endpoint list when it exceeds page sizeclerk link— verify app picker shows scroll indicators with many appsclerk switch-envwith no argument — verify interactive picker appearsclerk switch-env staging— verify direct argument still worksecho | clerk switch-env) — verify fallback to text output