From bc337de4be247bea6281254ff401cdd664a3c884 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 03:08:55 +0000 Subject: [PATCH] Show groups whose load failed instead of dropping them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An add whose load failed could leave nothing behind at all: the shell group row was only written after `resolveGroupTarget` had fetched the document and its latest version, so a version-less document (or any Onshape hiccup in those two calls) produced an add that silently did nothing. When the row did survive, nothing marked it as failed, and the duplicate check on POST /group blocked re-adding the document either way — leaving no way to tell whether the group was in the library. Write the shell group first, using the name the route already fetched, and record a LOAD_FAILED build issue on the group when a load throws, in both the add and reload workflows. A later successful load recomputes `buildIssues` and clears it. Surface that state: `GroupOut.isLoaded` dims a never-loaded group's title in the library list, and its page says it failed to load rather than that it has no visible elements. A group with no pinned version now links to the document instead of a `/v/placeholder` that 404s. --- src/__test_utils__/seed.ts | 8 ++- src/backend/db/schema.ts | 6 ++ src/backend/features/build-checker/issues.ts | 2 +- .../features/build-checker/routes.test.ts | 4 ++ src/backend/features/library/contract.ts | 11 +++- src/backend/features/library/db.test.ts | 37 +++++++++++- src/backend/features/library/db.ts | 24 ++++++-- src/backend/features/library/groups/routes.ts | 1 + src/backend/features/load/workflows.ts | 58 +++++++++++++++---- .../build-status/components/build-status.tsx | 10 +++- .../library/components/card-components.tsx | 9 ++- .../library/components/group-card.tsx | 1 + src/frontend/features/search/search.test.ts | 1 + src/frontend/lib/url.tsx | 1 + .../library/$libraryId/groups/$groupId.tsx | 7 ++- 15 files changed, 151 insertions(+), 29 deletions(-) diff --git a/src/__test_utils__/seed.ts b/src/__test_utils__/seed.ts index 5db24a1e..0459b62f 100644 --- a/src/__test_utils__/seed.ts +++ b/src/__test_utils__/seed.ts @@ -75,10 +75,12 @@ export async function seedUser( return id; } +/** Seeds a fully loaded group; pass overrides for e.g. an unloaded shell. */ export async function seedGroup( db: Db, id: string = TEST_GROUP_ID, - libraryId: LibraryId = TEST_LIBRARY_ID + libraryId: LibraryId = TEST_LIBRARY_ID, + overrides: Partial = {} ): Promise { await seedLibrary(db, libraryId); await db @@ -88,7 +90,9 @@ export async function seedGroup( libraryId, name: "Test Group", documentId: `doc-${id}`, - versionId: "inst-1" + versionId: "inst-1", + lastLoadedAt: Date.now(), + ...overrides }) .onConflictDoNothing(); return id; diff --git a/src/backend/db/schema.ts b/src/backend/db/schema.ts index 66009bde..46d6e0ee 100644 --- a/src/backend/db/schema.ts +++ b/src/backend/db/schema.ts @@ -19,6 +19,12 @@ export const libraries = sqliteTable("libraries", { // keyed by library id, rather than in a D1 column. }); +/** + * The `versionId` a group carries before a load pins a real one, so a group + * whose load failed still has a row that can be seen, deleted, and retried. + */ +export const PLACEHOLDER_VERSION_ID = "placeholder"; + export const group = sqliteTable( "groups", { diff --git a/src/backend/features/build-checker/issues.ts b/src/backend/features/build-checker/issues.ts index 112fac38..1d0065cd 100644 --- a/src/backend/features/build-checker/issues.ts +++ b/src/backend/features/build-checker/issues.ts @@ -79,7 +79,7 @@ export function getIssueDescription(issue: BuildIssue): string { case BuildIssueType.INSERTABLES_FAILED: return "Some child insertables failed to load"; case BuildIssueType.LOAD_FAILED: - return "This insertable failed to load"; + return "Failed to load from Onshape"; } } diff --git a/src/backend/features/build-checker/routes.test.ts b/src/backend/features/build-checker/routes.test.ts index 64669952..d604ec05 100644 --- a/src/backend/features/build-checker/routes.test.ts +++ b/src/backend/features/build-checker/routes.test.ts @@ -8,6 +8,7 @@ import { TEST_PART_STUDIO_ID, createTestApp, resetDb, + seedGroup, seedPartStudio } from "../../../__test_utils__"; import { getDb } from "../../db/client"; @@ -62,6 +63,9 @@ describe("GET /build-status", () => { }); it("reports a never-loaded entity as null", async () => { + await seedGroup(db, TEST_GROUP_ID, TEST_LIBRARY_ID, { + lastLoadedAt: null + }); await seedPartStudio(db); const res = await createTestApp().request( diff --git a/src/backend/features/library/contract.ts b/src/backend/features/library/contract.ts index 9115e31d..07b0c162 100644 --- a/src/backend/features/library/contract.ts +++ b/src/backend/features/library/contract.ts @@ -1,4 +1,8 @@ -import { ElementPath, InstancePath } from "../../lib/onshape/path"; +import { + DocumentPath, + ElementPath, + InstancePath +} from "../../lib/onshape/path"; import { ElementType } from "../../lib/onshape/element-type"; import { Vendor } from "./vendors"; @@ -24,10 +28,13 @@ export interface InsertableOut { export interface GroupOut { id: string; documentId: string; - path: InstancePath; + /** Only a `DocumentPath` until a load pins a version to link to. */ + path: InstancePath | DocumentPath; name: string; smallThumbnailUrl?: string; largeThumbnailUrl?: string; + /** False when no load has ever finished, i.e. the group is an empty shell. */ + isLoaded: boolean; insertableOrder: string[]; } diff --git a/src/backend/features/library/db.test.ts b/src/backend/features/library/db.test.ts index ae74cd6c..076204ae 100644 --- a/src/backend/features/library/db.test.ts +++ b/src/backend/features/library/db.test.ts @@ -2,7 +2,7 @@ import { env } from "cloudflare:workers"; import { asc } from "drizzle-orm"; import { beforeEach, describe, expect, it } from "vitest"; import { getDb } from "../../db/client"; -import { group } from "../../db/schema"; +import { group, PLACEHOLDER_VERSION_ID } from "../../db/schema"; import { resetDb, seedGroup, @@ -11,7 +11,7 @@ import { TEST_GROUP_ID, TEST_LIBRARY_ID } from "../../../__test_utils__"; -import { placeNewGroup, rebuildSearchDb } from "./db"; +import { getLibraryOut, placeNewGroup, rebuildSearchDb } from "./db"; const db = getDb(env.DB); @@ -96,3 +96,36 @@ describe("rebuildSearchDb", () => { expect(searchDb).toContain("WCP-0405"); }); }); + +describe("getLibraryOut", () => { + beforeEach(() => resetDb(db)); + + it("includes a shell group whose load never finished", async () => { + await seedGroup(db, TEST_GROUP_ID, TEST_LIBRARY_ID, { + versionId: PLACEHOLDER_VERSION_ID, + lastLoadedAt: null + }); + + const library = await getLibraryOut(db, TEST_LIBRARY_ID); + + expect(library.groupOrder).toEqual([TEST_GROUP_ID]); + const shell = library.groups[TEST_GROUP_ID]; + expect(shell.isLoaded).toBe(false); + // No version to link to, so the path stops at the document. + expect(shell.path).toEqual({ documentId: `doc-${TEST_GROUP_ID}` }); + }); + + it("pins a loaded group's path to its version", async () => { + await seedGroup(db); + + const library = await getLibraryOut(db, TEST_LIBRARY_ID); + + const loaded = library.groups[TEST_GROUP_ID]; + expect(loaded.isLoaded).toBe(true); + expect(loaded.path).toEqual({ + documentId: `doc-${TEST_GROUP_ID}`, + instanceId: "inst-1", + instanceType: "v" + }); + }); +}); diff --git a/src/backend/features/library/db.ts b/src/backend/features/library/db.ts index 28659ac5..95f390df 100644 --- a/src/backend/features/library/db.ts +++ b/src/backend/features/library/db.ts @@ -1,6 +1,12 @@ import { asc, eq, sql } from "drizzle-orm"; import { type Db } from "../../db/client"; -import { libraries, group, insertables, configurations } from "../../db/schema"; +import { + libraries, + group, + insertables, + configurations, + PLACEHOLDER_VERSION_ID +} from "../../db/schema"; import { LibraryId } from "./library-id"; import { InsertableOut, LibraryOut, Insertables, Groups } from "./contract"; import { ConfigurationRecord } from "../configurations/models"; @@ -56,17 +62,23 @@ export async function getLibraryOut( groupInsertables.sort((a, b) => a.name.localeCompare(b.name)); } const insertableOrder = groupInsertables.map((ins) => ins.id); + // A shell group has no version to link to, so its path stops at the + // document rather than pointing at a `/v/placeholder` that 404s. + const hasVersion = group.versionId !== PLACEHOLDER_VERSION_ID; groupsOut[group.id] = { id: group.id, documentId: group.documentId, - path: { - documentId: group.documentId, - instanceId: group.versionId, - instanceType: "v" - }, + path: hasVersion + ? { + documentId: group.documentId, + instanceId: group.versionId, + instanceType: "v" + } + : { documentId: group.documentId }, name: group.name, smallThumbnailUrl: group.smallThumbnailUrl ?? undefined, largeThumbnailUrl: group.largeThumbnailUrl ?? undefined, + isLoaded: group.lastLoadedAt !== null, insertableOrder }; } diff --git a/src/backend/features/library/groups/routes.ts b/src/backend/features/library/groups/routes.ts index 6c44cb15..bad799b0 100644 --- a/src/backend/features/library/groups/routes.ts +++ b/src/backend/features/library/groups/routes.ts @@ -224,6 +224,7 @@ groupRoutes.post( params: { groupId, documentId: body.newDocumentId, + documentName, libraryId, sessionId, selectedGroupId: body.selectedGroupId diff --git a/src/backend/features/load/workflows.ts b/src/backend/features/load/workflows.ts index 3ec63551..eacda583 100644 --- a/src/backend/features/load/workflows.ts +++ b/src/backend/features/load/workflows.ts @@ -15,7 +15,8 @@ import { import { getDocument } from "../../lib/onshape/endpoints/documents"; import { getLatestVersionId } from "../../lib/onshape/endpoints/versions"; import type { InstancePath } from "../../lib/onshape/path"; -import { group, libraries } from "../../db/schema"; +import { group, libraries, PLACEHOLDER_VERSION_ID } from "../../db/schema"; +import { addBuildIssue, BuildIssueType } from "../build-checker/issues"; import { type GroupTarget, @@ -94,6 +95,9 @@ export class LoadLibraryWorkflow extends WorkflowEntrypoint< const loaded = await loadGroup(ctx, target, forceReload); return { groupId, status: "reloaded", ...loaded }; } catch { + await ctx.step.do(`flag-failed-${groupId}`, () => + flagFailedGroup(ctx.env, groupId) + ); return { groupId, status: "failed" }; } }) @@ -112,6 +116,8 @@ export interface AddGroupParams { /** The new group's id, minted by the route. */ groupId: string; documentId: string; + /** The document's name, already fetched by the route. */ + documentName: string; libraryId: LibraryId; sessionId: string; /** An existing group to place the new group after. */ @@ -137,17 +143,21 @@ export class AddGroupWorkflow extends WorkflowEntrypoint< limit: createLimiter(LOAD_CONCURRENCY) }; + // Written before anything can fail, so an add that dies partway leaves a + // group the library still shows and an editor can retry or delete. + await step.do("create-shell-group", () => + createShellGroup(ctx.env, params) + ); + let result: GroupResult; try { const target = await resolveGroupTarget(ctx, params, ""); - - await step.do("create-shell-group", () => - createShellGroup(ctx.env, params, target.name) - ); - const loaded = await loadGroup(ctx, target, false); result = { groupId: params.groupId, status: "created", ...loaded }; } catch { + await step.do("flag-failed-group", () => + flagFailedGroup(ctx.env, params.groupId) + ); result = { groupId: params.groupId, status: "failed" }; } @@ -195,8 +205,7 @@ async function resolveGroupTarget( */ async function createShellGroup( env: AppBindings, - params: AddGroupParams, - groupName: string + params: AddGroupParams ): Promise { const db = getDb(env.DB); await db @@ -215,14 +224,41 @@ async function createShellGroup( id: params.groupId, documentId: params.documentId, libraryId: params.libraryId, - name: groupName, - // Placeholder value so failed loads can be retried - versionId: "placeholder", + name: params.documentName, + versionId: PLACEHOLDER_VERSION_ID, sortOrder }) .onConflictDoNothing(); } +/** + * Records the failure on the group row, so the library flags it instead of + * showing an empty group with nothing to explain it. A later successful load + * recomputes `buildIssues` from scratch and clears it. + */ +async function flagFailedGroup( + env: AppBindings, + groupId: string +): Promise { + const db = getDb(env.DB); + const row = await db + .select({ buildIssues: group.buildIssues }) + .from(group) + .where(eq(group.id, groupId)) + .get(); + if (!row) { + return; + } + await db + .update(group) + .set({ + buildIssues: addBuildIssue(row.buildIssues, { + type: BuildIssueType.LOAD_FAILED + }) + }) + .where(eq(group.id, groupId)); +} + /** Rebuild the library's search index and bump its cache version. */ async function finalizeLibrary( env: AppBindings, diff --git a/src/frontend/features/build-status/components/build-status.tsx b/src/frontend/features/build-status/components/build-status.tsx index 9ce4c72b..eec7b08f 100644 --- a/src/frontend/features/build-status/components/build-status.tsx +++ b/src/frontend/features/build-status/components/build-status.tsx @@ -36,7 +36,8 @@ import { BuildIssueType, getIssueDescription, getIssueSeverity, - getMaxSeverity + getMaxSeverity, + hasBuildIssue } from "@backend/features/build-checker/issues"; import { GroupBuildStatus, @@ -96,7 +97,12 @@ function useGroupBuildIssues( const hasUnhidden = groupStatus.insertableOrder.some( (id) => insertableStatuses?.[id]?.isVisible ); - if (hasUnhidden) { + // A group that never loaded has no insertables to unhide, so the failure + // is the whole story. + if ( + hasUnhidden || + hasBuildIssue(groupStatus.buildIssues, BuildIssueType.LOAD_FAILED) + ) { return groupStatus.buildIssues; } return addBuildIssue(groupStatus.buildIssues, { diff --git a/src/frontend/features/library/components/card-components.tsx b/src/frontend/features/library/components/card-components.tsx index 3c8c64b5..b0affa3b 100644 --- a/src/frontend/features/library/components/card-components.tsx +++ b/src/frontend/features/library/components/card-components.tsx @@ -20,7 +20,11 @@ import { CardThumbnail, type ThumbnailTarget } from "../../thumbnails/components/thumbnail"; -import { ConfigurablePath, InstancePath } from "@backend/lib/onshape/path"; +import { + ConfigurablePath, + DocumentPath, + InstancePath +} from "@backend/lib/onshape/path"; import { openCannotDeriveAssemblyAlert } from "../../../components/alerts"; import { useInsertMutation, @@ -34,7 +38,8 @@ import { useSearch } from "@tanstack/react-router"; import { RequireAccessLevel } from "../../auth/access-level"; interface OpenDocumentItemsProps { - path: InstancePath | ConfigurablePath; + /** Any Onshape path; a shell group's stops at the document. */ + path: DocumentPath | InstancePath | ConfigurablePath; } /** * Menu items which can be used to open or copy a link to a document. diff --git a/src/frontend/features/library/components/group-card.tsx b/src/frontend/features/library/components/group-card.tsx index 230c0232..b4f1e3f8 100644 --- a/src/frontend/features/library/components/group-card.tsx +++ b/src/frontend/features/library/components/group-card.tsx @@ -47,6 +47,7 @@ export function GroupCard(props: GroupCardProps): ReactNode { left={ !!insertable); if (groupInsertables.length === 0) { - return ( + return group.isLoaded ? ( + ) : ( + ); }