Conversation
📝 Walkthrough""" WalkthroughThis change refines the handling of log segments demarcated by Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
877-888: Log order fix looks good but consider optimizing database operations.The repositioning of
afterBuildSuccessand addition of screenshot capturing logs correctly addresses the log order issue mentioned in the PR objectives. However, the additional database update and realtime trigger after adding the screenshot log might be unnecessary overhead.Consider consolidating the log updates to reduce database operations:
$this->afterBuildSuccess($queueForRealtime, $dbForProject, $deployment); -$logs = $deployment->getAttribute('buildLogs', ''); if ($resource->getCollection() === 'sites') { + $logs = $deployment->getAttribute('buildLogs', ''); $date = \date('H:i:s'); $logs .= "[90m[$date] [90m[[0mappwrite[90m][97m Screenshot capturing started. [0m\n"; $deployment->setAttribute('buildLogs', $logs); - $deployment = $dbForProject->updateDocument('deployments', $deployment->getId(), $deployment); - $queueForRealtime - ->setPayload($deployment->getArrayCopy()) - ->trigger(); }The deployment update and realtime trigger in the screenshot section (lines 1036-1040) will handle the log updates.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (4)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (4)
750-756: Good improvement for handling separator edge cases.The enhanced logic properly handles cases where both start and end separators appear within the same log chunk, preventing partial log omission. The implementation correctly extracts the leftover after the start separator and processes the end separator if present.
839-843: Excellent simplification of detection log parsing.The replacement of manual
strpos/substroperations withexplode-based splitting is much cleaner and more readable. The logic correctly extracts detection logs and reconstructs the remaining logs without the detection segment.
1237-1238: Good simplification of error message processing.The replacement of complex separator-searching logic with simple
str_replaceoperations is much cleaner and more comprehensive. Using a limit of 1 for each replacement correctly handles only the first occurrence of each separator.
1327-1327: Method signature change requires verification of all callers.The signature update for
afterBuildSuccessis correctly implemented with proper PHPDoc and runtime assertion. However, ensure all callers of this method are updated to match the new signature.#!/bin/bash # Description: Search for calls to afterBuildSuccess method to ensure compatibility with new signature # Expected: Only the call on line 877 should exist, which already matches the new signature # Search for method calls rg -A 3 -B 1 'afterBuildSuccess\(' --type php # Search for method declarations in case there are overrides ast-grep --pattern 'function afterBuildSuccess($_) { $$$ }'Also applies to: 1332-1332, 1334-1334
2 participants
What does this PR do?
Fixes screenshot vs distribution logs order. Also improves how we parse SSR logs more safely
Test Plan
Before:
After:
Related PRs and Issues
x
Checklist