fix: replace TypeScript 'any' types with proper type definitions #4766
Conversation
Sam-61s
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
📝 WalkthroughWalkthroughReplaced broad Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4766--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
components/navigation/Filter.tsx (1)
26-26: Consider removing the defaultanyto encourage explicit typing.The default
T = anymaintains backward compatibility but weakens type safety guarantees. Consumers should ideally provide explicit type parameters when using this component.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/CaseTOC.tsxcomponents/helpers/applyFilter.tscomponents/navigation/Filter.tsxcomponents/roadmap/RoadmapColumn.tsxcomponents/roadmap/RoadmapItem.tsxcomponents/roadmap/RoadmapList.tsxcomponents/roadmap/RoadmapPill.tsxcomponents/roadmap/types.tspages/roadmap.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
components/roadmap/RoadmapColumn.tsx (2)
components/roadmap/RoadmapItem.tsx (1)
RoadmapItem(27-60)components/roadmap/types.ts (1)
RoadmapItem(4-11)
components/roadmap/types.ts (1)
components/roadmap/RoadmapItem.tsx (1)
RoadmapItem(27-60)
components/navigation/Filter.tsx (1)
components/helpers/applyFilter.ts (2)
applyFilterList(42-123)onFilterApply(131-163)
pages/roadmap.tsx (2)
components/roadmap/RoadmapItem.tsx (1)
RoadmapItem(27-60)components/roadmap/types.ts (1)
RoadmapItem(4-11)
🪛 GitHub Actions: PR testing - if Node project
components/roadmap/types.ts
[error] 7-7: 'React' is not defined. (no-undef)
[error] 12-12: Delete ⏎ prettier/prettier
[error] 7-7: 'React' is not defined. no-undef
[error] 12-12: Delete ⏎ prettier/prettier
components/navigation/Filter.tsx
[warning] 34-34: React Hook useEffect has missing dependencies: 'checks' and 'data'.
[warning] 38-38: React Hook useEffect has missing dependencies: 'data' and 'onFilter'.
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Lighthouse CI
🔇 Additional comments (7)
components/CaseTOC.tsx (1)
14-17: LGTM! Clean type safety improvement.The introduction of the
TocSectioninterface effectively replacesany[]with proper typing for table-of-contents data. The type definition is consistent throughout the component, improving IntelliSense and type checking without altering runtime behavior.Also applies to: 29-29, 50-50
components/roadmap/RoadmapColumn.tsx (1)
9-9: LGTM! Proper use of shared RoadmapItem type.The component correctly imports and uses the
RoadmapItemtype from the centralized types file, replacing the previousany[]typing. The type-only import is the appropriate pattern for this use case.Also applies to: 15-15
components/roadmap/RoadmapPill.tsx (1)
32-32: LGTM! Type widening aligns with RoadmapItem interface.Expanding the
descriptiontype to acceptReact.ReactNodealigns with the centralizedRoadmapItemtype definition and enables richer content while maintaining backward compatibility. The existing render logic at line 99 already handles React nodes correctly.pages/roadmap.tsx (1)
11-11: LGTM! RoadmapData properly typed with centralized interface.The
RoadmapDatainterface now usesRoadmapItem[]instead ofany[]for all outcome arrays, providing proper type safety throughout the roadmap page. The type-only import is the correct pattern.Also applies to: 19-21
components/roadmap/RoadmapList.tsx (1)
6-6: LGTM! Consistent use of centralized RoadmapItem type.The component correctly replaces inline object typing with the imported
RoadmapItemTypefrom the shared types file. The type alias pattern maintains consistency withRoadmapItem.tsx.Also applies to: 11-11
components/roadmap/RoadmapItem.tsx (1)
7-7: LGTM! Properly migrated to centralized type definition.The component correctly replaces inline object typing with the imported
RoadmapItemTypefrom the shared types file. The type alias pattern maintains consistency across roadmap components.Also applies to: 10-10
components/helpers/applyFilter.ts (1)
3-3: LGTM! Robust type guards replace unsafe any usage.The type guards have been significantly strengthened:
- Proper object checks using
typeof x === 'object' && x !== null && 'name' in xpatterns- Explicit string type checking with
typeof x === 'string'- The filter logic correctly handles mixed arrays containing both objects with
nameproperties and plain stringsThe implementation correctly uses
.some()for object matching and.includes()for string matching, ensuring type-safe filtering without altering runtime behavior.Also applies to: 63-81, 94-108, 145-153
Sam-61s
force-pushed
the
fix/types-issue
branch
from
224c337 to
3eb2dac
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/CaseTOC.tsx (1)
143-143: Stale JSDoc: update@param {any[]}to@param {TocSection[]}.The JSDoc still references
any[]forprops.toc, but the actual type is nowTocSection[](line 29).🔎 Proposed fix
- * @param {any[]} props.toc - The table of contents data. + * @param {TocSection[]} props.toc - The table of contents data.
♻️ Duplicate comments (3)
components/navigation/Filter.tsx (3)
11-16: Type signature mismatch foronFiltercallback.The
onFilterprop is typed as(data: T[]) => void, butonFilterApplyinapplyFilter.ts(line 133) expects(result: DataObject[], query: Filter) => voidand calls it with two arguments. Theas anycast on line 37 hides this mismatch.🔎 Proposed fix to align the signature
interface FilterProps<T = any> { data: T[]; - onFilter: (data: T[]) => void; + onFilter: (data: T[], query?: Record<string, string>) => void; checks: Check[]; className?: string; }
31-34: Missing dependencies andas anycast undermine type safety.Pipeline warning confirms
checksanddataare missing from the dependency array. Thedata as anycast also defeats the purpose of the generic type parameter.🔎 Proposed fix
useEffect(() => { setQuery(route.query as Record<string, string>); applyFilterList(checks, data as any, setFilters); - }, [route]); + }, [route, checks, data]);To fully remove the
as anycast,applyFilterListwould need to accept a generic type or a broader base type constraint thatTsatisfies.
36-38: Missing dependencies and doubleas anycasts hide type issues.Pipeline warning confirms
dataandonFilterare missing from the dependency array. The doubleas anycasts mask the signature mismatch mentioned earlier.🔎 Proposed fix for dependencies
useEffect(() => { onFilterApply(data as any, onFilter as any, routeQuery); - }, [routeQuery]); + }, [routeQuery, data, onFilter]);Note: Adding
onFilterto dependencies may cause re-renders if the parent doesn't memoize it withuseCallback. Once theonFiltersignature is corrected per the earlier comment, theas anycasts can be removed.
🧹 Nitpick comments (2)
components/CaseTOC.tsx (1)
50-73: Consider explicit typing to eliminate the type assertion.The implicit array typing on line 51 and the untyped object on line 54 require the type assertion on line 66. Explicit typing would make the code cleaner.
🔎 Proposed refactor
const convertContentToTocItems = (content: TocSection[], level: number = 1): TocItem[] => { - const tocItems = []; + const tocItems: TocItem[] = []; for (const section of content) { - const item = { + const item: TocItem = { lvl: level, content: section.title, slug: section.title .replace(/<|>|"|\\|\/|=/gi, '') .replace(/\s/gi, '-') - .toLowerCase() + .toLowerCase(), + children: undefined }; if (section.children && section.children.length > 0) { - const children = convertContentToTocItems(section.children, level + 1); - - (item as TocItem).children = children; + item.children = convertContentToTocItems(section.children, level + 1); } tocItems.push(item); } return tocItems; };components/helpers/applyFilter.ts (1)
144-153: Logic is correct but consider early termination for clarity.The approach of using
.some()for object elements followed by.includes()for string elements works correctly due to the OR logic. However, thepropertyValue.includes(query[property])call on an array containing{ name: string }objects will perform unnecessary comparisons.Consider a minor refactor for efficiency and clarity:
🔎 Optional: More explicit separation of object vs string array handling
return ( - propertyValue.some((data) => - typeof data === 'object' && data !== null && 'name' in data ? data.name === query[property] : false - ) || - propertyValue.includes(query[property]) || - false + propertyValue.some((data) => { + if (typeof data === 'object' && data !== null && 'name' in data) { + return data.name === query[property]; + } + return data === query[property]; + }) );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/CaseTOC.tsxcomponents/helpers/applyFilter.tscomponents/navigation/Filter.tsxcomponents/roadmap/RoadmapColumn.tsxcomponents/roadmap/RoadmapItem.tsxcomponents/roadmap/RoadmapList.tsxcomponents/roadmap/RoadmapPill.tsxcomponents/roadmap/types.tspages/roadmap.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- components/roadmap/RoadmapPill.tsx
- components/roadmap/RoadmapColumn.tsx
- components/roadmap/RoadmapList.tsx
- components/roadmap/RoadmapItem.tsx
- components/roadmap/types.ts
- pages/roadmap.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-11T21:30:32.478Z
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Applied to files:
components/navigation/Filter.tsx
🧬 Code graph analysis (1)
components/navigation/Filter.tsx (1)
components/helpers/applyFilter.ts (2)
applyFilterList(42-123)onFilterApply(131-163)
🪛 GitHub Actions: PR testing - if Node project
components/navigation/Filter.tsx
[warning] 34-34: React Hook useEffect has missing dependencies: 'checks' and 'data'. (react-hooks/exhaustive-deps)
[warning] 38-38: React Hook useEffect has missing dependencies: 'data' and 'onFilter'. (react-hooks/exhaustive-deps)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (6)
components/CaseTOC.tsx (1)
14-17: Well-designed type replacement for TOC structure.The
TocSectioninterface properly models the recursive TOC structure withtitleand optional nestedchildren. Replacingany[]withTocSection[]inCaseTOCPropsdelivers the type safety improvements outlined in the PR objectives.Also applies to: 29-29
components/helpers/applyFilter.ts (3)
1-4: LGTM on the DataObject interface update.The index signature now properly constrains values to
string | Array<{ name: string } | string>, which aligns with how the data is actually used in the filter functions. Thename: stringproperty is compatible sincestringis part of the union.
61-91: Type guards are well-implemented.The combination of
typeof a === 'object' && a !== null && 'name' in acorrectly narrows the type. The separate handling of string elements in lines 73-81 and 83-91 ensures all cases are covered.
92-117: Consistent type handling in the initialization branch.The else branch mirrors the type-checking logic correctly, ensuring both object-with-name and plain string elements are handled during initial list population.
components/navigation/Filter.tsx (2)
26-29: Good use of generics and typed state.The generic component signature
Filter<T = any>and the explicit state types forfiltersandrouteQueryimprove type clarity over the previous implementation.
40-84: Render logic is sound.The Select component rendering, query parameter management, and shallow routing are implemented correctly. No issues with this portion.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/navigation/Filter.tsx (2)
11-16: Signature mismatch:onFilterprop still doesn't matchonFilterApplyexpectations.The
onFilterprop is typed as(data: T[]) => void, but theonFilterApplyhelper (line 37) calls it with two arguments:onFilter(result, query). This mismatch remains unresolved from the previous review and is currently masked by theas anycast on line 37.🔎 Recommended fix
Update the
FilterPropsinterface to match whatonFilterApplyactually calls:interface FilterProps<T = any> { data: T[]; - onFilter: (data: T[]) => void; + onFilter: (data: T[], query: Record<string, string>) => void; checks: Check[]; className?: string; }Then remove the
as anycast on line 37.
36-38: Good: Dependencies added, but doubleas anycasts hide fundamental type mismatches.✅ The dependency array now correctly includes
routeQuery,data, andonFilter.❌ However, both
data as anyandonFilter as anycasts on line 37 mask underlying type incompatibilities:
- Data type:
onFilterApplyexpectsDataObject[]but receivesT[]- Callback signature:
onFilterApplyexpectsonFilter: (result: DataObject[], query: Filter) => voidbut the prop is typed as(data: T[]) => void(see comment on lines 11-16)Both issues must be resolved to safely remove these casts and achieve the PR's goal of eliminating
anytypes.🔎 Recommended approach
- Fix the
onFiltersignature per the comment on lines 11-16- Either constrain
T extends DataObjector make the helper functions generic- Remove both
as anycasts once types align
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/navigation/Filter.tsxcomponents/roadmap/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- components/roadmap/types.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-11T21:30:32.478Z
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Applied to files:
components/navigation/Filter.tsx
🧬 Code graph analysis (1)
components/navigation/Filter.tsx (1)
components/helpers/applyFilter.ts (2)
applyFilterList(42-123)onFilterApply(131-163)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4766 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 798 798
Branches 146 146
=========================================
Hits 798 798 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Sam-61s have a look at the coderabbit suggestion |
|
I think, I’ve addressed all the CodeRabbitAI review comments and updated the PR accordingly. |
|
/rtm |
3 participants
Description
RoadmapItemtype definition incomponents/roadmap/types.tsfor consistent typing across roadmap componentsanytypes with proper TypeScript interfaces and typesTocSectioninterface toCaseTOC.tsxfor table of contents data structureFiltercomponent generic withFilterProps<T>for improved type flexibilityRoadmapColumn,RoadmapList,RoadmapItem,RoadmapPill) to use shared typesapplyFilter.tswith proper type checking instead ofanyBenefits
Files Changed
created
components/roadmap/types.tsmodified
components/helpers/applyFilter.tscomponents/roadmap/RoadmapItem.tsxcomponents/roadmap/RoadmapColumn.tsxcomponents/roadmap/RoadmapList.tsxcomponents/roadmap/RoadmapPill.tsxcomponents/navigation/Filter.tsxcomponents/CaseTOC.tsxpages/roadmap.tsxTesting
npx tsc --noEmitpasses with 0 errorsRelated issue(s)
Resolves #4704
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.