Refactor extension internals for maintainability#21
Merged
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 45 minutes and 1 second. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (26)
WalkthroughThis PR refactors command execution and configuration handling across the pi-extensions ecosystem. It introduces a shared command entry schema in the ChangesCommand Config Refactoring & Modularization
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
commit: |
Vahor
force-pushed
the
refactor/maintainability
branch
3 times, most recently
from
May 23, 2026 14:14
5ce4081 to
9b0c9b7
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/shared/src/config/test/merge.test.ts (1)
1-60: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMove this test file under
__tests__/to match the repository test layout rule.This test uses Bun correctly, but its location under
src/config/test/does not follow the required__tests__/placement convention.As per coding guidelines, "Tests should be placed in
__tests__/directory alongside source files and use Bun test runner".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/shared/src/config/test/merge.test.ts` around lines 1 - 60, The test file is misplaced; move the test that targets mergeSettings into the repository's __tests__ layout: relocate the file containing the describe("mergeSettings", ...) block to an __tests__ directory adjacent to the source module, keep the test contents (including the Bun import: import { describe, expect, test } from "bun:test") unchanged, and update any relative imports if the new location changes import paths for mergeSettings so the tests still import mergeSettings correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/shared/src/runner/context.ts`:
- Around line 23-28: The code inserts raw result.stdout/result.stderr inside
triple-backtick fences in formatContextMessage which allows embedded ``` to
break the fence; fix by sanitizing fence delimiters before embedding: create or
use an escapeCodeFenceContent function and apply it to the outputs returned by
truncateContextOutput (i.e., replace occurrences of "```" with an escaped or
safe representation) and then use those escaped stdout/stderr when pushing the
fenced blocks in formatContextMessage so embedded backticks cannot terminate the
markdown fence.
In `@lib/shared/src/runner/index.ts`:
- Around line 64-68: The interactive branch currently calls
runInteractiveCommand(entry.command, resolvedCwd, ctx) without passing
entry.timeout, so interactive entries ignore configured timeouts; update the
call in the entry.interactive branch to pass the timeout (e.g.,
runInteractiveCommand(entry.command, resolvedCwd, ctx, entry.timeout) or
whatever the runInteractiveCommand signature expects), and ensure
runInteractiveCommand (and any helpers it uses) accept and honor that timeout
parameter so interactive and non-interactive pi.exec paths share the same
timeout behavior.
In `@lib/shared/src/runner/interactive.ts`:
- Around line 35-51: The interactive spawn path (spawn(...) creating child, and
the child.on("error") / child.on("exit") handlers in interactive.ts) lacks
timeout handling; add a timer when starting the child that, after the configured
timeout, kills the child process (first SIGTERM then SIGKILL if needed) and
calls finish with a timeout-specific CommandResult (e.g., code: 124 or a
distinct flag and stderr noting "timed out") to indicate timeout; ensure the
timer is cleared in both the "error" and "exit" handlers so normal completion
doesn't race with the timeout, and ensure child.kill is used safely (check
child.pid) to avoid orphaned processes.
In `@packages/pi-keymap/src/__tests__/config.test.ts`:
- Around line 5-8: Add a negative Jest test in
packages/pi-keymap/src/__tests__/config.test.ts that ensures decodeConfig (which
uses Schema.decodeUnknownSync with onExcessProperty: "error") rejects objects
with unknown keys: call decodeConfig with a valid minimal KeymapsConfigSchema
payload plus an extra unexpected property and assert it throws (e.g., expect(()
=> decodeConfig({...})).toThrow()) and optionally assert the error mentions the
excess property; reference decodeConfig, KeymapsConfigSchema and
Schema.decodeUnknownSync in the test.
In `@packages/pi-keymap/src/index.ts`:
- Around line 47-48: Avoid registering leader handlers when the configured
leader key is invalid: perform the leader validation before calling
registerLeaderKeymaps and only invoke registerLeaderKeymaps(leader, config, cwd,
pi, ctx) when that validation returns true (i.e., the leader config is usable);
leave registerDirectKeymaps(direct, cwd, pi, ctx) unchanged. Locate the existing
invalid-leader warning path and gate the call to registerLeaderKeymaps behind
its validation result so runtime leader handlers are not registered for invalid
configs.
---
Outside diff comments:
In `@lib/shared/src/config/test/merge.test.ts`:
- Around line 1-60: The test file is misplaced; move the test that targets
mergeSettings into the repository's __tests__ layout: relocate the file
containing the describe("mergeSettings", ...) block to an __tests__ directory
adjacent to the source module, keep the test contents (including the Bun import:
import { describe, expect, test } from "bun:test") unchanged, and update any
relative imports if the new location changes import paths for mergeSettings so
the tests still import mergeSettings correctly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 4974703c-990a-4ea3-88e5-9db416ad565d
📒 Files selected for processing (25)
.changeset/maintainability-refactor.mdlib/shared/package.jsonlib/shared/src/config/__tests__/config.test.tslib/shared/src/config/merge.tslib/shared/src/config/parse.tslib/shared/src/config/test/merge.test.tslib/shared/src/runner/config.tslib/shared/src/runner/context.tslib/shared/src/runner/index.tslib/shared/src/runner/interactive.tslib/shared/src/runner/renderer.tslib/shared/src/runner/spinner.tslib/shared/src/runner/types.tspackages/pi-hooks/README.mdpackages/pi-hooks/src/__tests__/config.test.tspackages/pi-hooks/src/config.tspackages/pi-hooks/src/index.tspackages/pi-keymap/README.mdpackages/pi-keymap/src/__tests__/config.test.tspackages/pi-keymap/src/actions.tspackages/pi-keymap/src/config.tspackages/pi-keymap/src/index.tspackages/pi-keymap/src/shortcuts.tspackages/pi-keymap/src/validation.tsscripts/generate-config-schemas.ts
💤 Files with no reviewable changes (1)
- lib/shared/src/runner/renderer.ts
Vahor
force-pushed
the
refactor/maintainability
branch
from
May 23, 2026 14:27
9b0c9b7 to
38d7e52
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is a maintainability/readability refactor for the extension internals while keeping Effect + Schema in place.
Shared config and command schemas
@vahor/shared/runner/config.pi-hooksandpi-keymapinstead of duplicating command config fields.$schemain config files at runtime, matching the generated editor schemas.onExcessProperty: "error", so unknown config keys fail instead of being silently dropped.Shared config merging
keymaps.key, they are merged bykeyby default.__tests__layout.Runner internals
config.tsfor command schema/typescontext.tsfor context-message formatting/truncationinteractive.tsfor interactive command executionspinner.tsfor status spinner renderingtypes.tsfor shared runner result typesrunCommandsas the small orchestration entrypoint.pi-keymap internals
actions.tsfor running command/prompt entriesvalidation.tsfor keymap validation and dead-end warningsshortcuts.tsfor direct and leader shortcut registrationsrc/index.tsfocused on loading config and wiring startup behavior.senddefaults tofalse, includingopen: falseprompts.Docs and schemas
keymapsarrays are merged bykey.Validation
bun run formatbun run typecheckbun testbun run schemas:checkbun run build