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 ? ( + ) : ( + ); }