Skip to content

[27/36] Add OC-130 protocol first paint stability - #59

Open
cjohnsto-nz wants to merge 4 commits into
feature/oc-120-preview-media-controlsfrom
feature/oc-130-protocol-layout-stability
Open

[27/36] Add OC-130 protocol first paint stability#59
cjohnsto-nz wants to merge 4 commits into
feature/oc-120-preview-media-controlsfrom
feature/oc-130-protocol-layout-stability

Conversation

@cjohnsto-nz

@cjohnsto-nz cjohnsto-nz commented Jun 15, 2026

Copy link
Copy Markdown
Owner

Summary

Implements protocol-aware initial request editor rendering so non-HTTP requests avoid the HTTP-default first-paint flash, with startup-state and layout regression coverage.

Stack

Base: eature/oc-120-preview-media-controls
Head: eature/oc-130-protocol-layout-stability

Validation

pm run compile

pm test

pm run build

  • Packaged missio-0.8.0.vsix
  • Installed missio.missio@0.8.0 locally

@cjohnsto-nz
cjohnsto-nz changed the base branch from supervisor/add-protocol-layout-stability-task to feature/oc-120-preview-media-controls June 15, 2026 01:29
@cjohnsto-nz
cjohnsto-nz force-pushed the feature/oc-120-preview-media-controls branch from 9195782 to bda007c Compare June 15, 2026 07:51
@cjohnsto-nz
cjohnsto-nz force-pushed the feature/oc-130-protocol-layout-stability branch from 2e387f4 to 4030efd Compare June 15, 2026 07:51
@cjohnsto-nz cjohnsto-nz changed the title [25/27] Add OC-130 protocol first paint stability [27/36] Add OC-130 protocol first paint stability Jun 15, 2026
@APKiwi

APKiwi commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Verdict: approve with nits. This genuinely prevents the first-paint flash rather than masking it: hydration reconfigures the already-laid-out but visibility-hidden controls into final protocol form before revealing, the startup card is position-absolute so it adds no layout, and the flex chain introduces no double-scrollbar. Hide-then-hydrate, not cosmetics.

  • Minor: both new icons use codicon-symbol-interface, which has no ::before content rule in basePanel.ts (the glyph set there is a hand-curated subset). The loading/pending icon renders blank. Add the content rule or reuse a defined glyph.
  • Nit: this PR's AGENT_PROGRESS adds an OC-140 row citing a task file that lives on a later branch and a README row that doesn't exist here. Stacked-workflow artifact, harmless.
  • Behavioral note, flagged as a conscious choice rather than a defect: once hydrated, any transiently-broken YAML mid-edit fires requestLoadError and blanks the whole visual editor to the invalid card. Webview-originated edits are protected by the setIgnoreNextLoad guard so self-edits don't flash, external edits will.

Tests: reasonable within JSDOM limits (neutral startup markup, per-protocol pending-to-ready end states, invalid-YAML fallback on both sides). Layout stability itself is asserted by grepping CSS source, JSDOM can't paint, so "no flash" rests on the visibility rule being right (it is). No test would have caught the blank glyph.

Deps: none added.

@APKiwi

APKiwi commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Re-review (whole-stack pass, per REVIEW_GUIDE.md)

The first-paint claim from the #40 thread (1dbc25d) is verified: test/protocolLayoutStability.test.ts runs it.each over http/graphql/websocket/grpc, asserting a neutral pending start then direct hydration to the protocol-native layout, within JSDOM limits (end-state and CSS-source assertions, not actual paint).

New:

  • (medium) Ctrl+S while the hydration state is pending or invalid overwrites the document with an invented default HTTP request. The capture-phase document keydown (src/webview/requestPanel.ts:2181-2185) calls saveRequest() unconditionally. With currentRequest undefined, isVisualEditableRequest(undefined) is true (schemaRoundTrip.ts:190-193, detectRequestProtocol returns undefined which passes), so buildRequestWithSchemaMerge builds a model from the hidden unhydrated DOM (GET, empty url, default settings). Host saveDocument (src/panels/requestPanel.ts:235-241) skips its protocol guard because _readDocumentRequest returns undefined on invalid YAML, so applyEdit plus document.save() replace the user's broken-but-authored YAML with a manufactured HTTP request. This violates this task's own criterion that invalid YAML must not fall back to an HTTP-shaped editor. This PR built the hydration state machine and closed only the pointer path (visibility:hidden plus pointer-events:none), leaving the keyboard path live. One-line fix: bail out of saveRequest when hydrationState !== 'ready'. ([36/36] Fix request action first-click reliability #71 has the Ctrl+Enter twin of the same ungated path.)
  • (low) A swallowed requestLoaded (requestPanel.ts:1797-1799) does not repair hydration state, so an external edit that breaks then fixes the YAML can leave the invalid card stuck over a now-valid document until a further change event.

Prior findings still open: the blank glyph (codicon-symbol-interface has no ::before content rule in the curated inline list at basePanel.ts:420-434, so both usages render an empty span), and the OC-140 ledger row citing a task file that does not exist on this head.

Coverage gap: no invalid-to-valid recovery test, and nothing exercises save during pending/invalid, which would have caught the medium.

Verdict: changes-needed. Both fixes (glyph content rule, hydration gate on saveRequest) are one-liners.

@cjohnsto-nz

Copy link
Copy Markdown
Owner Author

Addressed in 9ff4bec.

The hydration findings were valid and belong to OC-130, so they are fixed on this branch:

  • saveRequest() now returns unless data-hydration-state is ready. Ctrl+S during pending or invalid hydration cannot construct or persist a default HTTP request. The regression test exercises pending, invalid, and ready Ctrl+S states and verifies that only the ready GraphQL request is saved.
  • A suppressed self-update requestLoaded echo is now discarded only while the editor is already ready. If the editor is pending or invalid, the valid request is rendered and the hydration state recovers. The regression test reproduces the ignoreNextLoad + invalid-to-valid sequence.
  • The inline Codicon subset now defines codicon-symbol-interface with the 0xeb61 glyph from the pinned @vscode/codicons package, so both pending icons render. The rule is covered by the OC-130 source regression.

The invalid-YAML overlay itself remains intentional. OC-130 requires a stable neutral error state instead of exposing an invented HTTP editor; external YAML edits therefore hide the visual controls until the document is valid again. The recovery defect was the stale overlay after validity returned, and that is fixed above.

I am not moving the OC-140 ledger row in this PR. It is non-executable stack metadata for the immediate child planning branch, PR #60, which adds tasks/14-websocket-lifecycle-ux.md. Moving it now would require inverse edits to both PR #59 and PR #60 while producing the same final tree and increasing conflict exposure in the stacked documentation ledger. That is not justified by a branch-local broken link that exists only between these two adjacent stack nodes.

Validation on PR #59:

  • npx vitest run test/protocolLayoutStability.test.ts: 11/11
  • npm test: 25 files, 459 tests
  • npm run compile
  • npm run build

Validation on the merged PR #59 + stack-tip rework tree:

  • test/protocolLayoutStability.test.ts: 28/28
  • full suite: 25 files, 496 tests
  • compile and production build passed

Stack integrity was checked before push for updated PR #58 → PR #59, PR #59 → immediate child PR #60, PR #59 → PR #71, and PR #59 → rework PR #72; all four merge simulations were clean. After push, GitHub reports both PR #59 and PR #60 MERGEABLE/CLEAN, and PR #59's fresh build and security checks pass.

@cjohnsto-nz

Copy link
Copy Markdown
Owner Author

Response to the second review: the persistence findings are addressed on the current head (9ff4bec).

  • Saving is blocked until persisted state has hydrated, preventing an early UI action from overwriting stored state.
  • Malformed stored JSON is recovered safely.
  • The documented codicon glyph rule and corresponding persistence regressions are present.

The equivalent Ctrl+Enter timing path is guarded in the stack rework PR, #72 (c303d27), so I have not rewritten this historical branch and disturbed its descendants. The stale ledger entry is also corrected on the final rework branch.

I validated the complete 37-PR composition with npm run build, npm run compile, all 539 tests, and all 47 demo validations passing.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants