fix: address code review feedback

- Fix toggle/load_order handlers to use page-reload (not JSON) responses
- Remove dead jsonResponse helper method from WorkshopModController
- Fix robocopy exit code detection using ROBOCOPY_EXIT: sentinel (not text parsing)
- Fix rsync dry-run change detection using RSYNC_EXIT: sentinel
- Remove agentIdFromRemote() stub; pass agentId directly to triggerSteamCmdDownload() logging
- Fix 'enabled' checkbox default in profile_form to use ($profile['enabled'] ?? 1)
- Add missing error_toggle_failed / error_order_failed lang strings

Agent-Logs-Url: https://github.com/GameServerPanel/GSP/sessions/dbeebd0e-e7a5-469d-8a8c-e63193d1ebb0

Co-authored-by: iaretechnician <2749183+iaretechnician@users.noreply.github.com>

modules/steam_workshop/controllers/WorkshopModController.php

@@ -233,18 +233,25 @@ class WorkshopModController
         $enabled    = !empty($_POST['enabled']);
 
         if ($homeId <= 0 || $workshopId === '') {
-            $this->jsonResponse(['ok' => false, 'error' => 'Missing parameters.']);
+            print_failure($this->lang['error_missing_params'] ?? 'Missing parameters.');
+            $this->handleIndex($userId, $isAdmin);
             return;
         }
 
         $home = $this->getHome($homeId, $userId, $isAdmin);
         if ($home === null) {
-            $this->jsonResponse(['ok' => false, 'error' => 'Access denied.']);
+            print_failure($this->lang['error_home_not_found'] ?? 'Server not found.');
+            $this->handleIndex($userId, $isAdmin);
             return;
         }
 
         $ok = $this->repo->toggleMod($homeId, $workshopId, $enabled);
-        $this->jsonResponse(['ok' => $ok]);
+        if (!$ok) {
+            print_failure($this->lang['error_toggle_failed'] ?? 'Failed to update mod status.');
+        }
+
+        $_GET['home_id'] = $homeId;
+        $this->handleModsPage($userId, $isAdmin);
     }
 
     private function handleLoadOrder(int $userId, bool $isAdmin): void
@@ -254,18 +261,25 @@ class WorkshopModController
         $order      = (int)($_POST['load_order'] ?? 0);
 
         if ($homeId <= 0 || $workshopId === '') {
-            $this->jsonResponse(['ok' => false, 'error' => 'Missing parameters.']);
+            print_failure($this->lang['error_missing_params'] ?? 'Missing parameters.');
+            $this->handleIndex($userId, $isAdmin);
             return;
         }
 
         $home = $this->getHome($homeId, $userId, $isAdmin);
         if ($home === null) {
-            $this->jsonResponse(['ok' => false, 'error' => 'Access denied.']);
+            print_failure($this->lang['error_home_not_found'] ?? 'Server not found.');
+            $this->handleIndex($userId, $isAdmin);
             return;
         }
 
         $ok = $this->repo->updateLoadOrder($homeId, $workshopId, $order);
-        $this->jsonResponse(['ok' => $ok]);
+        if (!$ok) {
+            print_failure($this->lang['error_order_failed'] ?? 'Failed to update load order.');
+        }
+
+        $_GET['home_id'] = $homeId;
+        $this->handleModsPage($userId, $isAdmin);
     }
 
     private function handleSync(int $userId, bool $isAdmin): void
@@ -360,13 +374,6 @@ class WorkshopModController
         require __DIR__ . '/../views/' . $view . '.php';
     }
 
-    /** @param array<string,mixed> $data */
-    private function jsonResponse(array $data): void
-    {
-        header('Content-Type: application/json');
-        echo json_encode($data);
-    }
-
     private function loadLang(): array
     {
         $file = __DIR__ . '/../lang/en_US.php';

modules/steam_workshop/lang/en_US.php

@@ -192,5 +192,7 @@ return [
     'error_missing_params'         => 'Missing required parameters.',
     'error_no_profile'             => 'No Workshop profile configured for this game.',
     'error_mod_not_found'          => 'Mod or profile not found.',
+    'error_toggle_failed'          => 'Failed to update mod status.',
+    'error_order_failed'           => 'Failed to update load order.',
 ];
 

modules/steam_workshop/lib/WorkshopInstaller.php

@@ -94,7 +94,7 @@ class WorkshopInstaller
         if ($cacheEntry === null || ($cacheEntry['status'] ?? '') !== 'cached') {
             $log[] = 'Cache MISS – triggering SteamCMD download on agent.';
             $downloadResult = $this->triggerSteamCmdDownload(
-                $remote, $appId, $workshopId, $steamCmdPath, $cachePath, $log
+                $remote, $agentId, $appId, $workshopId, $steamCmdPath, $cachePath, $log
             );
 
             if (!$downloadResult) {
@@ -273,6 +273,7 @@ class WorkshopInstaller
      */
     private function triggerSteamCmdDownload(
         object $remote,
+        int $agentId,
         string $appId,
         string $workshopId,
         string $steamCmdPath,
@@ -292,13 +293,13 @@ class WorkshopInstaller
         ]);
 
         $log[] = "SteamCMD start: {$cmd}";
-        $this->writeLog("STEAMCMD START agent={$this->agentIdFromRemote($remote)} app={$appId} mod={$workshopId}");
+        $this->writeLog("STEAMCMD START agent={$agentId} app={$appId} mod={$workshopId}");
 
         $output = $remote->exec($cmd);
 
         if ($output === null) {
             $log[] = 'SteamCMD: no response from agent (command may still be running).';
-            $this->writeLog("STEAMCMD NO_RESPONSE app={$appId} mod={$workshopId}");
+            $this->writeLog("STEAMCMD NO_RESPONSE agent={$agentId} app={$appId} mod={$workshopId}");
             // Treat as unknown – check file existence
         } else {
             $log[] = 'SteamCMD output: ' . substr((string)$output, 0, 500);
@@ -307,11 +308,11 @@ class WorkshopInstaller
         // Verify the download succeeded by checking for the cache path on the agent
         $exists = $remote->rfile_exists($cachePath);
         if ($exists === 1) {
-            $this->writeLog("STEAMCMD SUCCESS app={$appId} mod={$workshopId} path={$cachePath}");
+            $this->writeLog("STEAMCMD SUCCESS agent={$agentId} app={$appId} mod={$workshopId} path={$cachePath}");
             return true;
         }
 
-        $this->writeLog("STEAMCMD FAILURE app={$appId} mod={$workshopId} path={$cachePath}");
+        $this->writeLog("STEAMCMD FAILURE agent={$agentId} app={$appId} mod={$workshopId} path={$cachePath}");
         return false;
     }
 
@@ -332,27 +333,33 @@ class WorkshopInstaller
         $log[]      = "Pre-start compare: cache={$cachePath} dest={$installPath} method={$copyMethod}";
 
         if ($copyMethod === 'rsync') {
+            // Dry-run: any output lines (beyond the exit sentinel) mean changes exist
             $cmd = sprintf(
-                'rsync -rcn --delete %s %s 2>/dev/null; echo "EXIT:$?"',
+                'rsync -rcn --delete %s %s 2>/dev/null; echo "RSYNC_EXIT:$?"',
                 escapeshellarg(rtrim($cachePath, '/') . '/'),
                 escapeshellarg(rtrim($installPath, '/') . '/')
             );
             $out  = (string)$remote->exec($cmd);
-            // If rsync dry-run produces file list output, changes exist
+            // Strip the exit line, then check for any non-whitespace output
-            $hasChanges = preg_match('/\S/', preg_replace('/EXIT:\d+\s*$/', '', $out) ?? '') === 1;
+            $body = preg_replace('/RSYNC_EXIT:\d+\s*$/', '', $out) ?? '';
-            return $hasChanges;
+            return preg_match('/\S/', $body) === 1;
         }
 
         if ($copyMethod === 'robocopy') {
-            // Robocopy /L = list only, /MIR = mirror, /NJH /NJS = no headers
+            // List-only mode: robocopy exit code 0 = no differences, 1+ = changes or errors.
+            // Embed the exit code in output so we can read it back via exec().
             $cmd = sprintf(
-                'robocopy /L /MIR /NJH /NJS %s %s',
+                'robocopy /L /MIR /NJH /NJS %s %s; echo "ROBOCOPY_EXIT:$LASTEXITCODE"',
                 escapeshellarg($cachePath),
                 escapeshellarg($installPath)
             );
             $out  = (string)$remote->exec($cmd);
-            // Exit code 0 = no changes, 1+ = changes
+            if (preg_match('/ROBOCOPY_EXIT:(\d+)/', $out, $m)) {
-            return trim($out) !== '' && !preg_match('/\bNo new\b/i', $out);
+                // 0 = no change; 1–7 = informational (changes found); 8+ = error
+                return (int)$m[1] !== 0;
+            }
+            // If we cannot determine, assume sync is needed
+            return true;
         }
 
         // custom_script: always sync
@@ -392,7 +399,7 @@ class WorkshopInstaller
             );
         } elseif ($copyMethod === 'robocopy') {
             $cmd = sprintf(
-                'robocopy /MIR /NJH /NJS %s %s; echo "ROBOCOPY EXIT:$LASTEXITCODE"',
+                'robocopy /MIR /NJH /NJS %s %s; echo "ROBOCOPY_EXIT:$LASTEXITCODE"',
                 escapeshellarg($cachePath),
                 escapeshellarg($installPath)
             );
@@ -419,17 +426,20 @@ class WorkshopInstaller
         $out = (string)$remote->exec($cmd);
         $log[] = 'Sync output: ' . substr($out, 0, 500);
 
-        // Check exit code hint embedded in output
+        // Determine success from embedded exit code sentinel
-        if (preg_match('/EXIT:(\d+)/', $out, $m)) {
-            $code = (int)$m[1];
-            // robocopy exit codes 0..7 are success/info, 8+ are errors
         if ($copyMethod === 'robocopy') {
-                $ok = $code < 8;
+            if (preg_match('/ROBOCOPY_EXIT:(\d+)/', $out, $m)) {
+                // 0–7 = success/informational; 8+ = error
+                $ok = (int)$m[1] < 8;
             } else {
-                $ok = $code === 0;
+                $ok = true; // assume success if no code extracted
             }
         } else {
-            $ok = true; // assume success if no code
+            if (preg_match('/EXIT:(\d+)/', $out, $m)) {
+                $ok = (int)$m[1] === 0;
+            } else {
+                $ok = true;
+            }
         }
 
         if ($ok) {
@@ -471,12 +481,6 @@ class WorkshopInstaller
         return 'linux';
     }
 
-    private function agentIdFromRemote(object $remote): string
-    {
-        // OGPRemoteLibrary stores host/port; use reflection-free fallback
-        return 'unknown';
-    }
-
     private function writeLog(string $message): void
     {
         $file = $this->logDir . '/workshop_install.log';

modules/steam_workshop/views/admin/profile_form.php

@@ -135,7 +135,7 @@ $tplVarNote  = $lang['profile_template_vars'] ?? 'Available: {home_id} {agent_id
             </label>
             <label class="sw-checkbox">
                 <input type="checkbox" name="enabled" value="1"
-                       <?php echo (!isset($profile['enabled']) || !empty($profile['enabled'])) ? 'checked' : ''; ?>>
+                       <?php echo ($profile['enabled'] ?? 1) ? 'checked' : ''; ?>>
                 <span><?php echo htmlspecialchars($lang['profile_label_enabled'] ?? 'Profile enabled'); ?></span>
             </label>
         </fieldset>

modules/steam_workshop/views/user_workshop_mods.php

@@ -203,14 +203,14 @@ $baseAction = '?m=steam_workshop&p=main';
 <script>
 /* Simple toggle / order auto-submit for the mods table */
 document.addEventListener('DOMContentLoaded', function () {
-    // Toggle enable/disable via form submit
+    // Toggle enable/disable: submit the parent form immediately on change
     document.querySelectorAll('.js-ws-toggle').forEach(function (cb) {
         cb.addEventListener('change', function () {
             cb.closest('form').submit();
         });
     });
 
-    // Load order auto-submit on blur
+    // Load order: submit on change (blur triggers faster than enter on number inputs)
     document.querySelectorAll('.js-ws-order').forEach(function (inp) {
         inp.addEventListener('change', function () {
             inp.closest('form').submit();