[closed] [codex] Fix visited timestamp under clock skew

[juliusmarminge]

Closed and replaced because the original head branch included unrelated existing work. Replacement PR: #2408

Note

Add SSH tunnel environments, Tailscale Serve support, and hosted static app pairing

• Introduces desktop-managed SSH environments: the desktop app can discover SSH hosts, launch a remote T3 server via SSH tunnel, manage the lifecycle (connect/disconnect/password prompt), and expose the environment to the renderer via new IPC channels in preload.ts and main.ts.
• Adds new @t3tools/ssh and @t3tools/tailscale packages implementing SSH config parsing, tunnel orchestration, askpass helpers, and Tailscale CLI integration for status/serve management.
• Introduces AdvertisedEndpoint contract (remoteAccess.ts) with loopback, LAN, Tailscale IP, and MagicDNS HTTPS endpoint providers; the desktop Connections settings UI is significantly expanded to manage and select endpoints, enable Tailscale Serve, and generate hosted pairing URLs.
• Adds a hosted static app mode: the web app can boot without a paired backend, showing an onboarding screen, and supports a hosted pairing flow where a pairing token in the URL hash auto-saves a backend (hostedPairing.ts, __root.tsx).
• Fixes markThreadVisited to store the server-provided timestamp, avoiding clock skew regressions (uiStateStore.ts).
• WebSocket close events are now classified as intentional or reconnectable, suppressing spurious disconnect toasts on deliberate disconnects.
• Risk: significant surface area added across desktop IPC, SSH tunneling, and routing; SSH environments require the desktop bridge and will reject with a typed error in browser-only contexts.

📊 Macroscope summarized 9678d3b. 38 files reviewed, 13 issues evaluated, 3 issues filtered, 5 comments posted

🗂️ Filtered Issues

apps/desktop/src/sshScripts/remote-pick-port.cjs — 0 comments posted, 3 evaluated, 2 filtered

• line 4: If process.argv[3] is not provided and the file at filePath is empty or doesn't exist, both preferred and defaultPort become NaN. Since Number.isInteger(NaN) returns false, start falls back to defaultPort which is NaN. This causes the loop to never execute because NaN < end is always false, silently failing the port scan. [ Failed validation ]
• line 6: If filePath exists but is a directory (or a file without read permissions), fs.readFileSync will throw a synchronous exception at module load time. This exception is not caught because it occurs outside the async IIFE's .catch() handler on line 31. The script will crash instead of gracefully falling back to defaultPort. Consider wrapping the file read in a try-catch or using fs.statSync to verify it's a readable file. [ Cross-file consolidated ]

packages/ssh/src/command.ts — 0 comments posted, 2 evaluated, 1 filtered

• line 205: At line 205, child.exitCode.pipe(Effect.map(Number)) converts the exit code using the Number constructor. If the child process is terminated by a signal (rather than exiting normally), child.exitCode may resolve to null. Since Number(null) returns 0, the check at line 222 (exitCode !== 0) would incorrectly pass, treating a signal-killed process as successful instead of an error. [ Failed validation ]

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ce2cae33-56b1-43c1-88c1-58fb4809048b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch t3code/e1713ff7

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[macroscopeapp]

🟢 Low src/tunnel.ts:318

packageSpec is substituted directly into the shell script without escaping, so a value like file:./path with spaces/pkg produces invalid shell syntax in the exec npx --yes @@T3_PACKAGE_SPEC@@ "$@" command. A single quote in packageSpec also breaks the printf on line 236 because it is embedded inside single quotes. Consider applying shellSingleQuote() to packageSpec to match how nodeScriptPath is handled.

export function buildRemoteT3RunnerScript(input?: RemoteT3RunnerOptions): string {
-  const packageSpec = input?.packageSpec?.trim() || "t3@latest";
+  const packageSpec = shellSingleQuote(input?.packageSpec?.trim() || "t3@latest");
   const nodeScriptPath = input?.nodeScriptPath?.trim() || "";
   return stripTrailingNewlines(
     applyScriptPlaceholders(REMOTE_RUNNER_SCRIPT, {
       T3_PACKAGE_SPEC: packageSpec,
-      T3_NODE_SCRIPT_PATH: shellSingleQuote(nodeScriptPath),
+      T3_NODE_SCRIPT_PATH: nodeScriptPath ? shellSingleQuote(nodeScriptPath) : "''",
     }),
   );
 }

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file packages/ssh/src/tunnel.ts around lines 318-327:

`packageSpec` is substituted directly into the shell script without escaping, so a value like `file:./path with spaces/pkg` produces invalid shell syntax in the `exec npx --yes @@T3_PACKAGE_SPEC@@ "$@"` command. A single quote in `packageSpec` also breaks the `printf` on line 236 because it is embedded inside single quotes. Consider applying `shellSingleQuote()` to `packageSpec` to match how `nodeScriptPath` is handled.

Evidence trail:
packages/ssh/src/tunnel.ts lines 220-237 (REMOTE_RUNNER_SCRIPT template with unquoted @@T3_PACKAGE_SPEC@@ in exec and printf), lines 318-326 (buildRemoteT3RunnerScript showing packageSpec is raw while nodeScriptPath uses shellSingleQuote), lines 173-175 (shellSingleQuote definition), lines 55-58 (RemoteT3RunnerOptions interface showing packageSpec is arbitrary string)

[macroscopeapp]

🟡 Medium settings/ConnectionsSettings.tsx:724

endpointCopyOptions.map at line 724 uses endpointDefaultPreferenceKey(endpoint) as the React key, but this value is intentionally non-unique—multiple desktop LAN interfaces all return "desktop-core:lan:http" and multiple loopback endpoints all return "desktop-core:loopback:http". Duplicate keys cause React warnings and incorrect list rendering. Use endpoint.id instead, which is unique per endpoint.

-                              key={option.key}
+                              key={endpoint.id}

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file apps/web/src/components/settings/ConnectionsSettings.tsx around line 724:

`endpointCopyOptions.map` at line 724 uses `endpointDefaultPreferenceKey(endpoint)` as the React `key`, but this value is intentionally non-unique—multiple desktop LAN interfaces all return `"desktop-core:lan:http"` and multiple loopback endpoints all return `"desktop-core:loopback:http"`. Duplicate keys cause React warnings and incorrect list rendering. Use `endpoint.id` instead, which is unique per endpoint.

Evidence trail:
apps/web/src/components/settings/ConnectionsSettings.tsx lines 471-490 (endpointDefaultPreferenceKey function collapses multiple endpoints to same key), lines 562-576 (endpointCopyOptions built with key: endpointDefaultPreferenceKey(endpoint)), lines 724-726 (key={option.key} used in .map)

[macroscopeapp]

🟢 Low src/config.ts:14

splitDirectiveArgs splits only on whitespace, so SSH config lines using = as the directive separator (e.g., Host=myserver) produce a directive value of "host=myserver". After lowercasing, this fails to match "host", causing valid Host entries to be silently ignored.

-function splitDirectiveArgs(value: string): ReadonlyArray<string> {
-  return value
-    .trim()
    .split(/\s+/u)
    .map((entry) => entry.trim())
    .filter((entry) => entry.length > 0);
+function splitDirectiveArgs(value: string): ReadonlyArray<string> {
+  const withSpaceSeparators = value
+    .replace(/=(?!=)/gu, " ")
+    .trim();
+  return withSpaceSeparators
+    .split(/\s+/u)
+    .map((entry) => entry.trim())
+    .filter((entry) => entry.length > 0);
 }

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file packages/ssh/src/config.ts around lines 14-20:

`splitDirectiveArgs` splits only on whitespace, so SSH config lines using `=` as the directive separator (e.g., `Host=myserver`) produce a directive value of `"host=myserver"`. After lowercasing, this fails to match `"host"`, causing valid `Host` entries to be silently ignored.

Evidence trail:
packages/ssh/src/config.ts lines 14-20 (splitDirectiveArgs splits on /\s+/u only), lines 100-108 (directive matching uses normalizedDirective === "host" and === "include"). SSH config man page documents = as valid separator: 'Configuration options may be separated by whitespace or optional whitespace and exactly one =' (https://man.openbsd.org/ssh_config.5).

[macroscopeapp]

🟢 Low settings/ConnectionsSettings.tsx:178

When updateHeightAfterPaint is called again before prior animation frames complete, the previous firstFrameId and secondFrameId are overwritten without cancelling. The orphaned frame callbacks then execute after unmount, calling setHeight on an unmounted component.

-    const updateHeightAfterPaint = () => {
+    const updateHeightAfterPaint = () => {
+      if (firstFrameId !== null) {
+        window.cancelAnimationFrame(firstFrameId);
+        firstFrameId = null;
+      }
+      if (secondFrameId !== null) {
+        window.cancelAnimationFrame(secondFrameId);
+        secondFrameId = null;
+      }
       updateHeight();
-      firstFrameId = window.requestAnimationFrame(() => {
+      firstFrameId = window.requestAnimationFrame(() => {
         updateHeight();
-        secondFrameId = window.requestAnimationFrame(updateHeight);
+        secondFrameId = window.requestAnimationFrame(updateHeight);
       });
     };

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file apps/web/src/components/settings/ConnectionsSettings.tsx around lines 178-188:

When `updateHeightAfterPaint` is called again before prior animation frames complete, the previous `firstFrameId` and `secondFrameId` are overwritten without cancelling. The orphaned frame callbacks then execute after unmount, calling `setHeight` on an unmounted component.

Evidence trail:
apps/web/src/components/settings/ConnectionsSettings.tsx lines 168-205 at REVIEWED_COMMIT. Lines 184-189: `updateHeightAfterPaint` overwrites `firstFrameId` and `secondFrameId` without cancelling prior values. Lines 195-201: cleanup only cancels the latest stored IDs, not previously overwritten ones.

[macroscopeapp]

🟡 Medium runtime/service.ts:680

If bootstrapDesktopSshBearerSession throws (lines 680-683), the registrySnapshot from line 672 is never rolled back, leaving registry modifications from prepareSavedEnvironmentRecordForConnection (line 673) persisted despite the failure. This is inconsistent with line 689, which rolls back when didPersistBearerToken is false. Consider wrapping lines 680-691 in a try-catch that also calls persistSavedEnvironmentRegistryRollback(registrySnapshot) on failure.

Also found in 1 other location(s)

apps/desktop/src/sshScripts/remote-pick-port.cjs:6

If filePath exists but is a directory (or a file without read permissions), fs.readFileSync will throw a synchronous exception at module load time. This exception is not caught because it occurs outside the async IIFE's .catch() handler on line 31. The script will crash instead of gracefully falling back to defaultPort. Consider wrapping the file read in a try-catch or using fs.statSync to verify it's a readable file.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file apps/web/src/environments/runtime/service.ts around line 680:

If `bootstrapDesktopSshBearerSession` throws (lines 680-683), the `registrySnapshot` from line 672 is never rolled back, leaving registry modifications from `prepareSavedEnvironmentRecordForConnection` (line 673) persisted despite the failure. This is inconsistent with line 689, which rolls back when `didPersistBearerToken` is false. Consider wrapping lines 680-691 in a try-catch that also calls `persistSavedEnvironmentRegistryRollback(registrySnapshot)` on failure.

Evidence trail:
apps/web/src/environments/runtime/service.ts lines 670-695 at REVIEWED_COMMIT: Line 672 takes registrySnapshot, line 673 calls prepareSavedEnvironmentRecordForConnection (modifying registry), lines 680-682 call bootstrapDesktopSshBearerSession with no try-catch, lines 688-691 show explicit rollback only for the didPersistBearerToken===false case.

Also found in 1 other location(s):
- apps/desktop/src/sshScripts/remote-pick-port.cjs:6 -- If `filePath` exists but is a directory (or a file without read permissions), `fs.readFileSync` will throw a synchronous exception at module load time. This exception is not caught because it occurs outside the async IIFE's `.catch()` handler on line 31. The script will crash instead of gracefully falling back to `defaultPort`. Consider wrapping the file read in a try-catch or using `fs.statSync` to verify it's a readable file.