Skip to content

Prevent terminal hydration drift and improve settings semantics - #1237

Open
open-inspect[bot] wants to merge 1 commit into
mainfrom
react-doctor/2026-08-01-18f9b90/web-cleanup
Open

Prevent terminal hydration drift and improve settings semantics#1237
open-inspect[bot] wants to merge 1 commit into
mainfrom
react-doctor/2026-08-01-18f9b90/web-cleanup

Conversation

@open-inspect

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

Copy link
Copy Markdown
Contributor

Prevents persisted terminal state from producing a server/client hydration mismatch and gives grouped settings controls accurate accessible structure.

Root-cause tasks

Fixed 10 root-cause tasks:

  1. react-doctor/no-hydration-branch-on-browser-global - packages/web/src/app/(app)/session/[id]/page.tsx - terminal visibility read localStorage during initial render. The preference now uses useSyncExternalStore with a stable server snapshot, preventing hydration drift and inconsistent terminal rendering. Human severity: high.
  2. react-doctor/rerender-lazy-ref-init - packages/web/src/hooks/use-session-attachments.ts - an upload cache Map was allocated on every render and discarded after the first. Lazy initialization avoids unnecessary allocations in the attachment composer. Human severity: low.
  3. react-doctor/rerender-lazy-ref-init - packages/web/src/hooks/use-session-participant-profiles.ts - the attempted-profile ID Set was reallocated on every render. Lazy initialization reduces avoidable work during participant updates. Human severity: low.
  4. react-doctor/rerender-lazy-ref-init - packages/web/src/hooks/use-session-socket.ts - the subscription waiter Set was reallocated on every socket-hook render. Lazy initialization avoids churn on a high-activity session path. Human severity: low.
  5. react-doctor/label-has-associated-control - packages/web/src/components/automations/automation-form.tsx - the trigger-type group heading was an unassociated form label. It is now neutral text, avoiding misleading label semantics for assistive technology. Human severity: medium.
  6. react-doctor/label-has-associated-control - packages/web/src/components/automations/automation-form.tsx - the repository configuration label did not identify its popover trigger. Matching htmlFor/id values now associate the visible label with the control. Human severity: medium.
  7. react-doctor/label-has-associated-control - packages/web/src/components/settings/sandbox-settings.tsx - Service Ports was an unassociated label for two controls. A fieldset/legend now exposes the controls as one named group. Human severity: medium.
  8. react-doctor/label-has-associated-control - packages/web/src/components/settings/sandbox-settings.tsx - Tunnel Ports was an unassociated label for a dynamic control group. A fieldset/legend now provides group semantics without changing the visible heading. Human severity: medium.
  9. react-doctor/label-has-associated-control - packages/web/src/components/settings/sandbox-settings.tsx - Child Sessions was an unassociated label for two limits. A fieldset/legend now gives screen-reader users the shared context. Human severity: medium.
  10. react-doctor/label-has-associated-control - packages/web/src/components/settings/sandbox-settings.tsx - Resources was an unassociated label for CPU and memory controls. A fieldset/legend now names that control group. Human severity: medium.

All selected diagnostics were ungrouped. No non-null fixGroupId was selected or split. Diagnostics with a shared fixGroupId remain deferred as complete groups.

React Doctor results

  • Total diagnostics: 108 before, 98 after
  • Errors: 2 before, 1 after
  • Warnings: 106 before, 97 after
  • no-hydration-branch-on-browser-global: 1 → 0
  • rerender-lazy-ref-init: 3 → 0
  • label-has-associated-control: 13 → 7
  • Raw diagnostics cleared: 10
  • No new React Doctor rule sites were introduced.

Validation

  • npm run typecheck -w @open-inspect/web after the error fix and after each warning task: passed
  • npm run typecheck -w @open-inspect/web: passed
  • npm run lint -w @open-inspect/web: passed
  • npx prettier --check packages/web: passed
  • npm test -w @open-inspect/web: passed, 111 files and 852 tests
  • Focused automation, sandbox settings, attachment, participant-profile, and socket tests: passed, 5 files and 100 tests
  • Full React Doctor after-scan: passed, 98 diagnostics
  • Changed-scope React Doctor scan against origin/main: no new finding; it reports the pre-existing prefer-useReducer warning at sandbox-settings.tsx:234
  • npm run build -w @open-inspect/web: still fails at the same pre-existing /automations/new prerender with Cannot read properties of null (reading 'useContext'), digest 2120487162; compilation and TypeScript complete successfully before that failure

Existing failures

  • The production build failure above was reproduced before editing and remains identical after the changes.
  • The repository-wide formatter baseline reports pre-existing formatting in .opencode/package.json; the web-only formatting check passes.
  • The remaining effect-needs-cleanup error is a validated false positive: the effect calls a callback that creates the WebSocket, and the effect's returned teardown closes that same socket, matching the canonical rule exclusion.

Deferred

  • Broad component splitting (no-giant-component) and reducer migrations remain for later code-owner-reviewed batches.
  • Grouped prop/state adjustment findings were left intact; no fixGroupId was split.
  • Sensitive integration/auth settings and terminal iframe sandbox policy require human judgment.
  • Locale presentation, placeholder labeling, image optimization, dynamic chart imports, array-key identity, and migration-scale iteration/lookup changes remain deferred where runtime or UX intent is unclear.
  • Remaining custom-control label findings require component-level accessibility decisions rather than mechanical edits.

Visual verification

Browser visual verification was not run. The UI edits are intended to be visually neutral semantic changes, and the terminal change affects hydration/state synchronization rather than layout; lint, typecheck, and focused interaction tests cover the touched behavior.


Created with Open-Inspect

@open-inspect open-inspect Bot added the react-doctor Opened by react-doctor label Aug 1, 2026
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

Terraform Validation Results

Step Status
Format
Init
Validate

Note: Terraform plan was skipped because secrets are not configured. This is expected for external contributors. See docs/GETTING_STARTED.md for setup instructions.

Pushed by: @open-inspect[bot], Action: pull_request

{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>

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.

htmlFor="automation-repository-configuration"
className="block text-sm font-medium text-foreground mb-1.5"
>
Repository Configuration

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).

const uploadedByIdRef = useRef(
new Map<string, { sessionId: string; attachment: SessionAttachmentReference }>()
);
const uploadedByIdRef = useRef<Map<

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

react-doctor Opened by react-doctor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant