Repository navigation
Include referenced package export types - #382
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR extracts locally-declared TypeScript types referenced by exported functions, attaches them as ChangesReferenced Types Extraction & Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 unit tests (beta)
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 |
|
🔎 Preview deployed: https://kody-pr-382.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/worker/src/mcp/tools/search.ts (1)
336-352: 💤 Low valueOptional: extract a small helper for the duplicated
referencedTypesflattening.The same
(referencedTypes ?? []).flatMap((rt) => [rt.name, rt.definition])pattern appears here and again at lines 710-715/720-723. A shared helper would centralize the shape contract.♻️ Proposed helper
function flattenReferencedTypeFields( referencedTypes: | ReadonlyArray<{ name: string; definition: string }> | undefined, ): Array<string> { return (referencedTypes ?? []).flatMap((rt) => [rt.name, rt.definition]) }Then use at each call site:
- ...(exportDetail.referencedTypes ?? []).flatMap((referencedType) => [ - referencedType.name, - referencedType.definition, - ]), + ...flattenReferencedTypeFields(exportDetail.referencedTypes),🤖 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 `@packages/worker/src/mcp/tools/search.ts` around lines 336 - 352, Extract a small helper function named flattenReferencedTypeFields that takes the optional referencedTypes array (ReadonlyArray<{name:string;definition:string}> | undefined) and returns (referencedTypes ?? []).flatMap(rt => [rt.name, rt.definition]); then replace the duplicated patterns in the code (the flattening inside exportDetail, inside exportDetail.functions (fn), and the other occurrences around lines with similar patterns) by calling flattenReferencedTypeFields(exportDetail.referencedTypes) and flattenReferencedTypeFields(fn.referencedTypes) (and the other sites) so all repeated (referencedTypes ?? []).flatMap(...) usage is centralized under the new helper.packages/worker/src/mcp/tools/search-format.ts (1)
705-717: 💤 Low valueOptional:
typesSourceis now alwaysnull— consider whether the field still belongs in the structured contract.After this change every export always emits
typesSource: null, but the structured type at line 224 still keeps it asstring | null. Either drop the field fromSearchEntityDetailStructured.exports(tightening the contract since callers can no longer rely on a non-null value) or leave a brief comment explaining why it's retained as a placeholder for future re-introduction. As-is, it reads as dead schema.🤖 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 `@packages/worker/src/mcp/tools/search-format.ts` around lines 705 - 717, The exports objects produced in the package branch (inside the detail.type === 'package' block using buildPackageSearchProjection and buildPackageImportSpecifier) always set typesSource to null, so update the structured contract or the emitted objects: either remove the typesSource property from SearchEntityDetailStructured.exports (and strip it from the export objects produced here) to tighten the type, or keep the property but add a clear comment in the type definition explaining it is intentionally a placeholder for future non-null type sources; pick one approach and apply it consistently (adjust the code that constructs exportDetails and the SearchEntityDetailStructured type declaration to match).packages/worker/src/package-registry/manifest.ts (1)
430-482: 💤 Low valueHandle namespaced type references like
Foo.BarinreadTypeReferenceNameThe function currently only recognizes
Identifiernodes but silently dropsTSQualifiedNamereferences (e.g.,Foo.Barfrom re-exported namespaces). While uncommon in typical function signatures, supporting this would improve coverage.The proposed fix recursively resolves to the leftmost identifier:
♻️ Proposed extension
function readTypeReferenceName(node: unknown): string | null { if (!node || typeof node !== 'object') return null const type = getNodeType(node) if (type === 'Identifier') return readIdentifierName(node) if (type === 'TSQualifiedName') { // Resolve to the leftmost identifier so a local declaration named `Foo` // is still picked up when a signature uses `Foo.Bar`. return readTypeReferenceName((node as { left?: unknown }).left) } return null }🤖 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 `@packages/worker/src/package-registry/manifest.ts` around lines 430 - 482, readTypeReferenceName currently only returns identifiers and drops TSQualifiedName (namespaced types like Foo.Bar); update readTypeReferenceName to check getNodeType(node) and if it equals 'TSQualifiedName' recursively call readTypeReferenceName on (node as { left?: unknown }).left so the leftmost Identifier (e.g., Foo) is returned; keep existing checks for null/object and Identifier and ensure pushUniqueTypeName/collectTypeReferenceNamesFromNodes will then pick up namespaced references.
🤖 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.
Nitpick comments:
In `@packages/worker/src/mcp/tools/search-format.ts`:
- Around line 705-717: The exports objects produced in the package branch
(inside the detail.type === 'package' block using buildPackageSearchProjection
and buildPackageImportSpecifier) always set typesSource to null, so update the
structured contract or the emitted objects: either remove the typesSource
property from SearchEntityDetailStructured.exports (and strip it from the export
objects produced here) to tighten the type, or keep the property but add a clear
comment in the type definition explaining it is intentionally a placeholder for
future non-null type sources; pick one approach and apply it consistently
(adjust the code that constructs exportDetails and the
SearchEntityDetailStructured type declaration to match).
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 336-352: Extract a small helper function named
flattenReferencedTypeFields that takes the optional referencedTypes array
(ReadonlyArray<{name:string;definition:string}> | undefined) and returns
(referencedTypes ?? []).flatMap(rt => [rt.name, rt.definition]); then replace
the duplicated patterns in the code (the flattening inside exportDetail, inside
exportDetail.functions (fn), and the other occurrences around lines with similar
patterns) by calling flattenReferencedTypeFields(exportDetail.referencedTypes)
and flattenReferencedTypeFields(fn.referencedTypes) (and the other sites) so all
repeated (referencedTypes ?? []).flatMap(...) usage is centralized under the new
helper.
In `@packages/worker/src/package-registry/manifest.ts`:
- Around line 430-482: readTypeReferenceName currently only returns identifiers
and drops TSQualifiedName (namespaced types like Foo.Bar); update
readTypeReferenceName to check getNodeType(node) and if it equals
'TSQualifiedName' recursively call readTypeReferenceName on (node as { left?:
unknown }).left so the leftmost Identifier (e.g., Foo) is returned; keep
existing checks for null/object and Identifier and ensure
pushUniqueTypeName/collectTypeReferenceNamesFromNodes will then pick up
namespaced references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 142fd7d2-dafc-4e26-a1d2-960d8fa8461d
📒 Files selected for processing (5)
packages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-registry/manifest.node.test.tspackages/worker/src/package-registry/manifest.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/worker/src/package-registry/manifest.ts`:
- Around line 549-555: The current loop accepts the first referenced type even
if it alone exceeds maxReferencedTypeCharsPerExport because the guard only
breaks when referencedTypes.length > 0; change the logic in the loop that
computes nextChars (using totalChars + declaration.definition.length and
maxReferencedTypeCharsPerExport) to skip (continue) any single declaration whose
declaration.definition.length exceeds the remaining budget instead of
breaking/accepting it, and only break once no further declarations can fit (or
when budget is exhausted); apply the same correction to the analogous block that
uses the same variables/guard around lines handling referencedTypes and
nextChars (the other block at 581-586).
- Around line 790-797: The referencedTypes list is including types from generic
parameter bounds that getVariableFunctionSignature doesn't render; to fix this
stop collecting typeParameter constraint nodes when building references: modify
getVariableFunctionReferenceNodes (or getFunctionReferenceNodes it calls) so it
does not traverse typeParameters/TypeParameterDeclaration constraint nodes, then
keep using collectTypeReferenceNamesFromNodes -> collectReferencedTypes as is;
alternatively, if you prefer showing those types, update
getVariableFunctionSignature to include the generic clause—pick one approach and
implement it consistently so referencedTypes only contains types actually
rendered in the synthesized signature.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0488f234-ffbc-42cb-9046-5d06da89ed35
📒 Files selected for processing (3)
packages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-registry/manifest.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/mcp/tools/search.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
2 participants
Summary
Validation
npx vitest run --project node-unit packages/worker/src/package-registry/manifest.node.test.ts packages/worker/src/mcp/tools/search-format.node.test.tsnpm run typechecknpm run lintnpm run testSummary by CodeRabbit