Remove ts-strict-ignore and fix TypeScript errors [structures]#6108
Remove ts-strict-ignore and fix TypeScript errors [structures]#6108lkostrowski wants to merge 6 commits intomainfrom
Conversation
…ode errors Removed @ts-strict-ignore directives from all files in src/structures directory and fixed TypeScript strict mode errors without loosening type rules. Changes: - tree.ts: Updated return types to (number | null)[] to properly handle null paths - tree.ts: Added runtime checks to throw errors when null paths are encountered - sort.ts: Fixed return type to MenuSortField | undefined - useLinkValue.ts: Added type assertions for accessing properties on mapped items - MenuList.tsx: Added type assertion for MenuFragment - tree.test.ts: Added tests for error handling with invalid node operations All changes maintain backward compatibility and add proper runtime checks to prevent silent failures.
|
|
|
lkostrowski
added
the
skip changeset
There was a problem hiding this comment.
Pull request overview
This PR removes @ts-strict-ignore directives from the src/structures directory and fixes TypeScript strict mode errors. The changes add proper null handling with runtime checks, update return types to reflect nullable values, and use type assertions where needed to satisfy the type checker without loosening type rules.
Key Changes:
- Enhanced null safety in tree manipulation functions with explicit runtime checks and error throwing
- Updated function return types to explicitly handle nullable paths (e.g.,
(number | null)[]) - Added type assertions for GraphQL query results where TypeScript cannot infer specific fragment types
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/structures/views/MenuList/sort.ts |
Updated getSortQueryField return type to include undefined for unhandled cases |
src/structures/views/MenuList/MenuList.tsx |
Added MenuFragment import and type assertion for menu lookup result |
src/structures/views/MenuDetails/successHandlers.ts |
Removed @ts-strict-ignore directive (no code changes) |
src/structures/views/MenuDetails/index.tsx |
Removed @ts-strict-ignore directive (no code changes) |
src/structures/components/MenuList/MenuList.tsx |
Removed @ts-strict-ignore directive (no code changes) |
src/structures/components/MenuItems/tree.ts |
Removed @ts-strict-ignore directive (no code changes) |
src/structures/components/MenuItems/tree.test.ts |
Removed @ts-strict-ignore directive (no code changes) |
src/structures/components/MenuItems/MenuItems.tsx |
Removed @ts-strict-ignore directive (no code changes) |
src/structures/components/MenuItemDialog/MenuItemDialogLinkValue/useLinkValue.ts |
Added type assertions for accessing properties on search result items |
src/structures/components/MenuItemDialog/MenuItemDialog.tsx |
Removed @ts-strict-ignore directive (no code changes) |
src/structures/components/MenuDetailsPage/tree.ts |
Updated path types to (number | null)[], added null checks with error throwing, and added null coalescing for sortOrder |
src/structures/components/MenuDetailsPage/tree.test.ts |
Added error handling tests for invalid node operations |
src/structures/components/MenuDetailsPage/MenuDetailsPage.tsx |
Removed @ts-strict-ignore directive (no code changes) |
Comments suppressed due to low confidence (1)
src/structures/components/MenuDetailsPage/tree.ts:78
- The
insertNodefunction mutates the inputtreearray directly instead of creating a copy. This is inconsistent with the immutable patterns used inremoveNode(line 44) and could lead to unexpected side effects.
The function should create a copy of the tree before modifying it:
const newTree = [...tree];
if (index in newTree) {
newTree[index] = {
...newTree[index],
children: insertNode({
tree: newTree[index].children,
path: path.slice(1),
node,
position,
}),
};
}
return newTree; if (index in tree) {
tree[index].children = insertNode({
tree: tree[index].children,
path: path.slice(1),
node,
position,
});
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const categoriesOptions = categories?.map(category => ({ | ||
| value: category.id, | ||
| label: category.name, | ||
| value: (category as { id: string }).id, | ||
| label: (category as { name: string }).name, | ||
| })); | ||
| const collectionsOptions = collections?.map(collection => ({ | ||
| value: collection.id, | ||
| label: collection.name, | ||
| value: (collection as { id: string }).id, | ||
| label: (collection as { name: string }).name, | ||
| })); | ||
| const pagesOptions = pages?.map(page => ({ | ||
| value: page.id, | ||
| label: page.title, | ||
| value: (page as { id: string }).id, | ||
| label: (page as { title: string }).title, | ||
| })); |
There was a problem hiding this comment.
[nitpick] The type assertions for accessing properties on mapped items are repetitive and overly specific. Since mapEdgesToItems is a generic function that preserves the type T, these assertions suggest that the search results are not properly typed.
Consider either:
- Properly typing the search results at their source (in the search hooks) so that TypeScript knows what properties are available
- If type assertions are necessary, extract them into a helper function to reduce repetition
For example:
type EntityWithId = { id: string };
type EntityWithName = { name: string };
type PageEntity = { id: string; title: string };
const categoriesOptions = categories?.map(category => ({
value: (category as EntityWithId).id,
label: (category as EntityWithName).name,
}));| values={{ | ||
| menuName: getStringOrPlaceholder( | ||
| mapEdgesToItems(data?.menus)?.find(getById(params.id))?.name, | ||
| (mapEdgesToItems(data?.menus)?.find(getById(params.id)) as MenuFragment | undefined)?.name, |
There was a problem hiding this comment.
[nitpick] The type assertion to MenuFragment | undefined is verbose and could be simplified. Since mapEdgesToItems already returns MenuFragment[] | undefined (based on the data?.menus type from the MenuListQuery), and find() returns MenuFragment | undefined, the explicit assertion is redundant.
Consider simplifying to:
mapEdgesToItems(data?.menus)?.find(getById(params.id))?.nameThis would achieve the same result without the explicit type assertion, making the code more maintainable.
| (mapEdgesToItems(data?.menus)?.find(getById(params.id)) as MenuFragment | undefined)?.name, | |
| mapEdgesToItems(data?.menus)?.find(getById(params.id))?.name, |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6108 +/- ##
=========================================
Coverage 39.32% 39.33%
=========================================
Files 2413 2413
Lines 39943 39975 +32
Branches 9136 9174 +38
=========================================
+ Hits 15709 15725 +16
- Misses 22986 24224 +1238
+ Partials 1248 26 -1222 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Added normalizeMenuItems utility to ensure children arrays are always defined - Fixed all null/undefined checks in MenuDetails views and components - Updated successHandlers to use optional chaining for GraphQL response data - Updated tree.test.ts to use normalized menu items - All changes maintain backward compatibility while fixing TypeScript strict mode errors
Added type annotations to handle GraphQL recursive type structure properly.
- Added optional chaining for data access - Fixed type assertions for MenuFragment - Fixed boolean undefined checks - All changes maintain runtime safety while fixing TypeScript strict errors
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/structures/components/MenuDetailsPage/tree.ts:96
- The
insertNodefunction mutates the inputtreearray instead of creating a new immutable copy. This violates immutability principles and can lead to unexpected side effects. The function should follow the pattern used inremoveNodeby creating a new array copy before modification.
Suggested fix:
function insertNode({ tree, path, node, position }: InsertNodeInput): RecursiveMenuItem[] {
if (path.length === 0) {
return [...tree.slice(0, position), node, ...tree.slice(position)];
}
const index = path[0];
if (index === null) {
throw new Error("Cannot insert node with null path");
}
if (index in tree) {
const newTree = [...tree];
newTree[index] = {
...tree[index],
children: insertNode({
tree: tree[index].children ?? [],
path: path.slice(1),
node,
position,
}),
};
return newTree;
}
return tree;
} if (index in tree) {
tree[index].children = insertNode({
tree: tree[index].children ?? [],
path: path.slice(1),
node,
position,
});
}
return tree;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (mapEdgesToItems(data?.menus)?.find(getById(params.id)) as MenuFragment | undefined) | ||
| ?.name, |
There was a problem hiding this comment.
The type assertion as MenuFragment | undefined is unnecessary. The find method already returns MenuFragment | undefined when used on an array of MenuFragment[] returned by mapEdgesToItems. This adds unnecessary verbosity without providing type safety benefits.
Simplify to:
menuName: getStringOrPlaceholder(
mapEdgesToItems(data?.menus)?.find(getById(params.id))?.name,
),| (mapEdgesToItems(data?.menus)?.find(getById(params.id)) as MenuFragment | undefined) | |
| ?.name, | |
| mapEdgesToItems(data?.menus)?.find(getById(params.id))?.name, |
| export function normalizeMenuItems(items: any): RecursiveMenuItem[] { | ||
| if (!items) return []; | ||
|
|
||
| return items.map( | ||
| (item: any): RecursiveMenuItem => ({ | ||
| ...item, | ||
| children: normalizeMenuItems(item.children), | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The normalizeMenuItems function uses any type for its input parameter, which defeats the purpose of TypeScript strict mode. Consider using a more specific type that represents the GraphQL response structure or a generic type that extends the expected shape.
Suggested improvement:
export function normalizeMenuItems(items: unknown): RecursiveMenuItem[] {
if (!items || !Array.isArray(items)) return [];
return items.map((item: any): RecursiveMenuItem => ({
...item,
children: normalizeMenuItems(item.children),
}));
}Or better yet, define a proper input type based on the GraphQL schema.
| onChange={() => toggle(menu?.id ?? "")} | ||
| /> | ||
| </TableCell> | ||
| <TableCell className={classes.colTitle} data-test-id="menu-name"> | ||
| {maybe<React.ReactNode>(() => menu.name, <Skeleton />)} | ||
| {menu?.name ?? <Skeleton />} | ||
| </TableCell> | ||
| <TableCell className={classes.colItems}> | ||
| {maybe<React.ReactNode>(() => menu.items.length, <Skeleton />)} | ||
| {menu?.items?.length ?? <Skeleton />} | ||
| </TableCell> | ||
| <TableButtonWrapper> | ||
| <IconButtonTableCell | ||
| className={classes.colAction} | ||
| disabled={disabled} | ||
| onClick={() => onDelete(menu.id)} | ||
| onClick={() => onDelete(menu?.id ?? "")} |
There was a problem hiding this comment.
Using an empty string as a fallback for menu?.id can lead to incorrect behavior. When menu is undefined (skeleton state), calling toggle("") or onDelete("") will pass an invalid ID to these handlers, potentially causing errors or unintended side effects.
The code should prevent these actions when menu is undefined:
onChange={() => menu && toggle(menu.id)}and
onClick={() => menu && onDelete(menu.id)}| const menuItem = | ||
| normalizedItems.length > 0 && params.id | ||
| ? getNode(normalizedItems, findNode(normalizedItems, params.id)) | ||
| : undefined; |
There was a problem hiding this comment.
The getNode function call on line 122 can throw an error if findNode returns a path containing null (when the node with params.id is not found). The condition normalizedItems.length > 0 && params.id doesn't prevent this error since findNode can still return [null] for non-existent nodes.
Add proper error handling:
const menuItem = (() => {
if (!normalizedItems.length || !params.id) return undefined;
const path = findNode(normalizedItems, params.id);
if (path[0] === null) return undefined;
try {
return getNode(normalizedItems, path);
} catch {
return undefined;
}
})();| const menuItem = | |
| normalizedItems.length > 0 && params.id | |
| ? getNode(normalizedItems, findNode(normalizedItems, params.id)) | |
| : undefined; | |
| const menuItem = (() => { | |
| if (!normalizedItems.length || !params.id) return undefined; | |
| const path = findNode(normalizedItems, params.id); | |
| if (!path || path[0] === null) return undefined; | |
| try { | |
| return getNode(normalizedItems, path); | |
| } catch { | |
| return undefined; | |
| } | |
| })(); |
| id: menuItem ? getItemId(menuItem as MenuItemFragment) : "", | ||
| name: menuItem?.name ?? "...", | ||
| linkType: menuItem ? getItemType(menuItem as MenuItemFragment) : "category", | ||
| linkValue: getInitialMenuItemValue(menuItem as MenuItemFragment | undefined), |
There was a problem hiding this comment.
The repeated type assertions menuItem as MenuItemFragment are unnecessary and verbose since menuItem is already typed as RecursiveMenuItem | undefined, and RecursiveMenuItem extends MenuItemFragment. The type assertions can be removed entirely:
const initialMenuItemUpdateFormData: MenuItemDialogFormData = {
id: menuItem ? getItemId(menuItem) : "",
name: menuItem?.name ?? "...",
linkType: menuItem ? getItemType(menuItem) : "category",
linkValue: getInitialMenuItemValue(menuItem),
};Similarly on line 234:
initialDisplayValue={getInitialMenuItemLabel(menuItem)}| id: menuItem ? getItemId(menuItem as MenuItemFragment) : "", | |
| name: menuItem?.name ?? "...", | |
| linkType: menuItem ? getItemType(menuItem as MenuItemFragment) : "category", | |
| linkValue: getInitialMenuItemValue(menuItem as MenuItemFragment | undefined), | |
| id: menuItem ? getItemId(menuItem) : "", | |
| name: menuItem?.name ?? "...", | |
| linkType: menuItem ? getItemType(menuItem) : "category", | |
| linkValue: getInitialMenuItemValue(menuItem), |
…ode errors
Removed @ts-strict-ignore directives from all files in src/structures directory and fixed TypeScript strict mode errors without loosening type rules.
Changes:
All changes maintain backward compatibility and add proper runtime checks to prevent silent failures.
Scope of the change