app/views/install/installer/templates/steps/step-5.phtml (1)

55-62: Consider adding confirmation for the "Reset Everything" button.

The hard reset button could have destructive consequences. Consider whether a confirmation dialog should be added before executing the hard reset to prevent accidental data loss.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/views/install/installer/templates/steps/step-5.phtml` around lines 55 -
62, The "Reset Everything" button (data attribute data-install-hard-reset) can
trigger destructive actions without confirmation; modify the click handler that
listens for [data-install-hard-reset] to present a confirmation dialog/modal
before proceeding (e.g., browser confirm or site modal) and only call the
existing hard-reset routine if the user explicitly confirms (optionally require
typing a keyword or clicking a secondary confirm button); ensure the cancel path
aborts without side effects and that any analytics/logging still reflect whether
the action was confirmed.

src/Appwrite/Platform/Tasks/Install.php (1)

752-753: Shell commands for system info are safe but may fail silently on non-Linux systems.

The commands nproc and reading from /proc/meminfo are Linux-specific. On other platforms (macOS during local dev, Windows WSL edge cases), these will return empty/null values, which is handled gracefully with the ?: null fallback and (int) round(...) conversion.

Consider adding a brief inline comment noting this is Linux-specific for maintainability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Appwrite/Platform/Tasks/Install.php` around lines 752 - 753, The code in
Install.php that sets the 'cpus' and 'ram' values using shell_exec('nproc') and
reading /proc/meminfo is Linux-specific and may return empty on non-Linux
systems; add a brief inline comment immediately above the 'cpus' and 'ram'
assignments (the lines that call nproc and grep MemTotal) stating that these
commands are Linux-only and may return null/zero on other platforms (e.g.,
macOS/Windows), so the fallback behavior is intentional for portability and
debugging.

app/views/install/installer/js/modules/state.js (1)

141-149: Consider mirroring localStorage fallback back to sessionStorage for consistency.

getInstallLock() mirrors the localStorage value back to sessionStorage when falling back (line 71), but getStoredInstallId() does not. This creates an inconsistency where the install lock will be synced but the install ID won't be if sessionStorage was cleared.

♻️ Suggested fix for consistency

     const getStoredInstallId = () => {
         try {
             const val = sessionStorage.getItem(INSTALL_ID_KEY);
             if (val) return val;
         } catch (error) {}
         try {
-            return localStorage.getItem(INSTALL_ID_LOCAL_KEY);
+            const val = localStorage.getItem(INSTALL_ID_LOCAL_KEY);
+            if (val) {
+                sessionStorage.setItem(INSTALL_ID_KEY, val);
+                return val;
+            }
         } catch (error) {}
         return null;
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/views/install/installer/js/modules/state.js` around lines 141 - 149,
getStoredInstallId currently reads INSTALL_ID_KEY from sessionStorage and falls
back to INSTALL_ID_LOCAL_KEY in localStorage but does not mirror the
localStorage value back into sessionStorage like getInstallLock does; update
getStoredInstallId so that when it successfully retrieves the install id from
localStorage (INSTALL_ID_LOCAL_KEY) it also writes that value into
sessionStorage under INSTALL_ID_KEY, handling possible storage exceptions
(try/catch) the same way as existing reads to maintain consistency with
getInstallLock.

src/Appwrite/Platform/Installer/Http/Installer/Complete.php (1)

111-123: IPv6 edge case handling could be clearer.

When $domain contains more than 2 colons (e.g., a bare IPv6 address like 2001:db8::1), the method returns the entire string lowercased rather than extracting the IP portion. This works for buildCookieDomain() since filter_var(..., FILTER_VALIDATE_IP) will correctly identify it as an IP and return an empty cookie domain, but the method name extractHostname is slightly misleading in this case.

Consider adding a brief inline comment to clarify the intent:

📝 Suggested clarification

         $parts = explode(':', $domain);
-        return count($parts) <= 2 ? strtolower($parts[0]) : strtolower($domain);
+        // More than 2 colons indicates bare IPv6; return as-is for FILTER_VALIDATE_IP
+        return count($parts) <= 2 ? strtolower($parts[0]) : strtolower($domain);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Appwrite/Platform/Installer/Http/Installer/Complete.php` around lines 111
- 123, Update the extractHostname(string $domain) function to clarify IPv6
handling: add a concise inline comment near the colon-splitting logic (the
branch that returns strtolower($domain) when count($parts) > 2) explaining that
bare IPv6 addresses contain multiple colons and we intentionally return the full
lowercased string so downstream checks (e.g., buildCookieDomain() using
filter_var(..., FILTER_VALIDATE_IP)) can detect it as an IP; also add a short
comment by the '[' bracket handling to note it covers scoped IPv6 literal
syntax. Keep comments minimal and focused, referencing extractHostname and
buildCookieDomain for clarity.

src/Appwrite/Platform/Tasks/Install.php (2)

982-983: Document the new callback and return contracts.

The added ?callable $progress parameter and the shaped array returned from execWithContainerProgress() are not self-describing. Add PHPDoc for the callback signature and return keys so callers do not have to infer the contract from implementation details. As per coding guidelines, "Include comprehensive PHPDoc comments".

Also applies to: 1053-1094

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Appwrite/Platform/Tasks/Install.php` around lines 982 - 983, Add
comprehensive PHPDoc to runDockerCompose to document the new ?callable $progress
parameter and the shaped array returned by execWithContainerProgress(): specify
the callable signature (e.g., function(string $container, string $status, int
$completed, int $total): void or whatever fields you expect) and describe exact
return array keys and value types produced by execWithContainerProgress() (e.g.,
['container' => string, 'status' => string, 'progress' => int, ...]). Update the
PHPDoc block above the runDockerCompose method and also add/adjust PHPDoc on
execWithContainerProgress() (and any related helper methods around lines
~1053-1094) so callers can rely on the parameter and return contracts without
reading implementation details.

---

1043-1050: countComposeServices() is counting container_name, not services.

This total stays correct only while every service emits exactly one container_name: line. If a service ever omits that field, the progress counters will drift or stay at 0/N. Counting the actual service entries would make this helper match its name and keep the installer counters stable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Appwrite/Platform/Tasks/Install.php` around lines 1043 - 1050,
countComposeServices() currently counts "container_name:" occurrences, which
breaks when services omit that field; change it to parse the compose YAML and
return the number of top-level service entries instead. In the
countComposeServices(string $composeFile) function, load and parse the compose
file (e.g., using yaml_parse_file or Symfony\Component\Yaml\Yaml::parseFile) and
if parsing succeeds, return count($parsed['services']) (with checks that
'services' exists and is an array); on any failure keep returning 0. This
ensures the function counts actual service keys rather than container_name
lines.

app/views/install/installer/js/modules/progress.js (1)

1010-1034: ⚠️ Potential issue | 🟠 Major

Don't clear client state after a failed reset.

src/Appwrite/Platform/Installer/Http/Installer/Reset.php Lines 36-65 can still return a non-2xx on CSRF failure, and thrown fetch errors also land in the empty catch here. Both cases fall through to clearInstallLock()/clearInstallId()/cleanupInstallFlow()/redirect, so the browser behaves as if the reset succeeded while the server-side lock/progress can still exist.

💡 Suggested fix

         try {
             const res = await fetch('/install/reset', {
                 method: 'POST',
                 headers: withCsrfHeader({ 'Content-Type': 'application/json' }),
                 body: JSON.stringify({ installId: installId || '', hard })
             });
-            if (hard && !res.ok) {
+            if (!res.ok) {
                 const data = await res.json().catch(() => ({}));
                 showToast?.({
                     status: 'error',
                     title: 'Reset failed',
-                    description: data?.message || 'Could not stop containers. Try running "docker compose down -v" manually.',
+                    description: data?.message || (hard
+                        ? 'Could not stop containers. Try running "docker compose down -v" manually.'
+                        : 'Could not reset installer state. Refresh the page and try again.'),
                     dismissible: true
                 });
                 return;
             }
-        } catch (e) {}
+        } catch (error) {
+            console.error('Reset request failed:', { installId, hard, error });
+            showToast?.({
+                status: 'error',
+                title: 'Reset failed',
+                description: error?.message || 'Could not reach the installer server.',
+                dismissible: true
+            });
+            return;
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/views/install/installer/js/modules/progress.js` around lines 1010 - 1034,
performReset currently clears client state and redirects even when the server
returned a non-2xx or the fetch threw, causing the UI to assume success; update
performReset to only call clearInstallLock(), clearInstallId(),
cleanupInstallFlow(), and set window.location.href after a confirmed successful
response (res.ok). Concretely: in the try block check if (!res.ok) for all cases
(not only when hard) — parse error JSON (or fallback {}) and call showToast with
the error message and return; in the catch block show an error toast describing
the fetch/CSRF failure and return; only when the request succeeded (res.ok)
proceed to run clearInstallLock, clearInstallId, cleanupInstallFlow, and
redirect. Ensure you reference the performReset function and the
clearInstallLock/clearInstallId/cleanupInstallFlow/window.location.href calls
when making the change.

src/Appwrite/Platform/Tasks/Install.php (1)

1069-1120: Add PHPDoc for the new compose-progress helpers.

countComposeServices() and execWithContainerProgress() now hide non-obvious parsing and return-value contracts, but the helpers are undocumented. A short docblock here would make this path much easier to maintain.

As per coding guidelines, "Include comprehensive PHPDoc comments".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Appwrite/Platform/Tasks/Install.php` around lines 1069 - 1120, Add
comprehensive PHPDoc blocks to both countComposeServices() and
execWithContainerProgress(): for countComposeServices(string $composeFile): int,
document that it reads the compose file path, returns 0 on unreadable file or on
preg_match_all failure, and that it counts lines matching the
/^\s*container_name:/m pattern; for execWithContainerProgress(string
$commandLine, int $totalServices, callable $progress, bool $isUpgrade): array,
document the meaning and types of parameters (commandLine run via proc_open,
totalServices used to cap progress, progress callable signature and expected
constants InstallerServer::STEP_DOCKER_CONTAINERS and STATUS_IN_PROGRESS),
describe returned array keys ('output' => array of stdout lines, 'exit' =>
process exit code), note that proc_open is used with stderr redirected, that
progress exceptions are swallowed, and reference the timeout behavior via
procCloseWithTimeout and self::PROC_CLOSE_TIMEOUT_SECONDS.