Skip to content

Prevent terminal hydration drift and improve settings semantics #1237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
open-inspect wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Prevent terminal hydration drift and improve settings semantics #1237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 37 additions & 10 deletions packages/web/src/app/(app)/session/[id]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,15 @@
import { useParams, useRouter, useSearchParams } from "next/navigation";
import { mutate } from "swr";
import useSWRMutation from "swr/mutation";
import { Suspense, useState, useRef, useEffect, useCallback, useMemo } from "react";
import {
Suspense,
useState,
useRef,
useEffect,
useCallback,
useMemo,
useSyncExternalStore,
} from "react";
import { useSessionSocket } from "@/hooks/use-session-socket";
import { SessionTimeline } from "@/components/session-timeline";
import { MediaLightbox } from "@/components/media-lightbox";
Expand Down Expand Up @@ -52,6 +60,27 @@ import { useBrowserLayoutStorage } from "@/hooks/use-browser-layout-storage";
import { focusSessionDetailsTrigger } from "@/lib/session-details-focus";
import { useSessionParticipantProfiles } from "@/hooks/use-session-participant-profiles";

const TERMINAL_VISIBILITY_KEY = "terminal-visible";
const TERMINAL_VISIBILITY_EVENT = "terminal-visibility-change";

function subscribeToTerminalVisibility(onStoreChange: () => void) {
window.addEventListener("storage", onStoreChange);
window.addEventListener(TERMINAL_VISIBILITY_EVENT, onStoreChange);
return () => {
window.removeEventListener("storage", onStoreChange);
window.removeEventListener(TERMINAL_VISIBILITY_EVENT, onStoreChange);
};
}

function getTerminalVisibility() {
return localStorage.getItem(TERMINAL_VISIBILITY_KEY) === "true";
}

function setTerminalVisibility(visible: boolean) {
localStorage.setItem(TERMINAL_VISIBILITY_KEY, String(visible));
window.dispatchEvent(new Event(TERMINAL_VISIBILITY_EVENT));
}

type SessionState = ReturnType<typeof useSessionSocket>["sessionState"];

export default function SessionPage() {
Expand Down Expand Up @@ -141,18 +170,16 @@ function SessionPageContent() {
const actionsButtonRef = useRef<HTMLButtonElement>(null);

// Terminal panel state
const [terminalOpen, setTerminalOpen] = useState(() => {
if (typeof window === "undefined") return false;
return localStorage.getItem("terminal-visible") === "true";
});
const terminalOpen = useSyncExternalStore(
subscribeToTerminalVisibility,
getTerminalVisibility,
() => false
);
const toggleTerminal = useCallback(() => {
const next = !terminalOpen;
localStorage.setItem("terminal-visible", String(next));
setTerminalOpen(next);
setTerminalVisibility(!terminalOpen);
}, [terminalOpen]);
const closeTerminal = useCallback(() => {
setTerminalOpen(false);
localStorage.setItem("terminal-visible", "false");
setTerminalVisibility(false);
}, []);
const ttydUrl = sessionState?.ttydUrl;
const ttydToken = sessionState?.ttydToken;
Expand Down
8 changes: 6 additions & 2 deletions packages/web/src/components/automations/automation-form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ export function AutomationForm({ mode, initialValues, onSubmit, submitting }: Au
{/* Trigger Type */}
{mode === "create" ? (
<div>
<label className="block text-sm font-medium text-foreground mb-1.5">Trigger Type</label>
<div className="block text-sm font-medium text-foreground mb-1.5">Trigger Type</div>

@open-inspect open-inspect Bot Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[deep review] This removes the invalid standalone label, but it leaves the mutually exclusive choices semantically unnamed and ungrouped: TriggerTypeSelector still renders plain buttons with no selected-state semantics. Please make this a real field boundary (fieldset/legend with native radios, or a named radiogroup with aria-checked) so the structure expresses the model instead of only changing the heading element.

<FieldDescription className="my-1">
Scheduled automations run on a repeating timer. Other types run when the connected
service sends an event (for example a GitHub webhook or Sentry alert).
Expand Down Expand Up @@ -420,12 +420,16 @@ export function AutomationForm({ mode, initialValues, onSubmit, submitting }: Au

{/* Repository Configuration */}
<div>
<label className="block text-sm font-medium text-foreground mb-1.5">
<label
htmlFor="automation-repository-configuration"
className="block text-sm font-medium text-foreground mb-1.5"
>
Repository Configuration

@open-inspect open-inspect Bot Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[deep review] The new htmlFor association does not become the button’s accessible name because the existing aria-label="Repository selection" takes precedence. That makes this added label/id wiring competing, ineffective semantics. Please keep one canonical naming path, ideally by removing the redundant aria-label and letting the visible associated label name the trigger (or using aria-labelledby if both visible strings are intentionally part of the name).

</label>
<Popover open={repoDropdownOpen} onOpenChange={setRepoDropdownOpen}>
<PopoverTrigger asChild>
<button
id="automation-repository-configuration"
type="button"
className="flex w-full items-center gap-2 rounded-sm border border-border bg-input px-3 py-2 text-sm text-foreground transition hover:border-foreground/20 focus-visible:border-ring focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring/40"
aria-label="Repository selection"
Expand Down
25 changes: 13 additions & 12 deletions packages/web/src/components/settings/sandbox-settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,8 +593,8 @@ export function SandboxSettingsEditor({
</div>
</div>

<div>
<label className="block text-sm font-medium text-foreground mb-1.5">Service Ports</label>
<fieldset>
<legend className="block text-sm font-medium text-foreground mb-1.5">Service Ports</legend>
<p className="text-xs text-muted-foreground mb-2">
Ports code-server and the web terminal bind to. Leave blank for the defaults (
{DEFAULT_CODE_SERVER_PORT} and {DEFAULT_TERMINAL_PORT}). Change a port to free the default
Expand Down Expand Up @@ -634,11 +634,12 @@ export function SandboxSettingsEditor({
/>
</div>
</div>
</div>
</fieldset>

<div>
<fieldset>
<legend className="sr-only">Tunnel Ports</legend>
<div className="flex items-center justify-between max-w-sm mb-1.5">
<label className="block text-sm font-medium text-foreground">Tunnel Ports</label>
<span className="block text-sm font-medium text-foreground">Tunnel Ports</span>
<Button
type="button"
variant="subtle"
Expand Down Expand Up @@ -680,10 +681,10 @@ export function SandboxSettingsEditor({
))
)}
</div>
</div>
</fieldset>

<div>
<label className="block text-sm font-medium text-foreground mb-1.5">Child Sessions</label>
<fieldset>
<legend className="block text-sm font-medium text-foreground mb-1.5">Child Sessions</legend>
<p className="text-xs text-muted-foreground mb-2">
Limit agent-spawned child sessions to prevent runaway sandbox usage.
</p>
Expand Down Expand Up @@ -721,10 +722,10 @@ export function SandboxSettingsEditor({
/>
</div>
</div>
</div>
</fieldset>

<div>
<label className="block text-sm font-medium text-foreground mb-1.5">Resources</label>
<fieldset>
<legend className="block text-sm font-medium text-foreground mb-1.5">Resources</legend>
<p className="text-xs text-muted-foreground mb-2">
Reserve CPU and memory for each sandbox. Leave blank to use the provider&apos;s default
reservation.
Expand Down Expand Up @@ -764,7 +765,7 @@ export function SandboxSettingsEditor({
/>
</div>
</div>
</div>
</fieldset>

<div>
<label
Expand Down
50 changes: 29 additions & 21 deletions packages/web/src/hooks/use-session-attachments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,14 @@ export function useSessionAttachments() {
const attachmentsRef = useRef<PendingAttachment[]>([]);
const attachmentsRevisionRef = useRef(0);
const activeUploadRef = useRef<AbortController | null>(null);
const uploadedByIdRef = useRef(
new Map<string, { sessionId: string; attachment: SessionAttachmentReference }>()
);
const uploadedByIdRef = useRef<Map<

@open-inspect open-inspect Bot Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[deep review] This behavior-neutral rewrite adds nullable state, render-time mutation, an alias, and callback dependencies merely to hold a stable Map; the same protocol is then repeated for sets in use-session-participant-profiles and use-session-socket. That moves complexity around without deleting any. Please restore the direct initialized refs and use .current inside callbacks/effects. If lazy mutable-ref initialization is genuinely a project requirement, give it one canonical hook rather than copy-pasting this ceremony across unrelated hooks.

string,
{ sessionId: string; attachment: SessionAttachmentReference }
> | null>(null);
if (uploadedByIdRef.current === null) {
uploadedByIdRef.current = new Map();
}
const uploadedById = uploadedByIdRef.current;

useEffect(() => {
attachmentsRef.current = attachments;
Expand Down Expand Up @@ -107,32 +112,35 @@ export function useSessionAttachments() {
}
}, []);

const removeAttachment = useCallback((id: string) => {
setAttachmentError(null);
const current = attachmentsRef.current;
const removed = current.find((attachment) => attachment.id === id);
if (!removed) return;
const removeAttachment = useCallback(
(id: string) => {
setAttachmentError(null);
const current = attachmentsRef.current;
const removed = current.find((attachment) => attachment.id === id);
if (!removed) return;

URL.revokeObjectURL(removed.previewUrl);
uploadedByIdRef.current.delete(removed.id);
attachmentsRevisionRef.current += 1;
activeUploadRef.current?.abort();
const next = current.filter((attachment) => attachment.id !== id);
attachmentsRef.current = next;
setAttachments(next);
}, []);
URL.revokeObjectURL(removed.previewUrl);
uploadedById.delete(removed.id);
attachmentsRevisionRef.current += 1;
activeUploadRef.current?.abort();
const next = current.filter((attachment) => attachment.id !== id);
attachmentsRef.current = next;
setAttachments(next);
},
[uploadedById]
);

const clearAttachments = useCallback(() => {
const current = attachmentsRef.current;
for (const attachment of current) {
URL.revokeObjectURL(attachment.previewUrl);
}
uploadedByIdRef.current.clear();
uploadedById.clear();
attachmentsRevisionRef.current += 1;
activeUploadRef.current?.abort();
attachmentsRef.current = [];
setAttachments([]);
}, []);
}, [uploadedById]);

/**
* Upload all pending attachments and return the references to send with the
Expand Down Expand Up @@ -170,7 +178,7 @@ export function useSessionAttachments() {
for (const pendingAttachment of pending) {
const fileName = pendingAttachment.file.name;
assertCurrent();
const cached = uploadedByIdRef.current.get(pendingAttachment.id);
const cached = uploadedById.get(pendingAttachment.id);
if (cached?.sessionId === sessionId) {
uploaded.push(cached.attachment);
continue;
Expand Down Expand Up @@ -204,7 +212,7 @@ export function useSessionAttachments() {
name: fileName || "image-attachment",
attachmentId,
};
uploadedByIdRef.current.set(pendingAttachment.id, { sessionId, attachment });
uploadedById.set(pendingAttachment.id, { sessionId, attachment });
uploaded.push(attachment);
}
assertCurrent();
Expand All @@ -228,7 +236,7 @@ export function useSessionAttachments() {
setIsUploading(false);
}
},
[]
[uploadedById]
);

return {
Expand Down
16 changes: 10 additions & 6 deletions packages/web/src/hooks/use-session-participant-profiles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ export function useSessionParticipantProfiles(
const { data, mutate } = useSWR<unknown>(
`/api/sessions/${sessionId}/participant-profiles` as const
);
const attemptedUnknownIds = useRef(new Set<string>());
const attemptedUnknownIdsRef = useRef<Set<string> | null>(null);
if (attemptedUnknownIdsRef.current === null) {
attemptedUnknownIdsRef.current = new Set();
}
const attemptedUnknownIds = attemptedUnknownIdsRef.current;

const profiles = useMemo<Record<string, SessionParticipantProfile>>(() => {
const parsed = sessionParticipantProfilesResponseSchema.safeParse(data);
Expand All @@ -34,19 +38,19 @@ export function useSessionParticipantProfiles(
}, [events, participants]);

useEffect(() => {
attemptedUnknownIds.current.clear();
}, [sessionId]);
attemptedUnknownIds.clear();
}, [attemptedUnknownIds, sessionId]);

useEffect(() => {
if (data === undefined) return;
const unknownIds = [...observedUserIds].filter(
(userId) => !profiles[userId] && !attemptedUnknownIds.current.has(userId)
(userId) => !profiles[userId] && !attemptedUnknownIds.has(userId)
);
if (unknownIds.length === 0) return;

for (const userId of unknownIds) attemptedUnknownIds.current.add(userId);
for (const userId of unknownIds) attemptedUnknownIds.add(userId);
void mutate();
}, [data, mutate, observedUserIds, profiles]);
}, [attemptedUnknownIds, data, mutate, observedUserIds, profiles]);

const profiledParticipants = useMemo(
() =>
Expand Down
27 changes: 17 additions & 10 deletions packages/web/src/hooks/use-session-socket.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,18 +74,25 @@ export function useSessionSocket(sessionId: string): UseSessionSocketReturn {
// Buffers streamed assistant text in a ref so token events (which arrive at
// high frequency) don't re-render; the text is appended on completion.
const pendingTextRef = useRef<PendingAssistantText | null>(null);
const subscriptionWaitersRef = useRef(new Set<(subscribed: boolean) => void>());
const subscriptionWaitersRef = useRef<Set<(subscribed: boolean) => void> | null>(null);
if (subscriptionWaitersRef.current === null) {
subscriptionWaitersRef.current = new Set();
}
const subscriptionWaiters = subscriptionWaitersRef.current;
const pendingPromptRef = useRef<{
resolve: (accepted: boolean) => void;
timeout: ReturnType<typeof setTimeout>;
} | null>(null);

const settleSubscriptionWaiters = useCallback((subscribed: boolean) => {
for (const resolve of subscriptionWaitersRef.current) {
resolve(subscribed);
}
subscriptionWaitersRef.current.clear();
}, []);
const settleSubscriptionWaiters = useCallback(
(subscribed: boolean) => {
for (const resolve of subscriptionWaiters) {
resolve(subscribed);
}
subscriptionWaiters.clear();
},
[subscriptionWaiters]
);

const settlePendingPrompt = useCallback((accepted: boolean) => {
const pending = pendingPromptRef.current;
Expand Down Expand Up @@ -166,13 +173,13 @@ export function useSessionSocket(sessionId: string): UseSessionSocketReturn {
if (settled) return;
settled = true;
clearTimeout(timeout);
subscriptionWaitersRef.current.delete(finish);
subscriptionWaiters.delete(finish);
resolve(subscribed);
};
const timeout = setTimeout(() => finish(false), PROMPT_SUBSCRIPTION_TIMEOUT_MS);
subscriptionWaitersRef.current.add(finish);
subscriptionWaiters.add(finish);
});
}, [isOpen]);
}, [isOpen, subscriptionWaiters]);

const sendPrompt = useCallback(
async (
Expand Down
Loading