feat: add design-review Claude Code skill - #884
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: |
85e8cb1 to
ca37ac6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.claude/skills/design-review/SKILL.md:
- Around line 72-74: In the design-review guidance text, update the phrase
“gitignored generated files” to “generated files ignored by Git,” preserving the
surrounding wording and meaning.
- Around line 33-34: Update the audit guidance in the design-review skill to
begin entry-point discovery from repository metadata and language-specific entry
points, supporting Python, Rust, Go, Java, and other module layouts. Make
package.json exports and barrel files conditional examples rather than universal
starting points, while preserving the existing README check where appropriate.
- Around line 29-32: Define a mode-specific TARGET_REF in Phase 1: use the
fetched pr-<n> ref for reviewer mode and resolve the repository’s default branch
for author mode instead of assuming main. Update all Phase 2 git show and git
grep commands to use TARGET_REF consistently, including the related lines 41–45.
- Around line 85-87: Update the typing compatibility guidance near the “narrow
types can widen” rule to qualify it for public types: explicitly state that
adding a member to a public union requires checking exhaustive consumers and
language-specific input/output variance rules, without implying that narrowing
can later occur.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 3ba794b4-dfe7-4573-9034-334fc2a36471
📒 Files selected for processing (1)
.claude/skills/design-review/SKILL.md
| - Reviewer: `gh pr view <n> --json title,body,files,additions,deletions` | ||
| and `git fetch origin pull/<n>/head:pr-<n>`. | ||
| - Author: diff against the merge base | ||
| (`git diff $(git merge-base HEAD main)...HEAD`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bind validation to the ref selected in Phase 1.
Reviewer mode fetches pr-<n>, while author mode assumes a branch named main. Phase 2 then refers to the “target ref” without defining it. A reviewer can inspect the wrong revision. Define TARGET_REF for each mode and use it in every git show and git grep command. Resolve the repository default branch instead of hardcoding main.
Also applies to: 41-45
🤖 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 @.claude/skills/design-review/SKILL.md around lines 29 - 32, Define a
mode-specific TARGET_REF in Phase 1: use the fetched pr-<n> ref for reviewer
mode and resolve the repository’s default branch for author mode instead of
assuming main. Update all Phase 2 git show and git grep commands to use
TARGET_REF consistently, including the related lines 41–45.
| - Audit: start from the entry points — `package.json` `exports`, the | ||
| barrel files, the README. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make audit entry-point discovery language-agnostic.
Audit mode hardcodes package.json and barrel files. This does not work reliably for Python, Rust, Go, or Java modules. Start with repository metadata and language-specific entry points when present. Treat package.json and barrel files as conditional examples.
🤖 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 @.claude/skills/design-review/SKILL.md around lines 33 - 34, Update the audit
guidance in the design-review skill to begin entry-point discovery from
repository metadata and language-specific entry points, supporting Python, Rust,
Go, Java, and other module layouts. Make package.json exports and barrel files
conditional examples rather than universal starting points, while preserving the
existing README check where appropriate.
| Does implementation complexity match problem size? Codegen, build | ||
| hooks, custom CI checks, and gitignored generated files earn their | ||
| keep at scale N; ask what N actually is here. A committed file that |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use standard wording for Git-ignored files.
Change gitignored generated files to generated files ignored by Git. This addresses the spelling warning and improves clarity.
🧰 Tools
🪛 LanguageTool
[grammar] ~73-~73: Ensure spelling is correct
Context: ...gen, build hooks, custom CI checks, and gitignored generated files earn their keep at scal...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 @.claude/skills/design-review/SKILL.md around lines 72 - 74, In the
design-review guidance text, update the phrase “gitignored generated files” to
“generated files ignored by Git,” preserving the surrounding wording and
meaning.
Source: Linters/SAST tools
| free" from the same mechanism. Typing rule: narrow types can widen | ||
| later without breaking; wide types can never narrow back. When unsure, | ||
| ship narrow. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== file context =="
sed -n '1,140p' .claude/skills/design-review/SKILL.md
echo
echo "== search for typing/compatibility rules in repo =="
rg -n "narrow|wide|compatibility|public type|type.*rule|union|switch|exhaustive" .claude/skills/design-review/SKILL.md .clRepository: raystack/apsara
Length of output: 6479
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== search public typing guidance repo-wide =="
rg -n -i "narrow types can widen|wide types can never|typing rule|type compatibility|unstable|compatibility|exhaustive|discriminated union" .
echo
echo "== repo files under .claude =="
find .claude -maxdepth 4 -type f 2>/dev/null | sortRepository: raystack/apsara
Length of output: 4100
🌐 Web query:
TypeScript discriminant union exhaustiveness adding new union member breaks switch consumers
💡 Result:
It is intended behavior for TypeScript to flag an error when a new member is added to a union type that is used in a switch statement, provided you have implemented an exhaustive check pattern [1][2][3]. This is a powerful feature that turns potential runtime bugs—such as unhandled cases—into compile-time errors [2][4][5]. The standard approach to enforce this is to leverage the never type in the default clause of your switch statement [1][6][5]. Because TypeScript narrows the type of the union to only those members not yet handled in the preceding case blocks, if you have covered every member, the type in the default block will be never [2][3][4]. There are two primary ways to implement this: 1. Exhaustive Assignment: Assign the switch value to a variable typed as never [7][2][5]. typescript function handle(u: MyUnion) { switch (u.kind) { case 'A': /* ... */ break; case 'B': /* ... */ break; default: const _exhaustiveCheck: never = u; // Error here if new union member is not handled return _exhaustiveCheck; } } 2. Exhaustive Assertion Function: Pass the switch value to a function that accepts only never [1][6][5]. typescript function assertNever(x: never): never { throw new Error("Unexpected object: " + x); } function handle(u: MyUnion) { switch (u.kind) { case 'A': /* ... */ break; case 'B': /* ... */ break; default: return assertNever(u); // Error here if new union member is not handled } } When you add a new member to the union, TypeScript's control flow analysis will no longer resolve the type in the default case to never (it will include the new member) [1][3]. Since the new member is not assignable to never, the compiler will immediately highlight the default clause as an error, identifying the missing case [1][2][8]. Additionally, you can use the @typescript-eslint/switch-exhaustiveness-check rule to enforce this pattern via linting [6][9].
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/unions-and-intersections.html
- 2: https://yceffort.kr/en/2026/01/typescript-exhaustive-check
- 3: https://www.carlos-menezes.com/posts/exhaustive-checking-typescript
- 4: https://www.fullstory.com/blog/discriminated-unions-and-exhaustiveness-checking-in-typescript/
- 5: https://camchenry.com/blog/typescript-check-switch-case-exhaustive
- 6: https://stackoverflow.com/questions/39419170/how-do-i-check-that-a-switch-block-is-exhaustive-in-typescript
- 7: https://basarat.gitbook.io/TypeScript/type-system/discriminated-unions
- 8: https://2ality.com/2025/02/typescript-never.html
- 9: https://typescript-eslint.io/rules/switch-exhaustiveness-check/
🌐 Web query:
TypeScript return position contravariance function parameter input position variant
💡 Result:
In TypeScript, function types exhibit different variance behaviors based on the position of the type parameters [1][2]. Function parameter (input) positions are contravariant, while return type (output) positions are covariant [3][2]. 1. Contravariance in Input Positions Function parameters are contravariant, meaning a function type (A) => void is assignable to (B) => void if B is a subtype of A (or conversely, if the parameter type A is a supertype of B) [4][5][2]. This is because a function that accepts a broader type (e.g., Animal) is safer than one that requires a narrower type (e.g., Dog). If you expect a function that can handle a Dog, you can safely provide a function that handles any Animal, as it will know how to process a Dog if one is passed to it [5][2]. 2. Covariance in Return Positions Return types are covariant, meaning a function type => B is assignable to => A if B is a subtype of A [2]. This is intuitive: if a consumer expects a function to return an Animal, providing a function that returns a Dog is safe because a Dog is an Animal [2]. 3. Strict Function Types Historically, TypeScript treated function parameters bivariantly for flexibility, but the introduction of the --strictFunctionTypes compiler flag (now enabled by default in --strict mode) enforces strict contravariance for function parameters [6][7]. Methods and constructors are a notable exception to this rule and are still checked bivariantly to ensure generic classes and interfaces, like Array, remain usable [6][7]. 4. Variance Annotations Since TypeScript 4.7, developers can use optional variance annotations (in, out) on generic type parameters to explicitly control or document variance [8][9]. - out T: Indicates the type parameter is covariant (used in output positions) [8][9]. - in T: Indicates the type parameter is contravariant (used in input positions) [8][9]. - in out T: Indicates the type parameter is invariant [8]. These annotations do not change structural behavior but can assist the compiler in checking and verifying variance in complex generic types [10][9].
Citations:
- 1: https://sammart.in/post/2021-07-25-typescript-function-type-parameter-contravariance/
- 2: https://adhdecode.com/articles/typescript/typescript-variance-covariance-contravariance/
- 3: https://stackoverflow.com/questions/62496072/difference-between-covariant-and-contravariant-positions-in-typescript
- 4: https://stackoverflow.com/questions/78142644/when-assigning-callbacks-in-typescript-parameters-are-assigned-in-the-opposite
- 5: https://debugmode.net/2026/03/26/contravariance-on-typescript-function-parameters/
- 6: Strict function types microsoft/TypeScript#18654
- 7: Proposal: covariance and contravariance generic type arguments annotations microsoft/TypeScript#10717
- 8: Optional variance annotations microsoft/TypeScript#48240
- 9: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/
- 10: https://www.typescriptlang.org/docs/handbook/2/generics
🌐 Web query:
TypeScript API type widening narrow widening breaking change public union member
💡 Result:
In TypeScript, concerns about "widening," "narrowing," and "public" members in the context of union types often relate to how the compiler maintains or loses type safety during exported destructuring or object manipulation. A notable issue involving exported discriminated unions was the loss of narrowed types when variables were destructured and exported [1]. Specifically, when variables were extracted from a discriminated union object and exported, the type information—which should have been narrowed based on the discriminant—was lost, causing the variables to revert to their un-narrowed, wider union types [1]. This was addressed and resolved in recent versions of TypeScript (e.g., via PR #59673) to ensure that exported destructured variables preserve their narrowed types [2][1]. Regarding the broader context of breaking changes and public API surface, tools that perform automated semantic versioning checks often categorize these behaviors differently [3][4]: 1. Narrowing of a return type or parameter is generally classified as a breaking change because it may restrict the inputs or outputs that consumers are expecting to handle [5][3][4]. 2. Widening of a return type or parameter (e.g., adding a new member to a union) is typically classified as a non-breaking (additive) change, as it allows existing consumers to continue working while accepting a broader range of values [5][3][4]. 3. The visibility of members in union types is also a historical area of interest; for example, it has been noted that union types may allow public access to members that are private in one of the constituent types [6]. This behavior is a fundamental aspect of how TypeScript resolves property access across diverse types in a union [6]. Finally, "widening" contexts—such as let variables, function returns, or object literals—can cause literal types to revert to their primitive supertypes (e.g., 'A' | 'B' widening to string) [7]. While this is generally by design, it can sometimes manifest as unexpected behavior in generic type inference [8][7].
Citations:
- 1: Variables (extracted from a discriminated union object) lose narrowed types after being exported microsoft/TypeScript#59652
- 2: make exported destructured discriminated union narrowing work microsoft/TypeScript#59673
- 3: https://github.com/antfu/tsnapi/
- 4: https://github.com/kyungseopk1m/semver-checks
- 5: https://github.com/clay-good/OpenLore/blob/main/openspec/changes/archive/add-public-api-surface-contract/proposal.md
- 6: Union type gives public access to private members of the constituent types microsoft/TypeScript#1870
- 7: Inferred string union gets widened unexpectedly microsoft/TypeScript#55156
- 8: Unexpected literal union type widening in generic type inference microsoft/TypeScript#32596
🌐 Web query:
TypeScript TypeScript API surface adding union member breaking type narrowing widening API compatibility
💡 Result:
Adding a new member to a union type in TypeScript is generally considered a breaking change when that union is used in a way that relies on exhaustiveness checking or specific narrowing logic [1][2]. While adding a new property to an object type is often non-breaking, modifying a union type changes the set of possible states a value can inhabit [1][3]. Breakage typically occurs in the following areas: Exhaustiveness Checking: If you use a switch statement or other narrowing pattern to handle all members of a union (often using a 'never' type in a default branch to ensure completeness), adding a new union member will cause the 'never' check to fail because the new member is unhandled [1]. This is a common and intentional safety feature to force developers to update their code when the API's contract changes [1]. Narrowing Logic: TypeScript's control flow analysis relies on narrowing the union based on discriminant properties or other type guards [1]. Adding a new member can alter how TypeScript performs this narrowing, sometimes leading to unexpected behavior if the new member overlaps with existing ones or if the narrowing logic is not exhaustive [4][5]. Type Inference and Widening: In generic contexts, adding members to a union can impact type inference [6][7]. TypeScript sometimes performs "widening" (e.g., converting a literal type to its primitive supertype) [8]. Changes to union members can affect the candidate set for type inference, potentially leading to a more general (widened) type being inferred than was previously the case, which might cause downstream type errors [6][7][8]. API Compatibility: From the perspective of library authors, changing a union type is considered a breaking change because it forces consuming code to handle the new member [2]. Strategies such as using interface merging, module augmentation, or providing optional properties are sometimes used to manage these changes without forcing an immediate break for all users [9]. For more information, see the official TypeScript documentation on Narrowing [1] and guidelines on semantic versioning for TypeScript types [2][3][10].
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/2/narrowing.html
- 2: https://www.semver-ts.org/formal-spec/2-breaking-changes.html
- 3: https://www.semver-ts.org/formal-spec/3-non-breaking-changes.html
- 4: Incorrect narrowing of Union type after discrimination when one type is assignable to the other (even when explicit type annotation is present) microsoft/TypeScript#56106
- 5: Narrowing union types requires at least 2 members microsoft/TypeScript#63645
- 6: Keyword to permit inferring a union for a type parameter microsoft/TypeScript#44312
- 7: Unexpected literal union type widening in generic type inference microsoft/TypeScript#32596
- 8: Inferred string union gets widened unexpectedly microsoft/TypeScript#55156
- 9: https://mui.com/blog/pattern-type-only-breaking-changes-minor-versions/
- 10: https://www.semver-ts.org/formal-spec/index.html
Qualify the typing compatibility rule for public types.
Do not state that narrowing can happen back. Add a concrete exception: widening a public union by adding a member must be checked against exhaustive consumers and language-specific input/output variance rules.
🤖 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 @.claude/skills/design-review/SKILL.md around lines 85 - 87, Update the
typing compatibility guidance near the “narrow types can widen” rule to qualify
it for public types: explicitly state that adding a member to a public union
requires checking exhaustive consumers and language-specific input/output
variance rules, without implying that narrowing can later occur.
A Claude Code skill for reviewing designs rather than code quality: API surface, maintenance contracts, whether the machinery fits the size of the problem. Works in three modes — reviewing a PR, self- reviewing a branch before opening a PR, and auditing an existing module.
ca37ac6 to
c8d0af6
Compare
1 participant
Summary
Adds a
design-reviewskill to.claude/skills/so anyone working on this repo with Claude Code can run a structured design review — of a PR, of their own branch before opening a PR, or of an existing module.This is not a code-quality review (that's
/review). It reviews the design:Test plan
/design-reviewon an open PR walks the phases and drafts a comment for approval