Conversation
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
WalkthroughThis pull request improves type safety across the directory component utilities. The 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/visual-editor/src/components/directory/DirectoryWrapper.tsx (1)
31-42: Consider extracting the inlinedirectoryChildrentype to a named export.The inline shape is fairly large and is the canonical definition for what
DirectoryListconsumers must supply. Exporting it as a named type makes it reusable by page templates and test fixtures without duplicating the shape.♻️ Proposed refactor
+export type DirectoryChild = { + id: string; + name: string; + slug: string; + meta?: { + entityType?: { + id: "dm_country" | "dm_region" | "dm_city"; + }; + }; + dm_addressCountryDisplayName?: string; + dm_addressRegionDisplayName?: string; +}; + export const DirectoryList = ({ streamDocument, directoryChildren, relativePrefixToRoot, }: { streamDocument: StreamDocument; - directoryChildren: { - id: string; - name: string; - slug: string; - meta?: { - entityType?: { - id: "dm_country" | "dm_region" | "dm_city"; - }; - }; - dm_addressCountryDisplayName?: string; - dm_addressRegionDisplayName?: string; - }[]; + directoryChildren: DirectoryChild[]; relativePrefixToRoot: string; }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/directory/DirectoryWrapper.tsx` around lines 31 - 42, Extract the large inline type used for directoryChildren in DirectoryWrapper.tsx into a named exported type (e.g., DirectoryChild or DirectoryChildrenItem) and replace the inline annotation with that named type; update the prop/type declaration for directoryChildren in DirectoryWrapper and any references to use the new exported type so page templates and tests can import it for reuse (ensure the exported type includes id, name, slug, optional meta.entityType with the specific union, and the optional dm_addressCountryDisplayName/dm_addressRegionDisplayName fields).packages/visual-editor/src/utils/directory/utils.ts (2)
10-13:sortByis not bounded tokeyof T, partially defeating the generic.Making the function generic but accepting
sortBy: stringinstead ofkeyof T & stringmeans callers can still pass a non-existent field and silently fall back to the?? ""default rather than getting a compile-time error.♻️ Proposed fix
-export const sortAlphabetically = <T extends Record<string, any>>( - directoryChildren: T[], - sortBy: string -): T[] => { +export const sortAlphabetically = <T extends Record<string, any>>( + directoryChildren: T[], + sortBy: keyof T & string +): T[] => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/directory/utils.ts` around lines 10 - 13, The function sortAlphabetically<T> accepts sortBy: string which allows passing keys that don't exist on T; change the signature to accept sortBy: keyof T & string so callers are constrained to actual property names of T, then update any callsites to pass a valid key (or cast explicitly if necessary) and keep the existing fallback (the ?? "") logic intact inside sortAlphabetically to preserve runtime safety; this ensures compile-time validation of the property while keeping current behavior.
23-23:Array.prototype.sortmutates the caller's array.
directoryChildren?.sort(sortFn)sorts in place, silently mutating whatever was passed in. Callers may not expect this side-effect.♻️ Proposed fix
- return directoryChildren?.sort(sortFn); + return directoryChildren ? [...directoryChildren].sort(sortFn) : directoryChildren;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/directory/utils.ts` at line 23, The current return uses directoryChildren?.sort(sortFn) which mutates the input array; change it to return a non-mutating sorted copy by making a shallow copy before sorting (e.g. use array spread or Array.from) so callers' arrays aren't mutated—update the expression that references directoryChildren and sortFn to produce and return a copied, sorted array while preserving the undefined/null behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/visual-editor/src/components/directory/DirectoryWrapper.tsx`:
- Around line 31-42: Extract the large inline type used for directoryChildren in
DirectoryWrapper.tsx into a named exported type (e.g., DirectoryChild or
DirectoryChildrenItem) and replace the inline annotation with that named type;
update the prop/type declaration for directoryChildren in DirectoryWrapper and
any references to use the new exported type so page templates and tests can
import it for reuse (ensure the exported type includes id, name, slug, optional
meta.entityType with the specific union, and the optional
dm_addressCountryDisplayName/dm_addressRegionDisplayName fields).
In `@packages/visual-editor/src/utils/directory/utils.ts`:
- Around line 10-13: The function sortAlphabetically<T> accepts sortBy: string
which allows passing keys that don't exist on T; change the signature to accept
sortBy: keyof T & string so callers are constrained to actual property names of
T, then update any callsites to pass a valid key (or cast explicitly if
necessary) and keep the existing fallback (the ?? "") logic intact inside
sortAlphabetically to preserve runtime safety; this ensures compile-time
validation of the property while keeping current behavior.
- Line 23: The current return uses directoryChildren?.sort(sortFn) which mutates
the input array; change it to return a non-mutating sorted copy by making a
shallow copy before sorting (e.g. use array spread or Array.from) so callers'
arrays aren't mutated—update the expression that references directoryChildren
and sortFn to produce and return a copied, sorted array while preserving the
undefined/null behavior.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/visual-editor/src/components/directory/DirectoryWrapper.tsxpackages/visual-editor/src/utils/directory/utils.ts
No description provided.