fix(lazy-async): resolve command before executing - #322
Conversation
WalkthroughRefactors lazy command tests into describe blocks, adds a config-driven lazy-loading test with snapshots. In core CLI, shifts lazy-command resolution from executeCommand to cliCore, constructs context with the resolved command, and simplifies executeCommand to operate only on concrete Command instances. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI Core
participant Resolver as Lazy Resolver
participant Exec as executeCommand
Note over CLI: New flow (post-change)
User->>CLI: invoke CLI with args
CLI->>Resolver: resolve(command)
Resolver-->>CLI: resolved Command
CLI->>Exec: executeCommand(resolvedCmd, ctx, decorators)
Exec-->>User: result/output
sequenceDiagram
autonumber
actor User
participant CLI as CLI Core
participant Exec as executeCommand
Note over Exec: Old flow (pre-change)
User->>CLI: invoke CLI with args
CLI->>Exec: executeCommand(cmdOrLazy, ctx, name, decorators)
Exec->>Exec: if Lazy → resolve()
Exec-->>User: result/output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
@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: 0
🧹 Nitpick comments (1)
packages/gunshi/src/cli.test.ts (1)
374-376: Minor inconsistency in property naming.Line 374-376 uses
optionsproperty instead ofargsproperty, which is inconsistent with the rest of the codebase pattern. All other commands in the test file useargsfor command arguments.Apply this diff to maintain consistency:
- options: { + args: { qux: { type: 'number', short: 'q' } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/gunshi/src/__snapshots__/cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
packages/gunshi/src/cli.test.ts(1 hunks)packages/gunshi/src/cli/core.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/cli/core.tspackages/gunshi/src/cli.test.ts
**/*.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/src/cli/core.tspackages/gunshi/src/cli.test.ts
🧠 Learnings (4)
📚 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/cli.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 : Use 'describe' and 'it' blocks for test organization
Applied to files:
packages/gunshi/src/cli.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 : Test files are in 'packages/gunshi/test/' with '.test.ts' extension
Applied to files:
packages/gunshi/src/cli.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 : Snapshot tests are used for renderer output validation
Applied to files:
packages/gunshi/src/cli.test.ts
🧬 Code graph analysis (2)
packages/gunshi/src/cli/core.ts (3)
packages/gunshi/src/utils.ts (2)
isLazyCommand(22-26)resolveLazyCommand(36-88)packages/gunshi/src/types.ts (3)
Command(485-536)CommandContext(328-416)CommandDecorator(600-602)packages/gunshi/src/constants.ts (1)
NOOP(13-13)
packages/gunshi/src/cli.test.ts (5)
packages/gunshi/src/types.ts (3)
CommandRunner(576-578)LazyCommand(542-551)Command(485-536)packages/gunshi/src/definition.ts (5)
CommandRunner(52-52)LazyCommand(56-56)Command(50-50)lazy(174-197)define(95-99)packages/gunshi/src/cli/bone.ts (1)
cli(75-81)packages/gunshi/src/cli/builtin.ts (1)
cli(78-85)packages/gunshi/test/utils.ts (1)
defineMockLog(21-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on Node.js 20
🔇 Additional comments (6)
packages/gunshi/src/cli/core.ts (4)
84-87: Proper lazy command resolution moved to cliCore.The change moves lazy command resolution outside of
executeCommandto thecliCorefunction. This is a good architectural improvement as it ensures commands are resolved early in the process andexecuteCommandoperates on concrete commands only.
98-98: Pass resolved command to context construction.The context now receives the resolved command instead of the potentially lazy command. This ensures type safety and consistency throughout the command execution pipeline.
104-104: executeCommand simplified to accept concrete Command only.The function signature is cleaner now, accepting only resolved
Command<G>instances. This eliminates the need for internal lazy resolution logic withinexecuteCommand.
314-318: Clean function signature and implementation.The
executeCommandfunction is now simplified - it expects a concreteCommand<G>and directly derives the base runner fromcmd.run || NOOP. This removes the complexity of handling lazy commands within the execution phase.packages/gunshi/src/cli.test.ts (2)
320-396: Test refactoring follows learnings and improves structure.The lazy command tests are now properly organized under a
describeblock following the established pattern. The test coverage is comprehensive, testing both function-style and object-style lazy commands along with regular commands. The implementation correctly tests lazy loading behavior with proper async handling usingsetTimeoutand promises.Based on web search results, async functions create binding of new async function and return a Promise which will be resolved with the value returned or rejected with exception uncaught within the function. The test structure properly handles the asynchronous nature of lazy loading.
397-432: Good addition of config-driven lazy loading test.This test case adds valuable coverage for dynamic lazy-loading scenarios using configuration. The test properly exercises the
lazyutility with a loader function and validates both default and loaded command usage through snapshot testing. This aligns with the learnings about using snapshot tests for renderer output validation.
1 participant
Description
Linked Issues
Additional context
Summary by CodeRabbit