omnidoc checks for non-existing but documented props#6582
Conversation
WalkthroughRefactors omnidoc APIs: renames Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Tests
participant Doc as DocReader (interface)
participant API as ApiDocReader
participant SB as StorybookDocReader
participant PJ as ProjectReader
Note over Test,Doc: Tests call DocReader's public API
Test->>Doc: getPublicComponentNames()
Doc->>API: (if API reader) getPublicComponentNames()
Doc->>SB: (if Storybook reader) getPublicComponentNames()
Doc->>PJ: (if Project reader) getPublicComponentNames()
Note right of API: filters out hooks / utilities
Test->>Doc: getRechartsPropsOf(component)
Doc->>API: getRechartsPropsOf(component)
Doc->>SB: getRechartsPropsOf(component)
Doc->>PJ: getRechartsPropsOf(component)
Note right of PJ: uses getPropsType → resolves ComponentType / aliases / SVGProps / Omit
Doc->>PJ: getAllPropsOf(component)
Note right of PJ: aggregates Recharts props + inherited SVG props
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
71fddd6 to
d893dd2
Compare
|
On my local this reports 38 props on the website, and 42 props in the storybook, that are documented but don't exist. Let's see that it fails with the same on CI and then decide what to do with them @ckifer ? |
|
🥳 |
|
Hm it looks mostly right. Would like to take a look at a few of them like Funnel - shape and Sunburst - margin though. |
|
Area.baseLine is also sus |
d893dd2 to
f7014fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
omnidoc/DocReader.ts(1 hunks)omnidoc/omnidoc.spec.ts(1 hunks)omnidoc/readApiDoc.spec.ts(2 hunks)omnidoc/readApiDoc.ts(1 hunks)omnidoc/readProject.spec.ts(1 hunks)omnidoc/readProject.ts(4 hunks)omnidoc/readStorybookDoc.spec.ts(5 hunks)omnidoc/readStorybookDoc.ts(1 hunks)src/cartesian/Area.tsx(2 hunks)src/cartesian/Line.tsx(2 hunks)src/cartesian/Scatter.tsx(2 hunks)src/cartesian/XAxis.tsx(2 hunks)src/cartesian/YAxis.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/cartesian/Area.tsx
- src/cartesian/YAxis.tsx
- omnidoc/readProject.spec.ts
- src/cartesian/Line.tsx
- src/cartesian/XAxis.tsx
- omnidoc/readStorybookDoc.ts
- omnidoc/readApiDoc.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode any user-facing strings or formatting choices in the library code; leave internationalization to library users
Files:
src/cartesian/Scatter.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Mock getBoundingClientRect using test/helper/MockGetBoundingClientRect.ts when rendering Recharts components (e.g., Tooltip, Legend, charts)
Applied to files:
omnidoc/readStorybookDoc.spec.ts
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : When testing selectors, verify render counts using the spy and rerenderSameComponent from createSelectorTestCase
Applied to files:
omnidoc/readStorybookDoc.spec.ts
📚 Learning: 2025-10-25T07:36:02.229Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-25T07:36:02.229Z
Learning: Recharts aims for simple, declarative, and composable charts; prioritize consistency, usability, performance, and accessibility
Applied to files:
omnidoc/readStorybookDoc.spec.ts
🧬 Code graph analysis (1)
src/cartesian/Scatter.tsx (2)
src/cartesian/Area.tsx (1)
Props(158-158)src/cartesian/Line.tsx (1)
Props(160-160)
⏰ 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). (2)
- GitHub Check: Build, Test, Pack
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
src/cartesian/Scatter.tsx (2)
2-11: LGTM!The
ComponentTypeimport is necessary for the explicit type annotation on the export. The import reorganization is clean and doesn't affect functionality.
749-749: Explicit type annotation compiles cleanly.The
ComponentType<Props>annotation successfully compiles without TypeScript errors, confirming the change is valid. This explicit annotation aids omnidoc's ability to extract documented props, and the runtime behavior remains unchanged.omnidoc/readStorybookDoc.spec.ts (1)
64-64: LGTM! Method rename applied consistently.All occurrences of
getPropsOfhave been correctly renamed togetRechartsPropsOf, maintaining test logic and assertions.Also applies to: 92-92, 96-96, 113-116, 119-119, 156-156, 186-186, 192-192
omnidoc/omnidoc.spec.ts (2)
55-237: Excellent addition: cross-check test for API documentation consistency.This test validates that props documented in API docs actually exist in the source code. The inline snapshot captures the current mismatches, providing a baseline to track documentation debt.
239-437: Excellent addition: cross-check test for Storybook documentation consistency.This test validates that props documented in Storybook actually exist in the source code. The inline snapshot captures the current mismatches, aligning with the PR objective to detect documented but non-existent props.
omnidoc/DocReader.ts (1)
7-10: LGTM! Clear and well-documented interface expansion.The new methods provide better granularity:
getPublicComponentNames()filters out hooks and utilitiesgetRechartsPropsOf()makes it explicit that SVG props are excludedgetAllPropsOf()includes inherited SVG props for documentation checksThe JSDoc comments clearly explain the differences.
Also applies to: 16-16, 18-22
omnidoc/readApiDoc.ts (1)
12-24: LGTM! Clean implementation of the new interface.The filtering logic in
getPublicComponentNames()appropriately excludes hooks and utilities. The delegation fromgetAllPropsOf()togetRechartsPropsOf()is correct since API docs don't track SVG inheritance.Also applies to: 26-36
omnidoc/readProject.ts (6)
48-63: LGTM! Filtering logic is appropriate.The conservative approach of only filtering by 'use' prefix and specific utilities is sensible, avoiding false exclusions of components.
107-114: LGTM! Handles Partial wrapper types.The FQN check for 'Partial' correctly unwraps the type to extract props. This handles the common case where types use
Partial<Props>.
197-248: Comprehensive type resolution with good fallback strategies.The multi-strategy approach (call signatures → construct signatures → type arguments → props property) handles various component declaration patterns. Error messages appropriately reference declarations rather than components.
250-261: LGTM! Orchestrates type resolution effectively.The method correctly prioritizes alias symbol resolution (for
ComponentType<Props>patterns) before falling back to call signature inspection.
263-266: LGTM! Clean refactor using new type resolution.The method correctly uses
getPropsType()for improved type resolution before collecting properties.
290-293: LGTM! Updated to use new type resolution.Correctly uses
getPropsType()for consistent type resolution across the class.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6582 +/- ##
=======================================
Coverage 94.56% 94.56%
=======================================
Files 491 491
Lines 41031 41031
Branches 4748 4749 +1
=======================================
Hits 38803 38803
Misses 2223 2223
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportBundle size has no change ✅ |
## Description Some of these we removed in 3.0 and some I don't know. ## Related Issue #6069 #6582 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Removed internal, deprecated, and unused props from component API docs and clarified several descriptions to simplify public-facing docs. * **Chores** * Streamlined Storybook stories to expose only public props by removing internal/deprecated story args and prop entries. * **Tests** * Updated snapshot expectations to align with the reduced public prop surface across docs and stories. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Funnel has shape prop, FunnelChart has not Sunburst does not have margin prop and I tried adding it - the renderer ignores it. |
Description
Extend omnidoc to detect props that are documented but not actually used in the source.
Related Issue
#6069
Summary by CodeRabbit
New Features
Tests
Improvements