fix datetime parsing issues, improve primary keys zod validation schema and error messages#204
fix datetime parsing issues, improve primary keys zod validation schema and error messages#204weijietan-fc wants to merge 31 commits intoDouglasNeuroInformatics:mainfrom
Conversation
WalkthroughThis PR centralizes frontend error notifications, changes project add-user routing to use path parameters, updates DATETIME column handling to use dtype-aware ns/us conversions and ns-based summaries, normalizes dataset primaryKeys input/usage, and adds many translation keys for failure messages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
rect rgb(245,255,240)
Note over UI,API: New frontend error flow with notifications
User->>UI: Trigger action (e.g., add manager)
UI->>API: POST /v1/...
API-->>UI: Error
UI->>UI: console.error(error)
UI->>UI: addNotification(t('...Failure'), 'error')
UI->>User: Display notification
end
sequenceDiagram
participant Client
participant OldAPI as Old API (/add-user/:id)
participant NewAPI as New API (/add-user/:projectId/:userEmail)
rect rgb(255,250,240)
Note over Client,OldAPI: Old call used request body for email
Client->>OldAPI: POST /add-user/123 Body: { newUserEmail }
OldAPI-->>Client: Success/Error
end
rect rgb(240,250,255)
Note over Client,NewAPI: New call uses path param for email
Client->>NewAPI: POST /add-user/123/user%40example.com
NewAPI-->>Client: Success/Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
web/src/components/Layout/DesktopSidebar.tsx (1)
1-34: Logo no longer navigates homeReplacing the
<Link>wrapper with a static<div>removes the only direct path back to the home/portal screen from the sidebar. Users now have to hunt through other menu items to reach the landing page, which is a regression from current behavior.Please restore the clickable navigation around the logo (either by reinstating the
Linkor adding an explicitonClick+navigatehandler) so the brand mark still works as a home control.-import { useNavigate } from '@tanstack/react-router'; +import { Link, useNavigate } from '@tanstack/react-router'; @@ - <div className="flex items-center justify-center"> - <Logo className="h-12 w-12 fill-slate-100" /> - </div> + <Link className="flex items-center justify-center" to="/portal"> + <Logo className="h-12 w-12 fill-slate-100" /> + </Link>web/src/features/projects/pages/ViewOneProjectDatasetPage.tsx (1)
42-61: Add missing dependencies to useEffect.The useEffect hook is missing several dependencies that are used inside the effect:
addNotification,t,params.projectId, andparams.datasetId. This can lead to stale closures or unexpected behavior.Apply this diff to fix the dependencies:
}, [columnPaginationDto, rowPaginationDto]); + }, [columnPaginationDto, rowPaginationDto, params.projectId, params.datasetId, addNotification, t]);web/src/features/dataset/pages/EditDatasetInfoPage.tsx (1)
40-59: Add missing dependencies to useCallback.The
handleSubmitcallback is missing several dependencies:addNotification,navigate, andt. This can cause stale closures and unexpected behavior.Apply this diff:
}, - [params.datasetId] + [params.datasetId, addNotification, navigate, t] );web/src/features/projects/pages/ManageProjectUsersPage.tsx (1)
38-52: Fix field name inconsistency.The form field is named
newManagerEmailbut the label and context suggest it should benewProjectUserEmailto match the updated terminology (project user vs manager).Apply this diff:
content={{ - newManagerEmail: { + newProjectUserEmail: { kind: 'string', label: t('newProjectUserEmail'), variant: 'input' } }} submitBtnLabel={t('addUser')} validationSchema={z.object({ - newManagerEmail: z.email() + newProjectUserEmail: z.email() })} onSubmit={(data) => { - addProjectUser(data.newManagerEmail); + addProjectUser(data.newProjectUserEmail); }}
🧹 Nitpick comments (1)
web/src/features/dataset/pages/CreateDatasetPage.tsx (1)
55-55: Remove commented-out code.The commented
.refine()line should be removed to keep the codebase clean.Apply this diff:
} }); -// .refine((val)=> val.datasetType === "TABULAR" && val.hasPrimaryKeys === false && val.primaryKeys === undefined, {error: "Must enter at least 1 primary key because hasPrimaryKeys is set to true."}); type CreateDatasetFormData = z.infer<typeof $CreateDatasetFormValidation>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
api/src/columns/columns.service.ts(2 hunks)api/src/datasets/datasets.service.ts(4 hunks)api/src/projects/projects.controller.ts(1 hunks)api/src/setup/setup.service.ts(1 hunks)core/src/datasets.ts(1 hunks)web/src/components/Layout/DesktopSidebar.tsx(2 hunks)web/src/features/dataset/components/DatasetCard.tsx(2 hunks)web/src/features/dataset/components/DatasetTable.tsx(2 hunks)web/src/features/dataset/components/ManagerCard.tsx(1 hunks)web/src/features/dataset/pages/CreateDatasetPage.tsx(4 hunks)web/src/features/dataset/pages/EditDatasetInfoPage.tsx(1 hunks)web/src/features/dataset/pages/ManageDatasetManagersPage.tsx(1 hunks)web/src/features/dataset/pages/ViewOneDatasetPage.tsx(3 hunks)web/src/features/projects/components/ProjectSelectColumns.tsx(1 hunks)web/src/features/projects/components/UserCard.tsx(1 hunks)web/src/features/projects/pages/AddProjectDatasetPage.tsx(2 hunks)web/src/features/projects/pages/EditProjectInfoPage.tsx(1 hunks)web/src/features/projects/pages/ManageProjectUsersPage.tsx(2 hunks)web/src/features/projects/pages/SelectProjectDatasetColumnsPage.tsx(2 hunks)web/src/features/projects/pages/ViewOneProjectDatasetPage.tsx(2 hunks)web/src/features/projects/pages/ViewOneProjectPage.tsx(3 hunks)web/src/translations/common.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
web/src/features/projects/pages/AddProjectDatasetPage.tsx (1)
core/src/datasets.ts (1)
$DatasetCardProps(80-80)
web/src/features/projects/pages/SelectProjectDatasetColumnsPage.tsx (1)
core/src/projects.ts (1)
$ProjectColumnSummary(164-164)
web/src/features/projects/pages/ViewOneProjectPage.tsx (1)
core/src/datasets.ts (1)
$DatasetCardProps(80-80)
web/src/features/dataset/components/DatasetCard.tsx (1)
core/src/licenses.ts (1)
licensesObjects(52-52)
web/src/features/dataset/pages/CreateDatasetPage.tsx (2)
core/src/datasets.ts (1)
$DatasetLicenses(82-82)core/src/licenses.ts (1)
$DatasetLicenses(49-49)
web/src/components/Layout/DesktopSidebar.tsx (1)
web/src/components/Logo.tsx (1)
Logo(3-22)
api/src/projects/projects.controller.ts (1)
api/src/core/decorators/route-access.decorator.ts (1)
RouteAccess(13-15)
web/src/features/dataset/components/DatasetTable.tsx (1)
web/src/services/react-query.ts (1)
queryClient(3-12)
🔇 Additional comments (14)
web/src/features/dataset/components/DatasetCard.tsx (1)
1-1: LGTM!The new imports are properly structured and necessary for the tooltip functionality.
Also applies to: 3-3, 6-6
web/src/features/dataset/pages/ManageDatasetManagersPage.tsx (1)
28-34: LGTM! Enhanced error handling with user notification.The error handling now provides user feedback via notification, improving the UX compared to silent console logging.
web/src/features/projects/pages/ViewOneProjectDatasetPage.tsx (2)
52-58: LGTM! User-facing error notification added.The error handling now displays a localized error message to the user, improving the experience when dataset fetch fails.
75-81: LGTM! User-facing error notification added.The error handling now displays a localized error message to the user, improving the experience when dataset deletion fails.
web/src/features/projects/pages/SelectProjectDatasetColumnsPage.tsx (1)
37-43: LGTM! User-facing error notification added.The error handling now displays a localized error message when fetching dataset columns fails.
web/src/features/dataset/pages/EditDatasetInfoPage.tsx (1)
50-56: LGTM! User-facing error notification added.The error handling now provides user feedback when dataset info update fails.
web/src/features/projects/pages/ManageProjectUsersPage.tsx (2)
17-34: LGTM! API integration updated correctly.The function rename, API endpoint update, and error notification are all correctly implemented and align with the backend changes.
48-48: LGTM! Correct Zod v4 syntax.Using
z.email()as a standalone validator is the correct syntax for Zod 4.Based on library documentation
web/src/features/dataset/pages/CreateDatasetPage.tsx (3)
17-54: LGTM! Validation schema correctly updated for optional primary keys.The schema now makes
hasPrimaryKeysandprimaryKeysoptional, with custom validation logic via.check()to enforce requirements for TABULAR datasets. This is the correct Zod 4 approach.Based on library documentation
83-90: LGTM! Safe handling of optional fields.The optional chaining for
primaryKeysand conditional file upload are both correct approaches for handling potentially undefined values.
34-53: Verify validation logic for edge cases.The custom validation has specific rules for TABULAR datasets. Ensure the validation correctly handles:
- When
datasetTypeis 'BASE' (should skip all primary key checks)- When
hasPrimaryKeysisfalsefor TABULAR (should allow empty primaryKeys)- When
hasPrimaryKeysistruebut primaryKeys array is empty vs undefinedThe current logic only checks for
undefined, but an empty array[]would pass validation. Consider whether this is intended behavior:// Current: allows empty array when hasPrimaryKeys is true if (ctx.value.hasPrimaryKeys === true && ctx.value.primaryKeys === undefined) { // This won't catch primaryKeys = [] } // Possible fix if empty array should be invalid: if (ctx.value.hasPrimaryKeys === true && (!ctx.value.primaryKeys || ctx.value.primaryKeys.length === 0)) { ctx.issues.push({ code: 'custom', input: ctx.value.primaryKeys, message: 'Must enter at least 1 primary key because hasPrimaryKeys is set to true.' }); }web/src/translations/common.json (3)
67-67: LGTM! Typo fixed.Corrected "New Manage Email" to "New Manager Email".
70-73: LGTM! New translation key added.The
newProjectUserEmailtranslation key supports the updated terminology in the project user management flow.
370-457: LGTM! Comprehensive error message translations added.All new error message keys have both English and French translations and follow a consistent naming convention. These keys support the user-facing error notifications added throughout the application.
| type: 'success' | ||
| }); | ||
| }; | ||
| const handleSetColumnMetadataPermissionLevel = useDestructiveAction( |
There was a problem hiding this comment.
consider using mutations with tanstack query here. You can have an onSuccess callback to then invalidate and keep all the API calls in the same place. Example:
export function useCreateInstrumentRecordMutation() {
const queryClient = useQueryClient();
return useMutation({
mutationFn: ({ data }: { data: $CreateInstrumentRecordData }) => {
return axios.post<$InstrumentRecord>('/v1/instrument-records', data);
},
onSuccess: async () => {
await queryClient.invalidateQueries({ queryKey: getInstrumentRecordsQueryKey() });
await queryClient.invalidateQueries({ queryKey: getProjectQueryKey() });
}
});
}| void navigate({ to: `/portal/datasets/${params.datasetId}` }); | ||
| }) | ||
| .catch(console.error); | ||
| .catch((error) => { |
There was a problem hiding this comment.
same here for the mutations. Then tanstack will automatically log
…ng converted to 1970-01-01
…uery upon changes to the dataset
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/features/projects/components/UserCard.tsx (1)
33-35: Rename the parameter to match its usage.The function parameter
userIdToRemoveis misleading because the URL construction usesuserIdfrom props, not the parameter. Since the function is called withuserId(line 70), the parameter should be renamed for clarity.Apply this diff:
- const removeManager = (userIdToRemove: string) => { + const removeManager = (userIdToRemove: string) => { axios - .delete(`/v1/projects/remove-user/${projectId}/${userId}`) + .delete(`/v1/projects/remove-user/${projectId}/${userIdToRemove}`) .then(() => {
♻️ Duplicate comments (1)
api/src/columns/columns.service.ts (1)
893-899: Critical: Method call syntax error and unguarded null divisionLine 894 calls
currSeries.lenas a property, butlen()is a method. Also, even when the null-count check passes,max()andmin()can still returnnullfor all-null series, and dividingnullby 1,000,000 yields0, producing epoch dates in summaries.Apply this fix:
datetimeSummary: - currSeries.len === currSeries.nullCount + currSeries.len() === currSeries.nullCount() ? null : { - max: new Date(Math.floor(currSeries.cast(pl.Datetime('ns'), true).max() / 1000000)), - min: new Date(Math.floor(currSeries.cast(pl.Datetime('ns'), true).min() / 1000000)) + max: (() => { + const val = currSeries.cast(pl.Datetime('ns'), true).max(); + return val == null ? null : new Date(Math.floor(Number(val) / 1_000_000)); + })(), + min: (() => { + const val = currSeries.cast(pl.Datetime('ns'), true).min(); + return val == null ? null : new Date(Math.floor(Number(val) / 1_000_000)); + })() },
🧹 Nitpick comments (4)
web/src/features/projects/pages/ManageProjectUsersPage.tsx (2)
17-34: Consider improving error logging and providing more specific error feedback.The enhanced error handling is a good addition. However, consider these improvements:
- Line 28:
console.errormay not be appropriate for production. Consider using a proper logging service or removing it.- The error notification uses a generic message (
addProjectUserFailure). Consider extracting and displaying specific error details from the API response when available (e.g.,error.response?.data?.message).Optional enhancement for better error feedback:
.catch((error) => { - console.error(error); addNotification({ - message: t('addProjectUserFailure'), + message: error.response?.data?.message ?? t('addProjectUserFailure'), type: 'error' }); });
38-53: Consider renaming the form field for consistency.The field is named
newManagerEmail(line 40) while the label usest('newProjectUserEmail')(line 42) and the function isaddProjectUser. For consistency and maintainability, consider renaming the field to match the updated terminology.Apply this diff to align the field name with the updated terminology:
content={{ - newManagerEmail: { + newProjectUserEmail: { kind: 'string', label: t('newProjectUserEmail'), variant: 'input' } }} submitBtnLabel={t('addUser')} validationSchema={z.object({ - newManagerEmail: z.email() + newProjectUserEmail: z.email() })} onSubmit={(data) => { - addProjectUser(data.newManagerEmail); + addProjectUser(data.newProjectUserEmail); }}Optional: Consider adding loading state management to improve UX and prevent double submissions during the async operation.
web/src/features/projects/components/UserCard.tsx (2)
27-31: Add error handling for the user fetch.The
useEffectlacks error handling. If the user fetch fails, the component renders nothing without informing the user or logging the error.Apply this diff:
useEffect(() => { - void axios.get<User>(`/v1/users/${userId}`).then((response) => { - setUser(response.data); - }); + void axios + .get<User>(`/v1/users/${userId}`) + .then((response) => { + setUser(response.data); + }) + .catch((error) => { + console.error(error); + addNotification({ + message: t('fetchUserFailure'), + type: 'error' + }); + }); }, [projectId, userId]);
70-70: Use strict equality.Prefer
===over==for type-safe comparison.Apply this diff:
- <Button disabled={userNumber == 1} variant={'primary'} onClick={() => removeManager(userId)}> + <Button disabled={userNumber === 1} variant={'primary'} onClick={() => removeManager(userId)}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/src/columns/columns.service.ts(3 hunks)api/src/tabular-data/tabular-data.service.ts(1 hunks)web/src/features/dataset/components/DatasetTable.tsx(2 hunks)web/src/features/dataset/pages/ViewOneDatasetPage.tsx(6 hunks)web/src/features/projects/components/UserCard.tsx(2 hunks)web/src/features/projects/pages/ManageProjectUsersPage.tsx(2 hunks)web/src/routes/portal/datasets/$datasetId.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/features/dataset/components/DatasetTable.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
web/src/routes/portal/datasets/$datasetId.tsx (2)
web/src/routes/public/dataset.$datasetId.tsx (1)
Route(37-63)web/src/services/react-query.ts (1)
queryClient(3-12)
web/src/features/dataset/pages/ViewOneDatasetPage.tsx (1)
web/src/features/dataset/components/DatasetTable.tsx (1)
DatasetTable(15-322)
🔇 Additional comments (5)
web/src/features/projects/pages/ManageProjectUsersPage.tsx (1)
47-49: LGTM! Correct Zod v4 validation usage.The change from
z.string().email()toz.email()is the correct migration path for Zod v4, which introduces standalone string format schemas.web/src/features/dataset/pages/ViewOneDatasetPage.tsx (2)
70-76: Excellent: Replaced silent failures with user-facing error notificationsThe addition of explicit error handlers that log to console and notify users via the notification store is a significant improvement over the previous silent catch blocks. Users will now see localized error messages when downloads or sharing operations fail.
Also applies to: 86-92, 105-111
27-27: LGTM: queryKey propagation enables consistent cache invalidationThe queryKey prop is correctly threaded from the route layer through ViewOneDatasetPage down to DatasetTable, enabling stable cache keys for query invalidation after mutations.
Also applies to: 37-37, 210-210
web/src/features/projects/components/UserCard.tsx (2)
43-49: Good addition of user-facing error notification.The error handling now properly informs the user when removal fails, which improves the user experience. The use of a translated message key is appropriate for internationalization.
59-59: Label formatting update looks intentional.The removal of colons from the user field labels appears to be an intentional UI change. The new format uses a space between the label and value.
Also applies to: 62-62, 65-65
| let datetimeDataArray: { value: Date | null }[]; | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access | ||
| if (colSeries.dtype.toString() === pl.Datetime('us').toString()) { | ||
| datetimeDataArray = dataArray.map((entry) => { | ||
| return { | ||
| value: entry.value === undefined || entry.value === null ? new Date(Math.floor(entry.value / 1000)) : null | ||
| }; | ||
| }); | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access | ||
| } else if (colSeries.dtype.toString() === pl.Datetime('ns').toString()) { | ||
| datetimeDataArray = dataArray.map((entry) => { | ||
| return { | ||
| value: | ||
| entry.value === undefined || entry.value === null ? new Date(Math.floor(entry.value / 1000000)) : null | ||
| }; | ||
| }); | ||
| } else { | ||
| datetimeDataArray = dataArray; | ||
| } |
There was a problem hiding this comment.
Critical: Inverted null-handling logic corrupts all datetime data
The ternary condition is backwards. Lines 172 and 180 say "if value is null/undefined, THEN divide it and create a Date, ELSE return null." This inverts the entire dataset:
- Null timestamps →
new Date(Math.floor(null / 1000))→new Date(0)→ 1970-01-01 - Valid numeric timestamps →
null
This was flagged in previous reviews but the fix applied the opposite logic.
Apply this fix to correct the condition:
if (colSeries.dtype.toString() === pl.Datetime('us').toString()) {
datetimeDataArray = dataArray.map((entry) => {
return {
- value: entry.value === undefined || entry.value === null ? new Date(Math.floor(entry.value / 1000)) : null
+ value: entry.value === undefined || entry.value === null ? null : new Date(Math.floor(entry.value / 1000))
};
});
} else if (colSeries.dtype.toString() === pl.Datetime('ns').toString()) {
datetimeDataArray = dataArray.map((entry) => {
return {
value:
- entry.value === undefined || entry.value === null ? new Date(Math.floor(entry.value / 1000000)) : null
+ entry.value === undefined || entry.value === null ? null : new Date(Math.floor(entry.value / 1000000))
};
});
} else {🤖 Prompt for AI Agents
In api/src/columns/columns.service.ts around lines 167 to 185 the null-handling
is inverted: the ternary currently treats null/undefined values as valid
timestamps and converts them, and valid numeric timestamps become null. Change
the condition so you convert only when entry.value is present (e.g. entry.value
!== undefined && entry.value !== null or entry.value != null) and return null
when the input is null/undefined; keep the existing divisors (divide by 1000 for
'us' and 1000000 for 'ns') so that present numeric timestamps are converted to
Date(...) and absent ones remain null.
| datetimeColumnValidation: { | ||
| max: new Date(), | ||
| min: '1970-01-01' | ||
| min: new Date() |
There was a problem hiding this comment.
🧩 Analysis chain
Verify appropriateness of new Date() as validation minimum
Using new Date() (current timestamp) as the minimum datetime validation constraint means any datetime value before "now" will fail validation. This likely prevents importing historical data or backdated records. Consider whether a sentinel far-past date (e.g., new Date('1900-01-01')) or null (no constraint) would be more appropriate.
Run this script to check if other datetime validations in the codebase use dynamic or static bounds:
🏁 Script executed:
#!/bin/bash
# Search for datetime validation patterns
rg -nP --type=ts 'datetimeColumnValidation|Datetime.*min|Datetime.*max' -C2Length of output: 1277
I need to understand the context around line 727 better. Let me check which function contains this code and what it's trying to accomplish.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get broader context around lines 720-730 to see the function and its purpose
sed -n '705,740p' api/src/columns/columns.service.ts
Length of output: 1136
Let me check how this validation is actually used and what the intended behavior should be:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for how datetimeColumnValidation is used in the codebase
rg -nP --type=ts 'datetimeColumnValidation' -C3
Length of output: 1096
Let me search more broadly to find where this validation is actually consumed:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for where datetime validation is read/checked, not just assigned
rg -nP --type=ts '\.datetimeColumnValidation|datetimeColumnValidation\s*[^=:]|datetimeColumnValidation\s*:' -C2 | head -50
Length of output: 733
Let me examine the context around lines 897-898 to understand the correct pattern:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get context around lines 890-905 to see the alternative datetime validation pattern
sed -n '880,910p' api/src/columns/columns.service.ts
Length of output: 973
Fix datetime validation bounds at line 727
The validation at line 727 sets both min and max to new Date() (the current timestamp), which means only datetime values at that exact millisecond would pass validation. This prevents importing historical data or any value not exactly "now".
Since data is available at line 721 after casting, compute the bounds from the actual series like the summary calculation does (e.g., min: new Date(Math.floor(currSeries.cast(pl.Datetime('ns'), true).min() / 1000000))) or set validation to null for no constraint.
🤖 Prompt for AI Agents
In api/src/columns/columns.service.ts around line 727 the datetime validation
incorrectly sets both min and max to new Date() (now) which only allows the
exact current millisecond; use the already-cast series available around line 721
to compute proper bounds or remove constraints: compute min as new
Date(Math.floor(currSeries.cast(pl.Datetime('ns'), true).min() / 1000000)) and
max similarly from .max(), or set min/max to null to disable bounds, and assign
those values to the validation object instead of new Date().
| rows[i][col.name] = 'Hidden'; | ||
| } else { | ||
| rows[i][col.name] = entry.value.$date ? new Date(entry.value.$date).toDateString() : null; | ||
| rows[i][col.name] = entry.value ? new Date(entry.value.$date!).toDateString() : null; |
There was a problem hiding this comment.
Non-null assertion risks runtime failure if $date is missing
The condition checks entry.value for truthiness, but then uses a non-null assertion (entry.value.$date!) without verifying that $date exists. If entry.value is a truthy object but $date is undefined, this will produce an invalid Date.
Replace the non-null assertion with an explicit check:
- rows[i][col.name] = entry.value ? new Date(entry.value.$date!).toDateString() : null;
+ rows[i][col.name] = entry.value?.$date ? new Date(entry.value.$date).toDateString() : null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rows[i][col.name] = entry.value ? new Date(entry.value.$date!).toDateString() : null; | |
| rows[i][col.name] = entry.value?.$date ? new Date(entry.value.$date).toDateString() : null; |
🤖 Prompt for AI Agents
In api/src/tabular-data/tabular-data.service.ts around line 344, the code uses a
non-null assertion on entry.value.$date which can throw if $date is missing;
change the logic to explicitly check that entry.value and entry.value.$date are
defined before constructing a Date and calling toDateString, returning null
otherwise, and ensure you handle both numeric and string date representations
safely (e.g., only call new Date(...) when $date is present and valid).
2 participants
close issues #202 #201 #193 #195 #197
Summary by CodeRabbit
New Features
Improvements
UI Changes