Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChanges to the installer flow across two files: (1) In Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR fixes installer state handling in two areas: on the frontend it redirects users back to step 1 (instead of silently starting a fresh install) when an existing install session cannot be resumed, and on the backend it adds auto-detection of existing Appwrite installations at server startup to correctly set the upgrade flag and locked database type from
Confidence Score: 4/5
Important Files Changed
Reviews (1): Last reviewed commit: "(fix): auto-detect upgrade mode and data..." | Re-trigger Greptile |
| foreach ($dbServices as $db) { | ||
| if (preg_match('/^\s*' . preg_quote($db, '/') . ':\s*$/m', $composeData)) { | ||
| return $db; | ||
| } | ||
| } |
There was a problem hiding this comment.
The second foreach loop (lines 241–245) is unreachable in practice. The first preg_match_all regex already includes the alternative (\w+): which will match any YAML key at the start of a line — including mariadb:, mongodb:, and postgresql:. In a valid docker-compose.yml, preg_match_all will almost always return a positive count (e.g. from version:, services:, etc.), enter the if block, and evaluate the same service names against $dbServices.
The second loop can only execute if preg_match_all returns false (a regex engine error) or 0 (no matches at all — essentially impossible in a non-empty compose file). Consider removing it to simplify the logic:
| foreach ($dbServices as $db) { | |
| if (preg_match('/^\s*' . preg_quote($db, '/') . ':\s*$/m', $composeData)) { | |
| return $db; | |
| } | |
| } | |
| } | |
| $envData = @file_get_contents($envPath); |
| if ($config->isUpgrade()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
lockedDatabase detection when isUpgrade is already set
If isUpgrade is already true (set explicitly in the saved config) but lockedDatabase is null, this early return means the database auto-detection never runs. The lockedDatabase will remain null even though the installation files are present and the database could be inferred.
If there is a scenario where isUpgrade can be persisted as true without a corresponding lockedDatabase value, the installer may behave incorrectly. Consider checking lockedDatabase independently:
private function autoDetectUpgrade(Config $config): void
{
$basePath = $config->isLocal() ? '/usr/src/code' : (getcwd() ?: '.');
$composePath = $basePath . '/docker-compose.yml';
$envPath = $basePath . '/.env';
if (!file_exists($composePath) && !file_exists($envPath)) {
return;
}
if (!$config->isUpgrade()) {
$config->setIsUpgrade(true);
}
if ($config->getLockedDatabase() !== null) {
return;
}
$database = $this->detectDatabaseFromFiles($composePath, $envPath);
if ($database !== null) {
$config->setLockedDatabase($database);
}
}
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
TablesDBConsoleClientTest::testPatchAttribute |
1 | 91ms | Logs |
TablesDBCustomServerTest::testManyToOneRelationship |
1 | 1.45s | Logs |
TablesDBCustomServerTest::testUpdateWithExistingRelationships |
1 | 240.84s | Logs |
UsageTest::testVectorsDBStats |
1 | 10.34s | Logs |
✨ Benchmark results
⚡ Benchmark Comparison
|
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
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. Screenshots may also be helpful.)
Related PRs and Issues
Checklist