Skip to content

Conversation

@abnegate
Copy link
Member

@abnegate abnegate commented Jan 13, 2026

Summary by CodeRabbit

Bug Fixes

  • Improved data migration reliability by enhancing timestamp handling in Appwrite migrations, automatically normalizing missing created/updated dates to ensure consistent formatting and data integrity.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 13, 2026

Walkthrough

The pull request modifies the Appwrite migration destination to implement date normalization during row creation. It computes missing $createdAt and $updatedAt fields by extracting values from existing row data or resource timestamps. Two new private helper methods (normalizeDateTime and stringifyDateValue) are introduced to standardize date formatting, and a use statement for Utopia\Database\DateTime is added. The updated $data array with normalized dates is then merged into the row document instead of the raw resource data.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fallback dates' directly relates to the core change: implementing fallback date normalization logic for createdAt/updatedAt fields when they're missing.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @src/Migration/Destinations/Appwrite.php:
- Around line 1044-1048: The current Unix timestamp detection around $resolved
uses a strict 10-character check and misses 9-digit (pre-2001) timestamps and
13-digit millisecond timestamps; update the logic in the method that returns
DateTime::format(new \DateTime($resolved)) to: detect numeric values
(ctype_digit or is_numeric), normalize 13-digit millisecond timestamps by
dividing by 1000, treat any numeric value within a reasonable Unix seconds range
(e.g., between -2147483648 and ~4102444800) as a seconds timestamp and prefix
with '@' before calling new \DateTime($resolved), and otherwise fall back to the
existing parse path so older 9-digit timestamps and millisecond inputs are
handled correctly.
🧹 Nitpick comments (1)
src/Migration/Destinations/Appwrite.php (1)

946-976: Date normalization logic looks good.

The fallback chain for extracting timestamps from various sources is well thought out for migration scenarios.

Minor nit: the (string) cast on line 967 is redundant since $data['$createdAt'] is already a string returned from normalizeDateTime().

Optional cleanup
         if (empty($updatedAt)) {
-            $data['$updatedAt'] = (string) $data['$createdAt'];
+            $data['$updatedAt'] = $data['$createdAt'];
         } else {
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4cb7a0e and 2acc9f2.

📒 Files selected for processing (1)
  • src/Migration/Destinations/Appwrite.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 81
File: src/Migration/Sources/CSV.php:215-233
Timestamp: 2025-07-02T06:24:50.844Z
Learning: In the utopia-php/migration codebase, invalid numeric values (like null) in parsed data are acceptable because the underlying database structure validator will catch and handle these validation errors, so explicit filtering during parsing is not required.
🧬 Code graph analysis (1)
src/Migration/Destinations/Appwrite.php (2)
src/Migration/Resources/Database/Columns/DateTime.php (1)
  • DateTime (8-79)
src/Migration/Resource.php (4)
  • getCreatedAt (209-212)
  • getUpdatedAt (217-220)
  • getId (127-130)
  • getPermissions (191-194)
🔇 Additional comments (2)
src/Migration/Destinations/Appwrite.php (2)

17-17: LGTM!

Import correctly added for the DateTime class used in the new normalization helpers.


1051-1061: LGTM!

Clean helper that correctly handles type coercion and returns null for unsupported types.

Comment on lines +1044 to +1048
if (\is_numeric($resolved) && \strlen($resolved) === 10 && (int) $resolved > 0) { // Unix timestamp
$resolved = '@' . $resolved;
}

return DateTime::format(new \DateTime($resolved));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unix timestamp detection has edge case limitations.

The 10-character length check (\strlen($resolved) === 10) only handles timestamps from ~Sep 2001 onwards. Timestamps for dates before Sep 9, 2001 (e.g., 946684800 for Jan 1, 2000) are 9 digits and would be passed to new \DateTime() without the @ prefix, likely causing parse failures.

If migrating data from older systems is a concern, consider a more robust check:

Suggested improvement
-        if (\is_numeric($resolved) && \strlen($resolved) === 10 && (int) $resolved > 0) { // Unix timestamp
+        // Unix timestamps: 9-10 digits for years 1970-2286
+        if (\is_numeric($resolved) && \preg_match('/^\d{9,10}$/', $resolved)) {
             $resolved = '@' . $resolved;
         }

Additionally, some source systems (e.g., JavaScript) may provide millisecond timestamps (13 digits). If that's a potential source, you might also want to handle those by dividing by 1000.

📝 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.

Suggested change
if (\is_numeric($resolved) && \strlen($resolved) === 10 && (int) $resolved > 0) { // Unix timestamp
$resolved = '@' . $resolved;
}
return DateTime::format(new \DateTime($resolved));
if (\is_numeric($resolved) && \preg_match('/^\d{9,10}$/', $resolved)) {
$resolved = '@' . $resolved;
}
return DateTime::format(new \DateTime($resolved));
🤖 Prompt for AI Agents
In @src/Migration/Destinations/Appwrite.php around lines 1044 - 1048, The
current Unix timestamp detection around $resolved uses a strict 10-character
check and misses 9-digit (pre-2001) timestamps and 13-digit millisecond
timestamps; update the logic in the method that returns DateTime::format(new
\DateTime($resolved)) to: detect numeric values (ctype_digit or is_numeric),
normalize 13-digit millisecond timestamps by dividing by 1000, treat any numeric
value within a reasonable Unix seconds range (e.g., between -2147483648 and
~4102444800) as a seconds timestamp and prefix with '@' before calling new
\DateTime($resolved), and otherwise fall back to the existing parse path so
older 9-digit timestamps and millisecond inputs are handled correctly.

@abnegate abnegate merged commit 52ca423 into main Jan 13, 2026
4 checks passed
@abnegate abnegate deleted the fix-import-stamps branch January 13, 2026 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants