fix: treat filter datetime inputs as local; convert to UTC on apply #2571
Conversation
ConsoleProject ID: Tip Appwrite has a Discord community with over 16 000 members. |
WalkthroughThis pull request adds consistent datetime value normalization across two filter components. The changes standardize how datetime values are initialized, processed, and stored by introducing conversion logic that transforms between local datetime formats and ISO strings. Specifically, datetime initialization now uses a helper function, and condition handling in both components converts datetime values to ISO strings before storage and filtering, ensuring uniform datetime format handling. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/filters/filtersModal.svelte (1)
59-79: Missing datetime initialization logic.Unlike
content.svelte(lines 81-84), this component doesn't initialize datetime values when a datetime column is selected. This creates inconsistent UX between the two filter interfaces.Consider adding initialization logic when the column changes:
$: if (column?.type === 'datetime' && value === null) { const now = new Date(); value = toLocalDateTimeISO(now.toISOString()).slice(0, 16); }Note: You'll need to import
toLocalDateTimeISOfrom'$lib/helpers/date'.
🧹 Nitpick comments (1)
src/lib/components/filters/filtersModal.svelte (1)
60-63: Consider extracting datetime conversion logic.The datetime conversion logic is duplicated between this file and
content.svelte(lines 112-115). Consider extracting it to a shared utility function in$lib/helpers/dateto ensure consistency and maintainability.Example utility function:
// In src/lib/helpers/date.ts export function prepareFilterDateTimeValue(value: any): any { if (typeof value === 'string' && value) { const date = new Date(value); if (!isNaN(date.getTime())) { return date.toISOString(); } } return value; }Then use it in both files:
const preparedValue = column?.type === 'datetime' ? prepareFilterDateTimeValue(value) : value;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/filters/content.svelte(3 hunks)src/lib/components/filters/filtersModal.svelte(2 hunks)
⏰ 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: e2e
- GitHub Check: build
🔇 Additional comments (2)
src/lib/components/filters/content.svelte (2)
81-84: Good datetime initialization.The initialization correctly converts the current time to local timezone and formats it for the datetime-local input.
17-17: No issues found. ThetoLocalDateTimeISOfunction implementation is correct.The function properly converts UTC ISO strings to local datetime format by adjusting for timezone offset, and handles edge cases (invalid dates return 'n/a', DST is accounted for via
getTimezoneOffset()). The component usage is also correct.
| const preparedValue = | ||
| column?.type === 'datetime' && typeof value === 'string' && value | ||
| ? new Date(value).toISOString() | ||
| : value; | ||
| addFilter(columnsArray, columnId, operatorKey, preparedValue, arrayValues); |
There was a problem hiding this comment.
Add error handling for datetime conversion.
The conversion from datetime-local string to UTC ISO lacks error handling. If value contains an invalid datetime string, new Date(value) returns an Invalid Date object, and calling toISOString() on it will throw a TypeError, potentially crashing the filter application.
Apply this diff to add error handling:
- const preparedValue =
- column?.type === 'datetime' && typeof value === 'string' && value
- ? new Date(value).toISOString()
- : value;
+ let preparedValue = value;
+ if (column?.type === 'datetime' && typeof value === 'string' && value) {
+ const date = new Date(value);
+ if (!isNaN(date.getTime())) {
+ preparedValue = date.toISOString();
+ }
+ }
addFilter(columnsArray, columnId, operatorKey, preparedValue, arrayValues);📝 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.
| const preparedValue = | |
| column?.type === 'datetime' && typeof value === 'string' && value | |
| ? new Date(value).toISOString() | |
| : value; | |
| addFilter(columnsArray, columnId, operatorKey, preparedValue, arrayValues); | |
| let preparedValue = value; | |
| if (column?.type === 'datetime' && typeof value === 'string' && value) { | |
| const date = new Date(value); | |
| if (!isNaN(date.getTime())) { | |
| preparedValue = date.toISOString(); | |
| } | |
| } | |
| addFilter(columnsArray, columnId, operatorKey, preparedValue, arrayValues); |
🤖 Prompt for AI Agents
In src/lib/components/filters/content.svelte around lines 112–116, the
conversion new Date(value).toISOString() can throw for invalid datetime strings;
wrap the conversion in a try/catch (or construct the Date and check
isNaN(date.getTime())) and only call toISOString() when the Date is valid; on
invalid input, avoid throwing by either leaving preparedValue as the original
value, set preparedValue to null/undefined to skip the filter, and/or surface a
user-friendly validation error before calling addFilter so the filter
application doesn't crash.
| const preparedValue = | ||
| column?.type === 'datetime' && typeof value === 'string' && value | ||
| ? new Date(value).toISOString() | ||
| : value; |
There was a problem hiding this comment.
Add error handling for datetime conversion.
This conversion logic has the same issue as in content.svelte: if value contains an invalid datetime string, new Date(value).toISOString() will throw a TypeError.
Apply this diff to add error handling:
- const preparedValue =
- column?.type === 'datetime' && typeof value === 'string' && value
- ? new Date(value).toISOString()
- : value;
+ let preparedValue = value;
+ if (column?.type === 'datetime' && typeof value === 'string' && value) {
+ const date = new Date(value);
+ if (!isNaN(date.getTime())) {
+ preparedValue = date.toISOString();
+ }
+ }
const newTag = generateTag(selectedColumn, operatorKey, preparedValue || arrayValues);📝 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.
| const preparedValue = | |
| column?.type === 'datetime' && typeof value === 'string' && value | |
| ? new Date(value).toISOString() | |
| : value; | |
| let preparedValue = value; | |
| if (column?.type === 'datetime' && typeof value === 'string' && value) { | |
| const date = new Date(value); | |
| if (!isNaN(date.getTime())) { | |
| preparedValue = date.toISOString(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/lib/components/filters/filtersModal.svelte around lines 60 to 63, the
datetime conversion uses new Date(value).toISOString() which can throw for
invalid strings; wrap the conversion in a try/catch (or validate the Date by
checking isNaN(date.getTime())) and only call toISOString() on a valid Date,
otherwise fall back to the original value (or null/empty as appropriate) and
optionally log or ignore the parse error so the component does not throw.
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit