feat(core): replace screen hooks with useCommandContext()#100
feat(core): replace screen hooks with useCommandContext()#100zrosenbauer merged 5 commits intomainfrom
Conversation
Replace useConfig(), useMeta(), and useStore() with a single useCommandContext() hook that exposes the full command Context. This gives screen components access to middleware-decorated properties (auth, http, etc.) that were previously inaccessible. Also removes internal-only exports from the public UI surface: KiddProvider, KiddProviderProps, ScreenRenderProps, render, Instance, and RenderOptions. Co-Authored-By: Claude <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: a3c8adb 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.
|
…en hook Use es-toolkit omit() to strip imperative keys (log, spinner, prompts, fail, colors, format) from the full Context before injecting into the React provider. Export ScreenContext type for consumers. Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughScreen renderers now receive the full middleware-decorated Context instead of a props object ({ args, config, meta, store }). Types changed: Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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.
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/runtime/runtime.ts (1)
57-65:⚠️ Potential issue | 🟠 MajorScreen renders still bypass middleware.
Line 60 calls
renderFnon the pre-middleware context and returns beforerunner.execute()can wrap it. Screen commands therefore skip both root and per-command middleware, souseCommandContext()still will not expose middleware-populated fields likeauth/httpor any middleware-seeded store state.Proposed fix
- if (command.render) { - const renderFn = command.render - const [renderError] = await attemptAsync(async () => { - await renderFn(ctx as Context) - }) - if (renderError) { - return err(renderError) - } - return ok() - } - - const finalHandler = command.handler ?? (async () => {}) + const finalHandler = + command.render !== undefined + ? async (renderCtx: Context): Promise<void> => { + await command.render(renderCtx) + } + : command.handler ?? (async () => {})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/runtime.ts` around lines 57 - 65, The screen render is invoked directly (command.render / renderFn) so middleware isn't run; instead wrap the render call with the same middleware runner (call runner.execute and pass a function that calls renderFn) so root and per-command middleware run and populate Context before render; replace the direct await renderFn(ctx) inside the attemptAsync with an await runner.execute(ctx, async () => renderFn(ctx as Context)) (or equivalent usage of runner.execute) and preserve the existing attemptAsync/error handling.
🤖 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/runtime/runtime.ts`:
- Around line 57-65: The screen render is invoked directly (command.render /
renderFn) so middleware isn't run; instead wrap the render call with the same
middleware runner (call runner.execute and pass a function that calls renderFn)
so root and per-command middleware run and populate Context before render;
replace the direct await renderFn(ctx) inside the attemptAsync with an await
runner.execute(ctx, async () => renderFn(ctx as Context)) (or equivalent usage
of runner.execute) and preserve the existing attemptAsync/error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f676c417-eb9c-4f9c-831f-bb5f7ee41d11
⛔ Files ignored due to path filters (1)
.changeset/use-command-context.mdis excluded by!.changeset/**
📒 Files selected for processing (8)
packages/core/src/runtime/register.tspackages/core/src/runtime/runtime.test.tspackages/core/src/runtime/runtime.tspackages/core/src/types/command.tspackages/core/src/ui/index.tspackages/core/src/ui/provider.tsxpackages/core/src/ui/screen.test.tspackages/core/src/ui/screen.tsx
Screen render was invoked directly, bypassing the middleware chain. Now render goes through runner.execute() so root and per-command middleware populate the context before the screen component renders. 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/runtime/runtime.test.ts`:
- Around line 367-371: The test currently stubs mockedCreateRunner.execute to
reject directly so it never invokes the supplied handler; change the mock so
execute calls the provided handler and make the render function itself reject.
Concretely, update mockedCreateRunner.mockReturnValue to set execute:
vi.fn().mockImplementation((opts) => opts.handler(opts.ctx)) (or similar so
execute calls handler) and set renderFn (the render function wired into
setupDefaults) to vi.fn().mockRejectedValue(new Error('Render failed')) so the
rejection originates from render, exercising the render->runner.execute wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 696b508f-b9fd-4d3c-9066-422672f9a6c6
📒 Files selected for processing (2)
packages/core/src/runtime/runtime.test.tspackages/core/src/runtime/runtime.ts
Make the mock runner invoke the handler so the rejection originates from renderFn, not from the runner stub itself. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary
useConfig(),useMeta(), anduseStore()screen hooks with a singleuseCommandContext()hook that returns aScreenContextScreenContexttype that strips imperative I/O properties (log,spinner,prompts,fail,colors,format) from the fullContext— these conflict with Ink's declarative rendering modelomit()fromes-toolkitfor runtime stripping of imperative keysContexttoscreen()render function so middleware-decorated properties (auth,http, etc.) are accessible viauseCommandContext()KiddProvider,KiddProviderProps,ScreenRenderProps,render,Instance,RenderOptionsfrom exportsTest plan
useCommandContext()returns aScreenContextwith imperative keys strippedscreen()render function wraps component inKiddProviderwithScreenContextpnpm check(typecheck + lint + format) passes