feat(core): add interactive mode and key binding hooks#122
feat(core): add interactive mode and key binding hooks#122zrosenbauer wants to merge 7 commits intomainfrom
Conversation
Add interactive mode to the stories viewer that gives story components full terminal control. Introduce reusable key handling primitives (keys.ts, useKeyBinding, useKeyInput) and a double-escape hook to exit. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: f9b0f36 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an "interactive" viewer mode and keyboard infrastructure. New UI changes: help-overlay, status-bar, and preview now surface an interactive mode (entered via Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/src/stories/viewer/hooks/use-double-escape.ts`:
- Around line 30-37: The JSDoc for the exported hook useDoubleEscape is missing
a `@returns` tag; update the comment block above the useDoubleEscape function to
include a `@returns` description indicating it returns void (e.g., "@returns
{void} No return value — hook sets up side effects"), so the exported function
JSDoc complies with the project's TypeScript documentation standard.
In `@packages/core/src/ui/use-key-binding.ts`:
- Around line 70-103: The historyRef retains past key events across mode toggles
causing false sequence matches; in useKeyBinding add an effect that watches the
isActive option and when isActive transitions to true reset historyRef.current =
[] so stale events are cleared before inputHandler runs (use the existing
historyRef and isActive from options to implement this effect inside
useKeyBinding).
🪄 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: 3d9586f8-ebba-4aaf-807b-aac2d4ae697a
⛔ Files ignored due to path filters (1)
.changeset/interactive-story-mode.mdis excluded by!.changeset/**
📒 Files selected for processing (16)
examples/diagnostic-output/package.jsonpackages/core/src/stories/viewer/components/help-overlay.tsxpackages/core/src/stories/viewer/components/preview.tsxpackages/core/src/stories/viewer/components/status-bar.tsxpackages/core/src/stories/viewer/hooks/use-double-escape.tspackages/core/src/stories/viewer/hooks/use-panel-focus.tspackages/core/src/stories/viewer/stories-app.tsxpackages/core/src/test/context.tspackages/core/src/ui/index.tspackages/core/src/ui/keys.test.tspackages/core/src/ui/keys.tspackages/core/src/ui/output/output.tsxpackages/core/src/ui/use-key-binding.test.tspackages/core/src/ui/use-key-binding.tspackages/core/src/ui/use-key-input.test.tspackages/core/src/ui/use-key-input.ts
Deduplicate the groupKey::variantName ID parsing logic shared by resolveStory and buildPreviewContext into a single parseStoryId helper. Tighten resolveBoxBase return type and remove unnecessary cast. Co-Authored-By: Claude <noreply@anthropic.com>
Add missing @returns tag to useDoubleEscape JSDoc. Clear key history buffer in useKeyBinding when isActive transitions from false to true, preventing stale escape events from triggering false sequence matches. Co-Authored-By: Claude <noreply@anthropic.com>
There 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/core/src/stories/viewer/stories-app.tsx`:
- Around line 144-145: The keydown handler currently gates the "i" interactive
entry on selectedStoryId, which can be non-null while selectedStory is null
after reload/rename; change the condition from checking selectedStoryId to
checking selectedStory (e.g., selectedStory !== null or truthiness) so
enterInteractiveMode() only runs when the actual selectedStory exists; update
the conditional that references input === 'i' && selectedStoryId !== null to use
selectedStory instead and ensure any related logic that assumes selectedStory is
present still holds.
🪄 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: fb881bc8-c6d1-4d85-9d8d-728352bb9fdc
📒 Files selected for processing (2)
packages/core/src/lib/log.tspackages/core/src/stories/viewer/stories-app.tsx
Hard-migrate from the deprecated `spinner` option to `status` across CliOptions, RuntimeOptions, CreateContextOptions, CreateContextStatusOptions, and TestContextOptions. Remove all backwards-compatibility shims. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/ui/use-key-binding.ts (1)
61-73:⚠️ Potential issue | 🟡 MinorUse a destructured public signature and complete the exported JSDoc.
useKeyBindingis exported, so the two positional parameters and missing@returnsboth violate the repo's TS public-function standard. Fix the signature now before this API spreads through the UI surface.As per coding guidelines: "
**/*.ts: Functions with 2+ parameters must use object destructuring" and "All exported functions require JSDoc with@param,@returns." (contributing/standards/typescript/functions.md).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ui/use-key-binding.ts` around lines 61 - 73, The exported function useKeyBinding currently takes two positional params (bindings: readonly KeyBinding[], options: KeyBindingOptions = {}) and lacks complete JSDoc; change its public signature to a single destructured object parameter (e.g., { bindings, options }: { bindings: readonly KeyBinding[]; options?: KeyBindingOptions }) and update the exported JSDoc to include `@param` entries for bindings and options and an `@returns` description (void). Ensure references to the function in the file still match the new call style and keep the existing types KeyBinding and KeyBindingOptions in the signature.
🤖 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/core/src/ui/use-key-binding.ts`:
- Around line 25-26: The constant MAX_HISTORY_LENGTH currently hard-caps key
history to 10 and causes matches to fail for longer sequences; change logic in
useKeyBinding to derive the retained history size from the longest parsed
sequence returned by parseKeyPattern() (or refuse to register bindings whose
parseKeyPattern() result exceeds a configurable maximum) instead of always
truncating to MAX_HISTORY_LENGTH; update the history truncation step (where
MAX_HISTORY_LENGTH is used) to use the computed max sequence length from parsed
bindings (or throw/validate at registration time) so sequence matching can
succeed for arbitrarily long, or explicitly rejected, patterns.
---
Duplicate comments:
In `@packages/core/src/ui/use-key-binding.ts`:
- Around line 61-73: The exported function useKeyBinding currently takes two
positional params (bindings: readonly KeyBinding[], options: KeyBindingOptions =
{}) and lacks complete JSDoc; change its public signature to a single
destructured object parameter (e.g., { bindings, options }: { bindings: readonly
KeyBinding[]; options?: KeyBindingOptions }) and update the exported JSDoc to
include `@param` entries for bindings and options and an `@returns` description
(void). Ensure references to the function in the file still match the new call
style and keep the existing types KeyBinding and KeyBindingOptions in the
signature.
🪄 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: 4b08ca2f-f279-44bb-9ca0-63c475e88b93
📒 Files selected for processing (2)
packages/core/src/stories/viewer/hooks/use-double-escape.tspackages/core/src/ui/use-key-binding.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/types/cli.ts (1)
106-177:⚠️ Potential issue | 🟠 MajorBreaking API removal needs explicit migration handling.
At Line 106 (
CliOptions), removingspinnerbreaks consumers still passing that option. Please add a migration note/changelog entry (or a temporary compatibility path) before release.As per coding guidelines:
**reviews must flag breaking changes (incompatible API changes, removed exports).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/types/cli.ts` around lines 106 - 177, The CliOptions type removal of the spinner option is a breaking change for consumers; reinstate a temporary compatibility path by adding an optional deprecated spinner field back to the CliOptions interface (e.g., readonly spinner?: unknown) and make the runtime code that consumes CliOptions detect and log a deprecation warning (or map it into the new DisplayConfig) so existing callers continue working; additionally add a clear migration note to the changelog/upgrade guide explaining removal and how to migrate to DisplayConfig or the new API. Ensure references to CliOptions and spinner are updated and the deprecation is documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/types/cli.ts`:
- Around line 106-177: The CliOptions type removal of the spinner option is a
breaking change for consumers; reinstate a temporary compatibility path by
adding an optional deprecated spinner field back to the CliOptions interface
(e.g., readonly spinner?: unknown) and make the runtime code that consumes
CliOptions detect and log a deprecation warning (or map it into the new
DisplayConfig) so existing callers continue working; additionally add a clear
migration note to the changelog/upgrade guide explaining removal and how to
migrate to DisplayConfig or the new API. Ensure references to CliOptions and
spinner are updated and the deprecation is documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5258353-98e1-4ce3-aec1-6cc0daefebb4
📒 Files selected for processing (9)
packages/core/src/cli.tspackages/core/src/context/create-context.tspackages/core/src/context/status.tspackages/core/src/runtime/runtime.tspackages/core/src/runtime/types.tspackages/core/src/test/context.test.tspackages/core/src/test/context.tspackages/core/src/test/types.tspackages/core/src/types/cli.ts
💤 Files with no reviewable changes (2)
- packages/core/src/cli.ts
- packages/core/src/runtime/runtime.ts
… size Gate interactive mode entry on selectedStory instead of selectedStoryId to prevent entering interactive mode when the story was removed by a reload. Derive key history buffer size from the longest registered sequence binding instead of a hard-coded cap. Co-Authored-By: Claude <noreply@anthropic.com>
|
@coderabbitai This review finding is not valid. The
No changes needed here. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/src/ui/use-key-binding.ts`:
- Around line 61-73: The exported hook useKeyBinding currently accepts two
positional parameters and lacks a `@returns` JSDoc, violating the public-function
contract; change its signature to accept a single destructured parameter object
(e.g. { bindings, options }) so callers pass named properties, update the JSDoc
to include explicit `@param` tags for both bindings and options and add an
`@returns` {void} tag, and update any internal/call sites to use the new object
form; keep the implementation and the function name useKeyBinding unchanged
except for its parameter shape and JSDoc.
- Around line 76-83: The refs bindingsRef, prevActiveRef and historyRef are
being mutated during render (lines around bindingsRef.current = bindings and
prevActiveRef.current = isActive) which can leak uncommitted values into the
inputHandler callback; move all ref synchronizations and the isActive-based
history reset into a useEffect that runs when bindings or isActive change so
mutations occur inside a React effect boundary; also refactor the exported
useKeyBinding signature to accept a single object parameter (destructure the two
current parameters) to comply with the 2+ parameter rule and update any
callsites (e.g. useDoubleEscape) to pass an object.
🪄 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: cff539a7-7a6d-4f3e-921c-cc20d91d226b
📒 Files selected for processing (2)
packages/core/src/stories/viewer/stories-app.tsxpackages/core/src/ui/use-key-binding.ts
|
Tip For best results, initiate chat on the files or code changes.
(ˆ＿ˆ)ノ ✏️ Learnings added
🧠 Learnings used |
Refactor useKeyBinding to accept a single UseKeyBindingArgs object (2+ param rule) and move ref mutations into useEffect to prevent concurrent render leaks. Add @returns to JSDoc. Update caller useDoubleEscape and re-export UseKeyBindingArgs type. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary
ito give story components full terminal control (header/sidebar hidden), double-pressEscto exitkeys.ts(normalizer + pattern parser),useKeyBinding(declarative keymap with sequence support),useKeyInput(enhanced raw input hook)ViewerModeunion, status bar, help overlay, and preview component to support the new modeTest plan
pnpm checkpasses (typecheck + lint + format)pnpm testpasses including newkeys.test.ts,use-key-binding.test.ts,use-key-input.test.tsito enter interactive mode, confirm header/sidebar hideEscto exit interactive mode, confirm UI restores?help overlay shows interactive mode shortcuts