fix: improve type-safe - #330
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds typed helpers Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant DefAPI as defineWithTypes / define
participant Lazy as lazyWithTypes / lazy
participant CLI as CLI
participant Runner as CommandRunner
Dev->>DefAPI: Provide typed command definition
DefAPI-->>CLI: Registered command metadata (typed args/extensions)
Dev->>Lazy: Provide typed lazy loader
Lazy-->>CLI: Supplies typed CommandRunner
CLI->>Runner: run(ctx) with ctx.values + ctx.extensions
Runner-->>CLI: result/output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
kazupon
added
improvement
@gunshi/bone
@gunshi/definition
gunshi
@gunshi/plugin
@gunshi/plugin-completion
@gunshi/plugin-dryrun
@gunshi/plugin-global
@gunshi/plugin-i18n
@gunshi/plugin-renderer
@gunshi/resources
@gunshi/shared
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/docs/src/guide/essentials/lazy-async.md (1)
310-335: Add the missingnameproperty todefine.In this sample,
define({ ... })omits the requirednamefield, so the snippet won’t type-check or run as-is. Please includenamein the definition (even if you override it in thelazymeta) to keep the example functional.Apply:
- return define({ - description: `Config command (debug: ${isDebug})`, + return define({ + name: 'config', + description: `Config command (debug: ${isDebug})`,
🧹 Nitpick comments (3)
packages/docs/src/.vitepress/config.ts (1)
143-143: debug@4.4.3 bump is verified safe
- 4.4.3 exists on npm, patches the malicious 4.4.2 release; no known vulnerabilities.
Optionally, avoid hard-coded pnpm alias paths by using dynamic package resolution.packages/definition/src/index.test.ts (1)
3-3: Type assertion suggests incomplete type inference.
CommandRunneris imported solely for the type assertion at line 61. This suggests the loader's return type may not be sufficiently narrow, requiring manual type narrowing.Consider improving the type inference in the
lazy()function or its loader signature to eliminate the need for this type assertion, which could hide type-safety issues if the loader returns something other than aCommandRunner.Also applies to: 61-61
packages/gunshi/src/index.ts (1)
5-8: Document the newly exported helpers.Now that
defineWithTypes/lazyWithTypesare part of the public export set, the API list in the header comment should mention them to stay in sync.Also applies to: 29-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/bone/src/index.test.ts(1 hunks)packages/definition/README.md(1 hunks)packages/definition/src/index.test.ts(3 hunks)packages/docs/src/.vitepress/config.ts(1 hunks)packages/docs/src/guide/advanced/type-system.md(4 hunks)packages/docs/src/guide/essentials/composable.md(1 hunks)packages/docs/src/guide/essentials/lazy-async.md(7 hunks)packages/docs/src/guide/essentials/plugin-system.md(1 hunks)packages/gunshi/src/context.ts(1 hunks)packages/gunshi/src/definition.test.ts(5 hunks)packages/gunshi/src/definition.ts(5 hunks)packages/gunshi/src/index.ts(1 hunks)packages/gunshi/src/plugin/core.test.ts(5 hunks)packages/gunshi/src/types.ts(6 hunks)packages/gunshi/test/utils.ts(1 hunks)packages/plugin-completion/examples/i18n.node.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/plugin-completion/examples/i18n.node.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use ES modules throughout the codebase
Follow existing code style (enforced by ESLint and Prettier)
Files:
packages/gunshi/test/utils.tspackages/definition/src/index.test.tspackages/bone/src/index.test.tspackages/gunshi/src/context.tspackages/gunshi/src/plugin/core.test.tspackages/gunshi/src/types.tspackages/gunshi/src/definition.test.tspackages/gunshi/src/index.tspackages/gunshi/src/definition.ts
packages/gunshi/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
packages/gunshi/src/**/*.ts: All source code is in TypeScript with strict mode enabled
Type safety is a core feature - maintain strict TypeScript types throughout
Files:
packages/gunshi/src/context.tspackages/gunshi/src/plugin/core.test.tspackages/gunshi/src/types.tspackages/gunshi/src/definition.test.tspackages/gunshi/src/index.tspackages/gunshi/src/definition.ts
🧠 Learnings (3)
📚 Learning: 2025-07-21T07:12:47.997Z
Learnt from: CR
PR: kazupon/gunshi#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T07:12:47.997Z
Learning: Applies to packages/gunshi/test/**/*.test.ts : Use 'describe' and 'it' blocks for test organization
Applied to files:
packages/definition/src/index.test.tspackages/gunshi/src/definition.test.ts
📚 Learning: 2025-07-21T07:12:47.997Z
Learnt from: CR
PR: kazupon/gunshi#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T07:12:47.997Z
Learning: Applies to packages/gunshi/test/**/*.test.ts : Add tests for new features in the corresponding test file
Applied to files:
packages/gunshi/src/plugin/core.test.tspackages/gunshi/src/definition.test.ts
📚 Learning: 2025-07-21T07:12:47.997Z
Learnt from: CR
PR: kazupon/gunshi#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T07:12:47.997Z
Learning: Applies to **/*.ts : Use ES modules throughout the codebase
Applied to files:
packages/docs/src/guide/essentials/lazy-async.md
🧬 Code graph analysis (6)
packages/definition/src/index.test.ts (1)
packages/gunshi/src/definition.ts (1)
lazy(271-295)
packages/bone/src/index.test.ts (2)
packages/gunshi/src/definition.ts (1)
defineWithTypes(181-198)packages/plugin-global/src/extension.ts (1)
GlobalExtension(12-41)
packages/gunshi/src/context.ts (1)
packages/gunshi/src/types.ts (3)
GunshiParams(40-57)Args(9-9)ExtendContext(28-28)
packages/gunshi/src/plugin/core.test.ts (2)
packages/gunshi/src/definition.ts (2)
defineWithTypes(181-198)ExtendContext(60-60)packages/gunshi/src/types.ts (1)
ExtendContext(28-28)
packages/gunshi/src/definition.test.ts (3)
packages/gunshi/test/utils.ts (1)
DeepWriteable(17-17)packages/gunshi/src/definition.ts (8)
CommandRunner(58-58)Command(56-56)Args(53-53)GunshiParams(61-61)define(131-136)defineWithTypes(181-198)lazy(271-295)lazyWithTypes(364-377)packages/gunshi/src/types.ts (4)
CommandRunner(606-608)Command(507-558)Args(9-9)GunshiParams(40-57)
packages/gunshi/src/definition.ts (1)
packages/gunshi/src/types.ts (11)
GunshiParamsConstraint(74-82)DefaultGunshiParams(65-65)Command(507-558)Prettify(21-21)Args(9-9)ExtractArgs(91-99)ExtractExtensions(115-117)ExtendContext(28-28)CommandLoader(617-619)LazyCommand(567-581)GunshiParams(40-57)
🪛 markdownlint-cli2 (0.18.1)
packages/docs/src/guide/advanced/type-system.md
192-192: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🔇 Additional comments (2)
packages/definition/src/index.test.ts (2)
44-44: LGTM! Improved type-safety.The removal of type-casting in the
command.run(ctx)call demonstrates better type inference in the updated API.
49-49: Verify the functional change in the lazy factory.The lazy factory now wraps the return value with
String(ctx.values.foo)instead of returningctx.values.foodirectly. While the test still passes, this changes the semantics of the test case.Ensure this reflects the intended usage pattern for lazy commands or consider reverting to the simpler pattern if the String coercion is not necessary.
1 participant
Description
Linked Issues
Additional context
Summary by CodeRabbit
New Features
Documentation
Chores
Misc