(fix): guard against missing Host header in dispatch#11710
Conversation
📝 WalkthroughWalkthroughAdds an optional database migration step to the installer and upgrade flow. Introduces installer step 6 with new UI template, CSS, and JS wiring; extends installer step IDs, step-clamping, and progress handling. The installer POST input now accepts a boolean Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 delivers two distinct changes: the headline fix guarding against a missing Key changes:
One issue found: When migration fails, Confidence Score: 5/5Safe to merge; the core bug fix is correct and the only remaining finding is a minor double-SSE-event on migration failure that does not affect data integrity. The headline fix ( src/Appwrite/Platform/Tasks/Install.php — double STATUS_ERROR event on migration failure. Important Files Changed
Reviews (2): Last reviewed commit: "(refactor): rename migrate param and add..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/install/installer/js/installer.js (1)
15-27:⚠️ Potential issue | 🟠 MajorNormalize step mapping is now inconsistent with the server remap rules.
Line 15 introduced a non-monotonic flow (
[1, 6, 4, 5]), but Line 22 still applies “first greater step” logic. In upgrade mode,step=2/3normalizes to6client-side, while server-side mapping resolves those to4, causing panel/init/action-bar desync.🔧 Suggested fix (align client normalization with server mapping)
- const normalizeStep = (step) => { - const numeric = clampStep(step); - if (stepFlow.includes(numeric)) return numeric; - if (numeric <= stepFlow[0]) return stepFlow[0]; - for (let i = 0; i < stepFlow.length; i += 1) { - if (numeric < stepFlow[i]) { - return stepFlow[i]; - } - } - return stepFlow[stepFlow.length - 1]; - }; + const normalizeStep = (step) => { + const numeric = Number(step); + if (Number.isNaN(numeric)) return stepFlow[0]; + + // Keep client behavior aligned with server-side View remapping. + if (isUpgrade && (numeric === 2 || numeric === 3)) return 4; + if (!isUpgrade && numeric === 6) return 4; + + const bounded = Math.max(1, Math.min(6, numeric)); + if (stepFlow.includes(bounded)) return bounded; + return stepFlow[0]; + }; - const clampStep = (step) => Math.max(1, Math.min(6, step)); + const clampStep = (step) => { + const numeric = Number(step); + if (Number.isNaN(numeric)) return 1; + return Math.max(1, Math.min(6, numeric)); + };Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/install/installer/js/installer.js` around lines 15 - 27, The normalizeStep logic assumes stepFlow is monotonic; when stepFlow is non-monotonic (e.g. [1,6,4,5]) the "first greater step" logic desynchronizes from server mapping — update normalizeStep to compute the mapping using a sorted version of stepFlow so you pick the nearest greater-or-equal step in numeric order instead of array order: use clampStep(step) as before, create a sortedStepFlow = [...stepFlow].sort((a,b)=>a-b), then apply the greater-or-equal logic against sortedStepFlow (return sortedStepFlow[0] if numeric <= first, iterate sortedStepFlow to find first >= numeric, else return last); also apply the same change where the same logic appears (the other occurrence around the later block referenced in the comment). Ensure you still call clampStep and preserve existing boundary behavior.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Installer/Http/Installer/Install.php (1)
46-46: Consider importingBooleanvalidator for consistency.Other validators (
Range,Text,WhiteList,Nullable) are imported viausestatements, butBooleanuses a fully qualified namespace. For consistency:♻️ Suggested fix
Add the import at the top of the file with the other validators:
use Utopia\Validator\Boolean;Then update line 46:
- ->param('runMigration', false, new \Utopia\Validator\Boolean(true), 'Run database migration after upgrade', true) + ->param('runMigration', false, new Boolean(true), 'Run database migration after upgrade', true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Installer/Http/Installer/Install.php` at line 46, Replace the fully-qualified Boolean validator usage with an import for consistency: add a use Utopia\Validator\Boolean; alongside the other validators at the top of the file, then update the param call that currently constructs new \Utopia\Validator\Boolean(true) (the ->param('runMigration', ...) call) to use new Boolean(true) instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/http.php`:
- Around line 105-107: In dispatch(), the Host header parsing (currently using
$lines[1] and trim(explode('Host: ', ...)[1] ?? '') ) is brittle and
case/order-sensitive; fix it by iterating over $lines (or the parsed headers
array), performing a case-insensitive match for the header name (e.g.,
strcasecmp or preg_match for '^Host\s*:'), extracting the header value after the
first colon, trimming whitespace, and stripping any port portion (split on ':'
from the right or parse_url style) before assigning to $domain so $domain
reliably contains the hostname regardless of header order, case, or port. Ensure
this logic lives in dispatch() where $lines and $domain are handled and still
guards against missing headers by defaulting to an empty string.
---
Outside diff comments:
In `@app/views/install/installer/js/installer.js`:
- Around line 15-27: The normalizeStep logic assumes stepFlow is monotonic; when
stepFlow is non-monotonic (e.g. [1,6,4,5]) the "first greater step" logic
desynchronizes from server mapping — update normalizeStep to compute the mapping
using a sorted version of stepFlow so you pick the nearest greater-or-equal step
in numeric order instead of array order: use clampStep(step) as before, create a
sortedStepFlow = [...stepFlow].sort((a,b)=>a-b), then apply the greater-or-equal
logic against sortedStepFlow (return sortedStepFlow[0] if numeric <= first,
iterate sortedStepFlow to find first >= numeric, else return last); also apply
the same change where the same logic appears (the other occurrence around the
later block referenced in the comment). Ensure you still call clampStep and
preserve existing boundary behavior.
---
Nitpick comments:
In `@src/Appwrite/Platform/Installer/Http/Installer/Install.php`:
- Line 46: Replace the fully-qualified Boolean validator usage with an import
for consistency: add a use Utopia\Validator\Boolean; alongside the other
validators at the top of the file, then update the param call that currently
constructs new \Utopia\Validator\Boolean(true) (the ->param('runMigration', ...)
call) to use new Boolean(true) instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c62fa0b-3076-4293-b011-5b6eebb4864a
📒 Files selected for processing (13)
app/http.phpapp/views/install/installer.phtmlapp/views/install/installer/css/styles.cssapp/views/install/installer/js/installer.jsapp/views/install/installer/js/modules/context.jsapp/views/install/installer/js/modules/progress.jsapp/views/install/installer/js/steps.jsapp/views/install/installer/templates/steps/step-6.phtmlsrc/Appwrite/Platform/Installer/Http/Installer/Install.phpsrc/Appwrite/Platform/Installer/Http/Installer/View.phpsrc/Appwrite/Platform/Installer/Server.phpsrc/Appwrite/Platform/Tasks/Install.phptests/unit/Platform/Modules/Installer/ModuleTest.php
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/views/install/installer/js/steps.js (1)
338-348: Dispatch the seededmigratevalue once.Other steps push their initial value through
dispatchStateChange()immediately. Here the default checkbox state only mutatesformState, so any listener that persists state or reacts toinstaller:state-changewill missmigrate=trueuntil the user toggles the control.Suggested tweak
if (checkbox) { if (formState.migrate !== undefined) { checkbox.checked = formState.migrate; } else { formState.migrate = checkbox.checked; + dispatchStateChange?.('migrate'); } checkbox.addEventListener('change', () => { formState.migrate = checkbox.checked; dispatchStateChange?.('migrate'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/install/installer/js/steps.js` around lines 338 - 348, The code sets formState.migrate from the checkbox default but doesn't notify listeners; after the branch that assigns formState.migrate = checkbox.checked (the else branch), invoke dispatchStateChange?.('migrate') so the initial seeded value is dispatched once; keep the existing change listener on checkbox for subsequent updates and only call dispatchStateChange when you set the seeded value (in the block that runs when formState.migrate was undefined).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Platform/Tasks/Install.php`:
- Around line 777-789: The exec() call running the migration ($command, $output,
$exit) must be bounded with the same proc_open() + procCloseWithTimeout()
pattern used elsewhere to avoid hangs; replace the raw exec() invocation with
proc_open to start the process, capture stdout/stderr, then call
procCloseWithTimeout() to enforce a timeout and obtain the exit code, and on
timeout or non-zero exit use updateProgress (InstallerServer::STEP_MIGRATION,
InstallerServer::STATUS_ERROR) with the captured output and throw the
RuntimeException as before; ensure you reuse the same timeout/closing helper and
preserve the existing messageOverride/details handling.
- Around line 773-779: In runDatabaseMigration(), replace the bare docker
compose exec invocation with the same explicit compose context used by
runDockerCompose(): add the -f <compose-file> flag, --project-name
<project-name>, and --project-directory <project-directory> arguments and
include -T to disable TTY allocation so the command runs non-interactively;
build the command using the same variables or helpers that runDockerCompose()
uses (e.g., the compose file path, project name, and project directory) and keep
the existing exec() call to capture output and exit code.
---
Nitpick comments:
In `@app/views/install/installer/js/steps.js`:
- Around line 338-348: The code sets formState.migrate from the checkbox default
but doesn't notify listeners; after the branch that assigns formState.migrate =
checkbox.checked (the else branch), invoke dispatchStateChange?.('migrate') so
the initial seeded value is dispatched once; keep the existing change listener
on checkbox for subsequent updates and only call dispatchStateChange when you
set the seeded value (in the block that runs when formState.migrate was
undefined).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d26ce83-f0e3-48ac-a6ae-300af21b8d67
📒 Files selected for processing (7)
app/views/install/installer/js/modules/progress.jsapp/views/install/installer/js/steps.jsapp/views/install/installer/templates/steps/step-6.phtmlsrc/Appwrite/Platform/Installer/Http/Installer/Install.phpsrc/Appwrite/Platform/Tasks/Install.phpsrc/Appwrite/Platform/Tasks/Upgrade.phptests/unit/Platform/Modules/Installer/ModuleTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/unit/Platform/Modules/Installer/ModuleTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- app/views/install/installer/js/modules/progress.js
- app/views/install/installer/templates/steps/step-6.phtml
- src/Appwrite/Platform/Installer/Http/Installer/Install.php
✨ 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