omnidoc now reads and compares default values#6606
Conversation
WalkthroughAdds a DefaultValue discriminated union and getDefaultValueOf(component, prop) to the DocReader API; implements default-value resolution across API docs, Storybook, and project source; exports several components' defaultProps; and expands tests to validate default-value consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as TestSuite
participant OmnidocAPI as ApiDocReader
participant OmnidocProj as ProjectDocReader
participant OmnidocSB as StorybookDocReader
participant Meta as componentsAndDefaultPropsMap
Test->>OmnidocAPI: getDefaultValueOf(component, prop)
OmnidocAPI->>OmnidocAPI: lookup api doc examples -> defaultVal
OmnidocAPI-->>Test: DefaultValue (known|none|unreadable)
Test->>OmnidocProj: getDefaultValueOf(component, prop)
OmnidocProj->>Meta: read componentMetaMap (defaultProps)
OmnidocProj->>OmnidocProj: check defaultProps, JSDoc `@default` tags -> PropMeta
OmnidocProj-->>Test: DefaultValue (known|none|unreadable)
Test->>OmnidocSB: getDefaultValueOf(component, prop)
OmnidocSB->>OmnidocSB: load story -> argTypes / table.defaultValue
OmnidocSB-->>Test: DefaultValue (known|none|unreadable)
Test->>Test: compare DefaultValue from API / Project / Storybook
alt mismatch
Test->>Test: record inconsistency
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
| defaultProps: Record<string, unknown> | undefined; | ||
| }; | ||
|
|
||
| export const componentMetaMap: Record<string, ComponentMeta> = { |
There was a problem hiding this comment.
I will fill in this map later. Almost every component triggers tons of warnings so I didn't want to flood the PR.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
omnidoc/readStorybookDoc.ts (1)
108-123: Also read Storybook table default summariesQuite a few of our Storybook docs only surface defaults through
argType.table?.defaultValue?.summary. When that happens, the new reader currently reports{ type: 'none' }, so we miss documented defaults entirely. Grabbing the table summary as a fallback keeps those values visible without waiting for Storybook authors to duplicate data indefaultValue.Please consider:
- if (!('defaultValue' in argType)) { - return { type: 'none' }; - } - - const { defaultValue } = argType; + const tableDefaultSummary = argType?.table?.defaultValue?.summary; + if (tableDefaultSummary != null) { + return { type: 'known', value: tableDefaultSummary }; + } + + if (!('defaultValue' in argType)) { + return { type: 'none' }; + } + + const { defaultValue } = argType;omnidoc/omnidoc.spec.ts (1)
111-125: Normalize types before comparing defaultsRight now
compareValuestreats"0"(string) and0(number) as different, so the snapshots report mismatches even when the value only differs by quoting. Normalizing before the equality check trims that noise while still flagging real discrepancies.- return stripQuotes(val1) === stripQuotes(val2); + const normalized1 = stripQuotes(val1); + const normalized2 = stripQuotes(val2); + return normalized1 === normalized2 || String(normalized1) === String(normalized2);omnidoc/readProject.spec.ts (1)
861-863: Rename test to match the expectationThe assertion checks for
{ type: 'unreadable' }, so the title that promises “return none” is misleading. Aligning the name keeps the intent clear when the suite reports failures.- it('should return none for default value of a prop without default', () => { + it('should return unreadable for default value of a prop without default', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
omnidoc/DocReader.ts(2 hunks)omnidoc/componentsAndDefaultPropsMap.ts(1 hunks)omnidoc/omnidoc.spec.ts(4 hunks)omnidoc/readApiDoc.spec.ts(1 hunks)omnidoc/readApiDoc.ts(2 hunks)omnidoc/readProject.spec.ts(4 hunks)omnidoc/readProject.ts(7 hunks)omnidoc/readStorybookDoc.spec.ts(1 hunks)omnidoc/readStorybookDoc.ts(2 hunks)src/cartesian/Area.tsx(3 hunks)src/cartesian/ReferenceLine.tsx(2 hunks)src/component/Text.tsx(1 hunks)www/src/docs/api/Area.ts(0 hunks)www/src/docs/api/ReferenceLine.ts(1 hunks)www/src/docs/api/ResponsiveContainer.ts(1 hunks)
💤 Files with no reviewable changes (1)
- www/src/docs/api/Area.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/component/Text.tsxsrc/cartesian/ReferenceLine.tsxsrc/cartesian/Area.tsx
🧠 Learnings (2)
📚 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/readApiDoc.spec.tsomnidoc/readProject.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/readApiDoc.spec.ts
🧬 Code graph analysis (5)
omnidoc/readApiDoc.ts (1)
omnidoc/DocReader.ts (1)
DefaultValue(1-1)
omnidoc/componentsAndDefaultPropsMap.ts (3)
src/cartesian/Area.tsx (1)
defaultAreaProps(671-687)src/cartesian/ReferenceLine.tsx (1)
referenceLineDefaultProps(292-302)src/component/Text.tsx (1)
textDefaultProps(352-364)
omnidoc/readProject.ts (2)
omnidoc/DocReader.ts (1)
DefaultValue(1-1)omnidoc/componentsAndDefaultPropsMap.ts (1)
componentMetaMap(9-13)
omnidoc/omnidoc.spec.ts (2)
omnidoc/DocReader.ts (1)
DefaultValue(1-1)src/util/DataUtils.ts (1)
isNotNil(201-203)
omnidoc/readStorybookDoc.ts (1)
omnidoc/DocReader.ts (2)
DocReader(3-35)DefaultValue(1-1)
⏰ 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 (14)
omnidoc/DocReader.ts (2)
1-2: LGTM!The
DefaultValuediscriminated union type is well-designed. It clearly distinguishes between a known default value (includingundefinedas an explicit value), the absence of a default, and cases where the default cannot be determined.
31-34: LGTM!The
getDefaultValueOfmethod signature is clear and consistent with the existing interface methods.omnidoc/readStorybookDoc.spec.ts (1)
192-194: LGTM!The test correctly verifies that the
getDefaultValueOfmethod retrieves the default value forReferenceLine'sstrokeWidthproperty. The expected numeric value1aligns with the updated API documentation.src/component/Text.tsx (1)
352-364: LGTM!Exporting
textDefaultPropsenables external modules to access the Text component's default props, supporting the new default value extraction infrastructure. The constant structure and values remain unchanged.www/src/docs/api/ReferenceLine.ts (1)
95-104: LGTM!The change from
'1'(string) to1(number) correctly aligns the documented default value with the actual implementation insrc/cartesian/ReferenceLine.tsx(line 299), wherestrokeWidth: 1is defined as a number.src/cartesian/ReferenceLine.tsx (2)
94-100: LGTM!The
@defaultValueJSDoc annotation correctly documents the default value for thepositionprop, matching the actual default of'middle'defined inreferenceLineDefaultProps(line 300).
292-302: LGTM!Exporting
referenceLineDefaultPropsenables external modules to access the ReferenceLine component's default props, supporting the new default value extraction infrastructure. The constant structure and values remain unchanged.omnidoc/readApiDoc.spec.ts (1)
83-85: LGTM!The test correctly verifies that the
getDefaultValueOfmethod retrieves the default value forReferenceLine'sstrokeWidthproperty from the API documentation. The expected numeric value1aligns with the updated documentation inwww/src/docs/api/ReferenceLine.ts.src/cartesian/Area.tsx (3)
115-118: LGTM!The
@defaultJSDoc annotation correctly documents the default value for theactiveDotprop, matching the actual default oftruedefined indefaultAreaProps(line 672).
671-687: LGTM!Exporting
defaultAreaPropsenables external modules to access the Area component's default props, supporting the new default value extraction infrastructure. The constant structure and values remain unchanged.
152-156: Verified: JSDoc documentation is accurate.The
@defaultValue 100annotation matches the actual value ofDefaultZIndexes.area, which is set to100.www/src/docs/api/ResponsiveContainer.ts (1)
40-49: Documentation change is correct and verified.The minWidth default value has been accurately updated to
'0'in the documentation. The actual component implementation insrc/component/ResponsiveContainer.tsx(line 123) defaultsminWidthto0, which is also documented in its JSDoc comment and confirmed by tests.omnidoc/readApiDoc.ts (1)
45-66: Consistent DefaultValue mappingMapping API docs through the shared
DefaultValueunion keeps the DocReader contract predictable. Thanks for covering the'undefined'sentinel as well.omnidoc/componentsAndDefaultPropsMap.ts (1)
9-13: Good foundation for project defaultsCentralizing the exported
defaultPropsin a single map like this gives ProjectDocReader a clean hookup point for future components. Nice and readable.
| getDefaultValueOf(component: string, prop: string): DefaultValue { | ||
| return this.getPropMeta(component, prop).reduce( | ||
| (acc: DefaultValue, meta: PropMeta): DefaultValue => { | ||
| if (acc.type === 'known' || acc.type === 'none') { | ||
| return acc; | ||
| } | ||
| if (meta.defaultValueFromObject.type !== 'unreadable') { | ||
| return meta.defaultValueFromObject; | ||
| } | ||
| if (meta.defaultValueFromJSDoc.type !== 'unreadable') { | ||
| return meta.defaultValueFromJSDoc; | ||
| } | ||
| return acc; | ||
| }, | ||
| { type: 'unreadable' }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Let @default JSDoc override “none” object defaults.
Line 333: When a prop lacks a defaultProps entry we set defaultValueFromObject to { type: 'none' }. Because the reducer short-circuits on 'none', the JSDoc branch never runs, so props with documented @default values always resolve to 'none'. That defeats the purpose of the new comparison logic. Only short-circuit on 'known', and keep 'none' as a fallback after JSDoc has been considered.
return this.getPropMeta(component, prop).reduce(
(acc: DefaultValue, meta: PropMeta): DefaultValue => {
- if (acc.type === 'known' || acc.type === 'none') {
- return acc;
- }
- if (meta.defaultValueFromObject.type !== 'unreadable') {
- return meta.defaultValueFromObject;
- }
- if (meta.defaultValueFromJSDoc.type !== 'unreadable') {
- return meta.defaultValueFromJSDoc;
- }
- return acc;
+ if (acc.type === 'known') {
+ return acc;
+ }
+ if (meta.defaultValueFromObject.type === 'known') {
+ return meta.defaultValueFromObject;
+ }
+ if (meta.defaultValueFromJSDoc.type === 'known') {
+ return meta.defaultValueFromJSDoc;
+ }
+ if (acc.type === 'unreadable') {
+ if (meta.defaultValueFromObject.type === 'none') {
+ return meta.defaultValueFromObject;
+ }
+ if (meta.defaultValueFromJSDoc.type === 'none') {
+ return meta.defaultValueFromJSDoc;
+ }
+ }
+ return acc;
},
{ type: 'unreadable' },
);🤖 Prompt for AI Agents
In omnidoc/readProject.ts around lines 327 to 343, the reducer currently
short-circuits when acc.type is 'known' or 'none', which prevents @default JSDoc
values from overriding 'none' object defaults; remove the check for acc.type ===
'none' so the reducer only short-circuits on 'known', and make sure the order
remains: if meta.defaultValueFromObject.type !== 'unreadable' return that,
otherwise if meta.defaultValueFromJSDoc.type !== 'unreadable' return that, and
finally return acc (allowing 'none' to be used only after JSDoc has been
considered).
There was a problem hiding this comment.
This is intentional because the implementation is more important to us than the jsdoc.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
omnidoc/omnidoc.spec.ts
Outdated
| "Component "Area", prop "animationDuration": Documented default value "1500", but actually in project it is "1500"", | ||
| "Component "Area", prop "id": Documented default value, but actually none exists in the project", | ||
| "Component "Text", prop "angle": Documented default value, but actually none exists in the project", | ||
| "Component "ReferenceLine", prop "x": Documented default value, but actually none exists in the project", |
There was a problem hiding this comment.
I will go through them one by one, some of them are correct in the docs and some are correct in the impl. This is main reason why I didn't just generate everything from the typescript definition - we have multiple sources of truth.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
omnidoc/readProject.ts (1)
154-180: Refactor the double negative for clarity.The nested negation on lines 154-180 makes the filtering logic hard to follow. Consider refactoring to a positive condition or extracting to a helper function.
Apply this diff to improve readability:
+ private isReactInternal(propName: string): boolean { + return ( + propName.startsWith('UNSAFE_') || + propName === 'constructor' || + propName === 'context' || + propName === 'props' || + propName === 'refs' || + propName === 'state' || + propName === 'contextType' || + propName === 'defaultProps' || + propName === 'displayName' || + propName === 'componentDidCatch' || + propName === 'componentDidMount' || + propName === 'componentDidUpdate' || + propName === 'componentWillMount' || + propName === 'componentWillReceiveProps' || + propName === 'componentWillUnmount' || + propName === 'componentWillUpdate' || + propName === 'forceUpdate' || + propName === 'getSnapshotBeforeUpdate' || + propName === 'render' || + propName === 'setState' || + propName === 'shouldComponentUpdate' + ); + } + private collectPropertiesFromType(componentName: string, type: Type): ReadonlyArray<PropMeta> { const properties: PropMeta[] = []; // Get direct properties const typeProperties = type.getProperties(); for (const prop of typeProperties) { const propName = prop.getName(); // Skip built-in React properties and methods - if ( - !( - !propName.startsWith('UNSAFE_') && - propName !== 'constructor' && - propName !== 'context' && - propName !== 'props' && - propName !== 'refs' && - propName !== 'state' && - propName !== 'contextType' && - propName !== 'defaultProps' && - propName !== 'displayName' && - propName !== 'componentDidCatch' && - propName !== 'componentDidMount' && - propName !== 'componentDidUpdate' && - propName !== 'componentWillMount' && - propName !== 'componentWillReceiveProps' && - propName !== 'componentWillUnmount' && - propName !== 'componentWillUpdate' && - propName !== 'forceUpdate' && - propName !== 'getSnapshotBeforeUpdate' && - propName !== 'render' && - propName !== 'setState' && - propName !== 'shouldComponentUpdate' - ) - ) { + if (this.isReactInternal(propName)) { continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
omnidoc/omnidoc.spec.ts(4 hunks)omnidoc/readProject.ts(7 hunks)omnidoc/readStorybookDoc.ts(2 hunks)storybook/StorybookArgs.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- omnidoc/readStorybookDoc.ts
🧰 Additional context used
🧬 Code graph analysis (2)
omnidoc/readProject.ts (2)
omnidoc/DocReader.ts (1)
DefaultValue(1-1)omnidoc/componentsAndDefaultPropsMap.ts (1)
componentMetaMap(9-13)
omnidoc/omnidoc.spec.ts (2)
omnidoc/DocReader.ts (1)
DefaultValue(1-1)src/util/DataUtils.ts (1)
isNotNil(201-203)
⏰ 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). (1)
- GitHub Check: Build, Test, Pack
🔇 Additional comments (14)
omnidoc/omnidoc.spec.ts (5)
5-6: LGTM!The imports are appropriately used in the new default props consistency test suite below.
59-59: LGTM!Changing to descriptive string messages improves error readability compared to the previous object format.
Also applies to: 71-73
104-135: LGTM!The helper functions correctly handle the DefaultValue discriminated union cases. The logic properly:
- Ignores unreadable values
- Compares known values with quote normalization
- Detects mismatches between documented and actual none states
179-201: LGTM!The test correctly validates that
@defaultJSDoc annotations match the actual default values in the project, using the PropMeta structure to access both sources.
203-203: LGTM!Good use of
describe.todoto track future work on type definition consistency validation.omnidoc/readProject.ts (9)
4-18: LGTM!The new type definitions are well-structured.
PropMetaappropriately captures both the origin and default values from multiple sources, and the comment correctly notes that names aren't unique across origins.
29-73: LGTM!The optional
kindparameter ongetPublicSymbolNamesis backwards-compatible, and the additional utility exclusions ingetPublicComponentNamescorrectly filter non-component exports.
105-117: LGTM!The path normalization on line 108 correctly addresses the Windows compatibility issue flagged in previous reviews, ensuring origin detection works consistently across platforms.
119-132: LGTM!Correctly returns
'unreadable'when no JSDoc tag exists, since the absence of documentation doesn't prove the absence of a default value (which may be defined indefaultProps).
134-144: LGTM, with note on String conversion.The distinction between
'unreadable'(no metadata) and'none'(has metadata, no default) is correct. Note thatString(value)on line 141 may not handle complex objects ideally, but this is acceptable for the current string-comparison use case.
211-215: LGTM!Correctly returns an array since a single prop can have multiple declarations from different origins (e.g., both recharts and DOM).
313-317: LGTM!Correctly unwraps
ForwardRefandMemowrappers to access the underlying component's props type.
328-348: LGTM!The reducer correctly prioritizes object defaults over JSDoc (as noted in the comment), which ensures that actual implementation takes precedence over potentially outdated documentation. The short-circuit on
'known'or'none'is appropriate.
350-362: LGTM!The
metaToNameshelper correctly deduplicates and sorts prop names, and the updated methods properly use the new PropMeta-based collection with appropriate origin filtering.
| test.each(apiDocReader.getPublicComponentNames().filter(c => !apiComponentsWithKnownIssues.includes(c)))( | ||
| 'if component %s has default props in the API, then that default value must be the same as in the project', | ||
| component => { | ||
| const allProps = apiDocReader.getRechartsPropsOf(component); | ||
| const missingDefaultProps: string[] = []; | ||
|
|
||
| for (const prop of allProps) { | ||
| const apiDefaultProp = apiDocReader.getDefaultValueOf(component, prop); | ||
| const projectDefaultProp = projectReader.getDefaultValueOf(component, prop); | ||
| const problem = compareDefaultValues(apiDefaultProp, projectDefaultProp); | ||
| if (problem) { | ||
| missingDefaultProps.push(`Component "${component}", prop "${prop}": ${problem}`); | ||
| } | ||
| } | ||
|
|
||
| expect(missingDefaultProps).toEqual([]); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Missing import for test.each.
The code uses test.each but only imports describe, it, and expect from vitest. Either import test from vitest or change to it.each.
Apply this diff to add the missing import:
-import { describe, it, expect } from 'vitest';
+import { describe, it, expect, test } from 'vitest';Also applies to: 160-177
🤖 Prompt for AI Agents
In omnidoc/omnidoc.spec.ts around lines 139-156 and also 160-177, the test file
uses test.each but the file only imports describe, it, and expect from vitest;
add test to the import list from 'vitest' (or alternatively replace test.each
with it.each) so the test.each calls resolve correctly—update the top-of-file
import to include test and run the spec to verify no unresolved identifier
errors remain.
| type?: SBType | SBType['name']; | ||
| table?: { | ||
| defaultValue?: { summary: string; detail?: string } | unknown; | ||
| defaultValue?: unknown; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify alignment with Storybook's official ArgTypes.
The type change from { summary: string; detail?: string } | unknown to unknown loosens the constraint. Please confirm this aligns with the actual Storybook 9.0.17 ArgTypes definition.
🌐 Web query:
What is the type of table.defaultValue in Storybook 9.0.17 ArgTypes?
💡 Result:
In Storybook 9.0.17 ArgTypes, table.defaultValue has type: { summary: string; detail?: string }. [1][2]
Sources:
[1] Storybook ArgTypes docs — table.defaultValue type.
[2] Storybook 9 ArgTypes / DocBlock documentation.
Revert to official Storybook type for table.defaultValue.
The type was incorrectly changed to unknown. In Storybook 9.0.17 ArgTypes, table.defaultValue has type: { summary: string; detail?: string }. The current change reduces type safety; restore the correct type definition.
🤖 Prompt for AI Agents
In storybook/StorybookArgs.ts at line 31 the table.defaultValue type was changed
to `unknown`; restore it to the official Storybook type used in ArgTypes — an
object of shape { summary: string; detail?: string } (or import and use the
corresponding Storybook ArgTypes type) so table.defaultValue has the correct
typed properties and preserves type safety.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6606 +/- ##
==========================================
- Coverage 94.16% 94.16% -0.01%
==========================================
Files 493 493
Lines 41068 41067 -1
Branches 4773 4774 +1
==========================================
- Hits 38672 38671 -1
Misses 2391 2391
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 209 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
|
Description
I also fixed a few to demonstrate. There's tons left though, so this will take a few more PRs to go through.
Related Issue
#6069
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests