mirror of
https://github.com/nexu-io/open-design.git
synced 2026-09-20 06:15:06 +08:00
lefarcen
d5aa100291
* fix(collab): say a team project is downloading during its first open A member's first open of a team-shared project spends its longest stretch on the project route BEFORE any local project row exists: `PUT /collab/bootstrap` has to discover the shared owner and start the content pull first. ProjectView owns every download indicator we have — the design-files tab badge, the "syncing" empty state, the composer placeholder — but it is deliberately not mounted around the synthetic "Untitled" placeholder, so none of them can run during that window. The route therefore fell through to `resolving-deep-link` (or `loading-projects`) and rendered the shell's generic "Loading workspace…" spinner, which is indistinguishable from a wedged app. Reported from dogfooding as "no download status on first open, looks stuck" (OPEND-2095). Name that lane instead. `projectRouteSurfaceState` gains a `firstOpenTeamMaterializing` input, set only after a verified active Team context named the project — never inferred from a missing row, which is also how an unknown id looks — and refines ONLY the two loader outcomes, so every terminal outcome (dead daemon, bounded deep-link failure, resolved project) still decides exactly as before and a stuck flag cannot outlive them. The new surface reuses the same sentence ProjectView shows once it mounts, so the hand-off reads as one continuous download instead of two unrelated waits. It stays indeterminate on purpose: `vela resource pull` is awaited as a whole and reports no byte progress, so a percentage here would be invented rather than measured. * fix(collab): key the first-open indicator on the run, not the project id Review finding on #7350: the deep-link effect that owns this indicator also depends on `projects`, `projectsLoading` and `daemonLive`. Any of those changing while `bootstrapFirstOpenTeamProjectRoute` is still pending cancels that run and starts a fresh one FOR THE SAME ROUTE, which re-claims the same project id. Keyed on the id alone, the cancelled run's `finally` then retracted the LIVE run's claim while its bootstrap was still downloading — putting the anonymous "Loading workspace…" spinner back during exactly the slow path this indicator exists to explain. Give each effect run a monotonic id and let a run retract only its OWN claim. `releaseFirstOpenTeamMaterializationClaim` is exported so that rule is unit tested without mounting App, matching how `projectRouteSurfaceState` already makes the route's other decisions testable. * fix(collab): claim the first-open download only for a named project Second review finding on #7350: `firstOpenTeamContext` is `exactOpenContext ?? deepLinkContext`, and on the ambient path that is just the shell's Workspace context. It proves the caller is in an active Team; it does NOT name or authorize the requested project id. This lane is entered precisely because the local scope lookup returned not-found, so a missing, revoked, or unauthorized project URL was shown "Syncing files from the team…" right up until it became "project missing" — presenting an unverified download as fact, and changing a failure surface this PR states it leaves untouched. Gate the claim on positive evidence that something named THIS project: - a project-scoped opening witness (`exactOpenContext`) warrants it at once; - the ambient path waits for `PUT /collab/bootstrap` to answer 2xx, which is the hub confirming the project exists and is shared to this Team. `bootstrapFirstOpenTeamProjectRoute` gains an `onSharedProjectConfirmed` callback for that moment, and `firstOpenDownloadClaimIsWarranted` states the rule as a pure predicate so the unknown-project-id case is unit tested rather than argued. * fix(collab): stop reporting cancelled team pulls as failures Revert the first-open download indicator and keep the one verified fix the investigation produced. The indicator is gone because it was measured, repeatedly, not to work. Against a live vela Team workspace with two distinct accounts and two isolated daemons it never rendered once — empty project or 3 MB one, warm data root or cold, even racing the share. `proactiveContentPull` materializes the content before the user can open it and wins that race every time, so the window the indicator was built for does not exist. Its unit specs were green throughout, which is exactly why they are not kept: they would have locked in coverage for a surface nobody can reach. What the investigation did find is that a deliberately cancelled `vela` child was reported as a failure. `runVelaCommand` marks an abort with `name: 'AbortError'` / `code: 'ABORT_ERR'` and keeps a separate `timeout` termination, but nothing downstream read those markers. The proactive pull scheduler aborts as ordinary control flow — `mergeIntentUpdate` when a higher published version supersedes the fetch, `clearIntent` when an intent is superseded or satisfied — so routine version churn logged `authorized proactive team pull failed closed`, and sent at least one investigation chasing a fault that never happened. `isAbortedOperationError` now draws that line at both sinks: the `failed closed` warning in collab-sync, and the single `onError` consumer that backs proactive-content-pull's three raise sites (that module is deliberately import-free, so the distinction belongs at its consumer). Behaviour is unchanged; this is log correctness only. An aborted intent already bumped `revision` and re-looped, or was removed outright, so it never reached the retry/backoff accounting. The predicate lives in a dependency-free module on purpose. Importing it from `vela-command.ts` pulled `runtimes/env` + `runtimes/registry` into the route module graph, where `collab-sync-routes.test.ts` mocks `integrations/vela.js` with a single export — turning a passing pull into `register_failed`. That regression was caught only by running the neighbouring suite. * perf(collab): serve the team-project display read from its own cache `GET /api/workspace/projects/team` is the display read: the Home team-project grid and the deep-link "is this project shared to my team?" check both go through it. Measured against a live vela Team workspace it cost ~1.1s on EVERY call, cold and warm alike, because each one spawned `vela team-projects list` and waited on a round trip to the API. server.ts already builds `teamProjectsDisplayCache` for precisely this, and its doc comment names the route it was written for: Short-TTL, single-flight cache for the read-only DISPLAY path (GET /api/workspace/projects/team) ... Deliberately NOT used by resolveSharedProject below: the pull gate and comment/presence relays must observe an unshare immediately, so those use the uncached exact lookup. The route was wired to the uncached lister anyway, so the cache built for it went unused by it while the low-frequency invalidation poller got the cached one. This swaps them back to what that comment describes. `listTeamProjects` has exactly one consumer inside `registerCollabContextRoutes`, so the change is contained to that read. Measured on the same live workspace after the change: before 1094ms, every call after 3746ms first (daemon cold, includes the CLI capability probe) 49ms / 57ms / 52ms / 64ms thereafter Freshness was verified rather than assumed, since immediate unshare visibility is the whole reason the uncached path exists: sharing a new project from the owner client and polling the member's catalog, the project appeared within 1-2s (12 -> 13 entries) — inside the 3s TTL, and share/unshare/workspace-change all invalidate the cache explicitly on top of that. The accompanying spec pins what the display path needs from the primitive: repeat reads inside the window cost one upstream call, an explicit invalidation forces a re-read so an unshare is never hidden, and entries never cross scopes. * fix(collab): drop the helper this swap orphaned, and correct its comment Self-review after merging main, applying what #7408's review rounds taught about comments that claim more than they check. The comment said the uncached lookup "stays where its doc says it belongs: the pull gate and comment/presence relays". That is false: this registration was `teamProjectsForRequest`'s only caller anywhere in the daemon, so the swap left it with zero references. The pull gate and the relays reach `teamProjectsLister` directly and are unaffected — the claim was right about them and wrong about the wrapper. Remove the orphaned wrapper rather than leave dead code behind a swap, and say what the invalidation actually rests on: `invalidateTeamProjectCatalog` from collab-sync and the project routes, plus the per-scope invalidations beside the cache, not the 3s TTL alone. Verified while checking, since the swap is only safe if these hold: - the display cache is keyed on `[workspaceId, workspaceMemberId]`, so one client cannot be served another's list; - the swapped registration is `registerCollabContextRoutes` — display reads, not a data-access gate; - `teamProjectsLister` and `withoutLocallyUnsharedProjects` keep other callers, so removing the wrapper orphans nothing further. * fix(collab): invalidate the team catalog on a sync-intent share/unshare Review catch, and a regression this PR introduces rather than an old gap: before the swap this read was uncached, so no caller had to announce a mutation. Moving `/api/workspace/projects/team` onto the display cache makes invalidation part of the mutation contract. The `/api/workspaces/:id/projects` move path already calls `invalidateTeamProjectCatalog`. The sync-intent route — the one `od project share` and CollabDemoView drive — did not, so a catalog GET issued right after sharing could answer with the pre-share list until the freshness window expired or a hub event happened to arrive. That also made the comment added next to the swap false, since it claimed share and unshare invalidate. Fixed at the seam rather than at the two call sites. `onTeamShareStateChanged` is announced from exactly the share and unshare handlers, so `announceTeamShareStateChange` now fires it together with the catalog invalidation: persisting the visibility record and dropping the catalog belong to the same event, and pairing them means a mutation added to this module later cannot pick up one and forget the other. (#7408's review found precisely that failure — a tenth mutating export missed because the others were enumerated by hand.) Red spec is route-level, as asked: share through sync-intent, then unshare, and assert the injected `invalidateTeamProjectCatalog` fires each time. It fired zero times before this change.
89 lines
2.8 KiB
TypeScript
89 lines
2.8 KiB
TypeScript
import { describe, expect, it } from 'vitest';
|
|
|
|
import { createSwrCache } from '../src/collab/swr-cache.js';
|
|
|
|
// `GET /api/workspace/projects/team` is the DISPLAY read: the Home team-project
|
|
// grid and the deep-link "is this project shared to my team?" check both go
|
|
// through it. Measured against a live vela Team workspace it costs ~1.1s every
|
|
// single call — cold and warm alike — because each one spawns
|
|
// `vela team-projects list` and waits for a round trip to the API.
|
|
//
|
|
// server.ts already builds `teamProjectsDisplayCache` for exactly this, and its
|
|
// own doc comment names the route:
|
|
//
|
|
// "Short-TTL, single-flight cache for the read-only DISPLAY path
|
|
// (GET /api/workspace/projects/team) ... Deliberately NOT used by
|
|
// resolveSharedProject below: the pull gate and comment/presence relays must
|
|
// observe an unshare immediately, so those use the uncached exact lookup."
|
|
//
|
|
// The uncached lookup is meant for the relay/pull gate. The display route was
|
|
// wired to it anyway, so the cache built for it went unused by it.
|
|
//
|
|
// Freshness does not depend on the TTL alone: the cache is explicitly
|
|
// invalidated on share, unshare, and workspace change, so an unshare is still
|
|
// visible immediately on the surfaces that matter.
|
|
//
|
|
// This spec pins the two properties the display path needs from that cache.
|
|
describe('team-projects display cache behaviour', () => {
|
|
it('serves repeat display reads within the freshness window from one upstream call', async () => {
|
|
let upstreamCalls = 0;
|
|
const cache = createSwrCache(
|
|
async () => {
|
|
upstreamCalls += 1;
|
|
return [{ projectId: 'p-1' }];
|
|
},
|
|
() => 'scope-a',
|
|
3000,
|
|
);
|
|
|
|
await cache();
|
|
await cache();
|
|
await cache();
|
|
|
|
// Without the cache this is 3 spawns of `vela team-projects list`, i.e.
|
|
// ~3.3s of round trips for one screen.
|
|
expect(upstreamCalls).toBe(1);
|
|
});
|
|
|
|
it('re-reads upstream after an explicit invalidation, so an unshare is not hidden', async () => {
|
|
let upstreamCalls = 0;
|
|
const cache = createSwrCache(
|
|
async () => {
|
|
upstreamCalls += 1;
|
|
return [{ projectId: 'p-1' }];
|
|
},
|
|
() => 'scope-a',
|
|
3000,
|
|
);
|
|
|
|
await cache();
|
|
expect(upstreamCalls).toBe(1);
|
|
|
|
// share / unshare / workspace-change all call this in server.ts.
|
|
cache.invalidate();
|
|
await cache();
|
|
|
|
expect(upstreamCalls).toBe(2);
|
|
});
|
|
|
|
it('never shares an entry across scopes', async () => {
|
|
// A workspace switch must not be served another workspace's catalog.
|
|
const seen: string[] = [];
|
|
let scope = 'scope-a';
|
|
const cache = createSwrCache(
|
|
async () => {
|
|
seen.push(scope);
|
|
return [{ projectId: scope }];
|
|
},
|
|
() => scope,
|
|
3000,
|
|
);
|
|
|
|
await cache();
|
|
scope = 'scope-b';
|
|
await cache();
|
|
|
|
expect(seen).toEqual(['scope-a', 'scope-b']);
|
|
});
|
|
});
|