diff --git a/drizzle/0007_resume_group.sql b/drizzle/0007_resume_group.sql new file mode 100644 index 000000000..8c2dd9bff --- /dev/null +++ b/drizzle/0007_resume_group.sql @@ -0,0 +1,9 @@ +/* + Where a caller resumes moves from the browser to their user row, beside the + library it belongs to, so the entry redirect can compute the whole landing url + rather than the app bouncing itself into the group after it loads. + + Starts null, which is the library itself: nobody's last group is known until + they open one. +*/ +ALTER TABLE `users` ADD `group_id` text; diff --git a/drizzle/meta/0007_snapshot.json b/drizzle/meta/0007_snapshot.json new file mode 100644 index 000000000..6e6d47e71 --- /dev/null +++ b/drizzle/meta/0007_snapshot.json @@ -0,0 +1,510 @@ +{ + "version": "6", + "dialect": "sqlite", + "id": "7f0e79d4-3077-4a1f-aeab-4d98a104be46", + "prevId": "2416035c-6b28-4014-a7b3-bdc33f803962", + "tables": { + "configurations": { + "name": "configurations", + "columns": { + "id": { + "name": "id", + "type": "text", + "primaryKey": true, + "notNull": true, + "autoincrement": false + }, + "parameters": { + "name": "parameters", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": "'[]'" + }, + "records": { + "name": "records", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": "'[]'" + }, + "build_issues": { + "name": "build_issues", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": "'[]'" + } + }, + "indexes": {}, + "foreignKeys": { + "configurations_id_insertables_id_fk": { + "name": "configurations_id_insertables_id_fk", + "tableFrom": "configurations", + "tableTo": "insertables", + "columnsFrom": ["id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "checkConstraints": {} + }, + "favorites": { + "name": "favorites", + "columns": { + "id": { + "name": "id", + "type": "text", + "primaryKey": true, + "notNull": true, + "autoincrement": false + }, + "user_id": { + "name": "user_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "library_id": { + "name": "library_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "insertable_id": { + "name": "insertable_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "default_configuration": { + "name": "default_configuration", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "sort_order": { + "name": "sort_order", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": 0 + } + }, + "indexes": { + "favorites_user_id_library_id_insertable_id_unique": { + "name": "favorites_user_id_library_id_insertable_id_unique", + "columns": ["user_id", "library_id", "insertable_id"], + "isUnique": true + } + }, + "foreignKeys": { + "favorites_user_id_users_id_fk": { + "name": "favorites_user_id_users_id_fk", + "tableFrom": "favorites", + "tableTo": "users", + "columnsFrom": ["user_id"], + "columnsTo": ["id"], + "onDelete": "no action", + "onUpdate": "no action" + }, + "favorites_library_id_libraries_id_fk": { + "name": "favorites_library_id_libraries_id_fk", + "tableFrom": "favorites", + "tableTo": "libraries", + "columnsFrom": ["library_id"], + "columnsTo": ["id"], + "onDelete": "no action", + "onUpdate": "no action" + }, + "favorites_insertable_id_insertables_id_fk": { + "name": "favorites_insertable_id_insertables_id_fk", + "tableFrom": "favorites", + "tableTo": "insertables", + "columnsFrom": ["insertable_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "checkConstraints": {} + }, + "groups": { + "name": "groups", + "columns": { + "id": { + "name": "id", + "type": "text", + "primaryKey": true, + "notNull": true, + "autoincrement": false + }, + "library_id": { + "name": "library_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "name": { + "name": "name", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "document_id": { + "name": "document_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "version_id": { + "name": "version_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "sort_alphabetically": { + "name": "sort_alphabetically", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": false + }, + "sort_order": { + "name": "sort_order", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": 0 + }, + "small_thumbnail_url": { + "name": "small_thumbnail_url", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "large_thumbnail_url": { + "name": "large_thumbnail_url", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "build_issues": { + "name": "build_issues", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": "'[]'" + }, + "last_loaded_at": { + "name": "last_loaded_at", + "type": "integer", + "primaryKey": false, + "notNull": false, + "autoincrement": false + } + }, + "indexes": { + "groups_document_id_library_id_unique": { + "name": "groups_document_id_library_id_unique", + "columns": ["document_id", "library_id"], + "isUnique": true + } + }, + "foreignKeys": { + "groups_library_id_libraries_id_fk": { + "name": "groups_library_id_libraries_id_fk", + "tableFrom": "groups", + "tableTo": "libraries", + "columnsFrom": ["library_id"], + "columnsTo": ["id"], + "onDelete": "no action", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "checkConstraints": {} + }, + "insertables": { + "name": "insertables", + "columns": { + "id": { + "name": "id", + "type": "text", + "primaryKey": true, + "notNull": true, + "autoincrement": false + }, + "element_id": { + "name": "element_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "group_id": { + "name": "group_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "document_id": { + "name": "document_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "library_id": { + "name": "library_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "name": { + "name": "name", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "element_type": { + "name": "element_type", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "microversion_id": { + "name": "microversion_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "is_visible": { + "name": "is_visible", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": false + }, + "is_open_composite": { + "name": "is_open_composite", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": false + }, + "supports_fasten": { + "name": "supports_fasten", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": false + }, + "index_configurations": { + "name": "index_configurations", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": false + }, + "version_id": { + "name": "version_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "sort_order": { + "name": "sort_order", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": 0 + }, + "vendors": { + "name": "vendors", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": "'[]'" + }, + "small_thumbnail_url": { + "name": "small_thumbnail_url", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "large_thumbnail_url": { + "name": "large_thumbnail_url", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "fasten_info": { + "name": "fasten_info", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "part_data": { + "name": "part_data", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "build_issues": { + "name": "build_issues", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": "'[]'" + }, + "last_loaded_at": { + "name": "last_loaded_at", + "type": "integer", + "primaryKey": false, + "notNull": false, + "autoincrement": false + } + }, + "indexes": {}, + "foreignKeys": { + "insertables_group_id_groups_id_fk": { + "name": "insertables_group_id_groups_id_fk", + "tableFrom": "insertables", + "tableTo": "groups", + "columnsFrom": ["group_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + }, + "insertables_library_id_libraries_id_fk": { + "name": "insertables_library_id_libraries_id_fk", + "tableFrom": "insertables", + "tableTo": "libraries", + "columnsFrom": ["library_id"], + "columnsTo": ["id"], + "onDelete": "no action", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "checkConstraints": {} + }, + "libraries": { + "name": "libraries", + "columns": { + "id": { + "name": "id", + "type": "text", + "primaryKey": true, + "notNull": true, + "autoincrement": false + }, + "cache_version": { + "name": "cache_version", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": 0 + } + }, + "indexes": {}, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "checkConstraints": {} + }, + "users": { + "name": "users", + "columns": { + "id": { + "name": "id", + "type": "text", + "primaryKey": true, + "notNull": true, + "autoincrement": false + }, + "theme": { + "name": "theme", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": "'system'" + }, + "library_id": { + "name": "library_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": "'frc-design-lib'" + }, + "group_id": { + "name": "group_id", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + } + }, + "indexes": {}, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "checkConstraints": {} + } + }, + "views": {}, + "enums": {}, + "_meta": { + "schemas": {}, + "tables": {}, + "columns": {} + }, + "internal": { + "indexes": {} + } +} diff --git a/drizzle/meta/_journal.json b/drizzle/meta/_journal.json index 391bafada..6164e539f 100644 --- a/drizzle/meta/_journal.json +++ b/drizzle/meta/_journal.json @@ -1,55 +1,62 @@ { - "version": "7", - "dialect": "sqlite", - "entries": [ - { - "idx": 0, - "version": "6", - "when": 1785982272297, - "tag": "0000_init", - "breakpoints": true - }, - { - "idx": 1, - "version": "6", - "when": 1786201159430, - "tag": "0001_sad_arclight", - "breakpoints": true - }, - { - "idx": 2, - "version": "6", - "when": 1786201159431, - "tag": "0002_configuration_records", - "breakpoints": true - }, - { - "idx": 3, - "version": "6", - "when": 1786511719906, - "tag": "0003_drop_search_db", - "breakpoints": true - }, - { - "idx": 4, - "version": "6", - "when": 1786511719907, - "tag": "0004_explicit_thumbnail_urls", - "breakpoints": true - }, - { - "idx": 5, - "version": "6", - "when": 1786511719908, - "tag": "0005_index_configurations", - "breakpoints": true - }, - { - "idx": 6, - "version": "6", - "when": 1787265951428, - "tag": "0006_split_part_data", - "breakpoints": true - } - ] -} \ No newline at end of file + "version": "7", + "dialect": "sqlite", + "entries": [ + { + "idx": 0, + "version": "6", + "when": 1785982272297, + "tag": "0000_init", + "breakpoints": true + }, + { + "idx": 1, + "version": "6", + "when": 1786201159430, + "tag": "0001_sad_arclight", + "breakpoints": true + }, + { + "idx": 2, + "version": "6", + "when": 1786201159431, + "tag": "0002_configuration_records", + "breakpoints": true + }, + { + "idx": 3, + "version": "6", + "when": 1786511719906, + "tag": "0003_drop_search_db", + "breakpoints": true + }, + { + "idx": 4, + "version": "6", + "when": 1786511719907, + "tag": "0004_explicit_thumbnail_urls", + "breakpoints": true + }, + { + "idx": 5, + "version": "6", + "when": 1786511719908, + "tag": "0005_index_configurations", + "breakpoints": true + }, + { + "idx": 6, + "version": "6", + "when": 1787265951428, + "tag": "0006_split_part_data", + "breakpoints": true + }, + { + "idx": 7, + "version": "6", + "when": 1787265951429, + "tag": "0007_resume_group", + "breakpoints": true + } + ] +} diff --git a/src/__test_utils__/configuration-fixtures.ts b/src/__test_utils__/configuration-fixtures.ts index ae9be4b51..d99e6e927 100644 --- a/src/__test_utils__/configuration-fixtures.ts +++ b/src/__test_utils__/configuration-fixtures.ts @@ -87,7 +87,7 @@ export function configurationRecord( overrides: Partial = {} ): ConfigurationRecord { return { - configuration: {}, + canonicalConfiguration: "", hasMultipleParts: false, isOpenComposite: false, ...overrides diff --git a/src/__test_utils__/test-app.ts b/src/__test_utils__/test-app.ts index 82f352d75..4b886d034 100644 --- a/src/__test_utils__/test-app.ts +++ b/src/__test_utils__/test-app.ts @@ -19,8 +19,8 @@ export interface TestAppOptions { } /** - * The real app from `createApp`, with a stub caller in place of - * `productionCaller`. Drive it with `app.request(path, init, env)`. + * The real app from `createApp`, answering its auth questions from `options` + * instead of `productionAuth`. Drive it with `app.request(path, init, env)`. */ export function createTestApp(options: TestAppOptions = {}) { const signedIn = options.signedIn ?? true; diff --git a/src/backend/app.ts b/src/backend/app.ts index f5cc5ae80..e6cdddbda 100644 --- a/src/backend/app.ts +++ b/src/backend/app.ts @@ -1,5 +1,5 @@ /** - * Composition root: binds the caller onto every request and mounts each + * Composition root: binds the request's auth onto every request and mounts each * feature's routes. Everything it wires lives in a feature or in lib. */ import { accessRoutes, authRoutes } from "./features/auth/routes"; @@ -14,7 +14,7 @@ import { settingsRoutes } from "./features/settings/routes"; import { thumbnailRoutes } from "./features/thumbnails/routes"; import { logger } from "hono/logger"; import { cacheMiddleware } from "./lib/cache"; -import { bindCaller, getApp, type CallerFactory } from "./lib/context"; +import { bindAuth, getApp, type AuthResolver } from "./lib/context"; import { errorHandler } from "./lib/errors"; const apiRoutes = [ @@ -29,14 +29,14 @@ const apiRoutes = [ buildStatusRoutes ]; -export function createApp(makeCaller: CallerFactory) { +export function createApp(resolveAuth: AuthResolver) { const app = getApp(); // Reaches Workers Logs, which wrangler.jsonc enables. Only /init, /api/* // and /auth/* run the Worker, so static assets are not logged. app.use("*", logger()); - app.use("*", bindCaller(makeCaller)); + app.use("*", bindAuth(resolveAuth)); for (const routes of apiRoutes) { app.route("/api", routes); diff --git a/src/backend/db/schema.ts b/src/backend/db/schema.ts index 46d6e0ee0..12e0c1205 100644 --- a/src/backend/db/schema.ts +++ b/src/backend/db/schema.ts @@ -5,9 +5,9 @@ import { LibraryId } from "../features/library/library-id"; import { DEFAULT_SETTINGS, Theme } from "../features/settings/settings"; import { Vendor } from "../features/library/vendors"; import { - ParameterValues, ConfigurationParameter, ConfigurationRecord, + ParameterValues, PartMetadata } from "../features/configurations/models"; import { BuildIssue } from "../features/build-checker/issues"; @@ -145,7 +145,10 @@ export const users = sqliteTable("users", { libraryId: text("library_id") .$type() .notNull() - .default(DEFAULT_SETTINGS.libraryId) + .default(DEFAULT_SETTINGS.libraryId), + // The group last opened in that library, which entry resumes in. Null for + // the library itself; a stale one resolves to that, so it is never cleaned. + groupId: text("group_id") }); export const favorites = sqliteTable( @@ -164,6 +167,9 @@ export const favorites = sqliteTable( insertableId: text("insertable_id") .notNull() .references(() => insertables.id, { onDelete: "cascade" }), + // The selection the favorite opens with, as the user made it: a + // canonical one would drop a default-valued or hidden parameter, + // including a string they typed. Null for the element's own default. defaultConfiguration: text("default_configuration", { mode: "json" }).$type(), diff --git a/src/backend/features/auth/guards.ts b/src/backend/features/auth/guards.ts index 36fa17982..5a117104f 100644 --- a/src/backend/features/auth/guards.ts +++ b/src/backend/features/auth/guards.ts @@ -1,16 +1,14 @@ /** The two gates routes mount: signed in to Onshape at all, and on the admin team. */ import type { MiddlewareHandler } from "hono"; -import { handledError } from "../../lib/api-error"; -import { HttpStatus } from "http-status-ts"; +import { forbiddenError, signInRequiredError } from "../../lib/api-error"; import type { AppContext, AppContextEnv } from "../../lib/context"; import { hasEditorAccess } from "./access-level"; -import { isSignedIn } from "./caller"; +import { isSignedIn } from "./request-auth"; async function requireSignIn(c: AppContext): Promise { if (!(await isSignedIn(c))) { - throw handledError( - "You must be signed in to Onshape to use this functionality", - HttpStatus.UNAUTHORIZED + throw signInRequiredError( + "You must be signed in to Onshape to use this functionality" ); } } @@ -33,9 +31,8 @@ export const requireEditorMiddleware: MiddlewareHandler = async ( ) => { await requireSignIn(c); if (!hasEditorAccess(await c.var.getAccessLevel())) { - throw handledError( - "You must be on the admin team to use this functionality", - HttpStatus.FORBIDDEN + throw forbiddenError( + "You must be on the admin team to use this functionality" ); } await next(); diff --git a/src/backend/features/auth/caller.test.ts b/src/backend/features/auth/request-auth.test.ts similarity index 94% rename from src/backend/features/auth/caller.test.ts rename to src/backend/features/auth/request-auth.test.ts index 8a5d0b827..4483d54c4 100644 --- a/src/backend/features/auth/caller.test.ts +++ b/src/backend/features/auth/request-auth.test.ts @@ -2,11 +2,11 @@ import { env } from "cloudflare:workers"; import { env as processEnv } from "process"; import { afterEach, describe, expect, it } from "vitest"; import { AccessLevel } from "./access-level"; -import { productionCaller } from "./caller"; +import { productionAuth } from "./request-auth"; import { createApp } from "../../app"; import { jsonRequest } from "../../../__test_utils__"; -const app = createApp(productionCaller); +const app = createApp(productionAuth); /** What the real caller resolves for a request carrying no Onshape session. */ async function getMaxAccessLevel(override?: AccessLevel): Promise { diff --git a/src/backend/features/auth/caller.ts b/src/backend/features/auth/request-auth.ts similarity index 86% rename from src/backend/features/auth/caller.ts rename to src/backend/features/auth/request-auth.ts index 4ff2fc157..42d2e2a04 100644 --- a/src/backend/features/auth/caller.ts +++ b/src/backend/features/auth/request-auth.ts @@ -1,6 +1,7 @@ /** - * Resolves who is calling from their session, memoized in KV. `createApp` binds - * `productionCaller` onto every request; guards and routes read it via `c.var`. + * Answers a request's auth questions from its session, memoized in KV. + * `createApp` binds `productionAuth` onto every request; guards and routes ask + * it through `c.var`. */ import { env as processEnv } from "process"; import { OAuthApi } from "../../lib/onshape/client"; @@ -9,7 +10,7 @@ import { getSessionInfo, getUserId } from "../../lib/onshape/endpoints/users"; -import { type AppContext, type CallerFactory } from "../../lib/context"; +import { type AppContext, type AuthResolver } from "../../lib/context"; import { AccessLevel } from "./access-level"; import { getOauthClient, @@ -17,6 +18,7 @@ import { TOKEN_ENDPOINT } from "./onshape-oauth"; import { + accessLevelKey, getSession, getSessionCompanyId, getSessionId, @@ -108,34 +110,29 @@ function getAccessLevelOverride(c: AppContext): AccessLevel | undefined { return c.env.VITE_ACCESS_LEVEL_OVERRIDE as AccessLevel | undefined; } +/** Whether the session resolves to tokens Onshape will take. */ +async function hasOnshapeSession(c: AppContext): Promise { + try { + await c.var.getOnshapeApi(); + return true; + } catch { + return false; + } +} + /** * Whether the caller has a valid Onshape session, memoized on the request. - * `FORCE_SIGNED_IN` forces it true for testing. + * `FORCE_SIGNED_IN` stands in for the session it cannot have in development. */ export async function isSignedIn(c: AppContext): Promise { const cached = c.get("signedIn"); if (cached !== undefined) return cached; - let signedIn: boolean; - if (isForceSignedIn(c)) { - signedIn = true; - } else { - try { - await c.var.getOnshapeApi(); - signedIn = true; - } catch { - signedIn = false; - } - } - + const signedIn = isForceSignedIn(c) || (await hasOnshapeSession(c)); c.set("signedIn", signedIn); return signedIn; } -function accessLevelKey(sessionId: string): string { - return `access-level:${sessionId}`; -} - /** Returns the caller's access level, memoized in KV by session. */ export async function getCachedAccessLevel( c: AppContext @@ -156,10 +153,10 @@ export async function getCachedAccessLevel( } /** - * Production wiring. getUserId only runs behind requireSignInMiddleware; + * The real answers. getUserId only runs behind requireSignInMiddleware; * getAccessLevel falls back to USER for anyone without a real Onshape session. */ -export const productionCaller: CallerFactory = (c) => ({ +export const productionAuth: AuthResolver = (c) => ({ getOnshapeApi: () => getOnshapeApi(c), getUserId: () => { // FORCE_SIGNED_IN has no real Onshape session; use a stable fake id. diff --git a/src/backend/features/auth/routes.test.ts b/src/backend/features/auth/routes.test.ts index 57cdcdfb3..a18232915 100644 --- a/src/backend/features/auth/routes.test.ts +++ b/src/backend/features/auth/routes.test.ts @@ -46,3 +46,58 @@ describe("GET /access-data", () => { }); }); }); + +describe("GET /auth/sign-out", () => { + const SESSION_COOKIE = "frc-design-app-cookie"; + + /** A signed-in session, as the OAuth callback would have left it. */ + async function seedSession(sessionId: string) { + await env.KV.put( + `tokens:${sessionId}`, + JSON.stringify({ + accessToken: "a", + refreshToken: "r", + expiresAt: Date.now() + 10000 + }) + ); + await env.KV.put(`access-level:${sessionId}`, AccessLevel.ADMIN); + } + + function signOut(redirectUrl: string, sessionId?: string) { + return createTestApp().request( + `/auth/sign-out?redirectUrl=${encodeURIComponent(redirectUrl)}`, + { + method: "GET", + headers: sessionId + ? { Cookie: `${SESSION_COOKIE}=${sessionId}` } + : {}, + redirect: "manual" + }, + env + ); + } + + it("drops the session and everything keyed to it", async () => { + await seedSession("session-1"); + + const res = await signOut("/app/library/frc-design-lib", "session-1"); + + expect(res.status).toBe(302); + expect(res.headers.get("Location")).toBe("/app/library/frc-design-lib"); + expect(await env.KV.get("tokens:session-1")).toBeNull(); + expect(await env.KV.get("access-level:session-1")).toBeNull(); + expect(res.headers.get("Set-Cookie")).toContain(`${SESSION_COOKIE}=;`); + }); + + it("lands home rather than forwarding the caller offsite", async () => { + for (const redirectUrl of ["https://example.com", "//example.com"]) { + const res = await signOut(redirectUrl); + expect(res.headers.get("Location")).toBe("/"); + } + }); + + it("signs out a caller who has no session to lose", async () => { + const res = await signOut("/app"); + expect(res.status).toBe(302); + }); +}); diff --git a/src/backend/features/auth/routes.ts b/src/backend/features/auth/routes.ts index f9e1f5bf9..7fbde5a78 100644 --- a/src/backend/features/auth/routes.ts +++ b/src/backend/features/auth/routes.ts @@ -3,8 +3,9 @@ import { internalError } from "../../lib/api-error"; import { getApp } from "../../lib/context"; import { cacheMiddleware } from "../../lib/cache"; import { type AccessData } from "./access-level"; -import { isSignedIn } from "./caller"; +import { isSignedIn } from "./request-auth"; import { doCallback, doSignIn } from "./onshape-oauth"; +import { endSession } from "./session"; /** The OAuth redirects, mounted at /auth. */ export const authRoutes = getApp(); @@ -44,6 +45,23 @@ authRoutes.get("/sign-in", async (c) => { return c.redirect(authorizationUrl); }); +/** + * Standalone only: inside Onshape the panel's session is Onshape's to end. + * Where the caller lands is theirs to say, as long as it is this app. + */ +authRoutes.get("/sign-out", async (c) => { + await endSession(c); + return c.redirect(getLocalRedirect(c.req.query("redirectUrl"))); +}); + +/** A path within the app, so the parameter cannot forward a caller offsite. */ +function getLocalRedirect(redirectUrl: string | undefined): string { + if (!redirectUrl?.startsWith("/") || redirectUrl.startsWith("//")) { + return "/"; + } + return redirectUrl; +} + authRoutes.get("/callback", async (c) => { return doCallback(c); }); diff --git a/src/backend/features/auth/session.ts b/src/backend/features/auth/session.ts index f88f9afed..46eaf1d02 100644 --- a/src/backend/features/auth/session.ts +++ b/src/backend/features/auth/session.ts @@ -1,7 +1,7 @@ /** Session cookie plus the KV records it keys: the session and login state. */ import { HttpStatus } from "http-status-ts"; import { internalError } from "../../lib/api-error"; -import { getCookie, setCookie } from "hono/cookie"; +import { deleteCookie, getCookie, setCookie } from "hono/cookie"; import { type AppContext } from "../../lib/context"; const SESSION_COOKIE = "frc-design-app-cookie"; @@ -40,6 +40,31 @@ function sessionKey(sessionId: string): string { return `tokens:${sessionId}`; } +/** Keyed by session, so it is dropped along with one. */ +export function accessLevelKey(sessionId: string): string { + return `access-level:${sessionId}`; +} + +/** + * Signs the caller out: the tokens and what was resolved from them go, and the + * cookie with them, so the next request is simply a stranger's. + */ +export async function endSession(c: AppContext): Promise { + const sessionId = getCookie(c, SESSION_COOKIE); + if (sessionId) { + await Promise.all([ + c.env.KV.delete(sessionKey(sessionId)), + c.env.KV.delete(accessLevelKey(sessionId)) + ]); + } + // Matched to how it was set, or the browser keeps the cookie. + deleteCookie(c, SESSION_COOKIE, { + path: "/", + secure: true, + sameSite: "None" + }); +} + export async function saveSession( kv: KVNamespace, sessionId: string, diff --git a/src/backend/features/build-checker/checks.test.ts b/src/backend/features/build-checker/checks.test.ts index 6cdb91440..8a04a6ec9 100644 --- a/src/backend/features/build-checker/checks.test.ts +++ b/src/backend/features/build-checker/checks.test.ts @@ -5,7 +5,6 @@ import { BuildIssueType } from "./issues"; import { DEFAULT_CANONICAL_CONFIGURATION } from "../configurations/canonical"; import { thumbnailUrl } from "../thumbnails/keys"; import { checkGroup, checkInsertable } from "./checks"; -import { configurationRecord } from "../../../__test_utils__/configuration-fixtures"; /** What uploadThumbnails returns: the element's default configuration. */ const THUMBNAILS: ThumbnailUrls = { @@ -56,14 +55,10 @@ describe("checkGroup", () => { }); }); -/** Only the part number matters to these checks. */ -const record = (partNumber?: string) => configurationRecord({ partNumber }); - describe("checkInsertable", () => { const HEALTHY_INSERTABLE = { vendors: [Vendor.REV], - thumbnailUrls: THUMBNAILS, - probes: [record("217-2600")] + thumbnailUrls: THUMBNAILS }; it("returns no issues when vendors are parsed and thumbnails generated", () => { @@ -85,36 +80,4 @@ describe("checkInsertable", () => { }); expect(issues).toEqual([{ type: BuildIssueType.THUMBNAIL_FAILED }]); }); - - it("warns when a vendor part indexed without a part number", () => { - const issues = checkInsertable({ - ...HEALTHY_INSERTABLE, - probes: [record(), record()] - }); - expect(issues).toEqual([{ type: BuildIssueType.NO_PART_NUMBER }]); - }); - - it("does not warn when only some configurations lack one", () => { - const issues = checkInsertable({ - ...HEALTHY_INSERTABLE, - probes: [record(), record("217-2600")] - }); - expect(issues).toEqual([]); - }); - - // Nobody sells it, so having no part number is the expected state. - it("does not warn about a custom part", () => { - const issues = checkInsertable({ - ...HEALTHY_INSERTABLE, - vendors: [Vendor.CUSTOM], - probes: [record()] - }); - expect(issues).toEqual([]); - }); - - // Nothing was probed, so there is nothing to conclude. - it("does not warn when the insertable is not indexed", () => { - const issues = checkInsertable({ ...HEALTHY_INSERTABLE, probes: [] }); - expect(issues).toEqual([]); - }); }); diff --git a/src/backend/features/build-checker/checks.ts b/src/backend/features/build-checker/checks.ts index 1643e83c3..38ae9132e 100644 --- a/src/backend/features/build-checker/checks.ts +++ b/src/backend/features/build-checker/checks.ts @@ -1,7 +1,6 @@ import { ThumbnailUrls } from "../thumbnails/types"; -import { Vendor, isCustomPart } from "../library/vendors"; +import { Vendor } from "../library/vendors"; import { addBuildIssue, BuildIssue, BuildIssueType } from "./issues"; -import type { PartMetadata } from "../configurations/models"; interface GroupCheckInput { /** Whether the Onshape document has a designated thumbnail tab/element. */ @@ -42,8 +41,6 @@ interface InsertableCheckInput { vendors: Vendor[]; /** The uploaded thumbnail URLs, or `null` when generation failed. */ thumbnailUrls: ThumbnailUrls | null; - /** Every probe of the element: its own, plus one per indexed configuration. */ - probes: (PartMetadata | null)[]; } /** @@ -63,28 +60,5 @@ export function checkInsertable(input: InsertableCheckInput): BuildIssue[] { issues = addBuildIssue(issues, { type: BuildIssueType.NO_VENDORS }); } - issues = addBuildIssue( - issues, - ...checkIndexedPartNumber(input.vendors, input.probes) - ); - return issues; } - -/** - * A custom part is expected to have no part number; anything a vendor sells - * should have one in at least one configuration. - */ -/** Every probe of an element: its own, plus one per indexed configuration. */ -export function checkIndexedPartNumber( - vendors: Vendor[], - probes: (PartMetadata | null)[] -): BuildIssue[] { - const read = probes.filter((probe) => probe !== null); - if (isCustomPart(vendors) || read.length === 0) { - return []; - } - return read.some((probe) => probe.partNumber) - ? [] - : [{ type: BuildIssueType.NO_PART_NUMBER }]; -} diff --git a/src/backend/features/build-checker/contract.ts b/src/backend/features/build-checker/contract.ts index 1e35f8a7f..0b3afe963 100644 --- a/src/backend/features/build-checker/contract.ts +++ b/src/backend/features/build-checker/contract.ts @@ -4,6 +4,8 @@ import { ElementType } from "../../lib/onshape/element-type"; import { Vendor } from "../library/vendors"; export interface ConfigurationBuildStatus { + /** The insertable's id, which a configuration row is keyed by. */ + id: string; buildIssues: BuildIssue[]; parameters: ConfigurationParameter[]; } diff --git a/src/backend/features/build-checker/issues.ts b/src/backend/features/build-checker/issues.ts index 1d0065cd1..0c948a65e 100644 --- a/src/backend/features/build-checker/issues.ts +++ b/src/backend/features/build-checker/issues.ts @@ -21,7 +21,6 @@ export enum BuildIssueType { THUMBNAIL_FAILED = "thumbnail-failed", NO_THUMBNAIL_TAB = "no-thumbnail-tab", NO_VENDORS = "no-vendors", - NO_PART_NUMBER = "no-part-number", NO_PARTS = "no-parts", NO_UNHIDDEN_INSERTABLES = "no-unhidden-insertables", CONFIGURATION_LIMIT_EXCEEDED = "configuration-limit-exceeded", @@ -43,7 +42,6 @@ export type BuildIssue = | BuildIssueOf | BuildIssueOf | BuildIssueOf - | BuildIssueOf | BuildIssueOf | BuildIssueOf | BuildIssueOf @@ -62,8 +60,6 @@ export function getIssueDescription(issue: BuildIssue): string { return "No thumbnail tab set"; case BuildIssueType.NO_VENDORS: return "No vendors could be parsed"; - case BuildIssueType.NO_PART_NUMBER: - return "No part number in any configuration, though a vendor sells this"; case BuildIssueType.NO_PARTS: return "This part studio has no parts"; case BuildIssueType.NO_UNHIDDEN_INSERTABLES: @@ -97,7 +93,6 @@ export function getIssueSeverity(issue: BuildIssue): BuildIssueSeverity { case BuildIssueType.NO_THUMBNAIL_TAB: case BuildIssueType.CONFIGURATION_LIMIT_EXCEEDED: case BuildIssueType.MANUAL_INDEXING_REQUIRED: - case BuildIssueType.NO_PART_NUMBER: return BuildIssueSeverity.WARNING; case BuildIssueType.NO_VENDORS: return BuildIssueSeverity.INFO; diff --git a/src/backend/features/build-checker/routes.ts b/src/backend/features/build-checker/routes.ts index b2b9dace7..a5623ea7e 100644 --- a/src/backend/features/build-checker/routes.ts +++ b/src/backend/features/build-checker/routes.ts @@ -17,6 +17,8 @@ export const buildStatusRoutes = getApp(); buildStatusRoutes.get( "/build-status" + libraryRoute(), requireEditorMiddleware, + // The same for every editor, but only for an editor: a shared cache would + // hand it to whoever asked for the url next. cacheMiddleware(CachePolicy.PRIVATE_CACHE), async (c) => { const libraryId = getLibraryParam(c); @@ -82,7 +84,6 @@ buildStatusRoutes.get( const insertablesOut: Record = {}; for (const ins of allInsertables) { - const config = configMap.get(ins.id); insertablesOut[ins.id] = { buildIssues: ins.buildIssues, elementType: ins.elementType, @@ -90,12 +91,7 @@ buildStatusRoutes.get( supportsFasten: ins.supportsFasten, indexConfigurations: ins.indexConfigurations, vendors: ins.vendors, - configuration: config - ? { - buildIssues: config.buildIssues, - parameters: config.parameters - } - : undefined, + configuration: configMap.get(ins.id), lastLoadedAt: ins.lastLoadedAt }; } diff --git a/src/backend/features/configurations/canonical.test.ts b/src/backend/features/configurations/canonical.test.ts index 9e847fe6f..9ade2a127 100644 --- a/src/backend/features/configurations/canonical.test.ts +++ b/src/backend/features/configurations/canonical.test.ts @@ -1,10 +1,7 @@ import { describe, expect, it } from "vitest"; import { DEFAULT_CANONICAL_CONFIGURATION, - DEFAULT_CONFIGURATION_KEY, - canonicalConfigurationKey, - canonicalizeConfiguration, - encodeCanonicalConfiguration + canonicalizeConfiguration } from "./canonical"; import { ParameterValues, VisibilityType } from "./models"; import { QuantityType, Unit } from "./enums"; @@ -14,13 +11,6 @@ import { quantityParam } from "../../../__test_utils__/configuration-fixtures"; -/** The key of an already-canonical selection, which is what callers compare. */ -function keyOf(canonicalConfiguration: ParameterValues): string { - return canonicalConfigurationKey( - encodeCanonicalConfiguration(canonicalConfiguration) - ); -} - describe("canonicalizeConfiguration", () => { const size = enumParam("size", ["s", "l"]); const flag = boolParam("flag"); @@ -35,30 +25,28 @@ describe("canonicalizeConfiguration", () => { it("drops values that match the parameter default", () => { // "s" and "false" are the defaults, so Onshape renders them anyway. - expect(canon({ size: "s", flag: "false", length: "1 in" })).toEqual({}); + expect(canon({ size: "s", flag: "false", length: "1 in" })).toBe( + DEFAULT_CANONICAL_CONFIGURATION + ); }); it("keeps only what differs from the defaults", () => { - expect(canon({ size: "l", flag: "false" })).toEqual({ size: "l" }); + expect(canon({ size: "l", flag: "false" })).toBe("size=l"); }); - it("emits parameters in declaration order, not object order", () => { + it("names parameters in declaration order, not object order", () => { const a = canon({ flag: "true", size: "l" }); - const b = canon({ size: "l", flag: "true" }); - expect(Object.keys(a)).toEqual(["size", "flag"]); - expect(encodeCanonicalConfiguration(a)).toBe( - encodeCanonicalConfiguration(b) - ); - expect(keyOf(a)).toBe(keyOf(b)); + expect(a).toBe("size=l;flag=true"); + expect(canon({ size: "l", flag: "true" })).toBe(a); }); it("collapses equivalent quantity spellings", () => { - const keys = ["2in", "2 in", "(1 + 1) in"].map((value) => - keyOf(canon({ length: value })) + const spellings = ["2in", "2 in", "(1 + 1) in"].map((value) => + canon({ length: value }) ); - expect(new Set(keys).size).toBe(1); - // ...and it is not the default key, since 2 in != the 1 in default. - expect(keys[0]).not.toBe(keyOf({})); + expect(new Set(spellings).size).toBe(1); + // ...and it is not the default, since 2 in != the 1 in default. + expect(spellings[0]).not.toBe(DEFAULT_CANONICAL_CONFIGURATION); }); it("drops a parameter hidden by its visibility condition", () => { @@ -70,19 +58,19 @@ describe("canonicalizeConfiguration", () => { } }); // size=l hides `hidden`, so its value can't affect the render. - expect(canon({ size: "l", hidden: "y" }, [size, hidden])).toEqual({ - size: "l" - }); + expect(canon({ size: "l", hidden: "y" }, [size, hidden])).toBe( + "size=l" + ); }); it("ignores parameters that aren't set", () => { - expect(canon({ size: "l" })).toEqual({ size: "l" }); + expect(canon({ size: "l" })).toBe("size=l"); }); }); // An indexed record holds enumerated values only; the insert menu holds the whole // selection. Canonicalizing both is what makes the two agree on a thumbnail. -describe("canonical keys agree across surfaces", () => { +describe("canonical forms agree across surfaces", () => { const size = enumParam("size", ["s", "l"]); const flag = boolParam("flag"); const finish = enumParam("finish", ["matte", "gloss"], { @@ -91,7 +79,7 @@ describe("canonical keys agree across surfaces", () => { const length = quantityParam("length"); const parameters = [size, flag, finish, length]; - it("keys an enumerated record and the equivalent selection alike", () => { + it("spells an enumerated record and the equivalent selection alike", () => { // What indexing stores: enumerated values, no cosmetic/quantity params. const record = canonicalizeConfiguration( { size: "l", flag: "false" }, @@ -102,10 +90,10 @@ describe("canonical keys agree across surfaces", () => { { size: "l", flag: "false", finish: "matte", length: "1 in" }, parameters ); - expect(keyOf(record)).toBe(keyOf(selection)); + expect(record).toBe(selection); }); - it("keys a non-default cosmetic or quantity value differently", () => { + it("spells a non-default cosmetic or quantity value differently", () => { // Enumeration never varies these, but they do change what renders, so // the selection must not collide with the enumerated record. const record = canonicalizeConfiguration({ size: "l" }, parameters); @@ -114,9 +102,9 @@ describe("canonical keys agree across surfaces", () => { { size: "l", length: "2 in" } ]; for (const selection of selections) { - expect( - keyOf(canonicalizeConfiguration(selection, parameters)) - ).not.toBe(keyOf(record)); + expect(canonicalizeConfiguration(selection, parameters)).not.toBe( + record + ); } }); }); @@ -133,16 +121,30 @@ describe("quantity canonicalization", () => { it("spells a length in meters, whatever unit was typed", () => { for (const value of ["2 in", "50.8 mm", "5.08 cm", "(1 + 1) in"]) { - expect( - canonicalizeConfiguration({ length: value }, [length]) - ).toEqual({ length: "0.0508 m" }); + expect(canonicalizeConfiguration({ length: value }, [length])).toBe( + "length=0.0508 m" + ); } }); it("spells an angle in radians", () => { + const spelled = canonicalizeConfiguration({ angle: "180 deg" }, [ + angle + ]).replace("angle=", ""); + expect(spelled).toMatch(/ rad$/); + // To the decimals the parser's angle tolerance distinguishes, rather + // than a precision of this module's own. + expect(Number.parseFloat(spelled)).toBeCloseTo(Math.PI, 10); + }); + + // Values the parser reads as equal have to spell the same, or they key two + // renders of one configuration. + it("spells a difference below the tolerance the same way", () => { expect( - canonicalizeConfiguration({ angle: "180 deg" }, [angle]) - ).toEqual({ angle: `${Number(Math.PI.toFixed(7))} rad` }); + canonicalizeConfiguration({ length: "0.02540000000001 m" }, [ + length + ]) + ).toEqual(canonicalizeConfiguration({ length: "1 in" }, [length])); }); // The document's display units used to decide the spelling, so the same @@ -154,38 +156,14 @@ describe("quantity canonicalization", () => { }); it("drops a value equal to the default in another unit", () => { - expect( - canonicalizeConfiguration({ length: "25.4 mm" }, [length]) - ).toEqual({}); - }); - - it("keeps an unparseable value as typed", () => { - expect( - canonicalizeConfiguration({ length: "#value" }, [length]) - ).toEqual({ length: "#value" }); - }); -}); - -describe("canonicalConfigurationKey", () => { - it("maps an empty configuration to the default key", () => { - expect(keyOf({})).toBe(DEFAULT_CONFIGURATION_KEY); - }); - - it("gives different configurations different keys", () => { - expect(keyOf({ a: "1" })).not.toBe(keyOf({ a: "2" })); - }); -}); - -describe("encodeCanonicalConfiguration", () => { - it("encodes the default as the empty string", () => { - expect(encodeCanonicalConfiguration({})).toBe( + expect(canonicalizeConfiguration({ length: "25.4 mm" }, [length])).toBe( DEFAULT_CANONICAL_CONFIGURATION ); }); - it("joins values in the order canonicalizing emitted them", () => { - expect(encodeCanonicalConfiguration({ size: "l", flag: "true" })).toBe( - "size=l;flag=true" + it("keeps an unparseable value as typed", () => { + expect(canonicalizeConfiguration({ length: "#value" }, [length])).toBe( + "length=#value" ); }); }); diff --git a/src/backend/features/configurations/canonical.ts b/src/backend/features/configurations/canonical.ts index 069424f20..b6ff9a020 100644 --- a/src/backend/features/configurations/canonical.ts +++ b/src/backend/features/configurations/canonical.ts @@ -1,52 +1,40 @@ /** - * A `canonicalConfiguration` addresses a render — thumbnails, R2 keys, search - * records. Only insert/derive needs the user's literal `configuration`. + * A `configuration` is the selection a user made, and is what insert and derive + * send to Onshape. A `canonicalConfiguration` is the one text spelling every + * equivalent selection shares, and is the only form that addresses a render — + * thumbnails, R2 keys, stored records. It is always text; nothing holds a + * half-canonical map. + * + * Canonicalizing is lossy — "2 + 3 in" evaluates away — so anything that hands + * a selection back to the user stores the configuration and derives this. */ import { type ConfigurationParameter, ParameterType, type ParameterValues } from "./models"; -import { QuantityType, Unit, getUnitDisplayStr } from "./enums"; -import { evaluateCondition } from "./utils"; -import { evaluateBaseValue } from "./input-parser"; +import { encodeConfiguration, evaluateCondition } from "./utils"; +import { evaluateBaseValue, formatBaseValue } from "./input-parser"; -/** The element default, which is what an empty canonical configuration encodes. */ +/** The element default, which is what a canonical configuration overriding nothing is. */ export const DEFAULT_CANONICAL_CONFIGURATION = ""; -/** The key for an element's default configuration (what everything falls back to). */ -export const DEFAULT_CONFIGURATION_KEY = "default"; - -/** Decimals kept on a base-unit value: 0.1 µm, past any real CAD tolerance. */ -const BASE_UNIT_PRECISION = 7; - -/** Base units, so the spelling never depends on the document's display units. */ -function baseUnit(quantityType: QuantityType): Unit { - if (quantityType === QuantityType.LENGTH) return Unit.METER; - if (quantityType === QuantityType.ANGLE) return Unit.RADIAN; - return Unit.UNITLESS; -} - /** Normalizes one parameter's raw value to its canonical spelling. */ function canonicalizeValue( parameter: ConfigurationParameter, value: string ): string { if (parameter.type === ParameterType.QUANTITY) { - // "1in", "1 in", "(0.5 + 0.5) in" and "25.4 mm" are one configuration. - // Unparseable values ride as-is. + // "1in", "1 in", "(0.5 + 0.5) in" and "25.4 mm" are one configuration: + // the parser reads them all to the same base value, which spells it in + // the units and precision Onshape itself compares in. Unparseable + // values ride as-is. const base = evaluateBaseValue( value, parameter.quantityType, parameter.unit ); - if (base === undefined) { - return value.trim(); - } - const unit = baseUnit(parameter.quantityType); - const rounded = Number(base.toFixed(BASE_UNIT_PRECISION)); - const suffix = getUnitDisplayStr(unit); - return suffix ? `${rounded} ${suffix}` : `${rounded}`; + return base === undefined ? value.trim() : formatBaseValue(base); } if (parameter.type === ParameterType.BOOLEAN) { return value.trim().toLowerCase(); @@ -56,13 +44,14 @@ function canonicalizeValue( /** * Reduces a configuration to the one spelling every equivalent selection shares, - * so their thumbnails resolve to one cache entry. Drops defaults and hidden values. + * so their thumbnails resolve to one cache entry. Drops defaults and hidden + * values, and so names only what a selection actually overrides. */ export function canonicalizeConfiguration( configuration: ParameterValues, parameters: ConfigurationParameter[] -): ParameterValues { - const canonicalConfiguration: ParameterValues = {}; +): string { + const overrides: ParameterValues = {}; for (const parameter of parameters) { const value = configuration[parameter.id]; if (value === undefined) { @@ -82,42 +71,8 @@ export function canonicalizeConfiguration( if (canonicalValue === canonicalDefault) { continue; } - canonicalConfiguration[parameter.id] = canonicalValue; - } - return canonicalConfiguration; -} - -/** Encodes canonical values for a url, an R2 key, or Onshape's `configuration`. */ -export function encodeCanonicalConfiguration( - canonicalConfiguration: ParameterValues -): string { - return Object.entries(canonicalConfiguration) - .map(([id, value]) => `${id}=${value}`) - .join(";"); -} - -/** - * A short, stable key for a url or R2 key, since a configuration is unbounded. - * Only avoids collisions within one element, so a fast sync hash is plenty. - */ -export function canonicalConfigurationKey( - canonicalConfiguration: string -): string { - if (canonicalConfiguration === DEFAULT_CANONICAL_CONFIGURATION) { - return DEFAULT_CONFIGURATION_KEY; - } - // cyrb53 - let h1 = 0xdeadbeef; - let h2 = 0x41c6ce57; - for (let i = 0; i < canonicalConfiguration.length; i++) { - const ch = canonicalConfiguration.charCodeAt(i); - h1 = Math.imul(h1 ^ ch, 2654435761); - h2 = Math.imul(h2 ^ ch, 1597334677); + overrides[parameter.id] = canonicalValue; } - h1 = Math.imul(h1 ^ (h1 >>> 16), 2246822507); - h1 ^= Math.imul(h2 ^ (h2 >>> 13), 3266489909); - h2 = Math.imul(h2 ^ (h2 >>> 16), 2246822507); - h2 ^= Math.imul(h1 ^ (h1 >>> 13), 3266489909); - const hash = 4294967296 * (2097151 & h2) + (h1 >>> 0); - return hash.toString(36); + // Built in parameter order, so equivalent selections spell it the same way. + return encodeConfiguration(overrides); } diff --git a/src/backend/features/configurations/input-parser.ts b/src/backend/features/configurations/input-parser.ts index 7d9848d58..30801f9ff 100644 --- a/src/backend/features/configurations/input-parser.ts +++ b/src/backend/features/configurations/input-parser.ts @@ -30,6 +30,28 @@ export function valueWithUnits(value: number, unit: Unit): ValueWithUnits { return { value: value * getUnitFactor(unit), type: getUnitType(unit) }; } +/** What a type is measured in once parsed, which is what Onshape stores. */ +function baseUnit(type: UnitType): Unit { + switch (type) { + case "length": + return Unit.METER; + case "angle": + return Unit.RADIAN; + case "number": + return Unit.UNITLESS; + } +} + +/** + * The one spelling of a value: its base unit, to the decimals its tolerance + * distinguishes. Two values this module reads as equal spell the same, which + * is what makes the spelling worth keying on. + */ +export function formatBaseValue(value: ValueWithUnits): string { + const precision = Math.round(-Math.log10(TOLERANCE[value.type])); + return formatValueWithUnits(value, baseUnit(value.type), precision); +} + function tolerantEqualsZero(value: ValueWithUnits) { return tolerantEquals(value, { value: 0, type: value.type }); } @@ -83,7 +105,7 @@ type UnitType = "length" | "angle" | "number"; type Operator = "+" | "-" | "*" | "/"; -interface ValueWithUnits { +export interface ValueWithUnits { value: number; type: UnitType; } @@ -660,7 +682,7 @@ export function evaluateBaseValue( input: string, quantityType: QuantityType, defaultUnit: Unit -): number | undefined { +): ValueWithUnits | undefined { let value: ValueWithUnits; try { value = evaluateExpressionValue(parseExpression(input), quantityType); @@ -674,7 +696,7 @@ export function evaluateBaseValue( ) { value = applyDefaultUnit(value, defaultUnit); } - return value.value; + return value; } export function evaluateExpression( diff --git a/src/backend/features/configurations/models.ts b/src/backend/features/configurations/models.ts index e4b9e612a..9ec4519c1 100644 --- a/src/backend/features/configurations/models.ts +++ b/src/backend/features/configurations/models.ts @@ -69,7 +69,6 @@ interface AlwaysShownVisibilityCondition { } export interface ConfigurationResult { - // defaultConfiguration: string; parameters: ConfigurationParameter[]; /** The insertable's search records, so the insert menu can show the part * number + name of the selected configuration. Empty when not indexed. */ @@ -85,8 +84,11 @@ export interface SearchRecord { name?: string; /** The vendor's page for this part, when one can be resolved. */ url?: string; - /** The (enumerated) parameter values that produce it; empty for the default. */ - configuration: ParameterValues; + /** + * The (enumerated) values that produce it, canonical so it names the same + * render the insert menu asks for; empty for the element's default. + */ + canonicalConfiguration: string; } export type ConfigurationParameter = @@ -161,14 +163,20 @@ export interface PartMetadata { } /** - * {@link PartMetadata} for one specific configuration — the same fields, plus - * the parameter values that produced them. Stored only for an indexed insertable. + * {@link PartMetadata} for one selection, as it came back from Onshape — keyed + * by the selection as written rather than as it is stored. */ -export interface ConfigurationRecord extends PartMetadata { - /** The parameter values that produce it. */ +export interface ProbedRecord extends PartMetadata { + /** The selection probed, before it is canonicalized for storage. */ configuration: ParameterValues; } +/** A probe as it is stored. Kept only for an indexed insertable. */ +export interface ConfigurationRecord extends PartMetadata { + /** The selection that produces it, in its canonical spelling. */ + canonicalConfiguration: string; +} + /** * An insertable's configuration: the parameters it exposes and a record for each * configuration we probed. Mirrors the `configurations` row. diff --git a/src/backend/features/configurations/part-number.test.ts b/src/backend/features/configurations/part-number.test.ts new file mode 100644 index 000000000..85438a047 --- /dev/null +++ b/src/backend/features/configurations/part-number.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, it } from "vitest"; +import { isPlaceholderPartNumber, meaningfulPartNumber } from "./part-number"; + +describe("isPlaceholderPartNumber", () => { + it.each(["N/A", "n/a", " N/a "])("reads %s as the placeholder", (value) => { + expect(isPlaceholderPartNumber(value)).toBe(true); + }); + + it.each(["NA-1234", "n", "WCP-1025"])( + "reads %s as a part number", + (value) => { + expect(isPlaceholderPartNumber(value)).toBe(false); + } + ); +}); + +describe("meaningfulPartNumber", () => { + it.each(["N/A", "n/a", " N/a "])("hides the placeholder %s", (value) => { + expect(meaningfulPartNumber(value)).toBeUndefined(); + }); + + it.each([undefined, null, "", " "])("hides a blank %s", (value) => { + expect(meaningfulPartNumber(value)).toBeUndefined(); + }); + + it("hides a number that only repeats the name it sits under", () => { + expect(meaningfulPartNumber(" spacer ", "Spacer")).toBeUndefined(); + }); + + it("keeps a real number, trimmed", () => { + expect(meaningfulPartNumber(" WCP-1025 ", "Gearbox")).toBe("WCP-1025"); + }); + + it("keeps one that merely contains the placeholder", () => { + expect(meaningfulPartNumber("NA-1234")).toBe("NA-1234"); + }); +}); diff --git a/src/backend/features/configurations/part-number.ts b/src/backend/features/configurations/part-number.ts new file mode 100644 index 000000000..66f98ff55 --- /dev/null +++ b/src/backend/features/configurations/part-number.ts @@ -0,0 +1,25 @@ +import { clean, equalsIgnoreCase } from "../../lib/text"; + +/** Admins write this in where a generic part has no real number to give. */ +const PLACEHOLDER_PART_NUMBER = new RegExp("^n/a$", "i"); + +/** Whether text is the placeholder, which identifies nothing anywhere. */ +export function isPlaceholderPartNumber(text: string): boolean { + return PLACEHOLDER_PART_NUMBER.test(text.trim()); +} + +/** + * The part number when it identifies the part, and nothing when it doesn't — a + * placeholder, or a repeat of the name it sits under. One rule for indexing and + * display alike, so a number nobody can search for is never shown either. + */ +export function meaningfulPartNumber( + partNumber: string | undefined | null, + name?: string | null +): string | undefined { + const text = clean(partNumber); + if (!text || isPlaceholderPartNumber(text)) { + return undefined; + } + return equalsIgnoreCase(text, name) ? undefined : text; +} diff --git a/src/backend/features/configurations/routes.test.ts b/src/backend/features/configurations/routes.test.ts index d606233cf..0f8c1ecdb 100644 --- a/src/backend/features/configurations/routes.test.ts +++ b/src/backend/features/configurations/routes.test.ts @@ -69,7 +69,7 @@ describe("configuration routes", () => { partNumber: "WCP-0405", name: "2x1 Tube", url: "https://wcproducts.com/products/wcp-0405", - configuration: {} + canonicalConfiguration: "" } ] }); diff --git a/src/backend/features/configurations/utils.test.ts b/src/backend/features/configurations/utils.test.ts index 668018524..c4266d8ff 100644 --- a/src/backend/features/configurations/utils.test.ts +++ b/src/backend/features/configurations/utils.test.ts @@ -1,52 +1,71 @@ import { describe, expect, it } from "vitest"; -import { findRecordForConfiguration, getPartUrl } from "./utils"; +import { + decodeConfiguration, + encodeConfiguration, + findRecordForConfiguration, + getPartUrl +} from "./utils"; import { PartMetadata, SearchRecord } from "./models"; import { Vendor } from "../library/vendors"; -function rec( - configuration: Record, - partNumber = "PN" -): SearchRecord { - return { partNumber, configuration }; +function rec(canonicalConfiguration: string, partNumber = "PN"): SearchRecord { + return { partNumber, canonicalConfiguration }; } describe("findRecordForConfiguration", () => { it("returns the record whose enumerated values match the selection", () => { - const records = [ - rec({ size: "s" }, "PN-S"), - rec({ size: "l" }, "PN-L") - ]; + const records = [rec("size=s", "PN-S"), rec("size=l", "PN-L")]; // The selection also carries a non-enumerated (quantity) param, ignored. expect( - findRecordForConfiguration({ size: "l", qty: "3" }, records) - ?.partNumber + findRecordForConfiguration("size=l;qty=3", records)?.partNumber ).toBe("PN-L"); }); it("prefers the most specific match when several apply", () => { - const records = [rec({}, "default"), rec({ size: "l" }, "PN-L")]; - expect( - findRecordForConfiguration({ size: "l" }, records)?.partNumber - ).toBe("PN-L"); + const records = [rec("", "default"), rec("size=l", "PN-L")]; + expect(findRecordForConfiguration("size=l", records)?.partNumber).toBe( + "PN-L" + ); }); - it("falls back to a shorter key-set when a parameter is hidden", () => { + it("falls back to a less specific record when a parameter is hidden", () => { const records = [ - rec({ mode: "a", detail: "x" }, "A-X"), + rec("mode=a;detail=x", "A-X"), // `detail` is hidden when mode=b, so this record omits it. - rec({ mode: "b" }, "B") + rec("mode=b", "B") ]; expect( - findRecordForConfiguration({ mode: "b", detail: "x" }, records) - ?.partNumber + findRecordForConfiguration("mode=b;detail=x", records)?.partNumber ).toBe("B"); }); it("returns undefined when nothing matches", () => { - const records = [rec({ size: "s" }, "PN-S")]; - expect( - findRecordForConfiguration({ size: "l" }, records) - ).toBeUndefined(); + const records = [rec("size=s", "PN-S")]; + expect(findRecordForConfiguration("size=l", records)).toBeUndefined(); + }); +}); + +describe("configuration text", () => { + it("encodes the empty configuration as the empty string", () => { + expect(encodeConfiguration({})).toBe(""); + expect(encodeConfiguration(undefined)).toBe(""); + }); + + it("joins values in the order they were set", () => { + expect(encodeConfiguration({ size: "l", flag: "true" })).toBe( + "size=l;flag=true" + ); + }); + + it("round-trips the values it names", () => { + const configuration = { size: "l", length: "0.0508 m" }; + expect(decodeConfiguration(encodeConfiguration(configuration))).toEqual( + configuration + ); + }); + + it("decodes the empty string as no values at all", () => { + expect(decodeConfiguration("")).toEqual({}); }); }); diff --git a/src/backend/features/configurations/utils.ts b/src/backend/features/configurations/utils.ts index 74a788148..8612d1778 100644 --- a/src/backend/features/configurations/utils.ts +++ b/src/backend/features/configurations/utils.ts @@ -16,30 +16,30 @@ import { import { Vendor, getVendorPartUrl, - parsePartNumberVendor, - toVendor + parseVendor, + parseVendorFromPartNumber } from "../library/vendors"; import { LogicalOp, QuantityType, Unit } from "./enums"; import { type EvaluateOptions, valueWithUnits } from "./input-parser"; /** - * The record a selection produces. Records key only on enumerated parameters, so - * several can match; the most specific (most keys) wins. + * The record a selection produces. Records name only enumerated parameters, so + * several can match; the most specific (the most named) wins. Both sides are + * canonical and built in parameter order, so this compares whole assignments. */ export function findRecordForConfiguration( - configuration: ParameterValues, + canonicalConfiguration: string, records: SearchRecord[] ): SearchRecord | undefined { + const selected = new Set(splitConfiguration(canonicalConfiguration)); let best: SearchRecord | undefined; - let bestKeys = -1; + let bestNamed = -1; for (const record of records) { - const keys = Object.keys(record.configuration); - const matches = keys.every( - (key) => configuration[key] === record.configuration[key] - ); - if (matches && keys.length > bestKeys) { + const named = splitConfiguration(record.canonicalConfiguration); + const matches = named.every((assignment) => selected.has(assignment)); + if (matches && named.length > bestNamed) { best = record; - bestKeys = keys.length; + bestNamed = named.length; } } return best; @@ -91,23 +91,31 @@ export function evaluateCondition( * The page for a part, in descending precision: a description that is already a * url, then the vendor the part number names, then the taggings standing in. */ +/** A description holding a link is the link, rather than a description. */ +const ABSOLUTE_URL = new RegExp("^https?://", "i"); + export function getPartUrl( record: PartMetadata, vendors: Vendor[] = [] ): string | undefined { - if (record.description && /^https?:\/\//i.test(record.description)) { + if (record.description && ABSOLUTE_URL.test(record.description)) { return record.description; } - const vendor = - parsePartNumberVendor(record.partNumber) ?? - toVendor(record.vendor) ?? - (vendors.length === 1 ? vendors[0] : undefined); + // WCP-123 -> WCP, then what the part says it is, then the insertable's + // tagging when it names one vendor and one only. + let vendor = parseVendorFromPartNumber(record.partNumber); + vendor ??= parseVendor(record.vendor); + if (!vendor && vendors.length === 1) { + vendor = vendors[0]; + } return getVendorPartUrl(vendor, record.partNumber); } -export function encodeConfigurationForQuery( - configuration?: ParameterValues -): string { +/** + * The text form of a configuration, which is Onshape's own: `id=value;id=value`. + * Values never carry a `;` or an `=`, which is what lets this round-trip. + */ +export function encodeConfiguration(configuration?: ParameterValues): string { if (!configuration) { return ""; } @@ -116,6 +124,28 @@ export function encodeConfigurationForQuery( .join(";"); } +/** The assignments a configuration text names, each still `id=value`. */ +export function splitConfiguration(configuration: string): string[] { + return configuration.split(";").filter((assignment) => assignment !== ""); +} + +/** + * The values a configuration text names. A canonical one names only what it + * overrides, so what it omits is the parameter's own default. + */ +export function decodeConfiguration(configuration: string): ParameterValues { + const values: ParameterValues = {}; + for (const assignment of splitConfiguration(configuration)) { + const separator = assignment.indexOf("="); + if (separator > 0) { + values[assignment.slice(0, separator)] = assignment.slice( + separator + 1 + ); + } + } + return values; +} + export function getOption( options: EnumOption[], optionId: string @@ -223,5 +253,5 @@ export function toRecords( records: ConfigurationRecord[] ): ConfigurationRecord[] { if (!partMetadata) return records; - return [{ ...partMetadata, configuration: {} }, ...records]; + return [{ ...partMetadata, canonicalConfiguration: "" }, ...records]; } diff --git a/src/backend/features/entry/routes.test.ts b/src/backend/features/entry/routes.test.ts index 07a28549b..162f12f84 100644 --- a/src/backend/features/entry/routes.test.ts +++ b/src/backend/features/entry/routes.test.ts @@ -5,10 +5,13 @@ import { users } from "../../db/schema"; import { LibraryId } from "../library/library-id"; import { Theme } from "../settings/settings"; import { + TEST_GROUP_ID, TEST_USER_ID, createTestApp, jsonRequest, + TEST_LIBRARY_ID, resetDb, + seedGroup, seedUser } from "../../../__test_utils__"; import { getDb } from "../../db/client"; @@ -84,6 +87,49 @@ describe("GET /init", () => { expect(location.searchParams.get("theme")).toBe(Theme.SYSTEM); }); + /** The library and group a user left off in, as their row records them. */ + async function seedResume(libraryId: LibraryId, groupId: string | null) { + await seedUser(db); + await db + .update(users) + .set({ libraryId, groupId }) + .where(eq(users.id, TEST_USER_ID)); + } + + async function entryPath(): Promise { + const res = await createTestApp().request( + "/init", + jsonRequest("GET"), + env + ); + return new URL(res.headers.get("Location")!, "http://x").pathname; + } + + it("resumes in the group they last opened", async () => { + await seedGroup(db); + await seedResume(TEST_LIBRARY_ID, TEST_GROUP_ID); + + expect(await entryPath()).toBe( + `/app/library/${TEST_LIBRARY_ID}/groups/${TEST_GROUP_ID}` + ); + }); + + // The group is gone, so the caller lands in the library rather than on a + // "group not found" page. + it("falls back to the library when the group has been deleted", async () => { + await seedResume(TEST_LIBRARY_ID, "deleted-group"); + + expect(await entryPath()).toBe(`/app/library/${TEST_LIBRARY_ID}`); + }); + + // Whichever library they switched to, they have not opened a group in it. + it("ignores a group belonging to another library", async () => { + await seedGroup(db); + await seedResume(LibraryId.MKCAD, TEST_GROUP_ID); + + expect(await entryPath()).toBe(`/app/library/${LibraryId.MKCAD}`); + }); + it("never caches the gate's verdict", async () => { const res = await createTestApp().request( "/init", diff --git a/src/backend/features/entry/routes.ts b/src/backend/features/entry/routes.ts index 00f08b835..c49cff177 100644 --- a/src/backend/features/entry/routes.ts +++ b/src/backend/features/entry/routes.ts @@ -2,9 +2,9 @@ * `/init` is where Onshape lands. It gates on auth, then resumes the caller in * the library and theme they last used. */ -import { eq } from "drizzle-orm"; +import { and, eq } from "drizzle-orm"; import { getDb } from "../../db/client"; -import { users } from "../../db/schema"; +import { group, users } from "../../db/schema"; import { cacheMiddleware } from "../../lib/cache"; import { getApp, type AppContext } from "../../lib/context"; import { getSessionCompanyId } from "../auth/session"; @@ -19,9 +19,22 @@ function getRelativeUrl(requestUrl: string) { /** Builds the url the caller resumes at, seeded with the library and theme they last used. */ async function getEntryUrl(c: AppContext): Promise { const db = getDb(c.env.DB); + // The join is the check on the stored group: one deleted, or left behind by + // a library switch, comes back null and lands the caller in the library. const user = await db - .select({ libraryId: users.libraryId, theme: users.theme }) + .select({ + libraryId: users.libraryId, + theme: users.theme, + groupId: group.id + }) .from(users) + .leftJoin( + group, + and( + eq(group.id, users.groupId), + eq(group.libraryId, users.libraryId) + ) + ) .where(eq(users.id, await c.var.getUserId())) .get(); @@ -33,7 +46,9 @@ async function getEntryUrl(c: AppContext): Promise { search.set("theme", user?.theme ?? DEFAULT_SETTINGS.theme); const libraryId = user?.libraryId ?? DEFAULT_SETTINGS.libraryId; - return `/app/library/${libraryId}?${search.toString()}`; + const path = `/app/library/${libraryId}`; + const groupPath = user?.groupId ? `${path}/groups/${user.groupId}` : path; + return `${groupPath}?${search.toString()}`; } export const entryRoutes = getApp(); diff --git a/src/backend/features/favorites/contract.ts b/src/backend/features/favorites/contract.ts index 8bf3e53f9..36d381bbb 100644 --- a/src/backend/features/favorites/contract.ts +++ b/src/backend/features/favorites/contract.ts @@ -5,7 +5,14 @@ export interface Favorite { id: string; insertableId: string; libraryId: LibraryId; + /** The selection it opens with, as stored; absent for the element default. */ defaultConfiguration?: ParameterValues; + /** + * That selection canonicalized against the insertable's parameters, which + * is what names its thumbnail. Derived per response rather than stored, so + * a reload that changes a parameter's default can't leave it stale. + */ + canonicalConfiguration?: string; } export interface FavoritesData { diff --git a/src/backend/features/favorites/routes.test.ts b/src/backend/features/favorites/routes.test.ts index 0e9e97e0e..7fd8e8b11 100644 --- a/src/backend/features/favorites/routes.test.ts +++ b/src/backend/features/favorites/routes.test.ts @@ -10,6 +10,7 @@ import { jsonRequest, resetDb, seedAssembly, + seedConfiguration, seedFavorite, seedPartStudio, seedTestData @@ -20,10 +21,22 @@ const db = getDb(env.DB); const favoritesUrl = `/api/favorites/library/${TEST_LIBRARY_ID}`; interface FavoritesBody { - favorites: Record; + favorites: Record< + string, + { + insertableId: string; + defaultConfiguration?: Record; + canonicalConfiguration?: string; + } + >; favoriteOrder: string[]; } +/** The one favorite a body holds, for the derived-canonical tests. */ +function soleFavorite(body: FavoritesBody) { + return body.favorites[body.favoriteOrder[0]]; +} + describe("favorites routes", () => { beforeEach(async () => { await resetDb(db); @@ -53,6 +66,51 @@ describe("favorites routes", () => { expect(res.headers.get("Cache-Control")).toBe("private, no-store"); }); + // Derived per response rather than stored, so it cannot go stale when a + // reload changes what the parameters default to. + it("derives each favorite's canonical form from what it stores", async () => { + await seedPartStudio(db); + await seedConfiguration(db); + const favoriteId = await seedFavorite(db, TEST_PART_STUDIO_ID); + await db + .update(favorites) + .set({ defaultConfiguration: { boolean: "false" } }) + .where(eq(favorites.id, favoriteId)); + + const res = await createTestApp().request( + favoritesUrl, + jsonRequest("GET"), + env + ); + expect(soleFavorite(await res.json()).canonicalConfiguration).toBe( + "boolean=false" + ); + }); + + // The whole reason the selection is stored as it was made: canonicalizing + // drops a value that is the parameter's default, which for a string + // parameter is text the user typed. + it("keeps a stored value that canonicalizing drops", async () => { + await seedPartStudio(db); + await seedConfiguration(db); + const favoriteId = await seedFavorite(db, TEST_PART_STUDIO_ID); + const defaultConfiguration = { boolean: "true" }; + await db + .update(favorites) + .set({ defaultConfiguration }) + .where(eq(favorites.id, favoriteId)); + + const res = await createTestApp().request( + favoritesUrl, + jsonRequest("GET"), + env + ); + const favorite = soleFavorite(await res.json()); + expect(favorite.defaultConfiguration).toEqual(defaultConfiguration); + // "true" is the parameter default, so it names no override at all. + expect(favorite.canonicalConfiguration).toBe(""); + }); + it("only returns the current user's favorites", async () => { await seedTestData(db); await seedFavorite(db, TEST_PART_STUDIO_ID, "other-user"); @@ -199,7 +257,7 @@ describe("favorites routes", () => { }); describe("POST /default-configuration/favorite/:favoriteId", () => { - it("persists the default configuration", async () => { + it("persists the selection the favorite opens with", async () => { await seedPartStudio(db); const favoriteId = await seedFavorite(db, TEST_PART_STUDIO_ID); const app = createTestApp(); diff --git a/src/backend/features/favorites/routes.ts b/src/backend/features/favorites/routes.ts index a03e07256..66a8f4d5b 100644 --- a/src/backend/features/favorites/routes.ts +++ b/src/backend/features/favorites/routes.ts @@ -1,4 +1,4 @@ -import { and, asc, eq } from "drizzle-orm"; +import { and, asc, eq, inArray } from "drizzle-orm"; import { cacheMiddleware } from "../../lib/cache"; import { getApp } from "../../lib/context"; import { @@ -8,8 +8,10 @@ import { libraryRoute } from "../../lib/route-params"; import { type Db, getDb } from "../../db/client"; -import { users, favorites } from "../../db/schema"; +import { users, favorites, configurations } from "../../db/schema"; +import { canonicalizeConfiguration } from "../configurations/canonical"; import type { Favorite, FavoritesData } from "./contract"; +import type { ConfigurationParameter } from "../configurations/models"; import type { LibraryId } from "../library/library-id"; import { z } from "zod"; import { validate } from "../../lib/validate"; @@ -22,6 +24,11 @@ const addFavoriteQuery = z.object({ id: z.string().min(1) }); +/** The configuration the favorite opens with, when it was made from one. */ +const addFavoriteBody = z.object({ + defaultConfiguration: z.record(z.string(), z.string()).optional() +}); + const favoriteOrderBody = z.object({ favoriteOrder: z.array(z.string()) }); const defaultConfigurationBody = z.object({ @@ -45,14 +52,28 @@ async function getFavorites( .orderBy(asc(favorites.sortOrder)) .all(); + // Canonicalized here rather than stored: the parameters a selection is + // canonical against move with the library, and only the row is ours to keep. + const parameters = await getParameters( + db, + rows.map((row) => row.insertableId) + ); + const favoritesOut: Record = {}; const favoriteOrder: string[] = []; for (const row of rows) { + const defaultConfiguration = row.defaultConfiguration ?? undefined; const fav: Favorite = { id: row.id, insertableId: row.insertableId, libraryId, - defaultConfiguration: row.defaultConfiguration ?? undefined + defaultConfiguration, + canonicalConfiguration: defaultConfiguration + ? canonicalizeConfiguration( + defaultConfiguration, + parameters.get(row.insertableId) ?? [] + ) + : undefined }; favoritesOut[row.id] = fav; favoriteOrder.push(row.id); @@ -60,6 +81,25 @@ async function getFavorites( return { favorites: favoritesOut, favoriteOrder }; } +/** The parameters of each insertable named, for canonicalizing against. */ +async function getParameters( + db: Db, + insertableIds: string[] +): Promise> { + if (insertableIds.length === 0) { + return new Map(); + } + const rows = await db + .select({ + id: configurations.id, + parameters: configurations.parameters + }) + .from(configurations) + .where(inArray(configurations.id, insertableIds)) + .all(); + return new Map(rows.map((row) => [row.id, row.parameters])); +} + /** GET /api/favorites/library/:libraryId */ favoriteRoutes.get( "/favorites" + libraryRoute(), @@ -78,10 +118,12 @@ favoriteRoutes.post( "/favorites" + libraryRoute(), requireSignInMiddleware, validate("query", addFavoriteQuery), + validate("json", addFavoriteBody), async (c) => { const libraryId = getLibraryParam(c); const userId = await c.var.getUserId(); const { insertableId, id: favoriteId } = c.req.valid("query"); + const { defaultConfiguration } = c.req.valid("json"); const db = getDb(c.env.DB); @@ -105,6 +147,7 @@ favoriteRoutes.post( userId, libraryId, insertableId, + defaultConfiguration, sortOrder: existingCount.length }) .onConflictDoNothing(); diff --git a/src/backend/features/library/insertables/routes.test.ts b/src/backend/features/library/insertables/routes.test.ts index 13f2eaa47..92d4f64cd 100644 --- a/src/backend/features/library/insertables/routes.test.ts +++ b/src/backend/features/library/insertables/routes.test.ts @@ -203,10 +203,11 @@ describe("insertable routes", () => { jsonRequest("POST", { indexConfigurations: true }), env ); - // Surfaced to the client rather than silently enabling. + // Surfaced to the client rather than silently enabling. How long to + // wait is the loader's business; the caller is told to try again. expect(res.status).toBe(429); - const body: { retryAfterSeconds: number } = await res.json(); - expect(body.retryAfterSeconds).toBe(450); + const body: { message: string } = await res.json(); + expect(body.message).toContain("rate limit"); const row = await readInsertable(TEST_PART_STUDIO_ID); expect(row?.indexConfigurations).toBe(false); diff --git a/src/backend/features/library/insertables/routes.ts b/src/backend/features/library/insertables/routes.ts index 4eda9937b..c70440e22 100644 --- a/src/backend/features/library/insertables/routes.ts +++ b/src/backend/features/library/insertables/routes.ts @@ -39,7 +39,6 @@ import { FastenMateBuilder } from "../../../lib/onshape/objects/assembly-feature import { parseFastenInfo } from "../../load/parse-fasten"; import { getFastenQuery } from "./fasten-query"; import { addBuildIssue, clearBuildIssue } from "../../build-checker/issues"; -import { checkIndexedPartNumber } from "../../build-checker/checks"; export const insertableRoutes = getApp(); @@ -165,12 +164,7 @@ insertableRoutes.post( const buildIssues = addBuildIssue( clearBuildIssue(row.buildIssues, ...INDEXING_ISSUE_TYPES), ...indexed.buildIssues, - ...indexing.buildIssues, - // Vendors are read, not re-derived: the load path wrote them. - ...checkIndexedPartNumber(row.vendors, [ - indexed.partMetadata, - ...indexed.records - ]) + ...indexing.buildIssues ); // A configurations row exists exactly when the insertable is configurable. diff --git a/src/backend/features/library/vendors.test.ts b/src/backend/features/library/vendors.test.ts index 6d07e7f4e..60618f9e3 100644 --- a/src/backend/features/library/vendors.test.ts +++ b/src/backend/features/library/vendors.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { Vendor, getVendorPartUrl, toVendor } from "./vendors"; +import { Vendor, getVendorPartUrl, parseVendor } from "./vendors"; describe("getVendorPartUrl", () => { // Each vendor writes its own casing, and only some have a per-part page. @@ -53,7 +53,7 @@ describe("toVendor", () => { ["", undefined], [undefined, undefined] ])("resolves %s", (text, expected) => { - expect(toVendor(text)).toBe(expected); + expect(parseVendor(text)).toBe(expected); }); }); diff --git a/src/backend/features/library/vendors.ts b/src/backend/features/library/vendors.ts index 29563eed4..81fd8e98c 100644 --- a/src/backend/features/library/vendors.ts +++ b/src/backend/features/library/vendors.ts @@ -1,3 +1,8 @@ +import { clean } from "../../lib/text"; + +/** A part number's leading letters, which name the vendor that sells it. */ +const VENDOR_PREFIX = new RegExp("^([A-Za-z]+)-"); + /** The vendors an insertable can come from, and how they are displayed. */ export enum Vendor { AM = "AM", @@ -19,8 +24,8 @@ export enum Vendor { * Resolves the free text Onshape carries as a vendor to one we know, written * either as its code or as its full name. */ -export function toVendor(vendor: string | undefined): Vendor | undefined { - const text = vendor?.trim().toUpperCase(); +export function parseVendor(vendor: string | undefined): Vendor | undefined { + const text = clean(vendor)?.toUpperCase(); if (!text) { return undefined; } @@ -35,10 +40,10 @@ export function toVendor(vendor: string | undefined): Vendor | undefined { * The vendor a part number names itself, e.g. `WCP-1025` — more precise than an * insertable's tagging, which is generic wherever one part spans vendors. */ -export function parsePartNumberVendor( +export function parseVendorFromPartNumber( partNumber: string | undefined ): Vendor | undefined { - return toVendor(/^([A-Za-z]+)-/.exec(partNumber?.trim() ?? "")?.[1]); + return parseVendor(VENDOR_PREFIX.exec(clean(partNumber) ?? "")?.[1]); } /** diff --git a/src/backend/features/load/context.ts b/src/backend/features/load/context.ts index 7c14b5e46..e7137b860 100644 --- a/src/backend/features/load/context.ts +++ b/src/backend/features/load/context.ts @@ -1,6 +1,6 @@ import type { WorkflowStep } from "cloudflare:workers"; import type { AppBindings } from "../../lib/context"; -import { getOnshapeApiFromSessionId } from "../auth/caller"; +import { getOnshapeApiFromSessionId } from "../auth/request-auth"; import type { OnshapeApi } from "../../lib/onshape/client"; import type { ElementType } from "../../lib/onshape/element-type"; import type { LibraryId } from "../library/library-id"; diff --git a/src/backend/features/load/load-insertable.test.ts b/src/backend/features/load/load-insertable.test.ts index 7d7db0c36..39d27c8c1 100644 --- a/src/backend/features/load/load-insertable.test.ts +++ b/src/backend/features/load/load-insertable.test.ts @@ -3,7 +3,7 @@ import { eq } from "drizzle-orm"; import { beforeEach, describe, expect, it } from "vitest"; import { getDb } from "../../db/client"; import { configurations, insertables } from "../../db/schema"; -import type { ParameterValues, PartMetadata } from "../configurations/models"; +import type { PartMetadata } from "../configurations/models"; import { configurationRecord } from "../../../__test_utils__/configuration-fixtures"; import { TEST_PARAMETERS, @@ -30,8 +30,8 @@ function readInsertable() { const partMetadata = (partNumber?: string): PartMetadata => configurationRecord({ partNumber }); -const record = (partNumber?: string, configuration: ParameterValues = {}) => - configurationRecord({ partNumber, configuration }); +const record = (partNumber?: string, canonicalConfiguration = "") => + configurationRecord({ partNumber, canonicalConfiguration }); describe("saveInsertable", () => { beforeEach(async () => { @@ -99,10 +99,7 @@ describe("saveInsertable", () => { }); it("writes the computed configuration records", async () => { - const records = [ - record("PN-1", { p: "v1" }), - record("PN-2", { p: "v2" }) - ]; + const records = [record("PN-1", "p=v1"), record("PN-2", "p=v2")]; await saveInsertable( db, insertableTarget(), diff --git a/src/backend/features/load/load-insertable.ts b/src/backend/features/load/load-insertable.ts index 31b490eda..fc4eed658 100644 --- a/src/backend/features/load/load-insertable.ts +++ b/src/backend/features/load/load-insertable.ts @@ -117,11 +117,7 @@ export async function loadInsertable( const buildIssues = addBuildIssue( hasParts - ? checkInsertable({ - vendors, - thumbnailUrls, - probes: [recordsResult.partMetadata, ...recordsResult.records] - }) + ? checkInsertable({ vendors, thumbnailUrls }) : parts.buildIssues, ...recordsResult.buildIssues, ...indexing.buildIssues diff --git a/src/backend/features/load/parse-configuration-records.ts b/src/backend/features/load/parse-configuration-records.ts index 70eed9c83..61d257535 100644 --- a/src/backend/features/load/parse-configuration-records.ts +++ b/src/backend/features/load/parse-configuration-records.ts @@ -10,7 +10,8 @@ import { ParameterValues, ConfigurationParameter, PartMetadata, - ConfigurationRecord + ConfigurationRecord, + ProbedRecord } from "../configurations/models"; import { addBuildIssue, @@ -22,7 +23,10 @@ import { IndexingBand, isIndexingEnabled } from "../configurations/combinations"; -import { canonicalizeConfiguration } from "../configurations/canonical"; +import { + DEFAULT_CANONICAL_CONFIGURATION, + canonicalizeConfiguration +} from "../configurations/canonical"; import { getParts } from "../../lib/onshape/endpoints/parts"; import { getElementMetadata } from "../../lib/onshape/endpoints/metadata"; import type { @@ -31,6 +35,7 @@ import type { } from "../../lib/onshape/types"; import { type LoadContext, getOnshapeApiFromContext } from "./context"; import { ONSHAPE_STEP_RETRIES } from "./steps"; +import { clean } from "../../lib/text"; /** Configurations fetched per workflow step. */ const BATCH_SIZE = 20; @@ -59,8 +64,7 @@ export const INDEXING_ISSUE_TYPES = [ BuildIssueType.CONFIGURATION_LIMIT_EXCEEDED, BuildIssueType.MANUAL_INDEXING_REQUIRED, BuildIssueType.MULTIPLE_PARTS, - BuildIssueType.UNSTABLE_COMPOSITE, - BuildIssueType.NO_PART_NUMBER + BuildIssueType.UNSTABLE_COMPOSITE ]; /** Whether to index an insertable, and how to flag it if we don't. */ @@ -107,12 +111,6 @@ export function decideIndexing( return { shouldIndex, buildIssues: [], configurations }; } -/** Trims a raw metadata value; a missing or blank one becomes `null`. */ -function normalizeText(value: string | undefined | null): string | undefined { - const trimmed = value?.trim(); - return trimmed ? trimmed : undefined; -} - /** What a part studio's parts resolve to, before build issues are decided. */ export interface PartsEvaluation { /** True when more than one part could be the one to index. */ @@ -156,7 +154,7 @@ export function parsePartStudioRecord( parts: OnshapePart[], configuration: ParameterValues, isOpenComposite: boolean -): ConfigurationRecord { +): ProbedRecord { const evaluation = evaluateParts(parts); // An element that is an open composite everywhere else has no part to read // in a configuration that loses it; toResult raises the build issue. @@ -170,11 +168,11 @@ export function parsePartStudioRecord( const part = evaluation.partToUse; return { configuration, - partNumber: normalizeText(part?.partNumber), - name: normalizeText(part?.name), - description: normalizeText(part?.description), - material: normalizeText(part?.material?.displayName), - vendor: normalizeText(part?.vendor), + partNumber: clean(part?.partNumber), + name: clean(part?.name), + description: clean(part?.description), + material: clean(part?.material?.displayName), + vendor: clean(part?.vendor), hasMultipleParts: evaluation.hasMultipleParts, isOpenComposite: evaluation.isOpenComposite }; @@ -192,10 +190,10 @@ const METADATA_FIELDS = { /** Reads a metadata property value as text; materials arrive as `{displayName}`. */ function readMetadataValue(value: unknown): string | undefined { if (typeof value === "string") { - return normalizeText(value); + return clean(value); } if (value && typeof value === "object" && "displayName" in value) { - return normalizeText((value as { displayName?: string }).displayName); + return clean((value as { displayName?: string }).displayName); } return undefined; } @@ -204,9 +202,9 @@ function readMetadataValue(value: unknown): string | undefined { export function parseAssemblyRecord( metadata: OnshapeMetadataObject, configuration: ParameterValues -): ConfigurationRecord { +): ProbedRecord { // An assembly is never a composite, so it reads nothing about one. - const record: ConfigurationRecord = { + const record: ProbedRecord = { configuration, hasMultipleParts: false, isOpenComposite: false @@ -242,7 +240,7 @@ export async function parseConfigurationRecords( ); const batches = planBatches(configurations, parameters); - const batchRecords: ConfigurationRecord[][] = []; + const batchRecords: ProbedRecord[][] = []; for (const batch of batches) { batchRecords.push( await fetchBatch( @@ -284,7 +282,7 @@ export async function loadConfigurationRecords( ); const batches = planBatches(configurations, parameters); - const batchRecords: ConfigurationRecord[][] = []; + const batchRecords: ProbedRecord[][] = []; for (const [index, batch] of batches.entries()) { batchRecords.push( await ctx.step.do( @@ -316,8 +314,8 @@ function planBatches( // so drop every all-defaults combination, not just the empty one. const toFetch = configurations.filter( (configuration) => - Object.keys(canonicalizeConfiguration(configuration, parameters)) - .length > 0 + canonicalizeConfiguration(configuration, parameters) !== + DEFAULT_CANONICAL_CONFIGURATION ); const batches: ParameterValues[][] = []; @@ -334,7 +332,7 @@ async function probeConfiguration( elementType: ElementType, configuration: ParameterValues, isOpenComposite: boolean -): Promise { +): Promise { if (elementType === ElementType.ASSEMBLY) { return parseAssemblyRecord( await getElementMetadata(client, elementPath, configuration), @@ -355,8 +353,8 @@ async function fetchBatch( elementType: ElementType, batch: ParameterValues[], isOpenComposite: boolean -): Promise { - const records: ConfigurationRecord[] = []; +): Promise { + const records: ProbedRecord[] = []; for (const configuration of batch) { records.push( await probeConfiguration( @@ -373,7 +371,7 @@ async function fetchBatch( /** Onshape's vendor when a part carries one, otherwise the parsed one. */ function resolveVendor( - record: ConfigurationRecord, + record: ProbedRecord, parameters: ConfigurationParameter[] ): string | undefined { return ( @@ -384,8 +382,8 @@ function resolveVendor( /** Folds the default probe and every batch together, the default first. */ function toResult( - defaultRecord: ConfigurationRecord, - batches: ConfigurationRecord[][], + defaultRecord: ProbedRecord, + batches: ProbedRecord[][], parameters: ConfigurationParameter[] ): ConfigurationRecordsResult { // The element's own probe describes the element, not a configuration of it, @@ -402,12 +400,12 @@ function toResult( // Canonical, so a record addresses the same thumbnail the insert menu does // for the same selection. - const records = batches.flat().map((record) => ({ + const records: ConfigurationRecord[] = batches.flat().map((record) => ({ ...record, // Read before canonicalizing, which drops a selection that is the // default — including a default vendor option. vendor: resolveVendor(record, parameters), - configuration: canonicalizeConfiguration( + canonicalConfiguration: canonicalizeConfiguration( record.configuration, parameters ) diff --git a/src/backend/features/load/parse-vendors.ts b/src/backend/features/load/parse-vendors.ts index 5c725deaa..06f0c3dc2 100644 --- a/src/backend/features/load/parse-vendors.ts +++ b/src/backend/features/load/parse-vendors.ts @@ -1,4 +1,4 @@ -import { Vendor, toVendor } from "../library/vendors"; +import { Vendor, parseVendor } from "../library/vendors"; import { ParameterType, type ConfigurationParameter, @@ -18,7 +18,7 @@ export function parseNameVendor(name: string): Vendor | undefined { /** A vendor an option names, as a token within its label or as the whole of it. */ function parseOptionVendor(optionName: string): Vendor | undefined { - return parseNameVendor(optionName) ?? toVendor(optionName); + return parseNameVendor(optionName) ?? parseVendor(optionName); } export function parseVendors( diff --git a/src/backend/features/search/search-index.test.ts b/src/backend/features/search/search-index.test.ts index 7a82df118..9e1996321 100644 --- a/src/backend/features/search/search-index.test.ts +++ b/src/backend/features/search/search-index.test.ts @@ -1,8 +1,225 @@ import { describe, expect, it } from "vitest"; -import { toSearchRecords } from "./search-index"; +import { + buildSearchDb, + normalizeForMatch, + processTerm, + tokenize, + tokenizeName, + tokenizePartNumber, + tokenizeQuery, + toSearchRecords +} from "./search-index"; +import { LibraryOut } from "../library/contract"; +import { ElementType } from "../../lib/onshape/element-type"; import { Vendor } from "../library/vendors"; import { configurationRecord as record } from "../../../__test_utils__/configuration-fixtures"; +// A part number identifies the part; splitting or folding it makes it name a +// different one, so it is indexed as typed alongside its segments. +describe("tokenizePartNumber", () => { + it("keeps the number whole, and adds its segments", () => { + expect(tokenizePartNumber("WCP-1025")).toEqual([ + "wcp-1025", + "wcp", + "1025" + ]); + }); + + it("keeps leading zeros, which spell the segment", () => { + expect(tokenizePartNumber("TTB-0016")).toEqual([ + "ttb-0016", + "ttb", + "0016" + ]); + }); + + it("leaves a fraction inside a number alone", () => { + expect(tokenizePartNumber("TTB-0016-5/32")).toEqual([ + "ttb-0016-5/32", + "ttb", + "0016", + "5", + "32" + ]); + }); + + it("does not read a number as a quantity", () => { + expect(tokenizePartNumber("217-2600")).toEqual([ + "217-2600", + "217", + "2600" + ]); + }); + + it.each(["", " "])("has nothing to say about %s", (value) => { + expect(tokenizePartNumber(value)).toEqual([]); + }); +}); + +// A name describes the part, so its sizes are read as sizes. +describe("tokenizeName", () => { + it("splits on punctuation, keeping the words whole", () => { + expect(tokenizeName('1" Linear (REV)')).toEqual([ + '1"', + "Linear", + "REV" + ]); + expect(tokenizeName("Bearings & Bushings #X-Contact")).toEqual([ + "Bearings", + "Bushings", + "X", + "Contact" + ]); + }); + + // The standards write the same measurement both ways, so one decimal form + // is what lets either spelling find the other. + it("canonicalizes fractions and decimals to a 2-dp decimal", () => { + expect(tokenizeName("1/2")).toEqual(["0.5"]); + expect(tokenizeName(".5")).toEqual(["0.5"]); + expect(tokenizeName("0.50")).toEqual(["0.5"]); + expect(tokenizeName("3/4")).toEqual(["0.75"]); + expect(tokenizeName("1-1/2")).toEqual(["1.5"]); + expect(tokenizeName("1/3")).toEqual(["0.33"]); + }); + + it.each([ + ['1/2" Hex Bearing (1.125" OD, 0.313" WD, Flanged)', '0.5"'], + // Stored to 2dp, so `1.125` and `1.13` are one size. + ['1/2" Hex Bearing (1.125" OD, 0.313" WD, Flanged)', '1.13"'], + ['#10-32 x 2.5" L SHCS', "10"], + // Sizes are stored to 2dp, so `.159` and `.16` are one size. + [".159 ID x SplineXL OD MotionX Hub", "0.16"] + ])("reads the sizes in %s", (name, size) => { + expect(tokenizeName(name)).toContain(size); + }); + + it("keeps a thread spec's halves apart", () => { + expect(tokenizeName("#10-32 Screw")).toEqual(["10", "32", "Screw"]); + }); + + // The standards list a part's dimensions in a comma-separated aside. + it("does not leave a comma stuck to the word before it", () => { + expect(tokenizeName('1.125" OD, Flanged')).toEqual([ + '1.13"', + "OD", + "Flanged", + '1.12"' + ]); + }); + + // One vendor writes .196 as .2 and the next writes .19, so the part is + // stored as both and either spelling finds it. + it("spells a measurement as what it rounds to and what it starts", () => { + expect(tokenizeName(".196 ID Hub")).toEqual([ + "0.2", + "ID", + "Hub", + "0.19" + ]); + expect(tokenizeName('2.140" L')).toEqual(['2.14"', "L"]); + }); + + // The mark is what makes `1"` a size rather than a prefix of 1.5 and 16T. + it("keeps an inch mark on the number it measures", () => { + expect(tokenizeName('1" Hex Shaft')).toEqual(['1"', "Hex", "Shaft"]); + expect(tokenizeName('1/2" Hex')).toEqual(['0.5"', "Hex"]); + expect(tokenizeName('1"x2" Tube')).toEqual(['1"', 'x2"', "Tube"]); + }); + + it("still drops quotes that quote something", () => { + expect(tokenizeName('The "Long" Bracket')).toEqual([ + "The", + "Long", + "Bracket" + ]); + }); +}); + +describe("processTerm", () => { + it.each(["MAXSpline", "MaxSpline"])("splits %s into its words", (term) => { + expect(processTerm(term)).toEqual( + expect.arrayContaining(["max", "spline", "maxspline"]) + ); + }); + + it.each([ + ["SplineXL", ["spline", "xl"]], + ["roboRIO", ["robo", "rio"]], + ["MAXTube", ["max", "tube"]] + ])("splits the product name %s", (term, words) => { + expect(processTerm(term)).toEqual(expect.arrayContaining(words)); + }); + + // Its segments are already separate tokens; splitting the code again would + // only invent words inside it. + it("leaves a part number whole", () => { + expect(processTerm("WCP-1025", "partNumbers")).toEqual(["wcp-1025"]); + }); +}); + +describe("tokenize", () => { + it("reads each field the way that field is written", () => { + expect(tokenize("TTB-0016-5/32", "partNumbers")).toContain("0016"); + expect(tokenize("TTB-0016-5/32", "partNames")).toEqual([ + "TTB", + "16", + "0.16", + "0.15" + ]); + }); +}); + +// A query has no field, so it has to offer both readings: the caller may have +// typed a size or a part number. +describe("tokenizeQuery", () => { + it("offers the part number as typed, and as a name would read it", () => { + expect( + tokenizeQuery("TTB-0016").map((term) => term.toLowerCase()) + ).toEqual(expect.arrayContaining(["ttb", "16", "0016", "ttb-0016"])); + }); + + // `1` prefix-matches every number in the library, so a size is not split + // into the segments a part number would be. + it("does not split a bare size into its digits", () => { + expect(tokenizeQuery("1/2")).toEqual(["0.5", "1/2"]); + }); + + it("leaves an ordinary word alone", () => { + expect(tokenizeQuery("bearing")).toEqual(["bearing"]); + }); + + // Splitting the placeholder leaves `n` and `a`, and a one-letter prefix + // matches most of the library. + // Nothing carries the placeholder, and searching its letters would answer + // with whatever starts with `n` or `a`. + it.each(["n/a", "N/A"])("has nothing to search for in %s", (query) => { + expect(tokenizeQuery(query)).toEqual([]); + }); + + it("still reads the rest of a query the placeholder is in", () => { + expect(tokenizeQuery("n/a bearing")).toEqual(["bearing"]); + }); + + // Answering as the caller types is the point, and the first keystroke is + // one character. + it.each(["l", "L", "1"])("still searches for a typed %s", (query) => { + expect(tokenizeQuery(query)).toEqual([query]); + }); + + it("keeps a letter typed beside another word", () => { + expect(tokenizeQuery("L bracket")).toEqual(["L", "bracket"]); + }); +}); + +describe("normalizeForMatch", () => { + it("reads a written size and its decimal as one string", () => { + expect(normalizeForMatch('1/2" Hex')).toBe( + normalizeForMatch('.5" hex') + ); + }); +}); + describe("toSearchRecords", () => { it("drops a part number that only repeats the name", () => { const [result] = toSearchRecords([ @@ -45,4 +262,112 @@ describe("toSearchRecords", () => { it("drops a record with neither", () => { expect(toSearchRecords([record({})])).toHaveLength(0); }); + + // The placeholder an admin writes in identifies nothing, so it is dropped + // here rather than indexed and shown. + it("drops a placeholder part number", () => { + const [result] = toSearchRecords([ + record({ partNumber: "N/A", name: "Spacer" }) + ]); + expect(result).toMatchObject({ partNumber: undefined, name: "Spacer" }); + }); + + it("will not link a placeholder to a vendor", () => { + const [result] = toSearchRecords( + [record({ partNumber: "N/A", name: "Spacer" })], + [Vendor.WCP] + ); + expect(result.url).toBeUndefined(); + }); + + it("drops a record the placeholder leaves with nothing", () => { + expect(toSearchRecords([record({ partNumber: "N/A" })])).toEqual([]); + }); + + it("keeps the first of a repeated (number, name)", () => { + expect( + toSearchRecords([ + record({ partNumber: "WCP-1025", name: "Gear" }), + record({ partNumber: "WCP-1025", name: "Gear" }) + ]) + ).toHaveLength(1); + }); +}); + +function library(name: string, vendors: Vendor[] = []): LibraryOut { + return { + groupOrder: ["g1"], + groups: { + g1: { + id: "g1", + documentId: "d1", + path: { documentId: "d1", instanceId: "v1", instanceType: "v" }, + name: "Group", + isLoaded: true, + insertableOrder: ["i1"] + } + }, + insertables: { + i1: { + id: "i1", + elementId: "e1", + groupId: "g1", + documentId: "d1", + versionId: "v1", + path: { + documentId: "d1", + instanceId: "v1", + instanceType: "v", + elementId: "e1" + }, + name, + microversionId: "mv1", + isVisible: true, + supportsFasten: false, + elementType: ElementType.PART_STUDIO, + isConfigurable: false, + vendors + } + } + }; +} + +describe("buildSearchDb", () => { + /** The document as the index stored it. */ + const stored = (db: ReturnType) => + db.getStoredFields("i1") as unknown as Record; + + it("keeps a placeholder part number out of the index and the records", () => { + const db = buildSearchDb(library("Spacer"), { + i1: [ + record({ + partNumber: "N/A", + name: "Spacer", + canonicalConfiguration: "" + }) + ] + }); + expect(db.search("n/a")).toEqual([]); + expect(stored(db).records).toEqual([ + expect.objectContaining({ partNumber: undefined }) + ]); + }); + + // The vendor is a resolution fallback, not something to match against. + it("never searches the vendor", () => { + const db = buildSearchDb(library("Spacer", [Vendor.WCP]), { + i1: [ + record({ + partNumber: "WCP-1025", + name: "Spacer", + vendor: "WestCoast Products", + canonicalConfiguration: "" + }) + ] + }); + expect(db.search("westcoast")).toEqual([]); + expect(stored(db).records).toEqual([ + expect.not.objectContaining({ vendor: expect.anything() }) + ]); + }); }); diff --git a/src/backend/features/search/search-index.ts b/src/backend/features/search/search-index.ts index 3d7593dba..09097faa5 100644 --- a/src/backend/features/search/search-index.ts +++ b/src/backend/features/search/search-index.ts @@ -7,31 +7,23 @@ import { LibraryOut } from "../library/contract"; import { Vendor } from "../library/vendors"; import { ConfigurationRecord, SearchRecord } from "../configurations/models"; import { getPartUrl } from "../configurations/utils"; +import { + isPlaceholderPartNumber, + meaningfulPartNumber +} from "../configurations/part-number"; +import { clean } from "../../lib/text"; -const deliminator = "^"; +/** Where a name breaks: punctuation and space, plus a quote used as a quote. */ +const NAME_SEPARATORS = new RegExp("(? camel case) - const camelSplit = term - .replace(/([a-z])([A-Z])/g, `$1${deliminator}$2`) - .split(deliminator); +/** Where a part number breaks into segments, keeping the whole alongside. */ +const PART_NUMBER_SEPARATORS = new RegExp("[-/]+"); - // Insert spaces to handle MAXTube->MAX Tube, VEXpro->VEX pro - const pascalSplit = term - .replace(/([A-Z])([A-Z][a-z])/g, `$1${deliminator}$2`) - .split(deliminator); - - const base = term.toLowerCase(); - - const terms = [...camelSplit, ...pascalSplit, base].map((t) => - t.toLowerCase() - ); - // Deduplicate - return Array.from(new Set(terms)); -} +/** camelCase and PascalCase boundaries: MAXSpline -> max spline, MAXTube -> max tube. */ +const WORD_BOUNDARIES = new RegExp( + "(?<=[a-z])(?=[A-Z])|(?<=[A-Z])(?=[A-Z][a-z])", + "g" +); // A mixed number, simple fraction, decimal (incl. leading-dot), or plain // integer. Alternatives are ordered longest-first so `1-1/2` is consumed whole, @@ -44,16 +36,28 @@ function withoutLeadingZeros(digits: string): string { return digits.replace(/^0+(?=\d)/, ""); } -/** One 2-dp decimal, the single form every number is stored and queried as. */ -function toDecimal(value: number): string { - return String(Math.round(value * 100) / 100); -} +/** How a measurement is spelled to 2dp: what it rounds to, and what it starts. */ +type DecimalSpelling = (value: number) => string; + +const rounded: DecimalSpelling = (value) => + String(Math.round(value * 100) / 100); +const truncated: DecimalSpelling = (value) => + String(Math.trunc(value * 100) / 100); + +/** + * Both spellings of a measurement, since the library writes the same one either + * way: `.196` is written `.2` by one vendor and `.19` by the next. Storing and + * searching both is what lets either find the part. Most numbers spell the same + * both ways and so cost nothing. + */ +const DECIMAL_SPELLINGS: DecimalSpelling[] = [rounded, truncated]; /** * Rewrites numbers and fractions to one 2-dp decimal, at index and query time - * alike — which is what lets the raw fragments go unstored. + * alike — which is what lets the raw fragments go unstored. Names only: a part + * number is an identifier, and 217-2600 is not two thousand six hundred. */ -function canonicalizeNumbers(text: string): string { +function canonicalizeNumbers(text: string, toDecimal: DecimalSpelling): string { return text.replace( NUMERIC_PATTERN, (match, mixedWhole, mixedNum, mixedDen, fracNum, fracDen) => { @@ -88,19 +92,117 @@ function canonicalizeNumbers(text: string): string { } /** - * For direct, non-tokenized comparison: the index's number canonicalization, + * For direct, non-tokenized comparison: the index's canonicalization, * lowercased, so a `.5` query lines up with a stored `"1/2 Bearing"`. */ export function normalizeForMatch(text: string): string { - return canonicalizeNumbers(text).toLowerCase(); + return canonicalizeNumbers(text, rounded).toLowerCase(); } -export function tokenize(text: string): string[] { - // Canonicalize before splitting: fractions span `/` and `-`. Casing stays, +/** + * A name's words, with its sizes in the one decimal spelling. The inch mark + * stays on its number, so `1"` is a size rather than a prefix of `1.5` and `16t`. + */ +export function tokenizeName(text: string): string[] { + const tokens = new Set(); + // Canonicalized before splitting: fractions span `/` and `-`. Casing stays, // since processTerm splits on camelCase. - return canonicalizeNumbers(text) - .split(/[-()"'#&\s^/]+/) - .filter(Boolean); + for (const toDecimal of DECIMAL_SPELLINGS) { + for (const token of splitWithMarks( + canonicalizeNumbers(text, toDecimal) + )) { + tokens.add(token); + } + } + return Array.from(tokens); +} + +/** Splits on `NAME_SEPARATORS`, keeping a `"` that measures its number. */ +function splitWithMarks(text: string): string[] { + const tokens: string[] = []; + for (const piece of text.split(NAME_SEPARATORS)) { + // The split consumed the separators, so an inch mark left inside a + // piece ends the token it measures: `1"x2"` is two sizes. + for (const token of piece.split(/(?<=")/)) { + if (token) tokens.push(token); + } + } + return tokens; +} + +/** + * A part number identifies, it does not describe: it is indexed as typed, plus + * its segments, so `WCP-1025` is found by the whole number or either half. + */ +export function tokenizePartNumber(text: string): string[] { + const whole = clean(text)?.toLowerCase(); + if (!whole) { + return []; + } + const segments = whole.split(PART_NUMBER_SEPARATORS).filter(Boolean); + return Array.from(new Set([whole, ...segments])); +} + +/** The fields holding an identifier rather than a description. */ +function isPartNumberField(field?: string): boolean { + return field === "partNumbers"; +} + +/** Splits a field's text the way that field reads; a query has no field. */ +export function tokenize(text: string, field?: string): string[] { + if (field === undefined) { + return tokenizeQuery(text); + } + return isPartNumberField(field) + ? tokenizePartNumber(text) + : tokenizeName(text); +} + +/** + * A query is split both ways, since the caller may have typed either kind of + * text: the words of a name, and the literal a part number is indexed as. + */ +export function tokenizeQuery(text: string): string[] { + const tokens: string[] = []; + // The name reading keeps its case, for processTerm to split camelCase on, + // so the literal reading of the same word is a duplicate rather than a + // second term to search. + const seen = new Set(); + for (const word of text.trim().split(/\s+/)) { + // Ingest drops the placeholder, so nothing carries it; typed, it is + // still the word for a part number nobody has, and searching its + // letters would answer with whatever starts with `n` or `a`. + if (!word || isPlaceholderPartNumber(word)) { + continue; + } + // Segments only for something carrying a letter, which is what a part + // number does: splitting a bare `1/2` would search `1`, and a prefix + // that short matches every number in the library. + const literal = /[a-z]/i.test(word) + ? tokenizePartNumber(word) + : [word.toLowerCase()]; + for (const token of [...tokenizeName(word), ...literal]) { + if (seen.has(token.toLowerCase())) { + continue; + } + seen.add(token.toLowerCase()); + tokens.push(token); + } + } + return tokens; +} + +/** + * Adds the words inside a compound term, so `MAXSpline` is found by `spline`. + * A part number is left whole: its segments are already separate tokens. + */ +export function processTerm(term: string, field?: string): string[] { + const base = term.toLowerCase(); + if (isPartNumberField(field)) { + return [base]; + } + const words = term.split(WORD_BOUNDARIES).map((word) => word.toLowerCase()); + return Array.from(new Set([...words, base])); } export interface SearchDocument { @@ -150,22 +252,11 @@ function uniqueJoin(values: (string | undefined)[]): string { ).join(" "); } -/** - * A part number repeating the name identifies nothing — it is what a generic - * part is given for want of a real one — so it is neither shown nor searched. - */ -function withoutRepeatedPartNumber( - record: ConfigurationRecord -): ConfigurationRecord { - const repeated = - record.partNumber?.trim().toLowerCase() === - record.name?.trim().toLowerCase(); - return repeated ? { ...record, partNumber: undefined } : record; -} - /** * Keeps the first of each distinct (part number, name) in enumeration order and - * drops records with neither. First-wins is what keeps the latest revision. + * drops records with neither. First-wins is what keeps the latest revision. A + * number that identifies nothing is dropped here, so it never reaches the + * index, the stored records, or a vendor url. */ export function toSearchRecords( records: ConfigurationRecord[], @@ -174,20 +265,21 @@ export function toSearchRecords( const seen = new Set(); const searchRecords: SearchRecord[] = []; for (const raw of records) { - const record = withoutRepeatedPartNumber(raw); - if (!record.partNumber && !record.name) { + const partNumber = meaningfulPartNumber(raw.partNumber, raw.name); + const name = clean(raw.name); + if (!partNumber && !name) { continue; } - const key = JSON.stringify([record.partNumber, record.name]); + const key = JSON.stringify([partNumber, name]); if (seen.has(key)) { continue; } seen.add(key); searchRecords.push({ - partNumber: record.partNumber, - name: record.name, - url: getPartUrl(record, vendors), - configuration: record.configuration + partNumber, + name, + url: getPartUrl({ ...raw, partNumber }, vendors), + canonicalConfiguration: raw.canonicalConfiguration }); } return searchRecords; diff --git a/src/backend/features/settings/routes.test.ts b/src/backend/features/settings/routes.test.ts index 4589bb980..e1bc59a57 100644 --- a/src/backend/features/settings/routes.test.ts +++ b/src/backend/features/settings/routes.test.ts @@ -36,4 +36,25 @@ describe("settings routes", () => { .get(); expect(row?.theme).toBe(Theme.DARK); }); + + it("POST /settings records and clears the open group", async () => { + const app = createTestApp(); + const post = (body: unknown) => + app.request("/api/settings", jsonRequest("POST", body), env); + const storedGroupId = async () => + ( + await db + .select() + .from(users) + .where(eq(users.id, TEST_USER_ID)) + .get() + )?.groupId; + + await post({ groupId: "group-1" }); + expect(await storedGroupId()).toBe("group-1"); + + // Null is leaving the group, which is not the same as saying nothing. + await post({ groupId: null }); + expect(await storedGroupId()).toBeNull(); + }); }); diff --git a/src/backend/features/settings/routes.ts b/src/backend/features/settings/routes.ts index c003eab4f..19edb743e 100644 --- a/src/backend/features/settings/routes.ts +++ b/src/backend/features/settings/routes.ts @@ -12,7 +12,9 @@ export const settingsRoutes = getApp(); const settingsBody = z.object({ theme: z.enum(Theme).optional(), - libraryId: z.enum(LibraryId).optional() + libraryId: z.enum(LibraryId).optional(), + // Null on leaving a group: the caller resumes in the library itself. + groupId: z.string().nullable().optional() }); /** POST /api/settings — update the caller's stored settings */ diff --git a/src/backend/features/settings/settings.ts b/src/backend/features/settings/settings.ts index c30a65ab3..e67949bad 100644 --- a/src/backend/features/settings/settings.ts +++ b/src/backend/features/settings/settings.ts @@ -11,11 +11,14 @@ export interface Settings { theme: Theme; /** The library the caller last opened, and lands in next time. */ libraryId: LibraryId; + /** The group they last opened in it; null for the library itself. */ + groupId: string | null; } export type SettingsUpdate = Partial; export const DEFAULT_SETTINGS: Settings = { theme: Theme.SYSTEM, - libraryId: LibraryId.FRC_DESIGN_LIB + libraryId: LibraryId.FRC_DESIGN_LIB, + groupId: null }; diff --git a/src/backend/features/thumbnails/keys.ts b/src/backend/features/thumbnails/keys.ts index 2223faffc..2ecb494d1 100644 --- a/src/backend/features/thumbnails/keys.ts +++ b/src/backend/features/thumbnails/keys.ts @@ -1,27 +1,26 @@ /** Thumbnail addressing, shared so the client builds the urls the worker serves. */ -import { - DEFAULT_CANONICAL_CONFIGURATION, - DEFAULT_CONFIGURATION_KEY -} from "../configurations/canonical"; +import { DEFAULT_CANONICAL_CONFIGURATION } from "../configurations/canonical"; import { ThumbnailSize } from "./types"; -/** Short on purpose: the real render can land at any moment and must take over. */ -export const THUMBNAIL_FALLBACK_CACHE_TTL = 60; - /** Marks a response as the element default standing in for an unrendered configuration. */ export const THUMBNAIL_FALLBACK_HEADER = "X-Thumbnail-Fallback"; -/** Defaults get their own prefix: everything falls back to them, so they never expire. */ +/** + * Defaults get their own prefix: everything falls back to them, so they never + * expire. A configuration is url-encoded into its segment, the way Onshape + * spells one, which keeps `/` and `;` out of the path. + */ export function thumbnailKey( elementId: string, microversionId: string, size: ThumbnailSize, - configurationKey: string = DEFAULT_CONFIGURATION_KEY + canonicalConfiguration: string = DEFAULT_CANONICAL_CONFIGURATION ): string { - if (configurationKey === DEFAULT_CONFIGURATION_KEY) { + if (canonicalConfiguration === DEFAULT_CANONICAL_CONFIGURATION) { return `thumbnails/default/${elementId}/${microversionId}/${size}`; } - return `thumbnails/config/${elementId}/${microversionId}/${configurationKey}/${size}`; + const segment = encodeURIComponent(canonicalConfiguration); + return `thumbnails/config/${elementId}/${microversionId}/${segment}/${size}`; } export interface ThumbnailUrlOptions { @@ -30,9 +29,9 @@ export interface ThumbnailUrlOptions { size: ThumbnailSize; /** Empty (the default) serves the element's own thumbnail. */ canonicalConfiguration: string; - /** Whether a miss should kick off generating this configuration. */ - warm?: boolean; - /** Only needed to warm: it is what the render resolves the element from. */ + /** Whether a miss should start rendering this configuration. */ + renderThumbnail?: boolean; + /** Only needed to render: what the render resolves the element from. */ insertableId?: string; /** * Which poll this is. The worker ignores it; it is what keeps each poll off @@ -47,16 +46,18 @@ export function thumbnailUrl({ microversionId, size, canonicalConfiguration, - warm, + renderThumbnail, insertableId, attempt }: ThumbnailUrlOptions): string { + // `v` is the one abbreviation: it is the cache version every immutable url + // carries, and a render is pinned to the microversion it was taken from. const query = new URLSearchParams({ v: microversionId }); if (canonicalConfiguration !== DEFAULT_CANONICAL_CONFIGURATION) { - query.set("c", canonicalConfiguration); - if (warm && insertableId) { - query.set("warm", "true"); - query.set("i", insertableId); + query.set("canonicalConfiguration", canonicalConfiguration); + if (renderThumbnail && insertableId) { + query.set("renderThumbnail", "true"); + query.set("insertableId", insertableId); } // Omitted on the first, so it shares a url with everything else asking // for this configuration. diff --git a/src/backend/features/thumbnails/routes.test.ts b/src/backend/features/thumbnails/routes.test.ts index 64c31edaf..2314a5a0f 100644 --- a/src/backend/features/thumbnails/routes.test.ts +++ b/src/backend/features/thumbnails/routes.test.ts @@ -2,16 +2,8 @@ import { env } from "cloudflare:workers"; import { afterEach, describe, expect, it, vi } from "vitest"; import { createTestApp, jsonRequest } from "../../../__test_utils__"; import { ThumbnailSize } from "./types"; -import { - THUMBNAIL_FALLBACK_CACHE_TTL, - THUMBNAIL_FALLBACK_HEADER, - thumbnailKey, - thumbnailUrl -} from "./keys"; -import { - DEFAULT_CANONICAL_CONFIGURATION, - canonicalConfigurationKey -} from "../configurations/canonical"; +import { THUMBNAIL_FALLBACK_HEADER, thumbnailKey, thumbnailUrl } from "./keys"; +import { DEFAULT_CANONICAL_CONFIGURATION } from "../configurations/canonical"; import { uploadConfigurationThumbnails } from "./store"; import type { OnshapeApi } from "../../lib/onshape/client"; @@ -34,6 +26,30 @@ function get(url: string, sessionId?: string) { return createTestApp().request(url, init, env); } +describe("thumbnailKey", () => { + it("gives the element default its own prefix", () => { + expect(thumbnailKey("e1", MICROVERSION, SIZE)).toBe( + `thumbnails/default/e1/${MICROVERSION}/${SIZE}` + ); + }); + + // A configuration's separators would otherwise open path segments of their + // own, so two different selections could name one key. + it("encodes a configuration into a single segment", () => { + const key = thumbnailKey("e1", MICROVERSION, SIZE, "a=1;b=2/3"); + expect(key).toBe( + `thumbnails/config/e1/${MICROVERSION}/a%3D1%3Bb%3D2%2F3/${SIZE}` + ); + expect(key.split("/")).toHaveLength(6); + }); + + it("gives different configurations different keys", () => { + expect(thumbnailKey("e1", MICROVERSION, SIZE, "a=1")).not.toBe( + thumbnailKey("e1", MICROVERSION, SIZE, "a=2") + ); + }); +}); + describe("thumbnail serving", () => { afterEach(() => vi.restoreAllMocks()); @@ -95,7 +111,7 @@ describe("thumbnail serving", () => { elementId, MICROVERSION, SIZE, - canonicalConfigurationKey(CANONICAL_CONFIGURATION) + CANONICAL_CONFIGURATION ), "config-bytes" ); @@ -126,9 +142,9 @@ describe("thumbnail serving", () => { expect(res.headers.get("Cache-Control")).toBe("private, no-store"); }); - // A configuration we haven't rendered stands in with the element's default, - // cached briefly so the real render can take over as soon as it lands. - it("falls back to the default thumbnail, cached only briefly", async () => { + // A configuration we haven't rendered stands in with the element's default. + // Nothing stores it, so the real render is seen the moment it lands. + it("falls back to the default thumbnail, stored by nobody", async () => { const elementId = "fallback-element"; await env.BLOB.put( thumbnailKey(elementId, MICROVERSION, SIZE), @@ -145,9 +161,7 @@ describe("thumbnail serving", () => { ); expect(res.status).toBe(200); expect(await res.text()).toBe("default-bytes"); - expect(res.headers.get("Cache-Control")).toBe( - `public, max-age=${THUMBNAIL_FALLBACK_CACHE_TTL}` - ); + expect(res.headers.get("Cache-Control")).toBe("private, no-store"); }); it("prefers the configuration's own thumbnail once it exists", async () => { @@ -161,7 +175,7 @@ describe("thumbnail serving", () => { elementId, MICROVERSION, SIZE, - canonicalConfigurationKey(CANONICAL_CONFIGURATION) + CANONICAL_CONFIGURATION ), "configured-bytes" ); @@ -180,7 +194,7 @@ describe("thumbnail serving", () => { }); }); -describe("warming a configuration's thumbnail", () => { +describe("rendering a configuration's thumbnail", () => { afterEach(() => vi.restoreAllMocks()); /** Seeds only the default, so a configuration request always misses. */ @@ -191,29 +205,33 @@ describe("warming a configuration's thumbnail", () => { ); } - it("passes warm as a boolean the validator accepts", () => { + it("passes renderThumbnail as a boolean the validator accepts", () => { const url = thumbnailUrl({ elementId: "any", microversionId: MICROVERSION, size: SIZE, canonicalConfiguration: CANONICAL_CONFIGURATION, - warm: true, + renderThumbnail: true, insertableId: INSERTABLE_ID }); - expect(new URL(url, "http://x").searchParams.get("warm")).toBe("true"); + expect( + new URL(url, "http://x").searchParams.get("renderThumbnail") + ).toBe("true"); }); - // Without one there is nothing to resolve the element from, so warming is - // simply not requested. - it("omits warm when no insertable is named", () => { + // Without one there is nothing to resolve the element from, so the render + // is simply not requested. + it("omits renderThumbnail when no insertable is named", () => { const url = thumbnailUrl({ elementId: "any", microversionId: MICROVERSION, size: SIZE, canonicalConfiguration: CANONICAL_CONFIGURATION, - warm: true + renderThumbnail: true }); - expect(new URL(url, "http://x").searchParams.get("warm")).toBeNull(); + expect( + new URL(url, "http://x").searchParams.get("renderThumbnail") + ).toBeNull(); }); it("starts the render on a miss", async () => { @@ -229,7 +247,7 @@ describe("warming a configuration's thumbnail", () => { microversionId: MICROVERSION, size: SIZE, canonicalConfiguration: CANONICAL_CONFIGURATION, - warm: true, + renderThumbnail: true, insertableId: INSERTABLE_ID }), SESSION_ID @@ -260,7 +278,7 @@ describe("warming a configuration's thumbnail", () => { microversionId: MICROVERSION, size: SIZE, canonicalConfiguration: CANONICAL_CONFIGURATION, - warm: true, + renderThumbnail: true, insertableId: INSERTABLE_ID }) ); @@ -272,7 +290,7 @@ describe("warming a configuration's thumbnail", () => { // Search results show many configurations at once; one cold search must not // kick off a render per row. - it("does not start the render when warm is absent", async () => { + it("does not start the render when renderThumbnail is absent", async () => { const elementId = "cold-element"; await seedDefaultOnly(elementId); const createSpy = vi.spyOn(env.THUMBNAIL_WORKFLOW, "create"); @@ -329,9 +347,9 @@ describe("warming a configuration's thumbnail", () => { expect(urlFor(1)).toBe("1"); }); - it("rejects a warm that is not a boolean", async () => { + it("rejects a renderThumbnail that is not a boolean", async () => { const res = await get( - `/api/thumbnail/${SIZE}/any?v=${MICROVERSION}&c=x&warm=maybe` + `/api/thumbnail/${SIZE}/any?v=${MICROVERSION}&configuration=x&renderThumbnail=maybe` ); expect(res.status).toBe(400); }); @@ -373,7 +391,7 @@ describe("uploadConfigurationThumbnails", () => { elementPath.elementId, MICROVERSION, size, - canonicalConfigurationKey(CANONICAL_CONFIGURATION) + CANONICAL_CONFIGURATION ); expect(await env.BLOB.head(key(ThumbnailSize.SMALL))).not.toBeNull(); expect(await env.BLOB.head(key(ThumbnailSize.LARGE))).not.toBeNull(); @@ -389,7 +407,7 @@ describe("uploadConfigurationThumbnails", () => { storedPath.elementId, MICROVERSION, size, - canonicalConfigurationKey(CANONICAL_CONFIGURATION) + CANONICAL_CONFIGURATION ), "bytes" ); @@ -415,7 +433,7 @@ describe("uploadConfigurationThumbnails", () => { partialPath.elementId, MICROVERSION, ThumbnailSize.SMALL, - canonicalConfigurationKey(CANONICAL_CONFIGURATION) + CANONICAL_CONFIGURATION ), "bytes" ); diff --git a/src/backend/features/thumbnails/routes.ts b/src/backend/features/thumbnails/routes.ts index 727226368..876d0623d 100644 --- a/src/backend/features/thumbnails/routes.ts +++ b/src/backend/features/thumbnails/routes.ts @@ -1,19 +1,12 @@ import { z } from "zod"; +import { HttpStatus } from "http-status-ts"; import { validate } from "../../lib/validate"; -import { CachePolicy, cacheMiddleware, setCacheTtl } from "../../lib/cache"; +import { CachePolicy, setCache } from "../../lib/cache"; import { getApp } from "../../lib/context"; import { ThumbnailSize } from "./types"; -import { - THUMBNAIL_FALLBACK_CACHE_TTL, - THUMBNAIL_FALLBACK_HEADER, - thumbnailKey -} from "./keys"; -import { - DEFAULT_CANONICAL_CONFIGURATION, - DEFAULT_CONFIGURATION_KEY, - canonicalConfigurationKey -} from "../configurations/canonical"; +import { THUMBNAIL_FALLBACK_HEADER, thumbnailKey } from "./keys"; +import { DEFAULT_CANONICAL_CONFIGURATION } from "../configurations/canonical"; import type { AppContext } from "../../lib/context"; import type { ThumbnailWorkflowParams } from "./workflow"; @@ -34,46 +27,55 @@ const canonicalConfigurationQuery = z const storedThumbnailQuery = z.object({ /** The microversion, part of the key — which is what makes a hit immutable. */ v: z.string().min(1), - c: canonicalConfigurationQuery, - warm: z.stringbool().default(false), - /** The insertable to render from; only sent with `warm`. */ - i: z.string().optional() + canonicalConfiguration: canonicalConfigurationQuery, + renderThumbnail: z.stringbool().default(false), + /** The insertable to render from; only sent with `renderThumbnail`. */ + insertableId: z.string().optional() }); /** - * GET /api/thumbnail/:size/:elementId?v=&c=&warm= — an unrendered configuration - * falls back to the element default, and `warm` kicks off the real render. + * GET /api/thumbnail/:size/:elementId?v=&canonicalConfiguration=&renderThumbnail= + * — an unrendered configuration falls back to the element default, and + * `renderThumbnail` starts the real render. + * + * Each answer says how it may be cached rather than the route saying it once: + * stored bytes are pinned by the url, a stand-in and a miss are not. */ thumbnailRoutes.get( "/thumbnail/:size/:elementId", - cacheMiddleware(CachePolicy.PUBLIC_CACHE), validate("param", storedThumbnailParams), validate("query", storedThumbnailQuery), async (c) => { const { size, elementId } = c.req.valid("param"); const { v: microversionId, - c: canonicalConfiguration, - warm, - i: insertableId + canonicalConfiguration, + renderThumbnail, + insertableId } = c.req.valid("query"); - const configurationKey = canonicalConfigurationKey( - canonicalConfiguration - ); - const object = await c.env.BLOB.get( - thumbnailKey(elementId, microversionId, size, configurationKey) + thumbnailKey( + elementId, + microversionId, + size, + canonicalConfiguration + ) ); if (object) { - return thumbnailResponse(object); + // The microversion and the configuration are both in the url, so + // these bytes are the only ones it will ever mean. + return setCache( + thumbnailResponse(object), + CachePolicy.PUBLIC_CACHE + ); } - if (configurationKey === DEFAULT_CONFIGURATION_KEY) { - return c.notFound(); + if (canonicalConfiguration === DEFAULT_CANONICAL_CONFIGURATION) { + return notRenderedYet(); } - if (warm && insertableId) { - await warmConfigurationThumbnail(c, { + if (renderThumbnail && insertableId) { + await startConfigurationRender(c, { insertableId, canonicalConfiguration }); @@ -84,16 +86,24 @@ thumbnailRoutes.get( thumbnailKey(elementId, microversionId, size) ); if (!fallback) { - return c.notFound(); + return notRenderedYet(); } - // Unlike a hit, this url does not pin these bytes. - setCacheTtl(c, THUMBNAIL_FALLBACK_CACHE_TTL); const response = thumbnailResponse(fallback); response.headers.set(THUMBNAIL_FALLBACK_HEADER, "1"); - return response; + // Unlike a hit, this url does not pin these bytes: the real render can + // land at any moment, and whoever asks next should see it. + return setCache(response, CachePolicy.NO_CACHE); } ); +/** Nothing to serve yet, and a render landing later must not be shadowed. */ +function notRenderedYet(): Response { + return setCache( + new Response(null, { status: HttpStatus.NOT_FOUND }), + CachePolicy.NO_CACHE + ); +} + function thumbnailResponse(object: R2ObjectBody): Response { const headers = new Headers(); object.writeHttpMetadata(headers); @@ -104,7 +114,7 @@ function thumbnailResponse(object: R2ObjectBody): Response { * Concurrent requests can each start a run. Rare, and the workflow skips a * render that is already stored, which a reused id would rule out permanently. */ -async function warmConfigurationThumbnail( +async function startConfigurationRender( c: AppContext, params: Omit ): Promise { diff --git a/src/backend/features/thumbnails/store.ts b/src/backend/features/thumbnails/store.ts index 3afc53a63..6bcd67efa 100644 --- a/src/backend/features/thumbnails/store.ts +++ b/src/backend/features/thumbnails/store.ts @@ -18,10 +18,7 @@ import { type ElementPath, type InstancePath } from "../../lib/onshape/path"; import { ThumbnailSize, ThumbnailUrls } from "./types"; import { thumbnailKey, thumbnailUrl } from "./keys"; -import { - DEFAULT_CANONICAL_CONFIGURATION, - canonicalConfigurationKey -} from "../configurations/canonical"; +import { DEFAULT_CANONICAL_CONFIGURATION } from "../configurations/canonical"; import { OnshapeApi } from "../../lib/onshape/client"; /** @@ -120,11 +117,15 @@ export async function uploadConfigurationThumbnails( microversionId: string, canonicalConfiguration: string ): Promise { - const configurationKey = canonicalConfigurationKey(canonicalConfiguration); const { elementId } = elementPath; const targets = [ThumbnailSize.SMALL, ThumbnailSize.LARGE].map((size) => ({ size, - key: thumbnailKey(elementId, microversionId, size, configurationKey) + key: thumbnailKey( + elementId, + microversionId, + size, + canonicalConfiguration + ) })); const keys = targets.map((target) => target.key); diff --git a/src/backend/index.ts b/src/backend/index.ts index 4fc3f2b61..ec0563bde 100644 --- a/src/backend/index.ts +++ b/src/backend/index.ts @@ -4,6 +4,6 @@ export { } from "./features/load/workflows"; export { ThumbnailWorkflow } from "./features/thumbnails/workflow"; import { createApp } from "./app"; -import { productionCaller } from "./features/auth/caller"; +import { productionAuth } from "./features/auth/request-auth"; -export default createApp(productionCaller); +export default createApp(productionAuth); diff --git a/src/backend/lib/api-error.ts b/src/backend/lib/api-error.ts index e70b24e53..d04c6f001 100644 --- a/src/backend/lib/api-error.ts +++ b/src/backend/lib/api-error.ts @@ -1,13 +1,19 @@ /** * The one shape every failed /api response takes; `kind` tells the client what - * to do and carries what that kind needs. A leaf module the frontend imports. + * to do about it. A leaf module the frontend imports. */ +import { HttpStatus } from "http-status-ts"; +// Type-only, so this stays a leaf the frontend can import: the statuses that +// can carry a body, which is every status an error of ours is sent with. +import type { ContentfulStatusCode } from "hono/utils/http-status"; export enum ApiErrorKind { /** `message` is written for the user; show it. */ HANDLED = "handled", - /** Onshape is rate limiting us, and said how long to wait. */ - RATE_LIMITED = "rate-limited", + /** No usable Onshape session: the client can offer to sign in again. */ + SIGN_IN_REQUIRED = "sign-in-required", + /** Signed in, but this caller is not allowed to do it. */ + FORBIDDEN = "forbidden", /** `message` is for the logs. The client shows its own wording instead. */ INTERNAL = "internal" } @@ -19,14 +25,15 @@ interface ApiErrorOf { export type ApiErrorBody = | ApiErrorOf - | ApiErrorOf - | (ApiErrorOf & { retryAfterSeconds: number }); + | ApiErrorOf + | ApiErrorOf + | ApiErrorOf; /** Thrown by a route; the app's error handler turns it into the response. */ export class ApiError extends Error { constructor( readonly body: ApiErrorBody, - readonly status: number + readonly status: ContentfulStatusCode ) { super(body.message); this.name = "ApiError"; @@ -35,22 +42,33 @@ export class ApiError extends Error { } /** The wording reaches the user, so write it for them. */ -export function handledError(message: string, status: number): ApiError { +export function handledError( + message: string, + status: ContentfulStatusCode +): ApiError { return new ApiError({ kind: ApiErrorKind.HANDLED, message }, status); } /** The client will show its own wording; this text is only for the logs. */ -export function internalError(message: string, status: number): ApiError { +export function internalError( + message: string, + status: ContentfulStatusCode +): ApiError { return new ApiError({ kind: ApiErrorKind.INTERNAL, message }, status); } -export function rateLimitedError( - message: string, - status: number, - retryAfterSeconds: number -): ApiError { +/** The caller has no session Onshape accepts; the client can offer them one. */ +export function signInRequiredError(message: string): ApiError { + return new ApiError( + { kind: ApiErrorKind.SIGN_IN_REQUIRED, message }, + HttpStatus.UNAUTHORIZED + ); +} + +/** The caller is known, and this is not theirs to do. */ +export function forbiddenError(message: string): ApiError { return new ApiError( - { kind: ApiErrorKind.RATE_LIMITED, message, retryAfterSeconds }, - status + { kind: ApiErrorKind.FORBIDDEN, message }, + HttpStatus.FORBIDDEN ); } diff --git a/src/backend/lib/cache.ts b/src/backend/lib/cache.ts index bc7c025e9..c4ebb9adb 100644 --- a/src/backend/lib/cache.ts +++ b/src/backend/lib/cache.ts @@ -1,7 +1,7 @@ import { type MiddlewareHandler } from "hono"; import { internalError } from "./api-error"; import { HttpStatus } from "http-status-ts"; -import { type AppContext, type AppContextEnv } from "./context"; +import { type AppContextEnv } from "./context"; /** A year — a versioned url's content never changes, only its version does. */ const IMMUTABLE_CACHE_TTL = 365 * 24 * 3600; @@ -23,9 +23,21 @@ export function immutableCacheControl( return `${policy}, max-age=${IMMUTABLE_CACHE_TTL}, immutable`; } -/** Overrides the route's immutable default for a body its url does not pin. */ -export function setCacheTtl(c: AppContext, maxAge: number): void { - c.set("cacheTtl", maxAge); +/** What a policy says, as the header says it. */ +function cacheControl(policy: CachePolicy): string { + return policy === CachePolicy.NO_CACHE + ? NO_STORE + : immutableCacheControl(policy); +} + +/** + * Declares how one response may be cached, for a route whose answers differ: + * the same url can serve bytes it pins and a stand-in it does not. Routes + * whose every answer is alike take {@link cacheMiddleware} instead. + */ +export function setCache(response: Response, policy: CachePolicy): Response { + response.headers.set("Cache-Control", cacheControl(policy)); + return response; } /** Declares how a route's response may be cached, and enforces what that takes. */ @@ -39,8 +51,6 @@ export function cacheMiddleware( }; } - const cacheControl = immutableCacheControl(policy); - return async (c, next) => { // An immutable response has to be pinned by something, or the next // version of it is unreachable behind the cache. @@ -56,10 +66,6 @@ export function cacheMiddleware( c.header("Cache-Control", NO_STORE); return; } - const ttl = c.get("cacheTtl"); - c.header( - "Cache-Control", - ttl === undefined ? cacheControl : `${policy}, max-age=${ttl}` - ); + c.header("Cache-Control", cacheControl(policy)); }; } diff --git a/src/backend/lib/context.ts b/src/backend/lib/context.ts index 172da95ef..acbc47a02 100644 --- a/src/backend/lib/context.ts +++ b/src/backend/lib/context.ts @@ -25,13 +25,11 @@ export interface AppBindings { } interface AppVariables { - /** Internal cache for `getOnshapeApi` in features/auth/caller.ts. */ + /** Internal cache for `getOnshapeApi` in features/auth/request-auth.ts. */ onshapeApi?: OAuthApi; - /** Internal cache for `isSignedIn` in features/auth/caller.ts. */ + /** Internal cache for `isSignedIn` in features/auth/request-auth.ts. */ signedIn?: boolean; - /** Set by `setCacheTtl`; read by `cacheMiddleware`. */ - cacheTtl?: number; - /** Injected by {@link bindCaller}; see {@link Caller}. */ + /** Injected by {@link bindAuth}; see {@link RequestAuth}. */ getOnshapeApi: () => Promise; getUserId: () => Promise; getAccessLevel: () => Promise; @@ -46,28 +44,31 @@ export interface AppContextEnv { export type AppContext = Context; /** - * Who is making the request, injected per request so tests can substitute a - * caller without an Onshape session. `productionCaller` is the real one. + * What a route may ask about the request it is serving: who is making it, and + * what they are allowed to do. Resolved lazily, so a route that asks nothing + * calls Onshape not at all, and answered per request, so a test can answer + * without a session. `productionAuth` answers for real. */ -export interface Caller { +export interface RequestAuth { getOnshapeApi: () => Promise; getUserId: () => Promise; getAccessLevel: () => Promise; isAuthenticated: () => Promise; } -export type CallerFactory = (c: AppContext) => Caller; +/** How one request's answers are resolved; the app is built with one. */ +export type AuthResolver = (c: AppContext) => RequestAuth; -/** Binds the caller's lookups onto each request, behind `c.var`. */ -export function bindCaller( - makeCaller: CallerFactory +/** Puts the request's own answers behind `c.var`, for routes to ask. */ +export function bindAuth( + resolveAuth: AuthResolver ): MiddlewareHandler { return async (c, next) => { - const caller = makeCaller(c); - c.set("getOnshapeApi", caller.getOnshapeApi); - c.set("getUserId", caller.getUserId); - c.set("getAccessLevel", caller.getAccessLevel); - c.set("isAuthenticated", caller.isAuthenticated); + const auth = resolveAuth(c); + c.set("getOnshapeApi", auth.getOnshapeApi); + c.set("getUserId", auth.getUserId); + c.set("getAccessLevel", auth.getAccessLevel); + c.set("isAuthenticated", auth.isAuthenticated); await next(); }; } diff --git a/src/backend/lib/errors.test.ts b/src/backend/lib/errors.test.ts index 7565f0b01..07aa12e32 100644 --- a/src/backend/lib/errors.test.ts +++ b/src/backend/lib/errors.test.ts @@ -13,8 +13,8 @@ describe("api error responses", () => { await resetDb(db); }); - // Written for the user, so the client shows it verbatim. - it("marks a gate's refusal as handled", async () => { + // Written for the user, and the kind tells the client what to offer them. + it("marks a signed-out caller's refusal as needing a sign-in", async () => { const app = createTestApp({ signedIn: false }); const res = await app.request( @@ -25,11 +25,27 @@ describe("api error responses", () => { expect(res.status).toBe(401); expect(await res.json()).toMatchObject({ - kind: ApiErrorKind.HANDLED, + kind: ApiErrorKind.SIGN_IN_REQUIRED, message: expect.stringContaining("signed in") }); }); + it("marks a caller without the access as forbidden", async () => { + const app = createTestApp({ accessLevel: AccessLevel.USER }); + + const res = await app.request( + `/api/reload-groups/library/${LibraryId.FRC_DESIGN_LIB}`, + jsonRequest("POST"), + env + ); + + expect(res.status).toBe(403); + expect(await res.json()).toMatchObject({ + kind: ApiErrorKind.FORBIDDEN, + message: expect.stringContaining("admin team") + }); + }); + // A malformed request is our bug, so the client falls back to its own // wording rather than showing a validator's message. it("marks a rejected request as internal", async () => { diff --git a/src/backend/lib/errors.ts b/src/backend/lib/errors.ts index cb1f6c2c6..f5a3a9149 100644 --- a/src/backend/lib/errors.ts +++ b/src/backend/lib/errors.ts @@ -5,9 +5,10 @@ import { OnshapeApiError, OnshapeRateLimitError } from "./onshape/client"; import { ApiError, ApiErrorKind, + forbiddenError, handledError, internalError, - rateLimitedError + signInRequiredError } from "./api-error"; import type { AppContextEnv } from "./context"; @@ -17,19 +18,19 @@ import type { AppContextEnv } from "./context"; */ function fromOnshapeError(error: OnshapeApiError): ApiError { if (error instanceof OnshapeRateLimitError) { - return rateLimitedError( - "Onshape rate limit reached. Please try again shortly.", - HttpStatus.TOO_MANY_REQUESTS, - error.retryAfterSeconds + return handledError( + "Onshape rate limit reached. Please try again later.", + HttpStatus.TOO_MANY_REQUESTS ); } - if ( - error.status === HttpStatus.UNAUTHORIZED || - error.status === HttpStatus.FORBIDDEN - ) { - return handledError( - "Onshape refused the request. Try signing in again.", - error.status + if (error.status === HttpStatus.UNAUTHORIZED) { + return signInRequiredError( + "Onshape did not accept the session. Try signing in again." + ); + } + if (error.status === HttpStatus.FORBIDDEN) { + return forbiddenError( + "Onshape rejected the operation. Make sure you have access to this document and you're signed in to the correct Onshape enterprise." ); } return internalError( @@ -40,14 +41,14 @@ function fromOnshapeError(error: OnshapeApiError): ApiError { export const errorHandler: ErrorHandler = (err, c) => { if (err instanceof ApiError) { - return c.json(err.body, err.status as never); + return c.json(err.body, err.status); } if (err instanceof OnshapeApiError) { const apiError = fromOnshapeError(err); if (apiError.body.kind === ApiErrorKind.INTERNAL) { console.error(err); } - return c.json(apiError.body, apiError.status as never); + return c.json(apiError.body, apiError.status); } // A raw HTTPException is a validator rejecting a malformed request, which // is our bug rather than something the user can act on. @@ -55,12 +56,12 @@ export const errorHandler: ErrorHandler = (err, c) => { console.error(err); return c.json( { kind: ApiErrorKind.INTERNAL, message: err.message }, - err.status as never + err.status ); } console.error(err); return c.json( { kind: ApiErrorKind.INTERNAL, message: "Internal Server Error" }, - HttpStatus.INTERNAL_SERVER_ERROR as never + HttpStatus.INTERNAL_SERVER_ERROR ); }; diff --git a/src/backend/lib/onshape/endpoints/metadata.ts b/src/backend/lib/onshape/endpoints/metadata.ts index ac7987084..64995cfbf 100644 --- a/src/backend/lib/onshape/endpoints/metadata.ts +++ b/src/backend/lib/onshape/endpoints/metadata.ts @@ -1,7 +1,7 @@ import { OnshapeApi } from "../client"; import { ElementPath, toElementApiPath } from "../path"; import { apiPath } from "../api-path"; -import { encodeConfigurationForQuery } from "../../../features/configurations/utils"; +import { encodeConfiguration } from "../../../features/configurations/utils"; import { ParameterValues } from "../../../features/configurations/models"; import type { OnshapeMetadataObject } from "../types"; @@ -11,7 +11,7 @@ export function getElementMetadata( elementPath: ElementPath, configuration: ParameterValues ): Promise { - const encoded = encodeConfigurationForQuery(configuration); + const encoded = encodeConfiguration(configuration); // Computed properties are expensive and unused, and indexing probes this // once per configuration. const query: Record = { diff --git a/src/backend/lib/onshape/endpoints/parts.ts b/src/backend/lib/onshape/endpoints/parts.ts index ac6dda7c6..27ef6629e 100644 --- a/src/backend/lib/onshape/endpoints/parts.ts +++ b/src/backend/lib/onshape/endpoints/parts.ts @@ -1,7 +1,7 @@ import { OnshapeApi } from "../client"; import { ElementPath, toElementApiPath } from "../path"; import { apiPath } from "../api-path"; -import { encodeConfigurationForQuery } from "../../../features/configurations/utils"; +import { encodeConfiguration } from "../../../features/configurations/utils"; import { ParameterValues } from "../../../features/configurations/models"; import type { OnshapeAssemblyDefinition, OnshapePart } from "../types"; @@ -12,7 +12,7 @@ import type { OnshapeAssemblyDefinition, OnshapePart } from "../types"; function configurationQuery( configuration: ParameterValues ): Record { - const encoded = encodeConfigurationForQuery(configuration); + const encoded = encodeConfiguration(configuration); return encoded ? { configuration: encoded } : {}; } diff --git a/src/backend/lib/text.test.ts b/src/backend/lib/text.test.ts new file mode 100644 index 000000000..24bf40f01 --- /dev/null +++ b/src/backend/lib/text.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from "vitest"; +import { clean, equalsIgnoreCase } from "./text"; + +describe("clean", () => { + it("trims the value", () => { + expect(clean(" WCP-1025 ")).toBe("WCP-1025"); + }); + + it.each([undefined, null, "", " "])( + "reads %s as nothing at all", + (value) => { + expect(clean(value)).toBeUndefined(); + } + ); +}); + +describe("equalsIgnoreCase", () => { + it("ignores case and surrounding space", () => { + expect(equalsIgnoreCase(" Spacer ", "spacer")).toBe(true); + }); + + it("separates different text", () => { + expect(equalsIgnoreCase("Spacer", "Standoff")).toBe(false); + }); + + // Two parts with nothing to say are not thereby the same part. + it("does not equate two blanks with a value", () => { + expect(equalsIgnoreCase(" ", undefined)).toBe(true); + expect(equalsIgnoreCase("Spacer", undefined)).toBe(false); + }); +}); diff --git a/src/backend/lib/text.ts b/src/backend/lib/text.ts new file mode 100644 index 000000000..96fbacd2f --- /dev/null +++ b/src/backend/lib/text.ts @@ -0,0 +1,14 @@ +/** Trimming and casing, defined once: both sides import this leaf. */ + +/** A value's meaningful text, or nothing when it is blank. */ +export function clean(text: string | undefined | null): string | undefined { + return text?.trim() || undefined; +} + +/** Whether two values say the same thing, ignoring case and surrounding space. */ +export function equalsIgnoreCase( + a: string | undefined | null, + b: string | undefined | null +): boolean { + return clean(a)?.toLowerCase() === clean(b)?.toLowerCase(); +} diff --git a/src/frontend/components/alerts.tsx b/src/frontend/components/alerts.tsx index 6f46118d6..a8d6dd7e1 100644 --- a/src/frontend/components/alerts.tsx +++ b/src/frontend/components/alerts.tsx @@ -1,8 +1,9 @@ import { modals } from "@mantine/modals"; -import { Box, Text } from "@mantine/core"; -import { Warning } from "@phosphor-icons/react"; +import { Text } from "@mantine/core"; +import { WarningIcon } from "@phosphor-icons/react"; import { AppTitle } from "./app-title"; -import { IconSize } from "../lib/style-constants"; +import { IconSize, StatusColor } from "../lib/style-constants"; +import { AppIcon } from "./app-icon"; interface OpenWarningAlertProps { title: string; @@ -14,7 +15,11 @@ function openWarningAlert(props: OpenWarningAlertProps): void { title: ( + } title={props.title} /> diff --git a/src/frontend/components/app-icon.tsx b/src/frontend/components/app-icon.tsx new file mode 100644 index 000000000..1e8e79053 --- /dev/null +++ b/src/frontend/components/app-icon.tsx @@ -0,0 +1,42 @@ +import { Box } from "@mantine/core"; +import type { Icon, IconWeight } from "@phosphor-icons/react"; +import { ComponentPropsWithRef, ReactNode } from "react"; +import { IconSize, StatusColor } from "../lib/style-constants"; + +export interface AppIconProps + // Rendered through Box, which owns these two as style props. + extends Omit, "color" | "display"> { + icon: Icon; + /** @default IconSize.SMALL */ + size?: IconSize; + /** A theme color; without one the icon takes the surrounding text's. */ + color?: StatusColor | string; + /** @default "regular" */ + weight?: IconWeight; + /** What a screen reader calls an icon that carries meaning on its own. */ + label?: string; +} + +/** + * A Phosphor icon in a theme color. Box resolves the name, and sizes through + * `fz` because its own `style` would drop the icon's. + */ +export function AppIcon({ + icon, + size = IconSize.SMALL, + color, + weight, + label, + ...others +}: AppIconProps): ReactNode { + return ( + + ); +} diff --git a/src/frontend/components/app-menu.tsx b/src/frontend/components/app-menu.tsx index 1c7b69d5a..b0834f4d9 100644 --- a/src/frontend/components/app-menu.tsx +++ b/src/frontend/components/app-menu.tsx @@ -1,7 +1,7 @@ import { PropsWithChildren, ReactNode } from "react"; import { FloatingPosition, Menu, ActionIcon } from "@mantine/core"; -import { DotsThree } from "@phosphor-icons/react"; -import { IconSize } from "../lib/style-constants"; +import { DotsThreeIcon } from "@phosphor-icons/react"; +import { IconSize, StatusColor } from "../lib/style-constants"; interface AppContextMenuProps { menuItems: ReactNode; @@ -77,12 +77,14 @@ export function MenuButton(props: MenuButtonProps): ReactNode { e.stopPropagation()} > - + ); diff --git a/src/frontend/components/app-modal.tsx b/src/frontend/components/app-modal.tsx index f19a9a379..22bb56965 100644 --- a/src/frontend/components/app-modal.tsx +++ b/src/frontend/components/app-modal.tsx @@ -1,17 +1,18 @@ import { Group, type MantineSpacing, Stack } from "@mantine/core"; import { PropsWithChildren, ReactNode } from "react"; -import { BORDER, CHROME_BACKGROUND } from "../lib/style-constants"; +import { BORDER, FRAME_BACKGROUND } from "../lib/style-constants"; interface AppModalBodyProps extends PropsWithChildren { /** Space between children; content that spaces itself should pass 0. */ gap?: MantineSpacing; } -/** A modal's content, padded away from the chrome framing it. */ +/** A modal's content, padded away from the header and footer framing it. */ export function AppModalBody(props: AppModalBodyProps): ReactNode { + const { gap = "sm", children } = props; return ( - - {props.children} + + {children} ); } @@ -23,7 +24,7 @@ export function AppModalFooter(props: PropsWithChildren): ReactNode { justify="space-between" wrap="nowrap" p="sm" - bg={CHROME_BACKGROUND} + bg={FRAME_BACKGROUND} style={{ borderTop: BORDER }} > {props.children} diff --git a/src/frontend/components/app-navbar.tsx b/src/frontend/components/app-navbar.tsx index 56dc80134..d2635a336 100644 --- a/src/frontend/components/app-navbar.tsx +++ b/src/frontend/components/app-navbar.tsx @@ -2,6 +2,7 @@ import { ActionIcon, Box, Button, + Center, Group, Input, Loader, @@ -10,12 +11,15 @@ import { TextInput, Tooltip } from "@mantine/core"; -import { Gear, MagnifyingGlass } from "@phosphor-icons/react"; +import { GearIcon, MagnifyingGlassIcon } from "@phosphor-icons/react"; import { BORDER, - CHROME_BACKGROUND, + FRAME_BACKGROUND, IconSize, - PrimaryColor + maskedImage, + PrimaryColor, + RADIUS, + StatusColor } from "../lib/style-constants"; import { ReactNode, RefObject, useRef } from "react"; import { useNavigate } from "@tanstack/react-router"; @@ -23,8 +27,12 @@ import { useNavigate } from "@tanstack/react-router"; import frcDesignBook from "/frc-design-book.svg"; import { openSettingsMenu } from "../features/settings/open-settings-menu"; import { VendorMenu } from "../features/settings/components/vendor-filters"; -import { useUiState } from "../lib/ui-state"; -import { getLibraryName, useLibraryId } from "../features/library/library-path"; +import { useGetUiState, useSetUiState } from "../lib/ui-state"; +import { + getLibraryName, + isComingSoon, + useLibraryId +} from "../features/library/library-path"; import { RequireAccessLevel } from "../features/auth/access-level"; import { useSaveSettings } from "../features/settings/settings"; import { useAccessData } from "../features/auth/access-level"; @@ -39,6 +47,9 @@ import { getLibraryVersionQuery } from "../features/library/queries"; * brand and settings alongside, over a row holding search and its filters. */ export function AppNavbar(): ReactNode { + // Nothing to search until the library opens. + const showSearch = !isComingSoon(useLibraryId()); + return ( {/* Stretched so the tabs run the full height and their underline @@ -48,7 +59,7 @@ export function AppNavbar(): ReactNode { px="sm" wrap="nowrap" align="stretch" - bg={CHROME_BACKGROUND} + bg={FRAME_BACKGROUND} style={{ borderBottom: BORDER }} > @@ -59,10 +70,12 @@ export function AppNavbar(): ReactNode { - - - - + {showSearch && ( + + + + + )} ); } @@ -109,7 +122,7 @@ function RunningJobLoader(): ReactNode { function FrcDesignBookIcon(): ReactNode { return ( - {/* Masked, not drawn, so the book takes the tile's contrast color - rather than the gray in the file. The url needs quoting: Vite - inlines the asset as a data uri containing apostrophes. */} + rather than the gray in the file. */} - + ); } @@ -207,13 +209,13 @@ export function SettingsButton() { return ( openSettingsMenu()} > - + ); } @@ -229,7 +231,8 @@ function selectAllInputText(ref: RefObject) { export function SearchBar() { const ref = useRef(null); - const [uiState, setUiState] = useUiState(); + const uiState = useGetUiState(); + const setUiState = useSetUiState(); const libraryId = useLibraryId(); const clearButton = uiState.searchQuery ? ( @@ -250,7 +253,7 @@ export function SearchBar() { // The panel opens to a library the caller is here to search. autoFocus flex={1} - leftSection={} + leftSection={} placeholder={`Search ${getLibraryName(libraryId)}...`} ref={ref} value={uiState.searchQuery ?? ""} diff --git a/src/frontend/components/app-select.tsx b/src/frontend/components/app-select.tsx deleted file mode 100644 index 14e9d3981..000000000 --- a/src/frontend/components/app-select.tsx +++ /dev/null @@ -1,34 +0,0 @@ -import { Select } from "@mantine/core"; -import { Dispatch, ReactNode } from "react"; -import { SelectOption } from "./select-utils"; - -interface AppSelectProps { - option: SelectOption; - /** - * A list of options to choose from. - * Should be wrapped in a useMemo to ensure stability. - */ - options: SelectOption[]; - label: string; - onSelect: Dispatch; -} - -export function AppSelect(props: AppSelectProps): ReactNode { - const { option, options, onSelect, label } = props; - - return ( - ({ + value: option, + label: capitalize(option) + }))} + value={value} + allowDeselect={false} + checkIconPosition="right" + comboboxProps={{ withinPortal: true }} + onChange={(selected) => { + if (selected !== null) { + onSelect(selected); + } + }} + /> + ); } export function SettingsMenuContent(): ReactNode { - const accessData = useAccessData(); - - let adminSettings: ReactNode = null; - // Unlike all other checks, this one uses maxAccessLevel so you can still switch from user to admin - if (hasEditorAccess(accessData.maxAccessLevel)) { - adminSettings = ( - <> - - Admin Settings - - - - - ); - } + const { maxAccessLevel } = useAccessData(); return ( <> - {adminSettings} + {/* Unlike all other checks, this one uses maxAccessLevel so you can + still switch back up from user to admin. */} + {hasEditorAccess(maxAccessLevel) && ( + <> + + Admin Settings + + + + + )} ); } function UserSettings(): ReactNode { - // The modal renders at the root, outside the route matches, so navigate by - // exact path: a bare navigate would resolve to the route `from` names. - const location = useRouterState({ select: (state) => state.location }); - const navigate = useNavigate(); - const saveSettings = useSaveSettings(); const libraryId = useLibraryId(); const isConnected = useIsConnectedToOnshape(); - const theme = location.search.theme ?? DEFAULT_SETTINGS.theme; return ( <> - { - // The url renders it; the write-behind decides what the - // entry redirect seeds next time. - saveSettings({ theme }); - void navigate({ - to: location.pathname, - search: (prev) => ({ ...prev, theme }) - }); - }} - /> + {/* Only worth offering from inside Onshape's panel, which is what the standalone app is roomier than. */} {isConnected && ( )} + {/* Onshape owns the session the panel runs in, so signing out is + only the standalone app's to offer. */} + {!isConnected && ( + + + + + + )} ); } /** - * The app's own url for the current library, free of the params Onshape - * launches it with — carrying those over is what would keep it embedded. + * The app's own url for the library, free of Onshape's launch params, which are + * what would keep it embedded. Settings follow on their own, being this browser's. */ -function standaloneUrl(libraryId: LibraryId, theme: Theme): string { - const url = new URL(`/app/library/${libraryId}`, window.location.origin); - url.searchParams.set("theme", theme); - return url.toString(); +function standaloneUrl(libraryId: LibraryId): string { + return new URL(`/app/library/${libraryId}`, window.location.origin).href; } -interface ThemeSelectProps { - theme: Theme; - onThemeSelect: Dispatch; -} - -function ThemeSelect(props: ThemeSelectProps): ReactNode { - const { theme, onThemeSelect } = props; - - // Use a memo to stabilize access levels so Select's activeItem tracks properly between renders - const themes = useSelectOptions( - [Theme.SYSTEM, Theme.DARK, Theme.LIGHT], - capitalize - ); +function ThemeSelect(): ReactNode { + const theme = useGetUiState().theme; + const saveSettings = useSaveSettings(); return ( - onThemeSelect(value as Theme)} + value={theme ?? DEFAULT_SETTINGS.theme} + options={[Theme.SYSTEM, Theme.DARK, Theme.LIGHT]} + onSelect={(theme) => saveSettings({ theme })} /> ); } @@ -159,32 +182,19 @@ function AdminSettings(): ReactNode { } function AccessLevelSelect(): ReactNode { - const accessData = useAccessData(); - const setUiState = useUiState()[1]; - - const { maxAccessLevel, currentAccessLevel } = accessData; - // Use a memo to stabilize access levels so Select's activeItem tracks properly between renders - const accessLevels = useSelectOptions( - useMemo( - () => - [ - AccessLevel.ADMIN, - AccessLevel.EDITOR, - AccessLevel.USER - ].filter((level) => isWithinAccessLevel(level, maxAccessLevel)), - [maxAccessLevel] - ), - capitalize - ); + const { maxAccessLevel, currentAccessLevel } = useAccessData(); + const setUiState = useSetUiState(); return ( - { - setUiState({ accessLevel: value as AccessLevel }); - }} + value={currentAccessLevel} + options={[ + AccessLevel.ADMIN, + AccessLevel.EDITOR, + AccessLevel.USER + ].filter((level) => isWithinAccessLevel(level, maxAccessLevel))} + onSelect={(accessLevel) => setUiState({ accessLevel })} /> ); } diff --git a/src/frontend/features/settings/components/vendor-filters.tsx b/src/frontend/features/settings/components/vendor-filters.tsx index ebd0626b0..d6761b7b2 100644 --- a/src/frontend/features/settings/components/vendor-filters.tsx +++ b/src/frontend/features/settings/components/vendor-filters.tsx @@ -1,27 +1,23 @@ import { ActionIcon, Button, Menu } from "@mantine/core"; -import { Funnel, FunnelX } from "@phosphor-icons/react"; +import { FunnelIcon, FunnelXIcon } from "@phosphor-icons/react"; import { IconSize } from "../../../lib/style-constants"; import { ReactNode } from "react"; import { getVendorName } from "@backend/features/library/vendors"; import { Vendor } from "@backend/features/library/vendors"; -import { useUiState } from "../../../lib/ui-state"; +import { useGetUiState, useSetUiState } from "../../../lib/ui-state"; import { AppContextMenu } from "../../../components/app-menu"; interface ClearFiltersButtonProps { - /** - * @default "Clear filters" - */ + /** @default "Clear filters" */ text?: string; - /** - * @default false - */ + /** @default false */ small?: boolean; } export function ClearFiltersButton(props: ClearFiltersButtonProps): ReactNode { - const [uiState, setUiState] = useUiState(); - const text = props.text ?? "Clear filters"; - const small = props.small ?? false; + const { text = "Clear filters", small = false } = props; + const uiState = useGetUiState(); + const setUiState = useSetUiState(); const vendorFilters = uiState.vendorFilters; const areAllTagsActive = vendorFilters === undefined; @@ -31,7 +27,7 @@ export function ClearFiltersButton(props: ClearFiltersButtonProps): ReactNode { disabled={areAllTagsActive} variant="default" size={small ? "xs" : undefined} - leftSection={} + leftSection={} onClick={() => { setUiState({ vendorFilters: undefined }); }} @@ -46,7 +42,8 @@ export function ClearFiltersButton(props: ClearFiltersButtonProps): ReactNode { * vendor checkbox items. `undefined` filters mean "all vendors active". */ export function VendorMenu(): ReactNode { - const [uiState, setUiState] = useUiState(); + const uiState = useGetUiState(); + const setUiState = useSetUiState(); const hasFilters = uiState.vendorFilters !== undefined; const menuItems = ( @@ -69,7 +66,7 @@ export function VendorMenu(): ReactNode { } + leftSection={} disabled={!hasFilters} onClick={() => setUiState({ vendorFilters: undefined })} > @@ -87,7 +84,7 @@ export function VendorMenu(): ReactNode { size="input-sm" title="Filter vendors" > - + ); diff --git a/src/frontend/features/settings/local-settings.ts b/src/frontend/features/settings/local-settings.ts deleted file mode 100644 index 2968c7bf5..000000000 --- a/src/frontend/features/settings/local-settings.ts +++ /dev/null @@ -1,36 +0,0 @@ -import { type LibraryId } from "@backend/features/library/library-id"; -import { - DEFAULT_SETTINGS, - type SettingsUpdate, - type Theme -} from "@backend/features/settings/settings"; - -const SETTINGS_STORAGE_KEY = "frc-design-app-settings"; - -function readStored(): SettingsUpdate { - try { - const raw = localStorage.getItem(SETTINGS_STORAGE_KEY); - return raw ? (JSON.parse(raw) as SettingsUpdate) : {}; - } catch { - return {}; - } -} - -/** Locally-persisted settings (used when not signed in), with defaults filled. */ -export function readLocalSettings(): { theme: Theme; libraryId: LibraryId } { - const stored = readStored(); - return { - theme: stored.theme ?? DEFAULT_SETTINGS.theme, - libraryId: stored.libraryId ?? DEFAULT_SETTINGS.libraryId - }; -} - -/** Merges and persists settings locally, used when not signed in. */ -export function writeLocalSettings(newSettings: SettingsUpdate): void { - try { - const merged = { ...readStored(), ...newSettings }; - localStorage.setItem(SETTINGS_STORAGE_KEY, JSON.stringify(merged)); - } catch { - // Ignore storage failures (e.g. private browsing). - } -} diff --git a/src/frontend/features/settings/settings.ts b/src/frontend/features/settings/settings.ts index f5e75733a..e7532ad9c 100644 --- a/src/frontend/features/settings/settings.ts +++ b/src/frontend/features/settings/settings.ts @@ -4,24 +4,28 @@ import { showErrorToast } from "../../lib/notifications"; import { apiPost } from "../../lib/api-client"; import { getAccessDataQuery } from "../auth/access-level"; import { queryClient } from "../../lib/query-client"; -import { writeLocalSettings } from "./local-settings"; +import { updateUiState } from "../../lib/ui-state"; + +/** + * Applies a setting where the app reads it, and saves it to the caller's row, + * which a first-run browser and the Onshape launch start from. + */ +async function saveSettings(newSettings: SettingsUpdate): Promise { + updateUiState(newSettings); + // Resolved here rather than read off a render: a placeholder that says + // signed out would skip the save for a user who has a server-side row. + const { signedIn } = + await queryClient.ensureQueryData(getAccessDataQuery()); + if (!signedIn) { + return; + } + await apiPost("/settings", { body: newSettings }); +} export function useSaveSettings() { const { mutate } = useMutation({ mutationKey: ["settings"], - mutationFn: async (newSettings: SettingsUpdate) => { - // Resolved here rather than read off a render: a placeholder that - // says signed out would silently persist locally for a user who - // has a server-side row. - const { signedIn } = - await queryClient.ensureQueryData(getAccessDataQuery()); - // Not signed in: no server-side user row; persist locally instead. - if (!signedIn) { - writeLocalSettings(newSettings); - return; - } - return apiPost("/settings", { body: newSettings }); - }, + mutationFn: saveSettings, onError: () => { showErrorToast("Unexpectedly failed to update settings."); } @@ -29,3 +33,13 @@ export function useSaveSettings() { return mutate; } + +/** + * Records where the caller is, for the entry redirect to resume at. Called from + * a route rather than a component, so it cannot be the mutation above. + */ +export function rememberOpenGroup(groupId: string | null): void { + void saveSettings({ groupId }).catch(() => { + // Resuming in the library instead of the group is not worth a toast. + }); +} diff --git a/src/frontend/features/thumbnails/components/thumbnail.tsx b/src/frontend/features/thumbnails/components/thumbnail.tsx index 8c9b946bf..b3a99fc13 100644 --- a/src/frontend/features/thumbnails/components/thumbnail.tsx +++ b/src/frontend/features/thumbnails/components/thumbnail.tsx @@ -8,9 +8,14 @@ import { ElementType } from "@backend/lib/onshape/element-type"; import { ThumbnailSize } from "@backend/features/thumbnails/types"; import { ElementPath } from "@backend/lib/onshape/path"; import { Box, Card, Center, HoverCard, Loader } from "@mantine/core"; -import { Question } from "@phosphor-icons/react"; +import { QuestionIcon } from "@phosphor-icons/react"; -import { ComponentPropsWithRef, ReactNode, useState } from "react"; +import { + ComponentPropsWithRef, + PropsWithChildren, + ReactNode, + useState +} from "react"; import { DEFAULT_CANONICAL_CONFIGURATION } from "@backend/features/configurations/canonical"; import { thumbnailUrl } from "@backend/features/thumbnails/keys"; import { SectionError } from "../../../components/app-zero-state"; @@ -49,11 +54,11 @@ export interface ThumbnailTarget { /** Empty means the element default. */ canonicalConfiguration: string; /** - * Surfaces where the user picked the configuration warm it; a cold search - * would otherwise start a render per row. + * Whether a miss should start rendering: surfaces where the user picked the + * configuration do, where a search would otherwise render a row at a time. */ - warm: boolean; - /** Only needed to warm: what the render resolves the element from. */ + renderThumbnail: boolean; + /** Only needed to render: what the render resolves the element from. */ insertableId?: string; } @@ -126,7 +131,7 @@ function Thumbnail(props: ThumbnailProps): ReactNode { let content; if (url === undefined || imageQuery.isError) { - content = ; + content = ; } else if (imageQuery.isPending) { content = ; } else { @@ -174,6 +179,12 @@ interface PreviewImageProps { largeThumbnailUrl?: string; } +/** A stored size, so the bytes a preview fetch returns are worth caching. */ +const PREVIEW_SIZE = ThumbnailSize.LARGE; + +/** Sized to the preview's footprint rather than to a row's. */ +const PREVIEW_SPINNER_SIZE = 36; + /** How often to re-check while the worker is still standing in the default. */ const PREVIEW_POLL_MS = 4000; @@ -181,7 +192,7 @@ const PREVIEW_POLL_MS = 4000; * How many polls apart to ask for the render again. One request is meant to * start it; this only covers the run never having been queued at all. */ -const WARM_EVERY_POLLS = 15; +const RENDER_EVERY_POLLS = 15; /** * The last render actually produced, kept across configuration changes: the @@ -195,42 +206,29 @@ function useLastRenderedUrl(image?: LoadedImage): string | undefined { return lastRendered; } -export function PreviewImage(props: PreviewImageProps): ReactNode { - const { - path, - insertableId, - microversionId, - canonicalConfiguration, - largeThumbnailUrl - } = props; - // A stored size, so the bytes this fetch returns are worth caching. - const size = ThumbnailSize.LARGE; - const isSignedIn = useIsSignedIn(); - const isConnected = useIsConnectedToOnshape(); - const isFetchingConfiguration = useIsFetchingConfiguration( - insertableId, - microversionId - ); - const targetElementType = useTargetElementType(); - - // Each poll gets a url of its own. The browser caches images by url for - // the life of the page, so reusing one leaves the stand-in on screen - // however many times the query refetches past it. +/** + * Polls for the configuration's render, which the worker produces in a + * workflow: the first request starts one, and the element default stands in + * until it lands. + */ +function usePreviewThumbnail(props: PreviewImageProps, enabled: boolean) { + const { path, insertableId, microversionId, canonicalConfiguration } = + props; + // A url per poll: the browser caches images by url for the life of the + // page, so reusing one leaves the stand-in up however often we refetch. const pollUrl = (attempt: number) => thumbnailUrl({ elementId: path.elementId, microversionId, - size, + size: PREVIEW_SIZE, canonicalConfiguration, - warm: attempt % WARM_EVERY_POLLS === 0, + renderThumbnail: attempt % RENDER_EVERY_POLLS === 0, insertableId, attempt }); - // The worker renders configurations in a workflow, so the first request - // starts one and stands in the element default until it lands. const queryKey = ["thumbnail", pollUrl(0)]; - const thumbnailQuery = useQuery({ + const query = useQuery({ queryKey, queryFn: ({ signal, client }) => loadImageResult( @@ -242,20 +240,50 @@ export function PreviewImage(props: PreviewImageProps): ReactNode { refetchInterval: (query) => query.state.data?.isFallback ? PREVIEW_POLL_MS : false, retry: 2, - enabled: !isFetchingConfiguration && isSignedIn === true + enabled }); - const lastRenderedUrl = useLastRenderedUrl(thumbnailQuery.data); + return { query, lastRenderedUrl: useLastRenderedUrl(query.data) }; +} + +interface PreviewBoxProps extends PropsWithChildren { + heightAndWidth: HeightAndWidth; +} + +/** Holds the preview's own footprint, whatever is being shown in it. */ +function PreviewBox(props: PreviewBoxProps): ReactNode { + const { heightAndWidth, children } = props; + return ( +
+ {children} +
+ ); +} - const heightAndWidth = getHeightAndWidth(size, 0.7); +export function PreviewImage(props: PreviewImageProps): ReactNode { + const { insertableId, microversionId, largeThumbnailUrl } = props; + const isSignedIn = useIsSignedIn(); + const isConnected = useIsConnectedToOnshape(); + const isFetchingConfiguration = useIsFetchingConfiguration( + insertableId, + microversionId + ); + const targetElementType = useTargetElementType(); + const { query, lastRenderedUrl } = usePreviewThumbnail( + props, + !isFetchingConfiguration && isSignedIn === true + ); + + const heightAndWidth = getHeightAndWidth(PREVIEW_SIZE, 0.7); + const spinner = ( + + + + ); // Not known yet: the stored thumbnail would be swapped for the live preview // a moment later. if (isSignedIn === undefined) { - return ( -
- -
- ); + return spinner; } // Not signed in: no live Onshape preview, so show the stored thumbnail @@ -265,21 +293,16 @@ export function PreviewImage(props: PreviewImageProps): ReactNode { ); } - // Placeholder data is the previous configuration's render, so the spinner - // has to cover it too: what is on screen is not what was asked for. - const isWaiting = - thumbnailQuery.isPlaceholderData || thumbnailQuery.data?.isFallback; - - if (thumbnailQuery.isError) { + if (query.isError) { const action = targetElementType === ElementType.ASSEMBLY ? "insert" : "derive"; return ( -
+ -
- ); - } else if (!thumbnailQuery.data) { - return ( -
- -
+ ); } + if (!query.data) { + return spinner; + } // A stand-in must not displace a render the user already has. const previewUrl = - thumbnailQuery.data.isFallback && lastRenderedUrl + query.data.isFallback && lastRenderedUrl ? lastRenderedUrl - : thumbnailQuery.data.url; + : query.data.url; + // Placeholder data is the previous configuration's render, so the spinner + // has to cover it too: what is on screen is not what was asked for. + const isWaiting = query.isPlaceholderData || query.data.isFallback; return ( <> diff --git a/src/frontend/lib/errors.ts b/src/frontend/lib/errors.ts index cbff181b8..24ba82532 100644 --- a/src/frontend/lib/errors.ts +++ b/src/frontend/lib/errors.ts @@ -1,5 +1,6 @@ import { ApiErrorKind, type ApiErrorBody } from "@backend/lib/api-error"; -import { showErrorToast } from "./notifications"; +import { renderNotification, showErrorToast } from "./notifications"; +import { startSignIn } from "../features/auth/sign-in"; /** * A failure worth telling the user about, from the backend or raised here. @@ -18,27 +19,22 @@ export function appError(message: string): AppError { return new AppError({ kind: ApiErrorKind.HANDLED, message }); } +/** The kinds whose message was written for the user to read. */ +const SPOKEN_KINDS: ApiErrorBody["kind"][] = [ + ApiErrorKind.HANDLED, + ApiErrorKind.SIGN_IN_REQUIRED, + ApiErrorKind.FORBIDDEN +]; + /** Builds an {@link AppError} from a failed /api response body. */ export function fromApiErrorBody(body: unknown): AppError { const parsed = body as Partial | undefined; - switch (parsed?.kind) { - case ApiErrorKind.HANDLED: - return new AppError({ - kind: ApiErrorKind.HANDLED, - message: parsed.message ?? "" - }); - case ApiErrorKind.RATE_LIMITED: - return new AppError({ - kind: ApiErrorKind.RATE_LIMITED, - message: parsed.message ?? "", - retryAfterSeconds: parsed.retryAfterSeconds ?? 0 - }); - default: - return new AppError({ - kind: ApiErrorKind.INTERNAL, - message: parsed?.message ?? "" - }); - } + const kind = SPOKEN_KINDS.find((spoken) => spoken === parsed?.kind); + // Anything unrecognized is a failure we did not write wording for. + return new AppError({ + kind: kind ?? ApiErrorKind.INTERNAL, + message: parsed?.message ?? "" + }); } export function getAppErrorHandler(defaultMessage: string, toastId?: string) { @@ -46,8 +42,8 @@ export function getAppErrorHandler(defaultMessage: string, toastId?: string) { } /** - * Only an error carrying wording meant for the user shows its own message; - * anything else gets `defaultMessage`, written for the caller's context. + * Only an error worded for the user shows its own message; anything else gets + * `defaultMessage`. One the caller can act on offers them that action. */ export function handleAppError( error: Error, @@ -56,8 +52,17 @@ export function handleAppError( ) { if (error instanceof AppError) { switch (error.body.kind) { + case ApiErrorKind.SIGN_IN_REQUIRED: + showErrorToast( + renderNotification(error.body.message, { + text: "Sign in", + onClick: startSignIn + }), + toastKey + ); + return; case ApiErrorKind.HANDLED: - case ApiErrorKind.RATE_LIMITED: + case ApiErrorKind.FORBIDDEN: showErrorToast(error.body.message, toastKey); return; case ApiErrorKind.INTERNAL: diff --git a/src/frontend/lib/notifications.tsx b/src/frontend/lib/notifications.tsx index f7b789326..70c55acb1 100644 --- a/src/frontend/lib/notifications.tsx +++ b/src/frontend/lib/notifications.tsx @@ -1,8 +1,8 @@ import { notifications } from "@mantine/notifications"; import type { ReactNode } from "react"; -import { CheckCircle, Info, XCircle } from "@phosphor-icons/react"; -import { IconSize } from "./style-constants"; -import { Group, Button } from "@mantine/core"; +import { CheckCircleIcon, InfoIcon, XCircleIcon } from "@phosphor-icons/react"; +import { IconSize, NO_SHRINK } from "./style-constants"; +import { Box, Group, Button } from "@mantine/core"; export interface NotificationAction { text: string; @@ -23,12 +23,14 @@ export function renderNotification( {/* Only reachable on a window too narrow for the row: the message is what gives, and the button keeps its label intact. */} - {message} + + {message} + @@ -88,7 +90,7 @@ export function showInfoToast( ): string { return showToast({ color: "blue", - icon: , + icon: , message, ...options }); @@ -109,16 +111,16 @@ export function showSuccessToast(message: string, id?: string): string { return showToast({ id, color: "green", - icon: , + icon: , message }); } -export function showErrorToast(message: string, id?: string): string { +export function showErrorToast(message: ReactNode, id?: string): string { return showToast({ id, color: "red", - icon: , + icon: , message }); } diff --git a/src/frontend/lib/onshape-params.ts b/src/frontend/lib/onshape-params.ts index ea986e353..0d538225f 100644 --- a/src/frontend/lib/onshape-params.ts +++ b/src/frontend/lib/onshape-params.ts @@ -10,18 +10,13 @@ export interface OnshapeParams extends ElementPath { elementType: ElementType; /** The color scheme Onshape is using, forwarded by the entry redirect. */ systemTheme: ColorTheme; - /** The caller's saved theme, seeded by the entry redirect. */ - theme: Theme; + /** The account's saved theme, seeded by the entry redirect and then taken + * into ui-state, which is where the app reads it from. */ + theme?: Theme; server: string; - /** Set on the sign-in redirect so the app confirms success once. */ - justSignedIn?: string; } -/** - * An actual color theme, as provided by Onshape. - * - * See also Theme, which is a superset of ColorTheme that also includes "system". - */ +/** A resolved theme, as Onshape provides it; Theme adds "system" on top. */ export type ColorTheme = "light" | "dark"; /** diff --git a/src/frontend/lib/part-number.test.ts b/src/frontend/lib/part-number.test.ts deleted file mode 100644 index fe629aa9d..000000000 --- a/src/frontend/lib/part-number.test.ts +++ /dev/null @@ -1,20 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { displayPartNumber } from "./part-number"; - -describe("displayPartNumber", () => { - it.each(["N/A", "n/a", " N/a "])("hides the placeholder %s", (value) => { - expect(displayPartNumber(value)).toBeUndefined(); - }); - - it("hides a number that only repeats the name it sits under", () => { - expect(displayPartNumber(" spacer ", "Spacer")).toBeUndefined(); - }); - - it("keeps a real number, trimmed", () => { - expect(displayPartNumber(" WCP-1025 ", "Gearbox")).toBe("WCP-1025"); - }); - - it("keeps one that merely contains the placeholder", () => { - expect(displayPartNumber("NA-1234")).toBe("NA-1234"); - }); -}); diff --git a/src/frontend/lib/part-number.ts b/src/frontend/lib/part-number.ts deleted file mode 100644 index e0987111d..000000000 --- a/src/frontend/lib/part-number.ts +++ /dev/null @@ -1,17 +0,0 @@ -/** Admins write this in where a generic part has no real number to give. */ -const PLACEHOLDER_PART_NUMBER = /^n\/a$/i; - -/** - * The part number to show, or nothing when it identifies nothing — a - * placeholder, or a repeat of the name it sits under. - */ -export function displayPartNumber( - partNumber: string | undefined, - name?: string -): string | undefined { - const text = partNumber?.trim(); - if (!text || PLACEHOLDER_PART_NUMBER.test(text)) { - return undefined; - } - return text.toLowerCase() === name?.trim().toLowerCase() ? undefined : text; -} diff --git a/src/frontend/lib/refresh.ts b/src/frontend/lib/refresh.ts index d347e0aba..9e4b8f829 100644 --- a/src/frontend/lib/refresh.ts +++ b/src/frontend/lib/refresh.ts @@ -1,7 +1,7 @@ import { useCallback, useEffect, useRef } from "react"; import { useRouter } from "@tanstack/react-router"; import { queryClient } from "./query-client"; -import { useJobStatusQuery } from "../features/library/queries"; +import { useIsJobRunning } from "../features/library/queries"; import { accessDataQueryKey, favoritesQueryKey, @@ -42,7 +42,7 @@ export function useRefreshFavorites(): () => Promise { /** Polls whether a load job is running and refreshes the library once it finishes. */ export function useJobStatus(): boolean { const refreshLibrary = useRefreshLibrary(); - const running = useJobStatusQuery().data?.running ?? false; + const running = useIsJobRunning(); // A ref, not state: tracking the previous value to detect the finished // transition shouldn't trigger a render (and set-state-in-effect is banned). const wasRunning = useRef(running); diff --git a/src/frontend/lib/style-constants.ts b/src/frontend/lib/style-constants.ts index 564eb5e06..65d2f257b 100644 --- a/src/frontend/lib/style-constants.ts +++ b/src/frontend/lib/style-constants.ts @@ -25,8 +25,32 @@ export enum FontWeight { export const BORDER = "1px solid var(--mantine-color-default-border)"; -/** A step off the page: the navbar's tab row, a modal's header and footer. */ -export const CHROME_BACKGROUND = +/** The corner every box of ours is cut with, matching the theme's default. */ +export const RADIUS = "var(--mantine-radius-sm)"; + +/** + * The colors state is spoken in, as Mantine names them. Named here rather than + * written at each control, so an error looks like an error everywhere. + */ +export enum StatusColor { + ERROR = "red", + WARNING = "yellow", + INFO = "blue", + SUCCESS = "green", + /** A control that should not compete with the library's accent. */ + NEUTRAL = "gray", + /** Secondary text: the metadata beside a name. */ + DIMMED = "dimmed" +} + +/** The same color as a tint to sit content on, e.g. a callout's background. */ +export function statusBackground(color: StatusColor): string { + return `var(--mantine-color-${color}-light)`; +} + +/** A step off the page, for the bars framing it: the navbar's tab row, a + * modal's header and footer. */ +export const FRAME_BACKGROUND = "light-dark(var(--mantine-color-gray-2), var(--mantine-color-dark-8))"; /** @@ -35,6 +59,23 @@ export const CHROME_BACKGROUND = */ export const TITLE_ICON_NUDGE = { transform: "translateY(-1px)" }; +/** Holds an icon or badge at its own size beside text that can outgrow the row. */ +export const NO_SHRINK = { flexShrink: 0 }; + +/** + * Paints an image in the current text color rather than its own. The url needs + * quoting: Vite inlines an asset as a data uri, which can contain apostrophes. + */ +export function maskedImage(url: string) { + return { + backgroundColor: "currentColor", + maskImage: `url("${url}")`, + maskSize: "contain", + maskRepeat: "no-repeat", + maskPosition: "center" + }; +} + /** * One height for a section header, set rather than left to the content: an * accordion is sized by its label, a group header by its menu button. diff --git a/src/frontend/lib/ui-state.ts b/src/frontend/lib/ui-state.ts index 147de9e24..fdfb4a88e 100644 --- a/src/frontend/lib/ui-state.ts +++ b/src/frontend/lib/ui-state.ts @@ -1,57 +1,56 @@ import { useSyncExternalStore } from "react"; import * as z from "zod"; import { AccessLevel } from "@backend/features/auth/access-level"; +import { LibraryId } from "@backend/features/library/library-id"; import { Vendor } from "@backend/features/library/vendors"; +import { DEFAULT_SETTINGS, Theme } from "@backend/features/settings/settings"; -// Increment this when a breaking change is made to the schema +/** Bumped when a change to the schema makes stored state unusable. */ const LATEST_VERSION = 3; +const STORAGE_KEY = "uiState"; + const VendorType = z.enum(Object.values(Vendor)); const AccessLevelType = z.enum(Object.values(AccessLevel)); +const ThemeType = z.enum(Object.values(Theme)); +const LibraryIdType = z.enum(Object.values(LibraryId)); const UiStateSchema = z.object({ - version: z.number().default(1), // We can't default the parsed version to LATEST_VERSION because of old versions floating around + // Defaulted to the first version rather than the latest: state stored + // before the field existed is old state, not current state. + version: z.number().default(1), isFavoritesOpen: z.boolean().default(false), isLibraryOpen: z.boolean().default(true), vendorFilters: z.array(VendorType).optional(), searchQuery: z.string().default(""), - openGroupId: z.string().optional(), fasten: z.boolean().default(true), /** The access level to view the app as; absent means the granted default. */ accessLevel: AccessLevelType.optional(), - /** Whether the quick-insert tip has been shown; it is only worth saying once. */ - hasSeenQuickInsertTip: z.boolean().default(false) + /** Set on leaving for Onshape, so the app can confirm the sign-in on return. */ + justSignedIn: z.boolean().default(false), + // The caller's settings, and the source of truth for them; a signed-in + // caller's row is what a browser that has never run the app starts from. + theme: ThemeType.default(DEFAULT_SETTINGS.theme), + libraryId: LibraryIdType.default(DEFAULT_SETTINGS.libraryId), + /** The group last opened in that library; null for the library itself. */ + groupId: z.string().nullable().default(DEFAULT_SETTINGS.groupId) }); -type UiState = z.infer; +export type UiState = z.infer; type Subscriber = () => void; const subscribers = new Set(); -let uiStateCache: UiState | null = null; - -function setUiState(uiState: UiState) { - const parsed = UiStateSchema.parse(uiState); - - // Sets always set latest version - parsed.version = LATEST_VERSION; +/** The state this session is working from; the store is written behind it. */ +let currentState: UiState | null = null; - // Only update if changed - if ( - uiStateCache === null || - JSON.stringify(parsed) !== JSON.stringify(uiStateCache) - ) { - uiStateCache = parsed; - writeStorage(JSON.stringify(parsed)); - subscribers.forEach((callback) => callback()); - } -} +const defaultState = (): UiState => UiStateSchema.parse({}); /** Blocked or partitioned storage must not break the app, only its memory. */ function readStorage(): string | null { try { - return window.localStorage.getItem("uiState"); + return window.localStorage.getItem(STORAGE_KEY); } catch { return null; } @@ -59,36 +58,37 @@ function readStorage(): string | null { function writeStorage(value: string): void { try { - window.localStorage.setItem("uiState", value); + window.localStorage.setItem(STORAGE_KEY, value); } catch { // Nothing to do; the in-memory cache still serves this session. } } /** - * Asynchronously retrieves the current UI state. + * What was stored, or the defaults when it cannot be used — older, hand-edited, + * or naming something dropped. Losing a preference beats failing to start. */ -export function getUiState(): UiState { - if (uiStateCache) return uiStateCache; - +function readStoredState(): UiState { const raw = readStorage(); - // Nothing in storage, initialize with defaults if (!raw) { - uiStateCache = UiStateSchema.parse({}); - return uiStateCache; + return defaultState(); } - - uiStateCache = UiStateSchema.parse( - // Convert null to undefined for optional fields - JSON.parse(raw, (_key, value) => value ?? undefined) - ); - - if (uiStateCache.version < LATEST_VERSION) { - // Always reset to defaults for simplicity - // Updated version will get set in the next version - uiStateCache = UiStateSchema.parse({}); + try { + const parsed = UiStateSchema.safeParse( + // A stored null reads as absent, which is what a default fills. + JSON.parse(raw, (_key, value) => value ?? undefined) + ); + return parsed.success && parsed.data.version >= LATEST_VERSION + ? parsed.data + : defaultState(); + } catch { + return defaultState(); } - return uiStateCache; +} + +export function getUiState(): UiState { + currentState ??= readStoredState(); + return currentState; } function subscribeToUiState(callback: Subscriber) { @@ -96,24 +96,34 @@ function subscribeToUiState(callback: Subscriber) { return () => subscribers.delete(callback); } -/** - * Asynchronously updates the current UI state. - */ +/** Merges into the state, stores it, and tells every reader it changed. */ export function updateUiState(partialState: Partial): UiState { - const newState = { + const newState: UiState = { ...getUiState(), ...partialState, + // Writing always stamps the version the shape actually has. version: LATEST_VERSION }; - setUiState(newState); + if (JSON.stringify(newState) === JSON.stringify(currentState)) { + return newState; + } + currentState = newState; + writeStorage(JSON.stringify(newState)); + subscribers.forEach((callback) => callback()); return newState; } export type SetUiState = (uiState: Partial) => void; -export function useUiState(): [UiState, SetUiState] { - // Create a react version of the state to trigger re-renders - const reactUiState = useSyncExternalStore(subscribeToUiState, getUiState); +/** The current state, re-rendering the caller whenever it changes. */ +export function useGetUiState(): UiState { + return useSyncExternalStore(subscribeToUiState, getUiState); +} - return [reactUiState, updateUiState]; +/** Merges into the state; every reader of it re-renders. */ +// The setter half of the pair above: a component reaches for one or the other, +// so both read as hooks though setting needs no state of its own. +// eslint-disable-next-line react-x/no-unnecessary-use-prefix +export function useSetUiState(): SetUiState { + return updateUiState; } diff --git a/src/frontend/lib/url.tsx b/src/frontend/lib/url.tsx index 4350aacd1..d4a18fbc3 100644 --- a/src/frontend/lib/url.tsx +++ b/src/frontend/lib/url.tsx @@ -8,9 +8,9 @@ import { ConfigurablePath, isConfigurablePath } from "@backend/lib/onshape/path"; -import { encodeConfigurationForQuery } from "@backend/features/configurations/utils"; +import { encodeConfiguration } from "@backend/features/configurations/utils"; import { notifications } from "@mantine/notifications"; -import { Link } from "@phosphor-icons/react"; +import { LinkIcon } from "@phosphor-icons/react"; import { IconSize } from "./style-constants"; export function makeUrl(path: ConfigurablePath): string; @@ -30,7 +30,7 @@ export function makeUrl(path: DocumentPath): string { // what Onshape's api takes; a url needs its own escaping. url += "?configuration=" + - encodeURIComponent(encodeConfigurationForQuery(path.configuration)); + encodeURIComponent(encodeConfiguration(path.configuration)); } return url; } @@ -39,7 +39,7 @@ export function makeUrl(path: DocumentPath): string { * Parses an Onshape document URL into an ElementPath. * Returns `undefined` if the URL could not be parsed successfully. */ -export function parseUrl(urlString: string): ElementPath | undefined { +export function parseOnshapeUrl(urlString: string): ElementPath | undefined { try { // Example pathname: /documents/769b556baf61d32b18813fd0/w/e6d6c2b3a472b97a7e352949/e/8a0c13d3b2b68a99502dc436 const url = new URL(urlString); @@ -68,13 +68,8 @@ export async function copyUrlToClipboard(url: string): Promise { await navigator.clipboard.writeText(url); notifications.show({ message: "Link copied to clipboard.", - icon: , + icon: , color: "blue", autoClose: 3000 }); } - -/** - * URL of the FRCDesign feedback Google Form. - */ -export const FEEDBACK_FORM_URL = "https://forms.gle/WVXUwnrrpLGKdiBx9"; diff --git a/src/frontend/routes/__root.tsx b/src/frontend/routes/__root.tsx index 110c7d8d2..b9b00e286 100644 --- a/src/frontend/routes/__root.tsx +++ b/src/frontend/routes/__root.tsx @@ -13,7 +13,7 @@ import { useColorScheme } from "@mantine/hooks"; import { queryClient } from "../lib/query-client"; import { createAppTheme } from "../theme"; import { getColorTheme } from "../lib/onshape-params"; -import { DEFAULT_SETTINGS } from "@backend/features/settings/settings"; +import { useGetUiState } from "../lib/ui-state"; import { NotFoundError, RootCrash } from "../components/root-error"; export const Route = createRootRoute({ @@ -26,21 +26,21 @@ export const Route = createRootRoute({ }); function RootComponent(): ReactNode { - // Both come off the url — the entry redirect seeds them and a switch - // rewrites them — so the first paint is already the right colors. + // The library comes off the url, so the first paint is already its color. const search = useSearch({ strict: false }); const params = useParams({ strict: false }); + const { theme: savedTheme, libraryId } = useGetUiState(); const theme = useMemo( - () => createAppTheme(params.libraryId ?? DEFAULT_SETTINGS.libraryId), - [params.libraryId] + () => createAppTheme(params.libraryId ?? libraryId), + [params.libraryId, libraryId] ); // Onshape puts its own scheme on the url when it launches us; standalone // there is none, and the OS is what "system" means. const osColorScheme = useColorScheme(); const colorTheme = getColorTheme( - search.theme ?? DEFAULT_SETTINGS.theme, + savedTheme, search.systemTheme ?? osColorScheme ); diff --git a/src/frontend/routes/app/library/$libraryId/groups/$groupId.tsx b/src/frontend/routes/app/library/$libraryId/groups/$groupId.tsx index acf80caad..fc1df2a7e 100644 --- a/src/frontend/routes/app/library/$libraryId/groups/$groupId.tsx +++ b/src/frontend/routes/app/library/$libraryId/groups/$groupId.tsx @@ -7,11 +7,16 @@ import { useParams } from "@tanstack/react-router"; import { Box, Button, Group } from "@mantine/core"; -import { ArrowLeft, ArrowUUpLeft, Warning } from "@phosphor-icons/react"; +import { + ArrowLeftIcon, + ArrowUUpLeftIcon, + WarningIcon +} from "@phosphor-icons/react"; import { BORDER, IconSize, - SECTION_HEADER_HEIGHT + SECTION_HEADER_HEIGHT, + StatusColor } from "../../../../../lib/style-constants"; import { ReactNode } from "react"; import { SearchResults } from "../../../../../features/search/components/search-results"; @@ -31,13 +36,15 @@ import { import { ClearFiltersButton } from "../../../../../features/settings/components/vendor-filters"; import { useLibraryQuery } from "../../../../../features/library/queries"; import { useLibraryId } from "../../../../../features/library/library-path"; -import { useUiState, updateUiState } from "../../../../../lib/ui-state"; +import { useGetUiState } from "../../../../../lib/ui-state"; +import { rememberOpenGroup } from "../../../../../features/settings/settings"; +import { AppIcon } from "../../../../../components/app-icon"; export const Route = createFileRoute("/app/library/$libraryId/groups/$groupId")( { component: GroupList, onEnter: (match) => { - updateUiState({ openGroupId: match.params.groupId }); + rememberOpenGroup(match.params.groupId); } } ); @@ -49,7 +56,7 @@ function GroupList(): ReactNode { from: "/app/library/$libraryId/groups/$groupId" }); - const uiState = useUiState()[0]; + const uiState = useGetUiState(); if (libraryQuery.isPending) { return ; @@ -69,7 +76,7 @@ function GroupList(): ReactNode { justifyUp action={