Conversation
📝 WalkthroughWalkthroughRefactors schedule-loading in src/Appwrite/Platform/Tasks/ScheduleBase.php to build lightweight candidate arrays inline and track updatedProjectIds to distinguish initial vs incremental loads. On initial load all referenced projects are loaded; on incremental loads only projects for updated/new schedules are fetched. Creates an in-memory project map seeded from existing schedules, implements batched project loading (up to 10,000 IDs) with timing/logging, and consolidates error handling. Validates that each schedule references an existing project and an active resource, removes invalid schedules with contextual logs, and populates schedules with project and resource Documents during finalization. Removed dependency on Utopia\Database\Exception. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
commit: |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/ScheduleBase.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Tasks/ScheduleBase.php (4)
app/realtime.php (1)
getProjectDB(80-132)src/Appwrite/Platform/Tasks/ScheduleExecutions.php (3)
getCollectionId(30-33)getSupportedResource(25-28)getName(20-23)src/Appwrite/Platform/Tasks/ScheduleFunctions.php (3)
getCollectionId(34-37)getSupportedResource(29-32)getName(24-27)src/Appwrite/Platform/Tasks/ScheduleMessages.php (3)
getCollectionId(23-26)getSupportedResource(18-21)getName(13-16)
⏰ 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). (7)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: Benchmark
🔇 Additional comments (5)
src/Appwrite/Platform/Tasks/ScheduleBase.php (5)
125-126: LGTM! Good optimization.Tracking updated project IDs enables incremental project loading, which reduces unnecessary database queries on subsequent syncs.
160-191: LGTM! Improved error handling and logging.The changes enhance the schedule update logic with:
- More precise timestamp comparison for detecting updates
- Inline candidate construction for clarity
- Better error messages including project and resource identifiers
- Explicit handling of inactive resources
196-199: LGTM! Good early exit optimization.Prevents unnecessary processing when no schedules are found.
284-284: LGTM! More descriptive logging.Including the task name makes the logs clearer when multiple schedule types are running.
201-241: Address the unsafe array access for project lookups.The incremental load logic has a potential code smell: line 244 directly accesses
$map[$schedule['projectId']]without checking if the key exists. While there's a safety net at line 252 that checksif (empty($project))and unsets the schedule, this will trigger an undefined key notice in PHP.The issue occurs when a schedule's projectId isn't in the current batch of updated projects. On incremental loads, only projectIds from
$updatedProjectIdsare collected (line 207), so schedules unchanged this iteration won't have their projects explicitly loaded. If such a schedule's projectId was somehow never cached in$map(edge case), accessing$map[$schedule['projectId']]directly fails silently with a notice.Replace line 244:
$project = $map[$schedule['projectId']] ?? null;This explicitly handles the missing key case and makes the intent clearer than relying on the empty check downstream.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…resource validation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/ScheduleBase.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Tasks/ScheduleBase.php (4)
app/realtime.php (1)
getProjectDB(80-132)src/Appwrite/Platform/Tasks/ScheduleExecutions.php (3)
getCollectionId(30-33)getSupportedResource(25-28)getName(20-23)src/Appwrite/Platform/Tasks/ScheduleFunctions.php (3)
getCollectionId(34-37)getSupportedResource(29-32)getName(24-27)src/Appwrite/Platform/Tasks/ScheduleMessages.php (3)
getCollectionId(23-26)getSupportedResource(18-21)getName(13-16)
⏰ 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). (19)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Platform/Tasks/ScheduleBase.php (2)
158-191: Incremental schedule update detection and candidate construction look solid; confirmactive/timestamps invariantsThe
resourceUpdatedAtcomparison plus$updatedProjectIdstracking gives you a clean incremental path, and the inline$candidatearray keeps the hot loop lightweight. One thing to double‑check is the data invariants:
strtotime($existing['resourceUpdatedAt'] ?? '0') !== strtotime($schedule->getAttribute('resourceUpdatedAt') ?? '0')assumesresourceUpdatedAtis always a valid, consistently formatted date string; unexpected values (includingnull) will be coerced and may mask changes.if (!$candidate['active'])will treat any falsy value (includingnull) as “inactive” and drop the schedule.If these assumptions match how
schedules.resourceUpdatedAtandschedules.activeare written, this is fine; otherwise you might want stricter validation or explicit defaults to avoid silently culling schedules.
201-241: Project caching and validation flow are well-structured; ensure project-level changes don’t require separate invalidationThe project-map caching and batched
projectslookup are a nice improvement:
- Initial load:
array_unique(array_map(... 'projectId'))ensures you hydrate all referenced projects once.- Incremental: using
$updatedProjectIdsplus$mapfrom existing schedules avoids redundant project queries while still wiring newly touched schedules.- The later loop validating
$project/$resource, checkingisResourceBlocked, and attaching both documents back into$this->schedulesgives a clear, consistent in-memory shape.One design assumption to validate: by only reloading projects when some schedule for that project is updated in the
schedulescollection, you’re effectively caching project documents across ticks. If any scheduling-relevant project attributes (e.g. region, database DSN, flags used insideisResourceBlocked) can change independently of schedule/resource updates, this cache might hold stale project data longer than intended.If such project-only changes are either impossible or always accompanied by a schedule/resource update, this approach is perfectly fine. If not, you may eventually want a complementary invalidation path (e.g., reacting to project updates or periodically refreshing cached projects) to keep
projectdocuments fresh.Also applies to: 243-277
…chedule # Conflicts: # composer.lock
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Appwrite/Platform/Tasks/ScheduleBase.php (2)
163-178: Consider removing unnecessary try-catch block.The
try-catchwraps simplegetAttribute()calls and array construction which shouldn't throw exceptions. If the intent is to guard against missing schedule data, the attributes are already retrieved from a Document object which returnsnullfor missing attributes. This block adds complexity without clear benefit.- try { - $candidate = [ - '$sequence' => $schedule->getSequence(), - '$id' => $schedule->getId(), - 'projectId' => $schedule->getAttribute('projectId'), - 'resourceId' => $schedule->getAttribute('resourceId'), - 'resourceType' => $schedule->getAttribute('resourceType'), - 'schedule' => $schedule->getAttribute('schedule'), - 'active' => $schedule->getAttribute('active'), - 'resourceUpdatedAt' => $schedule->getAttribute('resourceUpdatedAt'), - ]; - } catch (\Throwable $th) { - Console::error("Failed to load schedule for project {$schedule->getAttribute('projectId')} {$collectionId} {$schedule->getAttribute('resourceId')}"); - Console::error($th->getMessage()); - continue; - } + $candidate = [ + '$sequence' => $schedule->getSequence(), + '$id' => $schedule->getId(), + 'projectId' => $schedule->getAttribute('projectId'), + 'resourceId' => $schedule->getAttribute('resourceId'), + 'resourceType' => $schedule->getAttribute('resourceType'), + 'schedule' => $schedule->getAttribute('schedule'), + 'active' => $schedule->getAttribute('active'), + 'resourceUpdatedAt' => $schedule->getAttribute('resourceUpdatedAt'), + ];
260-276: Consider skipping resource reload for unchanged schedules.On incremental syncs, all schedules (not just updated ones) go through the resource loading loop. For schedules that already have a
resourceloaded and weren't updated, this causes redundantgetDocument()calls every sync tick (10 seconds).Consider skipping resource reload for schedules that weren't updated and already have a resource:
+ // Skip resource reload if schedule wasn't updated and already has resource loaded + if (isset($schedule['resource']) && !in_array($schedule['projectId'], $updatedProjectIds)) { + continue; + } + // In case the resource is not found (project deleted). try { $resource = $getProjectDB($project)->getDocument(static::getCollectionId(), $schedule['resourceId']);Note:
$updatedProjectIdswould need to track schedule sequences or resourceIds rather than just projectIds for precise matching, or this optimization could be based on whether the schedule already has a 'resource' key set.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/ScheduleBase.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Tasks/ScheduleBase.php (5)
app/realtime.php (1)
getProjectDB(80-132)src/Appwrite/Platform/Tasks/ScheduleExecutions.php (3)
getCollectionId(30-33)getSupportedResource(25-28)getName(20-23)src/Appwrite/Platform/Tasks/ScheduleMessages.php (3)
getCollectionId(23-26)getSupportedResource(18-21)getName(13-16)src/Appwrite/Platform/Tasks/ScheduleFunctions.php (3)
getCollectionId(34-37)getSupportedResource(29-32)getName(24-27)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Get.php (1)
getName(27-30)
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (5)
src/Appwrite/Platform/Tasks/ScheduleBase.php (5)
125-125: LGTM!Good addition to track updated project IDs for selective loading during incremental syncs.
196-206: LGTM!The early return issue from the previous review has been addressed - the method now continues execution to properly update
$lastSyncUpdate. The logic for determining which project IDs to load (all on initial, only updated on incremental) is correct and implements the intended optimization.
219-240: LGTM! Good batch loading implementation.The in-memory caching of already-loaded projects and batch loading with timing instrumentation is well implemented. Minor observation:
Query::limit(count($batch))on line 228 is technically redundant since batches are already capped at 10,000, but it's a reasonable safety measure.
242-277: LGTM! All previous review concerns addressed.This segment correctly addresses all past review feedback:
- Safety check using
?? nullfor undefined array key (line 243)- Proper use of
isEmpty()method instead ofempty()(line 245)- Typo "projetId" corrected to "projectId" (line 246)
The validation flow is thorough: checks project existence, blocked status, and resource existence with appropriate error handling and logging.
283-283: LGTM!Good use of
static::getName()for consistent and readable log messages across different schedule types.
Optimizing project fetching by using a find() query instead of getDocument() per schedule
And keeping projects in a map to prevent redundant projects.