Skip to content

Resolve settings viewport before first client render - #1792

Merged
ColeMurray merged 3 commits into
mainfrom
chore/react-doctor-20260905-nightly
Sep 8, 2026
Merged

Resolve settings viewport before first client render#1792
ColeMurray merged 3 commits into
mainfrom
chore/react-doctor-20260905-nightly

Conversation

@open-inspect

@open-inspect open-inspect Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Resolve the real media-query snapshot before the first client render, while exposing an unknown viewport during SSR and hydration.
  • Let the settings shell use that readiness directly instead of a separate mount effect or synthetic hydration store. Preserve the existing boolean media-query API for other consumers.
  • Remove the warning-row key changes: the rows are stateless, and presentation-derived keys introduce collisions without a demonstrated UI regression to fix. Warning identity changes are not part of this PR.

Review feedback addressed

  • Fresh mobile client navigation no longer mounts the desktop settings tree before switching to mobile. The regression test uses the real media-query hook, not a resolved-value hook mock.
  • The content-derived warning keys and speculative reorder test have been removed completely.

Validation

  • Built @open-inspect/shared and passed web typecheck.
  • Scoped ESLint and Prettier checks passed; git diff --check passed.
  • 25 tests passed across the settings shell, media-query hook, sidebar layout, session sidebar, and model selector.
  • Real renderToString / hydrateRoot test preserves the server busy placeholder, renders only the mobile child tree after hydration, and reports no recoverable hydration errors.
  • Query switching, viewport-change subscriptions, listener cleanup, and the existing boolean SSR fallback are covered.
  • Controlled comparison with current main: the fresh-client test records mount, update (the busy placeholder adds a commit). With this change it records only mount.
  • Controlled comparison with the original PR head: the same test records desktop then mobile; the corrected implementation records mobile only. Both baseline checks failed as expected, and all eight final lifecycle tests passed after restoring the corrected sources.

The prior React Doctor diagnostic counts are not claimed for this narrowed revision. CI must pass on the final head before merge.

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

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

Terraform Validation Results

Step Status
Format
Init
Validate
Tests

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

@open-inspect open-inspect Bot left a comment

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

Two blocking design issues remain. The hydration replacement changes fresh client-navigation behavior on mobile, and the warning-key change still does not provide a unique identity even though the persistence layer already has one. No changed file crosses 1,000 lines; the largest resulting source file is 471 lines.

This is submitted as a comment review rather than REQUEST_CHANGES because the authenticated GitHub App is also the pull request author, and GitHub does not permit authors to request changes on their own pull requests.

Comment thread packages/web/src/components/settings/settings-shell.tsx Outdated
Comment thread packages/web/src/components/sidebar/metadata-section.tsx Outdated
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Terraform Validation Results

Step Status
Format
Init
Validate
Tests

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: @ColeMurray, Action: pull_request

@ColeMurray ColeMurray changed the title Prevent stale settings hydration and warning row reuse Resolve settings viewport before first client render Sep 8, 2026

@ColeMurray ColeMurray left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Reviewed exact head 0aad072, final settings/media-query implementation and tests, existing consumers, and all review threads. Both findings are addressed and resolved: fresh mobile navigation uses the real viewport immediately without mounting desktop content, and speculative warning-key changes were removed. Controlled baseline/original-PR comparisons demonstrate the actual rendering benefit. Twenty-five focused tests, real SSR hydration, shared build, web typecheck and scoped lint/format passed. All final-head CI is green with intentional Terraform deployment skips. No remaining blockers.

@ColeMurray
ColeMurray merged commit bb7f21f into main Sep 8, 2026
13 checks passed
@ColeMurray
ColeMurray deleted the chore/react-doctor-20260905-nightly branch September 8, 2026 00:40
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.

2 participants