Add recommended guards helper#12
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: 884f455 The changes in this PR will be included in the next version bump. 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 |
📝 WalkthroughWalkthroughAdds a new exported helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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. ✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.test.ts (1)
175-183: Add one regression case for zero-argument invocation.Once the helper accepts a default options object, include a
recommendedGuards()test to lock in the intended custom error behavior for JavaScript callers.✅ Suggested test addition
describe("recommendedGuards", () => { + it("throws a packaged-state error when called without options", () => { + expect(() => recommendedGuards()).toThrow(/isPackaged/i); + }); + it("allows startup when the app is unpackaged and the opt-in env var is enabled", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.test.ts` around lines 175 - 183, Add a regression test that calls recommendedGuards() with no arguments and asserts it throws the same "isPackaged" error (e.g., expect(() => recommendedGuards()).toThrow(/isPackaged/i)) so JavaScript zero-argument callers keep the intended custom error behavior; place the test alongside the existing suite that already tests packaged-state cases and reference the recommendedGuards function to locate where to add it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/guards.ts`:
- Around line 10-15: The exported function recommendedGuards currently
destructures its parameter and will throw if called with no arguments; modify
its signature to accept a default empty object (e.g., recommendedGuards({ ... }:
RecommendedGuardsOptions = {})) so destructuring never receives undefined while
preserving the RecommendedGuardsOptions type and existing runtime behavior and
error handling inside the function.
---
Nitpick comments:
In `@src/index.test.ts`:
- Around line 175-183: Add a regression test that calls recommendedGuards() with
no arguments and asserts it throws the same "isPackaged" error (e.g., expect(()
=> recommendedGuards()).toThrow(/isPackaged/i)) so JavaScript zero-argument
callers keep the intended custom error behavior; place the test alongside the
existing suite that already tests packaged-state cases and reference the
recommendedGuards function to locate where to add it.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 4f95814c-d97d-495c-9ab4-a8a130f96bbf
📒 Files selected for processing (5)
README.mdsrc/guards.tssrc/index.test.tssrc/index.tssrc/types.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/index.test.ts (1)
124-187: Add a precedence test forisPackagedvsapp.isPackaged.Line 124+ covers most behavior, but there’s no regression test proving explicit
isPackagedoverridesapp.isPackagedwhen both are provided.Proposed test addition
describe("recommendedGuards", () => { + it("prefers explicit isPackaged over app.isPackaged when both are provided", () => { + expect( + recommendedGuards({ + isPackaged: true, + app: { isPackaged: false }, + env: { MY_APP_MCP: "1" }, + envVar: "MY_APP_MCP", + }), + ).toBe(false); + }); + it("throws a packaged-state error when called without options", () => { expect(() => recommendedGuards()).toThrow(/isPackaged/i); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.test.ts` around lines 124 - 187, Add a unit test in the "recommendedGuards" suite that asserts an explicit isPackaged option overrides an app-like object's app.isPackaged: call recommendedGuards with both isPackaged and app: { isPackaged: ... } provided (two variants if you want full coverage) and assert the return matches the explicit isPackaged value (e.g., when isPackaged: true but app.isPackaged: false expect false/startup blocked, and vice versa), using the same expect(...).toBe(...) style as the surrounding tests to prove precedence for the recommendedGuards function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/index.test.ts`:
- Around line 124-187: Add a unit test in the "recommendedGuards" suite that
asserts an explicit isPackaged option overrides an app-like object's
app.isPackaged: call recommendedGuards with both isPackaged and app: {
isPackaged: ... } provided (two variants if you want full coverage) and assert
the return matches the explicit isPackaged value (e.g., when isPackaged: true
but app.isPackaged: false expect false/startup blocked, and vice versa), using
the same expect(...).toBe(...) style as the surrounding tests to prove
precedence for the recommendedGuards function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d91c92a0-a0ea-4309-92ae-05e5ac7a6ebe
📒 Files selected for processing (2)
src/guards.tssrc/index.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/guards.ts
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
recommendedGuards()helper for unpackaged + env-var MCP opt-in checksVerification
pnpm run lintpnpm run typecheckpnpm run testpnpm run buildpnpm test:electronif Electron/CDP behavior changedCloses #5
Summary by CodeRabbit