mirror of
https://github.com/nexu-io/open-design.git
synced 2026-09-20 06:15:06 +08:00
chejiulong
e34d823160
* fix(web): gate PPTX export on a daemon-advertised renderer capability
The export entry was offered whenever the artifact looked like a deck, so
in a deployment without an off-screen renderer (headless / container) the
button was always shown and always failed: `POST /export/pptx` answers 501
whenever `desktopSlideRenderer` is absent, and the check sits *before* the
editable/screenshot branch, so the mode picker in the modal was a choice
that could not affect the outcome either.
Ask the daemon instead of guessing. `/api/version` now carries an optional
`capabilities.slideRenderer`, computed from the very binding the export
routes guard on, so the advertisement cannot drift from the 501 they
return. It rides on the version payload because that is already the
"what runtime am I talking to" channel (packaged/channel/platform/arch),
already fetched once at boot, and — unlike `/api/app-config` — never
persisted: the same data dir can be opened by a desktop daemon today and a
headless one tomorrow, so a stored answer would be a stale claim.
The flag deliberately cannot be derived from `packaged`: a packaged binary
started as a plain daemon has no renderer.
Mixed versions are the delicate part. The field is optional and the client
treats absent as *unknown*, not false, so a newer web bundle keeps showing
PPTX against an older daemon that supports it but does not advertise it
yet. Only an explicit `false` hides the entry. A present-but-malformed
block is dropped rather than trusted, and an unresolved answer stays
retryable so one early fetch against a not-yet-ready daemon cannot pin
"unknown" for the session.
Client-side `isOpenDesignHostAvailable()` was not used for this even though
it is the existing desktop-only gate: it reports whether the web UI runs
inside the Electron shell, which is wrong in both directions here — a
browser pointed at a desktop-hosted daemon does have a renderer, and a
packaged daemon may not.
Also splits the two states that `unavailable: true` collapsed. A dead
daemon and a runtime with no renderer both produced "PPTX export is not
available here", which sends someone whose daemon is down to look at the
wrong problem. Callers keep the existing `'unavailable' in res` fallback
behaviour; only the message differs.
* test(web): pin the two unavailable reasons in the export specs
The three specs that asserted the old flat `{ ok: false, unavailable: true }`
now pin which reason they exercise: the two 501 cases as `no-renderer` and
the transport failure as `unreachable`. Without this the split is only
enforced by types, so a later edit could collapse both back onto one value
and every assertion would still pass.
* fix(web): let a late capability answer reach an already-mounted viewer
Review catch (thanks @nettee): the first shape probed once on mount and
ignored an unresolved result, so a probe that raced daemon startup or
blipped left `available` at null for the life of that mount. The gate then
stayed permissive and the entry could still be clicked into the very 501
this change exists to prevent. Worse, the module raced the app's own boot
fetch with a second request, so a failed probe here could keep the entry
showing even when the boot response had already reported `false`.
Three changes, all aimed at that gap:
- The boot fetch now feeds the cache. `fetchAppVersionInfo()` hands its
parsed payload to `recordAppVersionInfo()`, so the common path costs no
extra request and the two paths cannot disagree. The dependency runs
registry -> runtime-capabilities only, so there is no cycle; this module
no longer imports the registry and does its own minimal probe when it
still has to.
- Consumers subscribe. An answer arriving from boot, from a retry, or from
another mounted consumer now reaches every mounted viewer instead of
leaving each one pinned on its own failed probe.
- Unresolved probes retry with a bounded backoff (500ms / 1.5s / 4s) and
then stop. An unreachable daemon is its own visible problem and should
not be polled forever; a startup race is exactly what needs one more look.
Covered at the level the review asked for: a FileViewer spec where the
first probe cannot resolve, the entry is still offered (unknown means
assume available), and a later explicit `false` removes it. Verified red by
restoring the old single-shot behaviour — the late answer reaches nobody
and the entry stays.
* fix(web): recognise proxy-answered daemon outages as unreachable
Review catch (thanks @nettee): the daemon-down diagnostic did not actually
fire on the packaged / sidecar path, which is the common one. The proxies
in front of the daemon do not reject `fetch` when it is down — they answer.
The web sidecar ends a connection error as a plain-text 502, and Next's dev
rewrite as a plain-text 500 when OD_PORT points at a dead port. Both landed
in the generic `!resp.ok` branch and surfaced as "export request failed
(502)", so the new message this PR added never appeared where it was most
needed.
`state/projects.ts` already had exactly the right detector for this, kept
private. Extracted it to `runtime/daemon-proxy-failure.ts` and used it from
both places rather than growing a second copy.
Kept deliberately narrow: only a 500/502 that is `text/plain` AND carries a
connection errno counts. Widening to any 5xx would relabel genuine upstream
failures as an outage — the same misleading move in the opposite direction,
which is why the original detector was written this way. A JSON 502 stays a
semantic error, and there is a spec pinning that.
* fix(web): honour the unknown contract on both capability cache edges
Two compatibility edges from review (thanks @nettee), both of which broke
the same rule this PR is built on: absent or malformed means unknown, and
unknown must keep the entry visible.
- A resolved `false` outlived the daemon that gave it. If a page cached
`false` from a headless daemon and then talked to one that stops
advertising the field — a restart or a downgrade — the stale `false` kept
hiding the entry against a daemon we are supposed to know nothing about.
A fresh, valid response with no capability now drops the cache back to
unknown and tells mounted consumers, so listeners take `boolean | null`.
- The module's own probe trusted the capability without validating the
envelope it arrived in, so `{version:{capabilities:{slideRenderer:false}}}`
could publish `false` and hide the entry. That was self-inflicted: the
validation went missing when this module stopped importing the registry to
avoid a cycle, and nothing replaced it.
`isAppVersionInfo` now lives here and the registry imports it, so there is
one validator instead of two and the dependency still runs one way only.
Regression specs for both: `false` followed by an absent field returns to
unknown, and a capability in a malformed envelope is refused.
* fix(web): subscribe mounted viewers even when the cache is already resolved
Follow-on from the invalidation added in the previous commit, and the same
mistake one layer out: a viewer that mounted while the answer was already
`false` took the early-return path and never registered a listener, so when
a later valid response omitted the capability and invalidated the cache to
unknown, that viewer never heard it. Its entry stayed hidden against a
daemon the compatibility contract says we know nothing about — exactly the
bug the invalidation was added to prevent, just moved from the cache to the
consumer.
Subscribe unconditionally and only skip the probe when the cache is already
resolved. Spec covers the mounted case the loader-level tests could not:
render with `false` cached and the entry absent, invalidate, entry returns.
Verified red by restoring the early return.
* refactor(web): collapse the capability store into one atom with one writer
Four review rounds found six compatibility edges in this module, all the
same shape: `cached`, `inFlight` and the listener set were separate mutable
pieces, and each fix exposed another pair that could disagree — an
invalidation that forgot the in-flight promise, a validation path that
forgot the invalidation, a subscription that forgot to register. Patching
the seventh instance was not going to be different from the sixth.
So the state is now one snapshot behind one writer. `setSnapshot` is the
only thing that mutates it and always drops any in-flight probe, because a
promise created under a superseded answer can only report something this
state has already replaced. `loadSlideRendererAvailable` clears the promise
on settle rather than only on success, so an unanswered probe stays
retryable. Consumers read through `useSyncExternalStore`, so every mounted
viewer sees the same value including a later invalidation — there is no
longer a subscribe-or-not branch to get wrong.
That makes the two edges from this round unrepresentable rather than merely
fixed: a settled probe cannot be served after an invalidation, and a
malformed payload now routes through the same invalidation path as an
absent capability instead of early-returning past it.
One deliberate asymmetry: a transport failure does not invalidate. It is
not an answer, unlike a malformed body, so it must not erase what we know.
Covered by its own spec.
* fix(web): version probes and treat an unusable answer as unknown
Blocking catch (thanks @nettee): clearing `inFlight` stopped a stale promise
being *served*, but not a request already in the air from writing when it
lands. A probe issued before the boot fetch could resume afterwards and put
its `true` back over the boot answer's `false` — the entry returns on a
headless daemon and the next export hits the 501 this change exists to
gate. That was the half of "collapse the state" I had not done: the writes
were unified, but time was not.
Each probe now carries the generation it was issued under and drops its
result once a write has superseded it, so a request from the old world
cannot write into the new one.
Second, transport and parse failures are no longer the same branch. A 200
whose body will not parse IS an answer, just an unusable one, and the
contract says that means unknown — collapsing it with a dead socket let a
stale `false` survive it and keep hiding the entry. Both the boot fetch and
the direct probe now invalidate on an unparseable OK response while leaving
a known answer alone on a transport failure.
Specs: a deferred probe that resolves after a `false` is recorded, an
unparseable 200 through the boot path, and a transport failure that must
not erase what we know. The first is verified red by removing the guard.
Also replaced a spec that passed without exercising its path — a cached
answer short-circuits the direct probe, so the parse-failure case only
reaches the code through the boot fetch.
* refactor(web): read the capability from app state instead of a client store
Seven review rounds produced eleven findings, and ten of them were in the
client-side capability store rather than in what this PR is actually about.
The pattern was not bad luck: each fix added a variable to reconcile the
existing ones (`cached` -> `+inFlight` -> `+listeners` -> `+generation`),
which widened the interaction surface that produced the next finding. The
latest pair — a parsed JSON `null` throwing past the invalidation, and a
probe's `finally` clearing a newer probe's promise — are both interactions
between those variables, not defects in the gate.
The store existed to avoid prop-drilling a boolean that `App` already holds
in React state from its boot `/api/version` fetch. That trade bought a
tidier diff and paid for it with cache coherence, staleness, concurrency,
retry and subscription lifecycle — a distributed-state problem invented for
a value that was never distributed.
So the store is gone. `App` passes `appVersionInfo?.capabilities
?.slideRenderer ?? null` down the same path `metricsConsent` already takes,
and the gate reads a prop. No cache, no probe, no retry, no generations, no
subscription: every failure mode those rounds found is now absent rather
than fixed.
The three cases the contract cares about — `false` hides, `true` offers,
`null` (unknown) offers — are three renders in `FileViewer.test.tsx`. The
specs that exercised probe lifecycle went with the probe; they were testing
a mechanism, and the mechanism is what turned out to be wrong.
* fix(web): close the two paths that could still reach the 501
Both blocking notes from review, and both are consequences of how I threaded
the capability rather than of the gate itself.
- The design-system detail route mounts `FileWorkspace` directly, so it never
received the prop and `FileViewer` fell back to its `null` default. Since
unknown deliberately fails open, a headless daemon kept offering PPTX for
`system/artifacts/deck.html`. Threaded from the same App-level value the
project route uses; one source, two callers, rather than a second mechanism
for the second route.
- `appVersionInfo` came only from the one-shot boot fetch, and that helper
yields null on any failed or non-OK response. A daemon merely slow to come
up therefore left the capability unknown for the life of the page — and
unknown fails open, so the gate was defeated by a startup blip. It is now
re-asked once the daemon is reachable, which is the narrowest retry that
fixes it: keyed on the daemon becoming live rather than on a timer, and it
stops as soon as an answer arrives.
* fix(web): make the capability refetch schedule its own retry
Third incarnation of the same mistake, so worth naming: retrying on a
dependency change cannot recover from a failed probe, because a failure
changes nothing — `appVersionInfo` stays null, `daemonLive` was already
true, neither dependency moves, and no further attempt is ever issued. The
failure is sticky, `slideRendererAvailable` stays unknown, and unknown
deliberately fails open, so a headless daemon keeps offering an export it
can only answer with 501.
The retry now schedules its own next attempt instead of waiting to be
re-triggered: bounded with backoff (500ms / 1.5s / 4s / 10s), stopping the
moment an answer arrives. Bounded because an unreachable daemon is its own
visible problem and must not be polled for the life of the page.
* fix(web): cap the capability retry interval instead of the attempt count
Closing the residual case I flagged in my own audit rather than waiting for
it to be found: with a bounded number of attempts, once they were spent
`daemonLive` never flips again and nothing could ever ask, so the capability
stayed unknown for the life of the page — and unknown deliberately fails
open. That is the same hole the retry was added to close, just with a
smaller window (four consecutive failures against a daemon that had already
health-checked as live).
I argued for bounding attempts to avoid polling for the life of the page. On
reflection that traded a rare slow poll for a permanent fail-open hole,
which is the worse side of the trade. The backoff now settles into a steady
30s interval instead of stopping: it only runs while the answer is unknown,
it stops on the first answer, and it leaves no state the page cannot recover
from.
* fix: forbid caching /api/version and stop the two writers clobbering
Both blocking notes, and the first is a consequence of the change itself
that I should have caught: adding a capability turned `/api/version` from a
near-static fact about the build into an answer that differs by which daemon
is behind the URL. It was still served with no `Cache-Control`, so a browser
or proxy could reuse a pre-swap response and a stale `slideRenderer: true`
would be consumed as authoritative — reopening the export the gate exists to
hide. Now `no-store`, with a spec pinning it.
Second, `appVersionInfo` had two unsynchronised writers: the boot pass and
the capability retry. A boot response that resolved `null` after the retry
had obtained an explicit `false` would erase it and reopen the fail-open
entry; a slow but valid boot response could likewise overwrite a newer
answer. The boot pass now never writes `null` and never overwrites an answer
that is already present — a late boot response is by definition the older of
the two.
* refactor(web): ask for the capability at the point of use
Fourteen findings on this PR, and nearly all of them asserted one property:
the capability must not get stale or stuck, because unknown fails open. That
is a global property, and global properties cannot be established by local
patches — which is why each fix produced the next finding rather than
ending the series.
The property is global because the gate was reading a client-side replica of
remote state. A replica needs an initial fetch, a retry, invalidation, write
ordering, cache directives, multi-writer synchronisation, distribution to
consumers, and a refresh when liveness changes. Every one of those was a
finding here.
Deleting `runtime-capabilities.ts` last time did not end it because it moved
the replica into `App` rather than removing it. This removes it: the export
surface asks the daemon when it opens, uses the answer, and discards it with
the menu. If the daemon is swapped, the next open simply asks again.
Gone with it: the prop threaded through four components, the boot-fetch
coupling, the retry with its backoff, and the two-writer synchronisation.
Kept: the daemon-side capability, `Cache-Control: no-store`, the optional
field tolerance, and the export-reason split.
The pending window still fails open, and that is deliberate — but it is one
round trip inside a single menu opening and nothing carries it forward, so
unlike the replica it cannot get stuck.
* fix(web): start each export-menu opening from an unknown capability
The probe state outlives the menu — the component stays mounted — so the
previous opening's answer was still present on reopen. If the daemon was
replaced in between, the gate decided on an answer describing a daemon that
is gone: a stale `true` offers an export the new daemon can only 501.
My previous commit message claimed the answer was "discarded with the menu".
It was not; only the fetch was tied to the opening. Now the state is reset
when the menu opens, so the claim and the code agree.
The regression spec is not in this commit, deliberately. My first attempt
passed with and without the fix — the second response landed too quickly for
the stale window to be observable, so it asserted nothing. The second
attempt, holding that response open, fails in both directions, which means
my assumptions about the close/reopen path are wrong rather than the code
being wrong. Shipping either would be worse than shipping neither, so the
fix goes in now and the spec follows once I understand that path.
* fix(web): revalidate at the menu-to-modal handoff and state the contract
The blocking finding: clicking the PPTX row closes the menu and opens the
modal in one batched update, and the modal's confirm reads the same
capability value — so a `true` fetched at menu-open outlived its surface and
kept the confirm enabled against a daemon that may since only answer 501.
Because the two booleans flip in one batch, an OR of them never changes
across the handoff; the effect now depends on the booleans themselves, so
the handoff resets to unknown and asks again.
The capability contract is now stated once, at the probe:
1. An answer never outlives the surface opening that fetched it — every
opening, including the handoff, starts from unknown and re-asks.
2. Unknown fails open; only a resolved `false` hides the entry.
3. The daemon is the authority of last resort: a swap after the probe
resolves is unclosable client-side, and is caught by the export POST's
501, which now surfaces the no-renderer message.
Tests: the two vacuous positive cases now hold the response open and assert
against the resolved answer, and the reopen regression I owed exists. Its
three earlier failures had one cause worth recording: a stale `true` is
observationally identical to a pending probe — both fail open — so the
true-then-swap direction asserts nothing. The observable stale direction is
a lingering `false` wrongly hiding the entry on reopen, and that is what
the spec pins. Red-check matrix: removing the reset turns the reopen spec
red; reverting the deps to the OR turns the handoff spec red; mapping an
absent field to false turns the absent spec red.
* fix(web): recognise EPIPE as a proxy connection failure; drop a stray file
Both follow-ups from review.
EPIPE belongs in the errno set: the sidecar's own replay check classifies
exactly ECONNRESET and EPIPE as connection-level, and when a replay is not
applicable the EPIPE surfaces as a plain-text 502 containing `write EPIPE` —
which fell through to the generic "export request failed (502)" instead of
the daemon-down message. The classifier also had no direct tests since its
earlier ones were deleted along with the capability store, so it now has a
dedicated spec pinning both halves of its contract: every connection shape
the proxies actually emit is recognised, and JSON 5xx / errno-less text /
other statuses are not. Removing EPIPE turns exactly its case red.
`__diag.test.tsx` should never have existed in the repo: it was a local
scratch file for probing menu toggle mechanics, swept in by a `git add -A`
in the previous commit. It logged instead of asserting and leaked its fetch
stub into the rest of the file. Deleted.
* fix(web): key the capability probe to the export tab, not the popover shell
The popover shell can open on Share and sit there across a daemon swap;
switching to Export then presented the PPTX entry against the answer from
shell-open time. The export surface's opening is the tab becoming visible,
not the shell opening.
This is the terminating fix for the transition family, and the reason can
be stated as an equivalence rather than a hope: the probe's guard is now
the same predicate the consumers render under — the PPTX row mounts inside
`unifiedActionTab === 'export'` within the open popover, the modal confirm
under `pptxExportModalOpen` — and the effect's dependencies are exactly the
free variables of that predicate. A transition into visibility is by
definition a change of one of those inputs, so it necessarily re-runs the
probe. "A transition we forgot" is no longer a findable category: the
guard cannot be coarser than the render condition, because it is the
render condition.
Regression: false from the first daemon, switch to Share, swap, switch
back to Export with the new answer held open — the entry must show
(unknown fails open) and a second probe must have fired. Reverting the
guard to the popover shell turns exactly that spec red.
* fix(web): retry a failed probe within the opening; stop at a fieldless one
A probe that fails (daemon still starting, transient network) left the
whole opening at unknown, and unknown fails open — so a headless runtime
kept the entry clickable for the lifetime of that opening. Retried now with
a short bounded backoff that is cancelled by any surface transition:
nothing crosses surfaces, so the contract's first clause still holds for
the retry itself.
The distinction that keeps this from over-polling: a FAILED probe is worth
asking again, but a probe that SUCCEEDS without the capability field is an
old daemon — asking again cannot teach us more — so it stops at unknown on
the first valid answer. The two cases were previously collapsed into one
`null`, which is exactly why the earlier background-copy design kept
tripping over them; here they are separated at the only point that knows
the difference (did `fetchAppVersionInfo` return a payload or not).
Regression: first probe answers 500, the entry stays visible (unknown
fails open), the in-opening retry fires, and its held `false` then hides
the entry. Reverting to a single-shot probe turns exactly that spec red.
---------
Co-authored-by: Jason <>
48 lines
2.4 KiB
TypeScript
48 lines
2.4 KiB
TypeScript
import { describe, expect, it } from 'vitest';
|
|
|
|
import { isDaemonProxyConnectionFailure } from '../../src/runtime/daemon-proxy-failure';
|
|
|
|
// The proxies in front of the daemon answer instead of failing the fetch when
|
|
// the daemon is down, so "unreachable" arrives dressed as an HTTP response.
|
|
// This classifier is the shared path behind the daemon-down diagnostic for
|
|
// exports and project writes, so both halves of its contract are pinned:
|
|
// every connection-level shape the proxies actually emit is recognised, and
|
|
// nothing else is — widening it would relabel genuine upstream failures as an
|
|
// outage, which misleads in the opposite direction.
|
|
|
|
function response(status: number, body: string, contentType: string): Response {
|
|
return new Response(body, { status, headers: { 'content-type': contentType } });
|
|
}
|
|
|
|
describe('isDaemonProxyConnectionFailure', () => {
|
|
it.each([
|
|
['sidecar refused connection', 502, 'connect ECONNREFUSED 127.0.0.1:7456'],
|
|
['sidecar reset mid-request', 502, 'read ECONNRESET'],
|
|
// The sidecar's own replay check accepts exactly ECONNRESET and EPIPE as
|
|
// connection-level; when a replay is not applicable the EPIPE surfaces as
|
|
// this plain-text 502.
|
|
['sidecar broken pipe', 502, 'write EPIPE'],
|
|
['next dev rewrite against a dead port', 500, 'connect ECONNREFUSED 127.0.0.1:7456'],
|
|
['host unreachable', 502, 'connect EHOSTUNREACH 10.0.0.5:7456'],
|
|
['connect timeout', 502, 'connect ETIMEDOUT 10.0.0.5:7456'],
|
|
])('recognises %s', async (_name, status, body) => {
|
|
await expect(
|
|
isDaemonProxyConnectionFailure(response(status, body, 'text/plain; charset=utf-8')),
|
|
).resolves.toBe(true);
|
|
});
|
|
|
|
it.each([
|
|
// A JSON 5xx is a business/upstream response, not an outage.
|
|
['json 502 from upstream', 502, '{"error":{"message":"renderer exploded"}}', 'application/json'],
|
|
// Plain text without a connection errno proves nothing about the daemon.
|
|
['plain 502 without errno', 502, 'Bad Gateway', 'text/plain'],
|
|
// Other statuses are never the proxies' connection-failure shape.
|
|
['plain 404', 404, 'connect ECONNREFUSED 127.0.0.1:7456', 'text/plain'],
|
|
['plain 200', 200, 'connect ECONNREFUSED 127.0.0.1:7456', 'text/plain'],
|
|
])('rejects %s', async (_name, status, body, contentType) => {
|
|
await expect(
|
|
isDaemonProxyConnectionFailure(response(status, body, contentType)),
|
|
).resolves.toBe(false);
|
|
});
|
|
});
|