(fix): clear stale install data before starting new installation#11716
(fix): clear stale install data before starting new installation#11716
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)
📝 WalkthroughWalkthroughThe changes refactor the installer's step advancement and installation state management. The click handler for step 5 now executes optional validation followed by unconditional clearing of stale install state, replacing the previous gated approach. The progress module updates its terminal state detection to return specific state strings ( 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 two related bugs in the Appwrite installer UI: (1) stale install data causing the installer to resume a failed/old installation instead of starting fresh when the user navigates back and retries, and (2) a wrong redirect to step 1 when the installer page is reloaded after a successful installation (should redirect to the console instead). Confidence Score: 5/5Safe to merge; only finding is a P2 style observation about a silent no-op that is likely intentional. All logic paths were traced end-to-end: install-lock/ID clearing in installer.js happens before loadStep so initStep5 always sees clean state; the isSnapshotTerminal contract change has a single caller that was fully updated; the 'completed' redirect path correctly skips recoverToLastStep(); and notifyInstallComplete handles undefined gracefully so the redirect is never blocked. No files require special attention beyond the P2 observation in progress.js around clearInstallId being called before startSslCheck. Important Files Changed
Reviews (1): Last reviewed commit: "(fix): clear stale install data before s..." | Re-trigger Greptile |
| if (result === 'completed') { | ||
| // Install already finished — redirect to console | ||
| // instead of bouncing back to step 1. | ||
| stopSyncedSpinnerRotation(); | ||
| setUnloadGuard(false); | ||
| clearInstallLock?.(); | ||
| clearInstallId?.(); | ||
| startSslCheck(null); |
There was a problem hiding this comment.
notifyInstallComplete silently no-ops in the completed-reload path
clearInstallId?.() is called on line 1104 before startSslCheck(null) on line 1105. By the time showRedirectStep runs and evaluates activeInstall?.installId || getStoredInstallId?.(), both sources are null — activeInstall was never set in this branch and the stored ID was just cleared. As a result, notifyInstallComplete is called with undefined and short-circuits immediately (if (!installId) return Promise.resolve()).
The redirect still happens correctly via .finally(), and since this path represents a reload of an already-completed install (no new session data to report), skipping the notification is arguably intentional. However, the getStoredInstallId?.() fallback added to showRedirectStep provides no benefit in this code path. A brief comment explaining the intentional skip would help future readers.
✨ Benchmark results
⚡ Benchmark Comparison
|
This comment has been minimized.
This comment has been minimized.
|
Found 1 test failure on Blacksmith runners: Failure
|
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