From 398dfaf8d5e56144249e0c72120f751cf1ad217e Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:36:17 -0700 Subject: [PATCH 01/53] chore(web): chat/wire comments say why, not what (CL-8378) --- apps/web/src/chat/wire/agent-address.ts | 17 +----- apps/web/src/chat/wire/blocks.ts | 36 +++--------- .../src/chat/wire/consumer-inference-text.ts | 22 ++------ apps/web/src/chat/wire/display-name.ts | 48 ++++------------ apps/web/src/chat/wire/id-leak-guard.ts | 39 ++----------- apps/web/src/chat/wire/mentions.ts | 36 +++--------- apps/web/src/chat/wire/participants.ts | 56 ++++--------------- apps/web/src/chat/wire/parts.ts | 49 ++++------------ apps/web/src/chat/wire/stream-events.ts | 53 +++--------------- apps/web/src/chat/wire/tools-unsupported.ts | 24 ++------ docs/chat-wire-contract.md | 28 ++++++++++ 11 files changed, 103 insertions(+), 305 deletions(-) create mode 100644 docs/chat-wire-contract.md diff --git a/apps/web/src/chat/wire/agent-address.ts b/apps/web/src/chat/wire/agent-address.ts index 06d4699c2..0e655ef7f 100644 --- a/apps/web/src/chat/wire/agent-address.ts +++ b/apps/web/src/chat/wire/agent-address.ts @@ -1,18 +1,7 @@ -// Mirrored from packages/chat/src: apps/web and @/chat -// must not import @corbits/chat, a server-only package. This is the browser- -// facing half of the same wire contract the hub's chat routes still speak; -// once the hub moves onto native mail threads (T5a/T5c) this file becomes -// the one source of truth and packages/chat's copy goes away. +// Mirrored from packages/chat/src (see docs/chat-wire-contract.md). -// Agent-address helpers shared by this package's own fan-out -// (`routes.ts`) and by `@/chat`'s composer: splitting an -// address into its local part and domain. Mirrors the split -// `@intx/types/src/agent-address.ts`'s `parseAgentAddress` does, -// but stays permissive about the left-hand side on purpose — the -// mention rule (see `mentions.ts`) only ever needs the substring before -// the first "@", never a validated `ins_`-prefixed instance id, so it -// tolerates any participant string rather than rejecting non-address -// ones the way `parseAgentAddress` does. +// Deliberately permissive, unlike `@intx/types`'s `parseAgentAddress`: the +// mention rule only needs the substring before "@", not a validated id. export function localPartOf(address: string): string { const at = address.indexOf("@"); diff --git a/apps/web/src/chat/wire/blocks.ts b/apps/web/src/chat/wire/blocks.ts index 5acf5bb42..97eeede9b 100644 --- a/apps/web/src/chat/wire/blocks.ts +++ b/apps/web/src/chat/wire/blocks.ts @@ -1,26 +1,15 @@ -// Mirrored from packages/chat/src: apps/web and @/chat -// must not import @corbits/chat, a server-only package. This is the browser- -// facing half of the same wire contract the hub's chat routes still speak; -// once the hub moves onto native mail threads (T5a/T5c) this file becomes -// the one source of truth and packages/chat's copy goes away. +// Mirrored from packages/chat/src (see docs/chat-wire-contract.md) — apps/web +// must not import @corbits/chat, a server-only package. import { type } from "arktype"; import type { BlockPart } from "./parts"; -// The typed vocabulary for `BlockPart.block` payloads. `BlockPart` stays -// `{ type: string, data: unknown }` on the wire so unknown types degrade to -// a labeled fallback instead of failing the whole message; `parseBlock` is -// the render-boundary parse that turns that envelope into a typed block. -// Every schema strips undeclared keys deeply: agent-authored extras (say, a -// hostile `tally` object on a poll) must never ride along on the parsed -// object a renderer trusts. +// Every schema strips undeclared keys deeply so agent-authored extras (a +// hostile `tally` on a poll) never reach a renderer that trusts them. -// An approve block carries only a reference to a platform approval plus the -// agent's framing. It deliberately has no action labels and no resolved -// state: free-form button text on an approval card is a spoofing surface, -// and the decision's status lives on the approval record, never in the -// message. +// No action labels or resolved state: those live on the approval record, +// never in the message, so an agent can't spoof a decision. export const ApproveBlockData = type({ approvalId: "string", title: "string", @@ -65,16 +54,9 @@ export const StreamBlockData = type({ }).onDeepUndeclaredKey("delete"); export type StreamBlockData = typeof StreamBlockData.infer; -// An agent-authored "connect this service" card, the -// generalization of `connect-github` to every connector and MCP preset: -// `request_connection` posts one of these instead of a prose deep link. -// It carries only the framing the agent decided — which service, and the -// consumer-language reason it would unlock — never an auth mode or a -// connected/disconnected verdict: an agent that could author "connected" -// (or steer OAuth vs key-paste) would be spoofing live state next to a -// live button. The card's real state and connect affordance come from a -// host-supplied actions port resolved against the tenant's actual -// connections at render time. +// No auth mode or connected/disconnected verdict: an agent that could author +// "connected" would be spoofing live state next to a live button. Real +// state and the connect affordance come from the host at render time. export const ConnectServiceBlockData = type({ connectorId: "string > 0", displayName: "string > 0", diff --git a/apps/web/src/chat/wire/consumer-inference-text.ts b/apps/web/src/chat/wire/consumer-inference-text.ts index 3f2431018..d7c816e41 100644 --- a/apps/web/src/chat/wire/consumer-inference-text.ts +++ b/apps/web/src/chat/wire/consumer-inference-text.ts @@ -1,14 +1,7 @@ -// Mirrored from packages/chat/src: apps/web and @/chat -// must not import @corbits/chat, a server-only package. This is the browser- -// facing half of the same wire contract the hub's chat routes still speak; -// once the hub moves onto native mail threads (T5a/T5c) this file becomes -// the one source of truth and packages/chat's copy goes away. +// Mirrored from packages/chat/src (see docs/chat-wire-contract.md). -/** - * Person-facing inference copy. HTTP status, raw provider dumps, and - * JSON error objects never belong on the timeline, in a sidebar preview, - * or next to the composer — DESIGN.md Honesty is one consumer sentence. - */ +// HTTP status, raw provider dumps, and JSON error objects never belong on +// the timeline — DESIGN.md Honesty is one consumer sentence. import { TOOLS_UNSUPPORTED_CONSUMER_MESSAGE, @@ -21,13 +14,8 @@ const TRAILING_HTTP_DUMP = /\s*\[HTTP\s+\d+\]:[\s\S]*$/i; export const CONSUMER_INFERENCE_FAILURE_NOTICE = "This didn't go through. Try again, or check the connection in Settings."; -/** - * Byte-for-byte copies of the two preambles `@intx/inference`'s - * `formatInferenceError` writes for `credential_failure` and - * `quota_exhausted`. Kept here so the bench-list preview path - * can refuse them without depending on `@/chat`. The chat-ui - * drift guard still owns matching these against the published director. - */ +// Byte-for-byte copies of `@intx/inference`'s preambles, kept here so the +// bench-list preview path can match them without depending on `@/chat`. export const CLASSIFIED_INFERENCE_FAILURE_PREAMBLES: readonly string[] = [ "This agent could not complete your request due to a credential error", "This agent could not complete your request because the API quota has been exhausted", diff --git a/apps/web/src/chat/wire/display-name.ts b/apps/web/src/chat/wire/display-name.ts index d73fbaa12..426964d7b 100644 --- a/apps/web/src/chat/wire/display-name.ts +++ b/apps/web/src/chat/wire/display-name.ts @@ -1,17 +1,7 @@ -// Mirrored from packages/chat/src: apps/web and @/chat -// must not import @corbits/chat, a server-only package. This is the browser- -// facing half of the same wire contract the hub's chat routes still speak; -// once the hub moves onto native mail threads (T5a/T5c) this file becomes -// the one source of truth and packages/chat's copy goes away. +// Mirrored from packages/chat/src (see docs/chat-wire-contract.md). -// A workflow definition's person-facing display name, derived once here -// so every caller that decides "what does this agent look like to a -// person" reads it the same way — never a scattered `description ?? name` -// (or worse, a raw address/run id) reimplemented per call site. -// -// Lives in `@/chat/wire` (mirrored from `@corbits/chat`'s -// `workbench-host-naming`) so this browser-facing surface never needs a -// server-only chat import. +// One derivation for "what does this agent look like to a person", never a +// scattered `description ?? name` reimplemented per call site. import { type } from "arktype"; import { ID_LEAK_PATTERN } from "./id-leak-guard"; @@ -20,13 +10,8 @@ const DisplayNameSource = type({ "description?": "string | null", }); -/** kebab-case identifier -> Title Case words: `"research-analyst"` -> - * `"Research Analyst"`. Words that aren't hyphen-separated (a name that - * already reads as prose) pass through with only their case fixed up, so - * this is safe to run over a definition's raw `name` unconditionally — - * PROVIDED that `name` is never an internal id in disguise; see - * `deriveDisplayName`'s own guard for why the raw run-id case never - * reaches this function at all. */ +// Safe to run over a raw `name` only because `deriveDisplayName` already +// guards against an internal id reaching this function. export function humanizeSlug(slug: string): string { return slug .split(/[-_\s]+/) @@ -35,21 +20,9 @@ export function humanizeSlug(slug: string): string { .join(" "); } -/** - * The display name a definition should render as: its own description - * when one was set at creation, otherwise a humanized reading of its - * immutable slug. A whitespace-only description reads as absent — never - * a blank display name — since it carries nothing a person actually - * typed. Throws on a shape that isn't at least `{ name }` — this is a - * trust boundary, not a formatting helper, so a malformed record fails - * loudly rather than rendering "undefined". - * - * Also throws when `name` is itself an internal-id shape (`run_…`, - * `wfd_…`, …, see `./id-leak-guard`) — the product rule is that a person - * never sees an internal identifier, so a caller that reaches this - * function with a run id where a definition's slug belongs gets a loud - * failure instead of a Title-Cased leak like "Run 737a058d…". - */ +// A trust boundary, not a formatter: throws on a malformed record and on a +// `name` that is itself an internal id, rather than leaking one into a +// rendered display name. export function deriveDisplayName(definition: { readonly name: string; readonly description?: string | null; @@ -87,9 +60,8 @@ export type UserFacingAgentDefinition = { export type WithDisplayName = T & { readonly displayName: string }; -/** Projects `deriveDisplayName` onto a definition, keeping every other - * field untouched — the read-boundary derivation done once here rather - * than as scattered `??` fallbacks in UI code. */ +// Read-boundary derivation, done once rather than as scattered `??` +// fallbacks in UI code. export function withDisplayName( definition: T, ): WithDisplayName { diff --git a/apps/web/src/chat/wire/id-leak-guard.ts b/apps/web/src/chat/wire/id-leak-guard.ts index fb0b1c5b9..a211c119d 100644 --- a/apps/web/src/chat/wire/id-leak-guard.ts +++ b/apps/web/src/chat/wire/id-leak-guard.ts @@ -1,28 +1,8 @@ -// Mirrored from packages/chat/src: apps/web and @/chat -// must not import @corbits/chat, a server-only package. This is the browser- -// facing half of the same wire contract the hub's chat routes still speak; -// once the hub moves onto native mail threads (T5a/T5c) this file becomes -// the one source of truth and packages/chat's copy goes away. +// Mirrored from packages/chat/src (see docs/chat-wire-contract.md, which +// also covers this file's rationale). -// The guard: a person never sees an internal -// identifier — not as a participant's name, not in a join/system line, -// not in a greeting, not in a chat title. This has recurred repeatedly -// (a raw definitionId/runId in insights, a `writing_systems` id in a -// Myra reply, and the "Run 737a058d…" / "I'm run_737a…") each -// time as a one-off display-time patch; this module is the one place -// every id-generating prefix is named, so a new leak is a missed test -// run rather than a missed grep. -// -// Prefix words mirror `@intx/hub-common`'s `generateId` (vendored; -// upstream source: `packages/hub-common/src/ids.ts`'s `PREFIXES`) for -// the six kinds named explicitly — a workflow run, a workflow -// definition, a tenant, a principal, an asset, and a git token — each -// normally followed by `_` and 32 lowercase hex characters (`run_737a…`). -// -// Matched with either `_`, a space, or `-` as the separator, not just -// `_`: `humanizeSlug`'s Title Case reading of a leaked id (its own -// "Run 737a058d48006e2bde12559576f422e0") replaces the underscore with a -// space, so the raw and the humanized-leak forms both need to trip this. +// Matches `_`, space, or `-` as separator so both the raw id and +// `humanizeSlug`'s Title-Cased reading of one ("Run 737a058d…") trip it. const ID_PREFIX_WORDS = ["run", "wfd", "tnt", "prn", "ast", "gtk"] as const; export const ID_LEAK_PATTERN = new RegExp( @@ -30,15 +10,8 @@ export const ID_LEAK_PATTERN = new RegExp( "i", ); -/** - * Throws when `value` carries an internal identifier — a raw id, or a - * humanized reading of one (`"Run 737a058d…"`, produced by title-casing - * `run_737a058d…`'s underscore split). The humanized case is why this - * checks the ORIGINAL prefixes case-insensitively rather than requiring - * the literal lowercase prefix: `humanizeSlug` capitalizes the first - * letter of the leading word, so a leaked run id renders as "Run …", not - * "run …", by the time it would reach a person. - */ +// Case-insensitive because `humanizeSlug` capitalizes the leading word +// ("Run …") before this would ever see it. export function assertNoLeakedInternalId(value: string, context: string): void { if (ID_LEAK_PATTERN.test(value)) { throw new Error( diff --git a/apps/web/src/chat/wire/mentions.ts b/apps/web/src/chat/wire/mentions.ts index cd682057f..ff36c43c1 100644 --- a/apps/web/src/chat/wire/mentions.ts +++ b/apps/web/src/chat/wire/mentions.ts @@ -1,28 +1,14 @@ -// Mirrored from packages/chat/src: apps/web and @/chat -// must not import @corbits/chat, a server-only package. This is the browser- -// facing half of the same wire contract the hub's chat routes still speak; -// once the hub moves onto native mail threads (T5a/T5c) this file becomes -// the one source of truth and packages/chat's copy goes away. +// Mirrored from packages/chat/src (see docs/chat-wire-contract.md). -// The "@ + handle" mention rule: the single wire-level contract between -// the composer (`@/chat`'s `mentions.ts`, which derives -// candidates from a workbench's participant records and splices the -// picked handle into the draft) and this package's own fan-out -// (`routes.ts`'s `POST /workbenches/:id/messages`). Both sides work off -// the same `ParticipantRecord[]` (see `participants.ts`), so a message -// the composer thinks mentions a participant is always exactly the set -// the server fans a copy to — the handle is the friendly, settings-held -// mention name, never the instance-id local part. +// The composer and the server fan-out both derive mentions from the same +// `ParticipantRecord[]`, so what the composer highlights is always exactly +// who gets a copy. import type { Part as PartType } from "./parts"; import type { ParticipantRecord } from "./participants"; -/** - * A participant is an agent address (mention-fannable) when it has the - * `local@domain` shape every agent address has and is not a principal. - * Humans are `prn_…` — bare or `prn_…@tenant.domain` — and read the - * workbench's own timeline directly, so they are never fanned a copy. - */ +// Humans (`prn_…`) read the workbench timeline directly and are never +// fanned a copy; only agent addresses are. export function isAgentAddress(participant: string): boolean { return participant.includes("@") && !participant.startsWith("prn_"); } @@ -31,14 +17,8 @@ function escapeForRegExp(text: string): string { return text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); } -/** - * The participants an ordinary message @mentions, restricted to agent - * addresses: the fan-out set for `POST /workbenches/:id/messages`. A - * mention is structural — `@` followed by the participant's own handle - * at a word boundary, appearing in any `TextPart` of the message — not - * a full parse of mention syntax; kept minimal per the anchor-mailbox - * rework's scope. - */ +// Structural match only (`@handle` at a word boundary) — not a full mention +// syntax parse. export function mentionedParticipants( parts: readonly PartType[], participants: readonly ParticipantRecord[], diff --git a/apps/web/src/chat/wire/participants.ts b/apps/web/src/chat/wire/participants.ts index f9f4d989b..7c2e85a0a 100644 --- a/apps/web/src/chat/wire/participants.ts +++ b/apps/web/src/chat/wire/participants.ts @@ -1,19 +1,7 @@ -// Mirrored from packages/chat/src: apps/web and @/chat -// must not import @corbits/chat, a server-only package. This is the browser- -// facing half of the same wire contract the hub's chat routes still speak; -// once the hub moves onto native mail threads (T5a/T5c) this file becomes -// the one source of truth and packages/chat's copy goes away. +// Mirrored from packages/chat/src (see docs/chat-wire-contract.md). -// Workbench participant records: the settings-backed source of mention -// handles. `chat/participants` (see `routes.ts`'s `ChatNamespaceSchemas`) -// holds `ParticipantRecord[]` — an address plus the short, -// unique-within-workbench handle a mention actually types (`@echo`), never -// the unusable instance-id local part (`@ins_cd03d8e3...`). Reading -// tolerates a bare address string too — a participant seeded before this -// record shape existed, or a caller PATCHing `chat/participants` with -// plain addresses — always upgrading it to a record on the way out (its -// handle defaults to the address's own local part); writing always -// produces records, never bare strings. +// Reading tolerates a bare address string (pre-rollout data) and upgrades it +// to a record; writing always produces records, never bare strings. import { type } from "arktype"; import { localPartOf } from "./agent-address"; @@ -46,24 +34,14 @@ function parseParticipantEntry(entry: unknown): ParticipantRecord { return result; } -/** - * Parses a whole `chat/participants` setting value into records, - * tolerant of the mixed shape a workbench written before and after this - * rollout might carry: some entries bare strings, some already records. - * Anything other than an array is treated as no participants at all. - */ +// Tolerant of a workbench with a mix of pre- and post-rollout entries. export function parseParticipants(raw: unknown): ParticipantRecord[] { if (!Array.isArray(raw)) return []; return raw.map(parseParticipantEntry); } -/** - * A short, word-safe handle derived from a workflow definition's name — - * e.g. "Echo Bot" -> "echo-bot" — lowercased, non-alphanumeric runs - * collapsed to a single hyphen, and leading/trailing hyphens trimmed. - * Falls back to the given address's own local part when the name yields - * nothing usable (e.g. a name of all punctuation, or empty). - */ +// Falls back to the address's local part when the name yields nothing +// usable (e.g. all punctuation). export function handleFromName(name: string, fallbackAddress: string): string { const slug = name .trim() @@ -73,11 +51,7 @@ export function handleFromName(name: string, fallbackAddress: string): string { return slug.length > 0 ? slug : localPartOf(fallbackAddress); } -/** - * De-duplicates a candidate handle against the handles already in use in - * a workbench: "echo" becomes "echo-2", then "echo-3", etc. — the first - * suffix not already taken. - */ +// "echo" becomes "echo-2", then "echo-3" — the first suffix not taken. export function dedupeHandle(handle: string, taken: ReadonlySet): string { if (!taken.has(handle)) return handle; let suffix = 2; @@ -85,12 +59,8 @@ export function dedupeHandle(handle: string, taken: ReadonlySet): string return `${handle}-${suffix}`; } -/** - * Appends a new participant to an existing record list, de-duplicating - * the desired handle against every handle already in the workbench. - * Same-address retries return the existing list by identity so a caller - * can tell "already present" apart from "appended a row". - */ +// Returns the existing list by identity on a same-address retry, so a +// caller can tell "already present" from "appended". export function addParticipant( existing: readonly ParticipantRecord[], address: string, @@ -104,12 +74,8 @@ export function addParticipant( return [...existing, { address, handle }]; } -/** - * Drops a participant from an existing record list by address — the - * inverse of `addParticipant`. Returns the same array reference when - * the address names no participant, so a caller can tell "nothing - * changed" apart from "removed the last matching entry" by identity. - */ +// Returns the same array reference when the address names no participant, +// so a caller can tell "nothing changed" by identity. export function removeParticipant( existing: readonly ParticipantRecord[], address: string, diff --git a/apps/web/src/chat/wire/parts.ts b/apps/web/src/chat/wire/parts.ts index 83d20a519..f0738b8c7 100644 --- a/apps/web/src/chat/wire/parts.ts +++ b/apps/web/src/chat/wire/parts.ts @@ -1,36 +1,18 @@ -// Mirrored from packages/chat/src: apps/web and @/chat -// must not import @corbits/chat, a server-only package. This is the browser- -// facing half of the same wire contract the hub's chat routes still speak; -// once the hub moves onto native mail threads (T5a/T5c) this file becomes -// the one source of truth and packages/chat's copy goes away. +// Mirrored from packages/chat/src (see docs/chat-wire-contract.md). import { type } from "arktype"; -// The wire contract for chat message content. Every message a thread -// carries is a `Part[]`; each part is a structural arktype schema with a -// `kind` discriminant, parsed at the trust boundary rather than cast. - export const TextPart = type({ kind: "'text'", text: "string", - /** Set only on the undelivered-turn notice `postUndeliveredNotice` - * posts in an unreachable agent's own voice — the client's - * one signal that this particular text bubble is a failed turn's - * notice, not an ordinary reply, so it renders the failed-turn strip - * (`PrFailedTurnStrip`) instead of a plain bubble. Absent on every - * other text part. */ + // Marks the undelivered-turn notice so it renders the failed-turn strip, + // not a plain bubble. "turnFailed?": "boolean", - /** Set with `turnFailed` when the cause is a missing/unresolvable - * model (`InferenceResolutionError`) or a model that cannot use tools - * — the failed-turn strip renders named recovery (picker + Settings - * hop) instead of Retry. Absent on every other text part. */ + // Set with `turnFailed` for a missing/unresolvable model or one that + // can't use tools, to render named recovery instead of Retry. "turnFailedReason?": "'model_unavailable' | 'tools_unsupported'", - /** Set only on the cancelled-turn notice `postCancelledNotice` - * (`./workbench-service.ts`) posts in the cancelled agent's own voice - * — distinct from `turnFailed`: a user cancelling a turn is - * not a failure, and the frontend renders it with its own honest copy - * rather than `FailedTurnStrip`'s "didn't reply" framing. Absent on - * every other text part. */ + // Marks a user-cancelled turn — distinct from `turnFailed` since + // cancelling isn't a failure and gets its own copy. "turnCancelled?": "boolean", }); export type TextPart = typeof TextPart.infer; @@ -59,14 +41,9 @@ export const BlockPart = type({ }); export type BlockPart = typeof BlockPart.infer; -// A file rides either as a reference into platform blob storage (`blobId`, -// for content already persisted) or as inline base64 bytes (`data`, for -// content the codec is encoding fresh). Exactly one of those two must be -// present. `artifactId` is an orthogonal, optional link back to a Library -// artifact (see `@corbits/artifacts`) — set when this file is also a -// persisted Library row (e.g. a workflow finalize tool's output), -// independent of whether the bytes themselves also live in chat's own blob -// store. +// Exactly one of `blobId` (already persisted) or `data` (inline base64) +// must be present, unless `artifactId` links this file to a Library row +// instead (see @corbits/artifacts). export const FilePart = type({ kind: "'file'", name: "string", @@ -77,10 +54,8 @@ export const FilePart = type({ }).narrow((part, ctx) => { const hasBlobId = part.blobId !== undefined; const hasData = part.data !== undefined; - // An artifact-backed file needs neither: its bytes live in the Library - // artifact row `artifactId` names, not in chat's own blob store, so - // `blobId`/`data` stay optional (but still mutually exclusive) once - // `artifactId` is set. + // An artifact-backed file's bytes live in the Library row, so blobId/data + // stay optional (still mutually exclusive) once artifactId is set. if (part.artifactId !== undefined) { if (hasBlobId && hasData) { return ctx.reject("`blobId` and `data` cannot both be set on a FilePart"); diff --git a/apps/web/src/chat/wire/stream-events.ts b/apps/web/src/chat/wire/stream-events.ts index dac88c850..90ca8eb54 100644 --- a/apps/web/src/chat/wire/stream-events.ts +++ b/apps/web/src/chat/wire/stream-events.ts @@ -1,24 +1,5 @@ -// Mirrored from packages/chat/src: apps/web and @/chat -// must not import @corbits/chat, a server-only package. This is the browser- -// facing half of the same wire contract the hub's chat routes still speak; -// once the hub moves onto native mail threads (T5a/T5c) this file becomes -// the one source of truth and packages/chat's copy goes away. - -// The wire contract for every event a workbench's `/stream` SSE -// connection carries — arktype schemas so a payload is validated at the -// one boundary that matters (about to leave the process on the wire), -// rather than trusted by convention the way a plain TS interface would -// leave it. `chat-ui`'s subscriber types are meant to mirror these -// `.infer` types exactly: this file is the contract, not a description -// of one. -// -// The organizing rule every event here follows: a subscriber must be -// able to render (or update its own state) from the event alone, with -// no follow-up GET. `chat.message` carries the full rendered row: a -// `GET /workbenches/:id/messages` page item and a freshly published -// `chat.message` are structurally the same shape, minus the fields -// (`reactions`, `pinned`, `clientId`) that are always absent on a -// message nobody has reacted to, pinned, or reconciled yet. +// Mirrored from packages/chat/src (see docs/chat-wire-contract.md, which +// also covers this file's organizing rule). import { type } from "arktype"; import { Part } from "./parts"; @@ -27,16 +8,8 @@ const WorkbenchMessageSender = type({ address: "string", }); -/** - * The full rendered timeline row a `chat.message` event carries — - * everything `postWorkbenchMessage`'s caller already has in hand from the - * insert it just did, so a subscriber never needs to refetch the - * message it was just told about. `ref` names the workbench the row - * lives on; the mail headers ride along only when the row was actually - * mailed (a human send through the mailbox fan-out) — Message-ID always, - * In-Reply-To and References when the row answers a thread — and are - * absent, never invented, for a row nobody mailed. - */ +// Mail headers ride along only when the row was actually mailed; absent, +// never invented, otherwise. export const ChatMessageEventData = type({ id: "string", workbenchId: "string", @@ -64,15 +37,8 @@ export const ChatTypingEventData = type({ }); export type ChatTypingEventData = typeof ChatTypingEventData.infer; -/** - * A presence delta: one principal either just became reachable on this - * stream (`"online"`, fired the moment its SSE connection opens, and - * again on an explicit `POST .../presence` ping) or just stopped being - * reachable (`"offline"`, fired once its *last* open connection for - * this workbench closes — a second open tab never fires it). Never - * persisted and never the answer to a poll: a subscriber folds this - * into the roster it already holds from `chat.presence.snapshot`. - */ +// "offline" fires only once a principal's *last* open connection closes — +// a second open tab never fires it. export const ChatPresenceEventData = type({ principalId: "string", state: "'online' | 'offline'", @@ -80,12 +46,7 @@ export const ChatPresenceEventData = type({ }); export type ChatPresenceEventData = typeof ChatPresenceEventData.infer; -/** - * The one-time roster a freshly opened stream is handed before any - * delta — what lets a subscriber render "who's here" immediately on - * connect without a separate fetch. Sent only to the connecting - * stream itself, never broadcast. - */ +// Sent only to the connecting stream itself, never broadcast. export const ChatPresenceSnapshotEventData = type({ members: type({ principalId: "string", diff --git a/apps/web/src/chat/wire/tools-unsupported.ts b/apps/web/src/chat/wire/tools-unsupported.ts index 6611c5886..586c3b2b5 100644 --- a/apps/web/src/chat/wire/tools-unsupported.ts +++ b/apps/web/src/chat/wire/tools-unsupported.ts @@ -1,26 +1,10 @@ -// Mirrored from packages/chat/src: apps/web and @/chat -// must not import @corbits/chat, a server-only package. This is the browser- -// facing half of the same wire contract the hub's chat routes still speak; -// once the hub moves onto native mail threads (T5a/T5c) this file becomes -// the one source of truth and packages/chat's copy goes away. - -/** - * Named consumer copy for an inference failure whose real cause is that - * the agent's model cannot use tools. Provider dumps, HTTP status, and - * catalog/registry capability strings never belong on the timeline. - */ +// Mirrored from packages/chat/src (see docs/chat-wire-contract.md). export const TOOLS_UNSUPPORTED_CONSUMER_MESSAGE = "This agent's model can't use tools."; -/** - * Conservative match: provider/inference errors about tools / - * function-calling plus unsupported / not supported / a missing required - * capability. An unrelated HTTP 400, an embedding model's "does not - * support generate", an ordinary reply that happens to mention tools, - * or connector prose like "The grep tool is not supported in this - * sandbox" must not match. Singular "tool" next to "not supported" is - * ordinary tool-availability copy, not a model-capability failure. - */ +// Conservative on purpose: must not match an unrelated HTTP 400, an +// embedding model's "does not support generate", or connector prose like +// "The grep tool is not supported in this sandbox". const TOOLS_UNSUPPORTED_RE = /\b(?:tools|tool use)\b[^.]{0,80}\b(?:is |are |use is )?not supported\b|\b(?:is |are )?not supported\b[^.]{0,80}\b(?:tools\b|tool use)\b|\bdoes(?: not|n't) support (?:tools?|tool use|function[\s-]?calling)\b|\bfunction[\s-]?calling\b[^.]{0,80}\b(?:not supported|unsupported|required|missing)\b|\b(?:not supported|unsupported|required|missing)\b[^.]{0,80}\bfunction[\s-]?calling\b/i; diff --git a/docs/chat-wire-contract.md b/docs/chat-wire-contract.md new file mode 100644 index 000000000..9398c9482 --- /dev/null +++ b/docs/chat-wire-contract.md @@ -0,0 +1,28 @@ +# Chat wire contract + +`apps/web/src/chat/wire/*` is the browser-facing half of the wire contract +also implemented server-side in `packages/chat/src`. `apps/web` cannot import +`@corbits/chat` — a server-only package — so this half is hand-mirrored +rather than shared by import. Once the hub moves onto native mail threads, +`packages/chat`'s copy goes away and this becomes the one source of truth. + +Keep both sides in sync by hand until then; a schema or type added to one +side belongs on the other. + +## Internal-id leak guard + +`chat/wire/id-leak-guard.ts` centralizes the check that a person never sees +an internal identifier (a raw `run_…`/`wfd_…`/etc., or `humanizeSlug`'s +Title-Cased reading of one, e.g. "Run 737a058d…"). This recurred repeatedly +as one-off display-time patches (insights, a Myra reply, a chat title) +before being consolidated here, so a new id-generating prefix is a missed +test run rather than a missed grep. Prefix words mirror `@intx/hub-common`'s +`generateId` (`PREFIXES` in `packages/hub-common/src/ids.ts`). + +## Stream events (`chat/wire/stream-events.ts`) + +Organizing rule: a subscriber must be able to render, or update its own +state, from the event alone — never a follow-up GET. `chat.message` carries +the full rendered row (mail headers included, when the row was actually +mailed); `chat.presence` deltas fold into the roster `chat.presence.snapshot` +already gave the connecting stream. From 478c416847910e4525edaeb88b6088b6914b285a Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:39:25 -0700 Subject: [PATCH 02/53] chore(web): chat top-level comments say why, not what (CL-8378) --- apps/web/src/chat/deployable-package.ts | 35 +++------ apps/web/src/chat/inference-failure.ts | 28 +------- apps/web/src/chat/markdown.tsx | 16 +---- apps/web/src/chat/strings.ts | 16 ++--- apps/web/src/chat/threads-api.ts | 96 ++++++------------------- apps/web/src/chat/workbench-tenants.ts | 6 +- docs/chat-mail-threading.md | 24 +++++++ docs/chat-wire-contract.md | 12 ++++ 8 files changed, 78 insertions(+), 155 deletions(-) create mode 100644 docs/chat-mail-threading.md diff --git a/apps/web/src/chat/deployable-package.ts b/apps/web/src/chat/deployable-package.ts index 99073884c..1d1ac08cf 100644 --- a/apps/web/src/chat/deployable-package.ts +++ b/apps/web/src/chat/deployable-package.ts @@ -1,11 +1,5 @@ -// The one contract between an agent that writes a package and the client -// that deploys it: a reply carrying `package.json` plus a -// `definition.json` of {name, systemPrompt, description?, schedule?} — -// either as mail attachments, or (since `@intx/tools-mail`'s `mail_send` -// has no attachments parameter) as two labelled fenced code blocks in the -// message body. The client renders the source tree itself -// (`agent-deploy.ts`), so the agent never has to know the deploy -// pipeline's shape. +// Either mail attachments, or — since `@intx/tools-mail`'s `mail_send` has +// no attachments parameter — two labelled fenced code blocks in the body. import { type } from "arktype"; import { reportError } from "@corbits/error-sink"; @@ -40,12 +34,7 @@ export interface DeployablePackage { export const PACKAGE_MANIFEST_NAME = "package.json"; export const AGENT_DEFINITION_NAME = "definition.json"; -/** - * What a message's attachments (or fenced blocks) resolve to: a parsed - * package, a named rejection when the message attempted a package but - * got a field wrong, or `null` when the message isn't a package attempt - * at all. - */ +// `null` when the message isn't a package attempt at all. export type PackageOutcome = DeployablePackage | { readonly reason: string } | null; export function isPackageRejection( @@ -111,10 +100,7 @@ function parsePackageFiles( }; } -/** The agent package a message's attachments describe: a parsed package, - * a rejection naming what's wrong, or `null` when the attachments aren't - * a package attempt at all (no package.json or no definition.json). - * Untrusted input: parsed, never cast. */ +// Untrusted input: parsed, never cast. export function deployablePackage(attachments: readonly MailAttachment[]): PackageOutcome { const manifest = attachments.find((attachment) => attachment.name === PACKAGE_MANIFEST_NAME); if (manifest === undefined) return null; @@ -211,11 +197,8 @@ function findNamedFencedBlocks(lines: readonly string[]): Map-source`), which renders title-cased with - * hyphens as spaces ("echo-bot" -> "Echo Bot"). */ +// Myra's deploy asset name is not a display name anyone should read; +// every other agent's slug renders title-cased ("echo-bot" -> "Echo Bot"). export function displayAgentName(definitionName: string): string { if (definitionName === MYRA_SOURCE_CONFIG.assetName) return MYRA_SOURCE_CONFIG.displayName; const slug = agentSlugFromSourceAssetName(definitionName); @@ -125,11 +112,8 @@ function workflowAssetsPath(tenantId: string): string { return `/api/tenants/${encodeURIComponent(tenantId)}/assets?kind=workflow&inherited=false`; } -/** Every agent the person has ever had a deployment of, keyed by its - * definition asset id. Deployments list every anchor run ever created for - * an asset (releases included, most recent first), joined here against the - * run listing for each run's address and against the workflow assets for a - * display name. */ +// Deployments list every anchor run ever created (releases included, newest +// first), joined against runs for addresses and assets for a display name. export async function listChatAgents(tenantId: string): Promise { const [deployments, assets, runs] = await Promise.all([ getJson(deploymentsPath(tenantId), DeploymentsSchema), @@ -174,10 +158,7 @@ export async function listChatAgents(tenantId: string): Promise): boolean { if (agent.liveAddress !== null) return false; return agent.latestStatus === undefined || !STARTING_DEPLOYMENT_STATUSES.has(agent.latestStatus); @@ -474,12 +455,8 @@ export type ChatThread = { readonly messages: readonly ChatMessage[]; }; -/** A chat is always titled by its agent's display name — never by mail - * metadata, which can be a run address or other addressing detail nobody - * should have to read. `agentName` is `undefined` only when the agent - * couldn't be resolved at all, in which case the title falls back to the - * person's own opening turn (never an agent reply, so a chat never titles - * itself off what the agent said). */ +// Falls back to the person's own opening turn (never an agent reply) only +// when the agent couldn't be resolved at all. export function chatTitle(turns: readonly MailTurn[], agentName: string | undefined): string { if (agentName !== undefined) return agentName; const first = turns.find((turn) => turn.author === "me"); @@ -525,11 +502,8 @@ export async function listChats(tenantId: string): Promise Date.parse(b.lastActivityAt) - Date.parse(a.lastActivityAt)); } -// --------------------------------------------------------------------- -// Reply-ready: a chat is unread until it has been opened at its newest -// message. Tracked per-viewer in localStorage — a convenience, not data of -// record, so a missing/blocked store just falls back to "ready". -// --------------------------------------------------------------------- +// Reply-ready: tracked per-viewer in localStorage, a convenience not data +// of record, so a missing/blocked store just falls back to "ready". const CHAT_SEEN_KEY_PREFIX = "workbench:chat-seen:"; @@ -598,12 +572,7 @@ export function subscribeToInbox(tenantId: string, onChange: () => void): () => }; } -// --------------------------------------------------------------------- -// Workbenches: a workbench is a child tenant, and its workbench conversation is that -// tenant's mailbox. Reads are the same stock mailbox routes as a chat, -// scoped to the child tenant id; participants are the child tenant's -// principals. -// --------------------------------------------------------------------- +// Workbenches: see docs/chat-mail-threading.md. export type WorkbenchParticipant = { readonly id: string; @@ -629,14 +598,8 @@ const PrincipalPage = type({ nextCursor: "string | null", }); -/** Everyone in the workbench: the child tenant's principals plus its live - * deployments' run addresses. A deployment's workflow principal only - * appears after its first run, so the run listing is what makes an agent - * addressable from the moment it is deployed into the workbench. `tenantDomain` - * is the workbench tenant's own domain (already fetched by the caller) — a - * person's routable mailbox address is `@`, exactly - * what the hub builds and the mailbox delivers to; their email and refId - * alone are never routable. */ +// A deployment's workflow principal only appears after its first run, so +// the run listing is what makes an agent addressable from deploy time. export async function listWorkbenchParticipants( tenantId: string, tenantDomain: string, @@ -687,10 +650,8 @@ function authorName(address: string): string { return local.length > 0 ? local : address; } -/** Case-insensitive whole-address match: the mailbox lowercases local parts - * on the wire, so a sent message's header `from` (mixed case) and envelope - * `from` (lowercase) both name the same participant. Never lowercases - * stored data — comparison only. */ +// Case-insensitive: the mailbox lowercases local parts on the wire, so a +// header `from` (mixed case) and envelope `from` (lowercase) still match. export function sameAddress(a: string, b: string): boolean { return a.toLowerCase() === b.toLowerCase(); } @@ -708,10 +669,7 @@ export function resolveParticipantName( ); } -/** A turn's avatar name: the matching participant's real name, including - * the person's own — never the "You" transcript label, which stays for the - * row's own text elsewhere. Falls back to `resolveParticipantName` when no - * participant matches the turn's address. */ +// Never the "You" transcript label, which stays for the row's own text. export function resolveAvatarName( message: Pick, participants: readonly WorkbenchParticipant[], @@ -755,10 +713,8 @@ async function readWorkbenchFolder( })); } -/** The workbench timeline: every turn oldest first, flat — no turn is ever - * dropped from the main list. A turn whose in-reply-to names a known - * message keeps that as `parentMessageId`, metadata for the sub-thread - * panel to walk the ancestor chain of whichever turn the person opened. */ +// Flat, oldest first — no turn is ever dropped from the main list. +// `parentMessageId` is metadata for the sub-thread panel only. export async function readWorkbench(tenantId: string): Promise { const [inbox, sent] = await Promise.all([ readWorkbenchFolder(tenantId, "INBOX"), @@ -799,14 +755,8 @@ export function ancestorChain( return chain; } -/** The one send seam for a workbench: a single mailbox send addressed to every - * agent in it. The hub triggers each addressed run and keeps the Sent - * copy, so the person's own turn comes back out of the mailbox like any - * other. The body carries a trailing roster of every agent's name and - * run address plus the person's own (the same rows the Participants panel - * reads), so an agent can hand a task to another agent in the workbench — the - * hub only delivers to a run address, which only the client otherwise - * knows — and can copy the person on that handoff so they can follow it. */ +// The one send seam for a workbench (see docs/chat-mail-threading.md for +// the roster mechanism). export async function sendToWorkbench(input: { readonly workbenchTenantId: string; readonly participants: readonly WorkbenchParticipant[]; diff --git a/apps/web/src/chat/workbench-tenants.ts b/apps/web/src/chat/workbench-tenants.ts index a2f17d0a9..645bb69f1 100644 --- a/apps/web/src/chat/workbench-tenants.ts +++ b/apps/web/src/chat/workbench-tenants.ts @@ -1,7 +1,5 @@ -// A workbench is a child tenant, so listing workbenches is listing the -// bench's children over the stock tenant routes. This is the whole -// workbench-listing surface the rest of the app reads — the workbench itself -// lives in the child tenant's mailbox (`threads-api.ts`). +// A workbench is a child tenant, so listing workbenches lists children +// over the stock tenant routes. import { createFetchStockHub, findOwnedTenants } from "../needs-converge"; import { listChats } from "./threads-api"; diff --git a/docs/chat-mail-threading.md b/docs/chat-mail-threading.md new file mode 100644 index 000000000..33734a7c0 --- /dev/null +++ b/docs/chat-mail-threading.md @@ -0,0 +1,24 @@ +# Chat and workbench mail threading + +`apps/web/src/chat/threads-api.ts` is the only seam between the chat UI and +mail: a chat has exactly one agent, and both sides are durable mail — the +person sends from their own mailbox (keeping a Sent copy), and an agent's +reply lands in the same mailbox's INBOX. No chat-specific hub route exists. + +A chat is keyed by its agent's definition asset id, not a run id: every hub +restart releases the old run and redeploys under a new one, so keying on a +run id would 409 the moment it turns terminal. An agent's address set spans +every run it has ever had, which keeps history intact across a redeploy; +sends resolve the current live run's address at send time. + +A workbench is a child tenant, and its conversation is that tenant's own +mailbox — reads are the same stock mailbox routes as a chat, scoped to the +child tenant id. + +## Workbench send roster + +`sendToWorkbench` appends a trailing roster of every participant's name and +address to the message body (the same rows the Participants panel reads). +The hub only delivers to a run address, which only the client otherwise +knows, so this is what lets an agent hand a task to another agent in the +workbench, or copy a person on the handoff so they can follow it. diff --git a/docs/chat-wire-contract.md b/docs/chat-wire-contract.md index 9398c9482..c82aa2a80 100644 --- a/docs/chat-wire-contract.md +++ b/docs/chat-wire-contract.md @@ -26,3 +26,15 @@ state, from the event alone — never a follow-up GET. `chat.message` carries the full rendered row (mail headers included, when the row was actually mailed); `chat.presence` deltas fold into the roster `chat.presence.snapshot` already gave the connecting stream. + +## Classified inference failures (`chat/inference-failure.ts`) + +By the time a reply reaches the chat timeline it is a plain text part with +no metadata — the failure's category is structured further upstream +(`packages/chat`'s orchestrator reads it directly), but nothing carries it +down to this render layer. So this file matches reply prose against the +exact preambles `@intx/inference`'s `formatInferenceError` writes for +`credential_failure` and `quota_exhausted`, anchored at the start of the +string (never a substring, so a reply that quotes one mid-sentence can't +false-positive). A test asserts the preambles stay byte-for-byte identical +to the vendored source they're copied from. From 2ba0cc739e6c3f4dd3e4defda048a3ed67ee4a3e Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:41:30 -0700 Subject: [PATCH 03/53] chore(web): chat turn/tool activity comments say why, not what (CL-8378) --- apps/web/src/chat/tool-activity.ts | 81 ++++++--------------------- apps/web/src/chat/turn-activity.tsx | 87 ++++++----------------------- 2 files changed, 35 insertions(+), 133 deletions(-) diff --git a/apps/web/src/chat/tool-activity.ts b/apps/web/src/chat/tool-activity.ts index fbaf948e5..c71ac23a9 100644 --- a/apps/web/src/chat/tool-activity.ts +++ b/apps/web/src/chat/tool-activity.ts @@ -1,24 +1,7 @@ -// Turning a turn's tool calls into something a person can read. -// -// The wire carries a tool call as an identifier plus an argument bag plus -// whatever the tool handed back (`ToolTracePart` in `@/chat/wire/parts`, -// assembled by the chat orchestrator from `inferenceDoneBlocks` / -// `toolDoneResult`). None of those three is fit to show anyone: the -// identifier is a symbol (`slack__post_message`, or an Interchange qualified -// id `@scope/package/export:tool`), the arguments are JSON, and the result -// is usually a content-block array. This module is the one place that -// translates all three into plain sentences — "Posted a message in Slack", -// "Searched memory for "outbound"" — so no surface downstream ever has to -// reach for `JSON.stringify` to say what happened. The phrase uses the -// segment after the last colon (the end tool name); a leftover path is a -// provider only when it maps onto a known brand, never `@corbits` or a -// package stem like `memory-tools`. -// -// Both the live strip (`turn-activity.tsx`, mid-turn) and the persisted -// transcript (`timeline.tsx`) render through here, which is why a phrase -// comes in two tenses: the same call reads "Searching the web for -// "pricing"" while it runs and "Searched the web for "pricing"" once it -// settles. +// The one place a tool call's raw identifier/args/result gets translated +// into a plain sentence, so no downstream surface reaches for +// `JSON.stringify`. Two tenses (running vs. settled) since both the live +// strip and the persisted transcript render through here. import type { Part, ToolTracePart } from "./wire/parts"; @@ -244,21 +227,9 @@ function splitQualifiedName(name: string): { return { leftover: undefined, toolName: name }; } -/** - * Splits a tool identifier into the provider it belongs to and the words - * describing what it does. - * - * `mcp_read`/`mcp_call` are the generic MCP dispatch tools - * (`tools/mcp/src/tool.ts`): every downstream call arrives under - * one of those two names, with the tool it actually invoked sitting in its - * `{server, tool}` arguments — so those are read first, or a whole - * conversation's worth of calls would all read alike. - * - * Interchange qualified ids (`@scope/package/export:tool`) take the - * segment after the last `:`. A leftover path is a provider only when a - * segment or `-tools` stem is a known brand; `memory`, `ad`, and - * `ask-user` never are. - */ +// `mcp_read`/`mcp_call` are generic MCP dispatch tools: the tool actually +// invoked sits in `{server, tool}` args, so those are read first — or a +// whole conversation's calls would all read alike. export function resolveToolIdentity(name: string, input: unknown): ToolIdentity { const args = asRecord(input); if (name === "mcp_read" || name === "mcp_call") { @@ -305,11 +276,8 @@ function hostname(url: string): string | undefined { return host.replace(/^www\./, ""); } -/** - * The one argument worth putting in the sentence, as the clause that - * carries it. Precedence is explicit and ordered — a search tool given - * both a query and a path is describing the query. - */ +// Precedence is explicit and ordered — a search tool given both a query +// and a path is describing the query. function argumentClause(input: unknown): string | undefined { const args = asRecord(input); if (args === undefined) return undefined; @@ -429,11 +397,8 @@ function domainHead(words: readonly string[], verb: string, tense: Tense): strin return undefined; } -/** - * What this tool call did, as a sentence — in the tense its status calls - * for. Never contains the tool's identifier, its argument JSON, or an - * internal id. - */ +// Never contains the tool's identifier, its argument JSON, or an internal +// id. export function describeToolCall(name: string, input: unknown, tense: Tense): string { const identity = resolveToolIdentity(name, input); const clause = argumentClause(input); @@ -481,12 +446,8 @@ function decodeOutput(output: unknown): unknown { } } -/** - * Pulls readable prose out of whatever a tool handed back. Tool results - * arrive as a string, as MCP content blocks, or as an arbitrary object; - * only the first two carry anything worth showing a person, and this - * returns undefined rather than stringifying the third. - */ +// Returns undefined for an arbitrary-object result rather than +// stringifying it. export function plainTextOfOutput(output: unknown): string | undefined { const decoded = decodeOutput(output); if (typeof decoded === "string") { @@ -536,11 +497,8 @@ function looksLikeContentBlocks(output: unknown): boolean { }); } -/** - * The detail a row opens onto: plain text, always. A failure keeps its - * first line — the reason, said plainly — and never goes silent: a tool - * that fails without saying why still says that much. - */ +// A failure keeps its first line and never goes silent, even when the +// tool didn't say why. export function summarizeToolOutput( status: ToolActivityStatus, output: unknown, @@ -614,13 +572,8 @@ export type TimelinePartGroup = readonly key: string; }; -/** - * Splits a message's parts into render groups, clustering every run of - * consecutive tool calls together. The cluster renders as a stack of - * chips, one per call — never folded into a summary line — so this is - * purely about keeping tool calls next to each other in the flow, not - * about hiding them. - */ +// Clusters consecutive tool calls into a stack of chips, never folded +// into a summary line. export function groupTimelineParts( parts: readonly Part[], keyPrefix: string, diff --git a/apps/web/src/chat/turn-activity.tsx b/apps/web/src/chat/turn-activity.tsx index 1c380ecfa..3f0c1df21 100644 --- a/apps/web/src/chat/turn-activity.tsx +++ b/apps/web/src/chat/turn-activity.tsx @@ -1,17 +1,5 @@ -// The live "what is the agent doing right now" strip for an in-flight -// turn: `chat.agent` events already carry the vendored Interchange -// `InferenceEvent` union verbatim (see `streaming-reply.ts`'s header for -// the wire path), but that module only ever reads `inference.text.delta` -// and the turn-boundary events — every tool call, thinking delta, and -// retry is dropped on the floor. This module is the trust boundary that -// narrows those other event shapes (`inference.tool_call.*`, `tool.*`, -// `inference.thinking.delta`, `inference.retry` — see -// `@intx/types/src/runtime.ts`'s `InferenceEvent`) and the pure -// state machine that turns them into one turn's activity list, plus the -// hook and presentational strip that render it. v1 is live-only: the -// strip disappears the moment the turn ends (`nextTurnActivityState` -// returns `null`), same as `streaming-reply.ts`'s reply text — no -// persisted trace yet. +// v1 is live-only: the strip disappears the moment the turn ends, same as +// `streaming-reply.ts`'s reply text — no persisted trace yet. import { useEffect, useState } from "react"; @@ -110,15 +98,9 @@ function parseToolDone(data: unknown): { callId: string; isError: boolean } | nu return { callId, isError: result.isError === true }; } -/** - * `inference.thinking.delta`'s `data.partial.thinking` is cumulative, same - * as `partial.text` (see `PartialMessage` in - * `@intx/types/src/runtime.ts`) — so the ordinary case replaces the - * char count outright. `thinking` is typed optional on `PartialMessage`; - * if a future adapter omits it on the delta event, falling back to the - * per-delta `token`'s length keeps the counter moving (as an increment, - * not a replacement, since `token` is only ever the new fragment). - */ +// `partial.thinking` is cumulative, so the ordinary case replaces the char +// count outright; a future adapter omitting it falls back to `token`'s +// length as an increment instead. function parseThinkingDelta( data: unknown, ): { kind: "cumulative" | "increment"; charCount: number } | null { @@ -143,11 +125,8 @@ function parseRetry(data: unknown): { attempt: number } | null { return typeof attempt === "number" ? { attempt } : null; } -/** Records (or refines) one tool call, opening a new running row if - * `callId` hasn't been seen yet this turn. Never touches `startedAtMs`, - * `status`, or `doneAtMs` on an existing entry — `inference.tool_call.end` - * and `tool.start` only ever add the arguments a row already opened by an - * earlier event was missing. */ +// Never touches `startedAtMs`/`status`/`doneAtMs` on an existing entry — +// a later event only ever fills in arguments an earlier one was missing. function upsertToolCall( toolCalls: readonly ToolCallActivity[], call: { @@ -195,18 +174,9 @@ function settleToolCall( ); } -/** - * The current turn's whole activity state machine, pure: `reactor.start` - * clears it (a fresh turn owns none of the previous turn's chips), - * `reactor.done`/`reactor.error`/`inference.done`/`inference.error` - * finalize it to `null` (the turn is over), and every tool-call, - * thinking, and retry event in between updates the open state — opening - * one implicitly if none exists yet, so activity that arrives before a - * `reactor.start` (or in a test driving events directly) is never - * silently dropped. Kept separate from the `useState` that holds it in - * `useTurnActivity`, matching `nextStreamingReplyState`'s split in - * `streaming-reply.ts`. - */ +// Opens the activity implicitly if none exists yet, so an event arriving +// before `reactor.start` is never silently dropped. Kept pure and separate +// from the `useState` that holds it, matching `nextStreamingReplyState`. export function nextTurnActivityState( current: TurnActivityState, event: { readonly eventType: string; readonly data: unknown }, @@ -309,25 +279,13 @@ export function nextTurnActivityState( return { ...base, thinking }; } -/** How long an open turn may sit with no consumed event before the strip - * clears itself — the backstop for a turn whose `reactor.done`/ - * `inference.done` (or `.error`) never arrives (agent down, SSE dropped - * mid-reconnect), mirroring `streaming-reply.ts`'s - * `PENDING_REPLY_CLEAR_MS`. */ +// Backstop for a turn whose done/error event never arrives (agent down, +// SSE dropped mid-reconnect), mirroring `streaming-reply.ts`. const TURN_ACTIVITY_STALE_MS = 120_000; -/** - * Owns the turn-activity state end to end, mirroring - * `useStreamingReply`'s shape exactly: feed it every stream event and it - * tracks the active turn's tool calls, thinking, and retries, clearing - * the moment the turn ends. `workbenchId` resets it immediately on a - * workbench switch — activity from the workbench just left belongs to that - * workbench, not the new one. `staleMs` (default `TURN_ACTIVITY_STALE_MS`) - * is a test seam, mirroring `useTypingIndicator`'s own configurable - * timeout. `clock` (default `REAL_CLOCK`) is the same kind of seam for the - * wall-clock reads and the backstop timer itself — a test drives both - * synchronously with a fake clock instead of sleeping on the real one. - */ +// `workbenchId` resets state immediately on a workbench switch — activity +// from the one just left doesn't belong to the new one. `staleMs`/`clock` +// are test seams so a test can drive time synchronously. export function useTurnActivity( workbenchId: string | null, staleMs: number = TURN_ACTIVITY_STALE_MS, @@ -370,12 +328,8 @@ function elapsedSeconds(startedAtMs: number, endMs: number): number { return Math.max(0, Math.round((endMs - startedAtMs) / 1000)); } -/** - * This turn's tool calls as the rows the conversation renders everywhere - * else — same sentences, same statuses. A call still running says so in - * the present tense and carries how long it has been going; a settled one - * drops the timer, since a finished step's duration is noise. - */ +// A running call carries how long it's been going; a settled one drops +// the timer, since a finished step's duration is noise. export function toolActivityRows( activity: NonNullable, nowMs: number, @@ -397,12 +351,7 @@ export function toolActivityRows( }); } -/** - * The live strip: one row per tool call this turn, a "Thinking…" row while - * thinking deltas are flowing, and a retry note if the model's own request - * needed one. Renders nothing once the turn ends — the persisted message - * takes over from there, in the same row idiom. - */ +// Renders nothing once the turn ends — the persisted message takes over. export function TurnActivityStrip({ activity }: { readonly activity: TurnActivityState }) { const hasRunningToolCall = activity?.toolCalls.some((call) => call.status === "running") ?? false; const [, forceTick] = useState(0); From 9746b2d66f5a212cc108c5d3211121b8454d7724 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:43:53 -0700 Subject: [PATCH 04/53] chore(web): more chat comments say why, not what (CL-8378) --- apps/web/src/chat/agent-part-adapter.ts | 10 ++---- apps/web/src/chat/avatar.tsx | 13 ++----- apps/web/src/chat/clock.ts | 7 ++-- apps/web/src/chat/composer.tsx | 7 ++-- apps/web/src/chat/failed-turn-models.ts | 12 ++----- apps/web/src/chat/loading-state.tsx | 36 +++++--------------- apps/web/src/chat/no-usable-model-banner.tsx | 9 ++--- apps/web/src/chat/relative-time.ts | 6 ++-- apps/web/src/chat/resume-failed-banner.tsx | 6 ++-- apps/web/src/chat/tool-activity-view.tsx | 19 +++-------- apps/web/src/chat/workbench-roster.ts | 14 ++------ docs/chat-mail-threading.md | 4 ++- 12 files changed, 35 insertions(+), 108 deletions(-) diff --git a/apps/web/src/chat/agent-part-adapter.ts b/apps/web/src/chat/agent-part-adapter.ts index d29757eab..c61edb9c9 100644 --- a/apps/web/src/chat/agent-part-adapter.ts +++ b/apps/web/src/chat/agent-part-adapter.ts @@ -1,11 +1,5 @@ -// Adapting the chat wire's parts (`./wire/parts`) to the shapes `@corbits/react-ui` -// renders. -// -// Only reasoning crosses this boundary now. Tool calls used to as well, -// but the conversation renders them through `tool-activity.tsx` instead: -// react-ui's `ToolBlock` shows one call at a time with its arguments and -// its result as `JSON.stringify` output, and the chat surface groups a -// turn's calls into rounds and never shows a reader JSON at all. +// Only reasoning crosses this boundary now — tool calls render through +// `tool-activity.tsx` instead, since react-ui's `ToolBlock` shows raw JSON. import type { PartReasoning } from "@corbits/react-ui"; import type { ReasoningPart } from "./wire/parts"; diff --git a/apps/web/src/chat/avatar.tsx b/apps/web/src/chat/avatar.tsx index e7beb4af5..8af3de600 100644 --- a/apps/web/src/chat/avatar.tsx +++ b/apps/web/src/chat/avatar.tsx @@ -2,12 +2,7 @@ import type { CSSProperties } from "react"; import { agentInitials } from "@/chat/threads-api"; -/* Avatar identity pastels. The palette is token references, not - hex: --avatar-1 through --avatar-4 are defined once in this package's - stylesheet :root (the proposed upstream contract for @corbits/react-ui's - theme) and consumed here by reference, so no product code hardcodes a - color value. Numbered like react-ui's --chart-1..5 series — slot order is - the deterministic resolution order, not a ranking. */ +// Token references, not hex, so no product code hardcodes a color value. export const AVATAR_COLORS = ["--avatar-1", "--avatar-2", "--avatar-3", "--avatar-4"] as const; export type AvatarColor = (typeof AVATAR_COLORS)[number]; @@ -128,10 +123,8 @@ export function CorbitAvatar({ ); } -/** The one avatar element for a `.shell-ch-avatar` slot: a Corbit avatar for - * an agent, a pastel initials chip (hashed off the person's own principal - * id, never their name) for a person — visually distinct per DESIGN.md's - * avatar identity rule. */ +// Person chip color hashes off principal id, never name — DESIGN.md's +// avatar identity rule. export function IdentityAvatar({ kind, name, diff --git a/apps/web/src/chat/clock.ts b/apps/web/src/chat/clock.ts index 13b879a12..f076335ac 100644 --- a/apps/web/src/chat/clock.ts +++ b/apps/web/src/chat/clock.ts @@ -1,8 +1,5 @@ -// The one seam `streaming-reply.ts` and `turn-activity.tsx` read wall-clock -// time and arm backstop timers through, so a test can swap in a fake that -// advances synchronously instead of sleeping on the real clock — real-timer -// waits on these hooks' 30ms/60ms backstops are exactly what made them flake -// under CI's sharded, CPU-contended `bun run test`. +// A test seam so a fake clock can advance synchronously instead of +// sleeping — real-timer waits on these backstops flaked under CI. export type Clock = { readonly now: () => number; readonly setTimeout: (callback: () => void, ms: number) => unknown; diff --git a/apps/web/src/chat/composer.tsx b/apps/web/src/chat/composer.tsx index c3bed8dec..720843fff 100644 --- a/apps/web/src/chat/composer.tsx +++ b/apps/web/src/chat/composer.tsx @@ -1,8 +1,5 @@ -// The one composer surface for a chat thread and a workbench workbench alike: -// Enter sends, Shift+Enter inserts a newline, and a right-hand action rail -// carries Send. There is no Stop action — neither `threads-api.ts` nor the -// hub API expose a way to cancel a running turn, so cancellation is not -// wired up here; add it to this rail once that capability exists upstream. +// No Stop action: neither `threads-api.ts` nor the hub API expose a way to +// cancel a running turn yet. import { ArrowUp, CircleNotch } from "@/lib/icons"; import { useState } from "react"; diff --git a/apps/web/src/chat/failed-turn-models.ts b/apps/web/src/chat/failed-turn-models.ts index 7b1fcc903..79b239e9f 100644 --- a/apps/web/src/chat/failed-turn-models.ts +++ b/apps/web/src/chat/failed-turn-models.ts @@ -44,11 +44,7 @@ function toChoices(models: readonly ModelInfo[], limit: number): readonly Failed }); } -/** - * Two-to-four tenant-available chat models for the failed-turn strip - * picker — the same connected, chat-capable filter Settings' agent - * model select uses, capped so the strip stays a quiet inline row. - */ +// Capped so the strip stays a quiet inline row. export function failedTurnModelChoices( models: readonly ModelInfo[], limit = FAILED_TURN_MODEL_PICKER_LIMIT, @@ -56,11 +52,7 @@ export function failedTurnModelChoices( return toChoices(chatCapableModels(models), limit); } -/** - * Same picker as {@link failedTurnModelChoices}, restricted to offerings - * that advertise function-calling — the recovery set when the failure - * was that the current model cannot use tools. - */ +// The recovery set when the failure was that the model can't use tools. export function failedTurnToolCapableModelChoices( models: readonly ModelInfo[], limit = FAILED_TURN_MODEL_PICKER_LIMIT, diff --git a/apps/web/src/chat/loading-state.tsx b/apps/web/src/chat/loading-state.tsx index 3fed134a4..86c4f1774 100644 --- a/apps/web/src/chat/loading-state.tsx +++ b/apps/web/src/chat/loading-state.tsx @@ -1,13 +1,5 @@ -// The one warm loader every page/workbench-level wait in this app renders — -// a bare skeleton/spinner/grey -// slab is never the right answer for "we don't know how long this takes": -// one honest headline plus a small rotating tip reads as useful rather than -// stalled, and it's the same shape everywhere so a reader learns it once. -// -// `delayMs` (default 200) holds the loader itself back: a wait that -// resolves before the delay elapses never gets an intermediate frame at -// all, which is what keeps a fast round-trip from flashing chrome the -// reader has no time to read. +// `delayMs` (default 200) holds the loader back so a fast round-trip +// never flashes chrome the reader has no time to read. import { CorbitsMark } from "@corbits/react-ui"; import { useEffect, useState } from "react"; @@ -17,11 +9,8 @@ import { CHAT_STRINGS } from "./strings"; const WORKBENCH_LOADING_TIP_INTERVAL_MS = 4000; const DEFAULT_LOADING_DELAY_MS = 200; -/** A small, honest product tip under the loading headline — rotates on a - * timer regardless of motion preference; the fade between tips is the - * only thing `prefers-reduced-motion` turns off (the CSS keyframe is - * scoped to `no-preference`, so a reduced-motion reader still sees each - * tip in turn, just without the crossfade). */ +// Rotates regardless of motion preference; only the crossfade is scoped +// to `no-preference` and turns off under `prefers-reduced-motion`. function WorkbenchLoadingTip() { const tips = CHAT_STRINGS.workbenchLoadingTips; const [index, setIndex] = useState(0); @@ -40,13 +29,7 @@ function WorkbenchLoadingTip() { ); } -/** - * The shared page/workbench-level loading treatment: one honest headline (never - * an internal stage name — "Starting the runtime…" tells the reader - * nothing they can act on) plus a rotating tip. Delays its own mount by - * `delayMs` so a wait that resolves quickly never flashes an intermediate - * frame — see this file's doc. - */ +// Headline is always one honest sentence, never an internal stage name. export function WorkbenchLoadingState({ delayMs = DEFAULT_LOADING_DELAY_MS, title = CHAT_STRINGS.workbenchLoadingTitle, @@ -62,12 +45,9 @@ export function WorkbenchLoadingState({ const [visible, setVisible] = useState(delayMs <= 0); useEffect(() => { - // A surface that swaps its own `delayMs` — a route that starts out - // "still reading" (delayed) and becomes "known to be waiting" - // (immediate) — reconciles onto this same element rather than - // remounting it, so dropping to 0 has to show the loader outright. - // Returning early here instead left the loader hidden for good and - // rendered the wait as a blank page. + // A route swapping `delayMs` to 0 reconciles onto this same element; + // returning early here instead left the loader hidden and the wait + // rendered as a blank page. if (delayMs <= 0) { setVisible(true); return; diff --git a/apps/web/src/chat/no-usable-model-banner.tsx b/apps/web/src/chat/no-usable-model-banner.tsx index 6c8334531..2ac3f0048 100644 --- a/apps/web/src/chat/no-usable-model-banner.tsx +++ b/apps/web/src/chat/no-usable-model-banner.tsx @@ -1,10 +1,5 @@ -// The pre-send half of the fix. A tenant whose one seeded -// `model_provider` row carries no credential (the shape seeding always -// leaves behind) can't run inference — the composer stays live (a person -// may still want to leave a note), but this banner says so before they -// invest a long message in a reply that was never coming, and leads -// straight into the connect flow that already works -// (`@/settings`'s `ConnectionsSection`). +// Says so before a person invests a long message in a reply that was +// never coming; the composer stays live since they may still want a note. import { Button } from "@corbits/react-ui"; import { WarningCircle } from "@/lib/icons"; import { CHAT_STRINGS } from "./strings"; diff --git a/apps/web/src/chat/relative-time.ts b/apps/web/src/chat/relative-time.ts index d0096efdd..cee1257ba 100644 --- a/apps/web/src/chat/relative-time.ts +++ b/apps/web/src/chat/relative-time.ts @@ -1,7 +1,5 @@ -/** How long ago a timestamp was, in the compact form the chat surfaces - * use for activity: "just now", "12m ago", "3h ago", "5d ago". An - * absent or unparseable timestamp renders as nothing rather than a - * placeholder date. */ +// An absent or unparseable timestamp renders as nothing, never a +// placeholder date. export function formatRelativeActivity(iso: string | null): string { if (iso === null) return ""; const date = new Date(iso); diff --git a/apps/web/src/chat/resume-failed-banner.tsx b/apps/web/src/chat/resume-failed-banner.tsx index 017b15aa5..e699b275b 100644 --- a/apps/web/src/chat/resume-failed-banner.tsx +++ b/apps/web/src/chat/resume-failed-banner.tsx @@ -1,7 +1,5 @@ -// Mid-turn reopen whose catch-up fetch (`fetchRunningTurn`) fails -// must not leave the workbench looking idle. This soft banner names the gap, -// quotes a `reportError` ref id, and offers Retry — never a silent -// `.catch(() => undefined)`. +// A failed catch-up fetch must not leave the workbench looking idle — +// never a silent `.catch(() => undefined)`. import { Button } from "@corbits/react-ui"; import { WarningCircle } from "@/lib/icons"; import { CHAT_STRINGS } from "./strings"; diff --git a/apps/web/src/chat/tool-activity-view.tsx b/apps/web/src/chat/tool-activity-view.tsx index f5fd94e87..84aea0f55 100644 --- a/apps/web/src/chat/tool-activity-view.tsx +++ b/apps/web/src/chat/tool-activity-view.tsx @@ -1,12 +1,5 @@ -// How a turn's tool calls sit in the conversation. -// -// One presentation serves both the live strip (`turn-activity.tsx`) and the -// persisted transcript (`timeline.tsx`), so a call that reads one way while -// it runs doesn't restyle itself the moment the turn ends. Chips, not -// collapsibles: a glyph, one sentence, a status icon, and — only when there -// is something to show — a disclosure onto plain-text detail. Calls stack -// one per call; nothing here ever folds several into a count. The sentences -// come from `tool-activity.ts`; nothing here formats a tool's own data. +// One presentation serves both the live strip and the persisted +// transcript, so a call doesn't restyle itself when the turn ends. import { BookBookmark, @@ -158,12 +151,8 @@ function ToolActivityLine({ ); } -/** - * A run of consecutive tool calls, stacked one chip per call — never - * folded into a summary line (§12.3: chips are not collapsibles). Each - * chip keeps its own disclosure onto its detail; there is no group-level - * trigger and no count of how many calls happened. - */ +// Never folded into a summary line (DESIGN.md §12.3: chips, not +// collapsibles) — no group-level trigger, no count. export function ToolActivityGroup({ rows }: { readonly rows: readonly ToolActivityRow[] }) { if (rows.length === 0) return null; return ( diff --git a/apps/web/src/chat/workbench-roster.ts b/apps/web/src/chat/workbench-roster.ts index fab2cc386..2f18d3640 100644 --- a/apps/web/src/chat/workbench-roster.ts +++ b/apps/web/src/chat/workbench-roster.ts @@ -1,14 +1,6 @@ -// A workbench mail send has no way to hand off between agents: the hub only -// delivers to run addresses, which only the client knows (a workbench's -// Participants panel already reads them off the deployment/run listing). -// Appending them as a trailing block on every workbench send gives every agent -// in the workbench everyone else's address, so one agent can mail another -// directly. Agent-to-agent mail goes run to run and never lands in the -// person's mailbox on its own, so the block also carries the person's own -// address and a line telling agents to copy it on any handoff — the mail -// tools have no `cc` field, so that means naming it as another `to` -// recipient. `stripRoster` is the inverse, used only to keep the block out -// of what a person sees echoed back as their own sent message. +// See docs/chat-mail-threading.md for the roster mechanism. `stripRoster` +// is the inverse, used to keep the block out of a person's echoed-back sent +// message. const ROSTER_HEADING = "Participants:"; diff --git a/docs/chat-mail-threading.md b/docs/chat-mail-threading.md index 33734a7c0..2d650e34d 100644 --- a/docs/chat-mail-threading.md +++ b/docs/chat-mail-threading.md @@ -21,4 +21,6 @@ child tenant id. address to the message body (the same rows the Participants panel reads). The hub only delivers to a run address, which only the client otherwise knows, so this is what lets an agent hand a task to another agent in the -workbench, or copy a person on the handoff so they can follow it. +workbench. The mail tools have no `cc` field, so the roster also tells +agents to copy the person on a handoff by naming them as another `to` +recipient. From f0c5fe4ea551b1599ac458b3d9e928a57289bea8 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:46:10 -0700 Subject: [PATCH 05/53] chore(web): chat block comments say why, not what (CL-8378) --- apps/web/src/chat/blocks/approval-actions.ts | 68 +++---------------- apps/web/src/chat/blocks/approve-block.tsx | 15 +--- .../chat/blocks/approve-card-state.test.ts | 5 +- .../web/src/chat/blocks/approve-card-state.ts | 35 +++------- .../chat/blocks/connect-service-actions.ts | 18 ++--- .../connect-service-block-container.tsx | 11 +-- .../src/chat/blocks/connect-service-block.tsx | 10 +-- apps/web/src/chat/blocks/registry.tsx | 6 +- docs/chat-wire-contract.md | 19 ++++++ 9 files changed, 54 insertions(+), 133 deletions(-) diff --git a/apps/web/src/chat/blocks/approval-actions.ts b/apps/web/src/chat/blocks/approval-actions.ts index 387a00e54..c501bb3c8 100644 --- a/apps/web/src/chat/blocks/approval-actions.ts +++ b/apps/web/src/chat/blocks/approval-actions.ts @@ -1,38 +1,16 @@ -// The in-chat approve card's one seam to the platform approval it -// references. `@/chat` owns no session and no query cache, so it -// never fetches or mutates approvals itself -- the host (the same app that -// already resolves `tenantId` for `ChatWorkspace`) supplies this port, -// mirroring how `onOpenArtifact`/`onOpenProfile` thread host callbacks -// through the timeline. The host's implementation is expected to call the -// same Interchange-native approve/reject routes Inbox uses and to invalidate -// the same query keys, so the card, Inbox, and the Activity band stay one -// source of truth. +// See docs/chat-wire-contract.md for why this is a host-supplied port. -/** The platform's own status vocabulary for an approval (`ApprovalResponse.status` - * in `@intx/types/src/approvals.ts`) -- never invented here. */ +// Never invented here — mirrors `ApprovalResponse.status`. export type ApprovalLiveStatus = "pending" | "approved" | "rejected" | "timeout" | "expired"; -/** - * A standing-consent offer the platform is willing to take for this - * approval's tool/resource pair -- present only when the host's read can - * name both (see `ApproveAction.scope` and its `hub-api` handler, which - * today rejects `scope: "always"` with `unsupported_scope` because the - * suspend path doesn't yet capture tool identity). Absence of this field - * means "not offerable here," never "offerable but hidden." - */ +// Absent means "not offerable here," never "offerable but hidden." export type StandingConsentOffer = { readonly verb: string; readonly resource: string; }; -/** - * The platform's own account of what is being asked -- who, what tool, with - * which arguments. This is the authoritative "what am I approving" a human - * decides against; the block's own `title`/`body` is the *agent's* framing - * and is never a substitute for it (a confused-deputy card that shows only - * agent-authored text next to live buttons is exactly the failure mode this - * type exists to close off). - */ +// The authoritative "what am I approving" — the block's own title/body is +// the agent's framing and never a substitute for it. export type PlatformApprovalDetail = { readonly agentName: string; readonly headline: string; @@ -49,23 +27,8 @@ export type PlatformApprovalDetail = { readonly standingConsent?: StandingConsentOffer; }; -/** - * What the host's status read told the card. `canAct` is a fact the host - * establishes from its own authorization read (e.g. whether the approval - * showed up in a grant-scoped "needs you" read) -- the card never infers it - * from the block's own data. A `"ready"` result always carries `detail`: - * there is no code path where a card may show live buttons without the - * platform's own description of the request alongside them. - * - * `forbidden` is distinct from `canAct: false`: it means the host's *read* - * itself was refused, so actionability -- and the platform detail -- could - * not be determined at all. The card's honest answer in that case is to - * show the Approve/Deny buttons (never the agent's framing standing in for - * a description) and let the real approve/reject call's own authorization - * decide, surfacing a 403 inline if it also refuses -- never guessing a - * spectator verdict, and never rendering unverified agent text as if it - * were the platform's account of the request. - */ +// See docs/chat-wire-contract.md for why `forbidden` is distinct from +// `canAct: false`. export type ApprovalStatusQuery = | { readonly kind: "loading" } | { @@ -81,11 +44,8 @@ export type ApprovalStatusQuery = export type ApprovalDecisionResult = | { readonly kind: "resolved"; readonly status: "approved" | "rejected" } | { readonly kind: "forbidden"; readonly message: string } - /** The approval was no longer pending by the time this decision reached - * the platform (HTTP 409: already resolved, run no longer running, or - * deployment unavailable) -- distinct from a transient `error` because - * the right response is never "let the user retry," it's "re-sync and - * show what actually happened." */ + // HTTP 409, no longer pending: distinct from `error` because the right + // response is "re-sync and show what happened," never "let it retry." | { readonly kind: "conflict"; readonly message: string } | { readonly kind: "error"; readonly message: string }; @@ -98,13 +58,7 @@ export type ApprovalActions = { readonly approve: (approvalId: string) => Promise; /** Calls the same native `/reject` route Inbox calls. */ readonly reject: (approvalId: string) => Promise; - /** - * Turns this one decision into a standing grant for the same tool/resource - * pair. Optional: the native route this would call (`POST .../approve` - * with `scope: "always"`) is defined in `@intx/types` but `hub-api` - * rejects it today (`unsupported_scope` -- the suspend path doesn't yet - * capture tool identity). Omit this until a host can wire it to something - * real; the card never shows the standing-consent link without it. - */ + // Optional: `hub-api` rejects `scope: "always"` today, so omit until a + // host can wire it to something real. readonly allowStanding?: (approvalId: string) => Promise; }; diff --git a/apps/web/src/chat/blocks/approve-block.tsx b/apps/web/src/chat/blocks/approve-block.tsx index 2ea790544..d32971691 100644 --- a/apps/web/src/chat/blocks/approve-block.tsx +++ b/apps/web/src/chat/blocks/approve-block.tsx @@ -1,15 +1,6 @@ -// The approve card renders the PLATFORM's own account of what is being -// asked -- who, what, with which arguments, read live from the host -- as -// the authoritative description next to any live Approve/Deny buttons. The -// agent's own framing (`data.title`/`body`/`risk`) is demoted to -// contextual color: it renders alongside the platform detail, never in -// place of it, and never at all when live buttons show with no platform -// detail available (the "undetermined" 403 fallback -- see -// `approve-card-state.ts`). Nothing here is a decision: resolved state is -// always re-rendered from the host's status read, never from the block's -// own data or from a decision response the card just made. When the host -// gives no `ApprovalActions` port, the card falls back to its -// pre-round-trip framing: fixed disabled buttons, no fetch. +// See docs/chat-wire-contract.md for why the platform's own detail is +// authoritative. No `ApprovalActions` port: falls back to pre-round-trip +// framing, fixed disabled buttons, no fetch. import { Button, toast } from "@corbits/react-ui"; import type { ApproveBlockData } from "../wire/blocks"; diff --git a/apps/web/src/chat/blocks/approve-card-state.test.ts b/apps/web/src/chat/blocks/approve-card-state.test.ts index 596487bfa..47df134e7 100644 --- a/apps/web/src/chat/blocks/approve-card-state.test.ts +++ b/apps/web/src/chat/blocks/approve-card-state.test.ts @@ -137,10 +137,7 @@ describe("deriveApproveCardView", () => { }); test("a forbidden status read keeps the buttons, never demotes to spectator", () => { - // A refused status read leaves the card with no platform detail to - // show. It still renders buttons: the refusal a person can act on is - // the one their own decision returns, surfaced inline, not a silently - // disabled card (`forbidden` → `undetermined`, never `spectator`). + // `forbidden` -> `undetermined`, never `spectator`. const view = deriveApproveCardView({ wired: true, live: { kind: "forbidden" }, diff --git a/apps/web/src/chat/blocks/approve-card-state.ts b/apps/web/src/chat/blocks/approve-card-state.ts index 7b490b50e..0e699a5e8 100644 --- a/apps/web/src/chat/blocks/approve-card-state.ts +++ b/apps/web/src/chat/blocks/approve-card-state.ts @@ -20,22 +20,16 @@ export type ApproveCardView = readonly deciding: DecisionInFlight; readonly error: string | null; } - /** The read succeeded but couldn't establish the viewer may act (or the - * read never returned a live status at all, e.g. hosts that only carry - * the fixed disabled framing) -- status and the platform's own detail - * shown, no buttons. */ + // Read succeeded but couldn't establish the viewer may act: status and + // detail shown, no buttons. | { readonly kind: "spectator"; readonly status: ApprovalLiveStatus; readonly detail: PlatformApprovalDetail; } - /** The read itself was forbidden, so the platform's own detail never - * arrived. Buttons render anyway: the refusal a person needs to see is - * the one their own decision earns, surfaced inline from approve/reject, - * not a disabled card they cannot ask about. There is deliberately no - * `detail` here: this is the one case where buttons show without it, so - * the view must never let the block's agent-authored `body`/`title` - * stand in as if it were that detail. */ + // Read was forbidden; buttons render anyway so the refusal a person + // needs to see is the one their own decision earns. No `detail`: the + // view must never substitute the agent-authored body/title for it. | { readonly kind: "undetermined"; readonly deciding: DecisionInFlight; @@ -45,10 +39,8 @@ export type ApproveCardView = readonly kind: "resolved"; readonly status: ApprovalLiveStatus; readonly detail: PlatformApprovalDetail; - /** Set when this render followed a decision call that came back - * `"conflict"` (HTTP 409): the card re-synced and found the approval - * already resolved by someone/something else, rather than by this - * click -- worth a calmer, more specific note than the bare status. */ + // Set on an HTTP 409 conflict: resolved by someone/something else, + // not this click. readonly resolvedElsewhere: boolean; } | { readonly kind: "not-found" } @@ -65,16 +57,9 @@ export function isTerminalStatus(status: ApprovalLiveStatus): boolean { return TERMINAL_STATUSES.includes(status); } -/** - * Derives what the card shows. `wired` is false when the host gave no - * `ApprovalActions` port at all -- the pre-round-trip fallback: static - * framing, fixed disabled buttons, no fetch ever attempted. - * - * `resolvedElsewhere` only ever marks a genuinely resolved render: it has - * no effect unless `live` is itself `"ready"` with a terminal status, so a - * stale flag left over from an earlier decision can never fabricate a - * resolution. - */ +// `resolvedElsewhere` has no effect unless `live` is already `"ready"` +// with a terminal status, so a stale flag can never fabricate a +// resolution. export function deriveApproveCardView(args: { readonly wired: boolean; readonly live: ApprovalStatusQuery; diff --git a/apps/web/src/chat/blocks/connect-service-actions.ts b/apps/web/src/chat/blocks/connect-service-actions.ts index 6ce58c058..3df4b82da 100644 --- a/apps/web/src/chat/blocks/connect-service-actions.ts +++ b/apps/web/src/chat/blocks/connect-service-actions.ts @@ -1,11 +1,5 @@ -// The host port behind the generic service connect card — -// the `ConnectGithubActions` shape generalized to any connector or MCP -// preset. The block's data carries only agent-authored framing; every -// live fact — connected or not, and which connect affordance the -// deployment actually supports (hosted OAuth, one-click keyless, or a -// pasted key) — is resolved here by the host against the tenant's real -// connections, so an agent can never author a verdict or steer the -// auth mode. +// Every live fact — connected or not, which auth mode applies — is +// resolved by the host, never authored by the agent. export type ConnectAffordance = "oauth" | "keyless" | "api-key"; @@ -31,12 +25,8 @@ export interface ConnectServiceActions { connectorId: string, listener: (query: ConnectServiceQuery) => void, ): () => void; - /** - * Re-reads live connect state for every subscribed connector and fans - * it to those listeners. ChatWorkspace calls this when a parsed - * `chat.settings` event lands so a mounted card flips without - * remounting. - */ + // Called on a `chat.settings` event so a mounted card flips without + // remounting. notifySettingsChanged(): Promise; /** One-click connect: starts the hosted OAuth hand-off (navigating * away and back) or completes a keyless preset in place. */ diff --git a/apps/web/src/chat/blocks/connect-service-block-container.tsx b/apps/web/src/chat/blocks/connect-service-block-container.tsx index 10bbb3b16..615059144 100644 --- a/apps/web/src/chat/blocks/connect-service-block-container.tsx +++ b/apps/web/src/chat/blocks/connect-service-block-container.tsx @@ -1,12 +1,5 @@ -// Wires the presentational `ConnectServiceBlockView` to a live -// `ConnectServiceActions` port — the `ConnectGithubBlockContainer` -// shape: one `getConnectState` read on mount, then live -// `subscribeConnectState` folds, keyed by the block's `connectorId` -// rather than the message id since the state is the tenant's, not the -// workbench's. With no port (or while loading), the card renders the -// disconnected key-paste-free framing with a disabled-by-inaction -// connect that goes nowhere, matching the "no port, no feature" -// fallback every other block uses. +// Keyed by `connectorId`, not the message id, since the state is the +// tenant's, not the workbench's. No port: "no port, no feature" fallback. import { useQuery, useQueryClient } from "@tanstack/react-query"; import { useEffect } from "react"; import type { ConnectServiceBlockData } from "../wire/blocks"; diff --git a/apps/web/src/chat/blocks/connect-service-block.tsx b/apps/web/src/chat/blocks/connect-service-block.tsx index e61427d16..47c7636e8 100644 --- a/apps/web/src/chat/blocks/connect-service-block.tsx +++ b/apps/web/src/chat/blocks/connect-service-block.tsx @@ -1,11 +1,5 @@ -// The generic service connect card — `connect-github`'s -// first-run card generalized to every connector and MCP preset, in the -// same `BlockCard` frame. Pure and props-driven like -// `ConnectGithubBlockView`: the disconnected arm renders one of three -// affordances the host resolved (one-click OAuth, one-click keyless, -// key-paste), the connected arm is a plain confirmation, and -// `./connect-service-block-container.tsx` wires the callbacks to a live -// `ConnectServiceActions` port. +// Pure and props-driven; `./connect-service-block-container.tsx` wires +// the callbacks to a live port. import { useState } from "react"; import { Button, Input } from "@corbits/react-ui"; diff --git a/apps/web/src/chat/blocks/registry.tsx b/apps/web/src/chat/blocks/registry.tsx index e0c01ca15..2f3a38f07 100644 --- a/apps/web/src/chat/blocks/registry.tsx +++ b/apps/web/src/chat/blocks/registry.tsx @@ -1,7 +1,5 @@ -// The closed, curated block registry: agents author data, this client owns -// the code. `BlockPartView` parses the wire envelope at the render boundary -// and routes each known type to its view; anything unknown or malformed -// renders a labeled fallback card instead of raw JSON or a crash. +// Closed and curated: agents author data, this client owns the code. +// Unknown or malformed renders a labeled fallback, never raw JSON. import type { Block } from "../wire/blocks"; import { parseBlock } from "../wire/blocks"; diff --git a/docs/chat-wire-contract.md b/docs/chat-wire-contract.md index c82aa2a80..092c41c5c 100644 --- a/docs/chat-wire-contract.md +++ b/docs/chat-wire-contract.md @@ -19,6 +19,25 @@ before being consolidated here, so a new id-generating prefix is a missed test run rather than a missed grep. Prefix words mirror `@intx/hub-common`'s `generateId` (`PREFIXES` in `packages/hub-common/src/ids.ts`). +## Approve block (`chat/blocks/approve-block.tsx`) + +The card renders the platform's own live account of what's being asked as +the authoritative description next to Approve/Deny; an agent's own framing +is demoted to contextual color and never shown alone. Resolved state is +always re-rendered from the host's status read, never from the block's own +data or a decision response the card just made — nothing here is itself a +decision. + +`@/chat` owns no session or query cache and never fetches/mutates approvals +itself — the host supplies an `ApprovalActions` port (`chat/blocks/approval- +actions.ts`), calling the same native approve/reject routes and +invalidating the same query keys as Inbox, so the card, Inbox, and the +Activity band stay one source of truth. `forbidden` is distinct from +`canAct: false`: it means the host's own status read was refused, so the +card shows Approve/Deny anyway and lets the real decision call's own +authorization decide, rather than guessing a verdict or rendering +unverified agent text as the platform's account of the request. + ## Stream events (`chat/wire/stream-events.ts`) Organizing rule: a subscriber must be able to render, or update its own From 1d3717eb59d0bd4e3fef6a3f79799e1bf675a3e6 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:49:39 -0700 Subject: [PATCH 06/53] chore(web): shell comments say why, not what, part 1 (CL-8378) --- apps/web/src/shell/bench-activity.ts | 4 -- apps/web/src/shell/brand-mark.tsx | 6 +- apps/web/src/shell/canvas-availability.tsx | 13 ++-- apps/web/src/shell/canvas-column.tsx | 39 +++--------- apps/web/src/shell/canvas-host-context.ts | 65 +++++--------------- apps/web/src/shell/chip.tsx | 13 +--- apps/web/src/shell/composer-insertion.tsx | 9 +-- apps/web/src/shell/docks.tsx | 8 +-- apps/web/src/shell/first-run-tour-store.ts | 7 +-- apps/web/src/shell/routine-activity.test.ts | 11 +--- apps/web/src/shell/routine-activity.ts | 17 +---- apps/web/src/shell/shell-chrome-provider.tsx | 18 ++---- apps/web/src/shell/sidebar.tsx | 24 ++------ apps/web/src/shell/stage-search.tsx | 21 ++----- 14 files changed, 55 insertions(+), 200 deletions(-) diff --git a/apps/web/src/shell/bench-activity.ts b/apps/web/src/shell/bench-activity.ts index 3e1fab446..8956fe02b 100644 --- a/apps/web/src/shell/bench-activity.ts +++ b/apps/web/src/shell/bench-activity.ts @@ -1,7 +1,3 @@ -// Everything happening in the selected bench right now, over stock -// listings only: workbenches are the bench's child tenants, chats are the -// person's mail threads, agents are the bench's workflow definitions, and -// running routines come through the seam in `./routine-activity.ts`. // Notifications have no backing feature in the hub, so nothing is fetched // for them — the column renders an honest empty state instead. diff --git a/apps/web/src/shell/brand-mark.tsx b/apps/web/src/shell/brand-mark.tsx index 4b5dfe623..61024cda1 100644 --- a/apps/web/src/shell/brand-mark.tsx +++ b/apps/web/src/shell/brand-mark.tsx @@ -1,7 +1,5 @@ -// A static inline SVG via react-ui's `CorbitsMark`, coloured with the -// primary orange — simple and crisp over clever: an animated dither -// background renders as a tiny pixelated square at this size, since its -// dither grid needs real screen real estate to read. +// Static, not the animated dither background: its grid needs real screen +// real estate this size doesn't have. import { CorbitsMark } from "@corbits/react-ui"; diff --git a/apps/web/src/shell/canvas-availability.tsx b/apps/web/src/shell/canvas-availability.tsx index 6d6cfa8bd..17c2fc39c 100644 --- a/apps/web/src/shell/canvas-availability.tsx +++ b/apps/web/src/shell/canvas-availability.tsx @@ -1,8 +1,5 @@ -// Canvas host surface for stage content: whether the shell has space for the -// fourth column, how main-stage chat opens auxiliary canvas content -// (profiles, artifacts) without owning the canvas column itself, and — for -// AppShell's own render, which no longer owns this state — what the canvas -// column is actually showing right now. +// Lets main-stage chat open auxiliary canvas content without owning the +// canvas column itself. import { useContext, type ReactNode } from "react"; import type { ProfileSubject } from "@/chat"; @@ -109,10 +106,8 @@ export function useOpenArtifactInCanvas(): (artifact: CanvasArtifactContent) => return useContext(CanvasHostContext).openArtifact; } -/** Opens (or replaces) the canvas's routine pane — the workbench header's - * "New routine" action, the `/workflows` page's own create button, and an - * existing routine's own "Edit" hop all call this. `routineId: null` starts - * a brand-new routine; a real id opens that routine for editing. */ +// `routineId: null` starts a brand-new routine; a real id opens it for +// editing. export function useOpenRoutineInCanvas(): (subject: RoutinePanelSubject) => void { return useContext(CanvasHostContext).openRoutine; } diff --git a/apps/web/src/shell/canvas-column.tsx b/apps/web/src/shell/canvas-column.tsx index 4e5ed4322..a9d67463c 100644 --- a/apps/web/src/shell/canvas-column.tsx +++ b/apps/web/src/shell/canvas-column.tsx @@ -1,23 +1,8 @@ -// Column 4: the optional canvas. Collapsed, it takes no space at all — the -// main pane gets the width back — and open, it hosts targeted auxiliary -// content: profile cards, and typed artifact renderers opened -// from a chat artifact chip or the Library page. Primary workbench -// conversation lives in the main stage, not here. -// -// co-edit presence is gone. A "doc"-kind, editable artifact -// renders `@/library`'s `ArtifactTextEditor` as a plain -// single-user controlled textarea, debounced-saved through the artifacts -// HTTP route; every other kind stays the read-only `ArtifactRenderer`. No -// co-viewer cursors, no shared doc — that capability returns, if it does, -// on top of the preserved `@corbits/presence` library, not here. -// -// The collapse/expand motion lives entirely in `shell.css` as a CSS -// transition on `transform`/`opacity` (plus width, so the main pane -// actually reflows) triggered by the `data-open` attribute — never a JS -// animation — so rapid toggling is inherently interruptible: the browser -// just reverses whichever transition is already in flight, there is no -// queue to get stuck. `prefers-reduced-motion` is handled the same way, in -// CSS, by shortening the transition to near-zero. +// Collapse/expand is a CSS transition on `data-open`, never JS animation, +// so rapid toggling is inherently interruptible — no queue to get stuck. + +// No co-edit presence: a "doc"-kind artifact is a plain single-user +// textarea. That capability, if it returns, lives on `@corbits/presence`. import { Button, @@ -64,11 +49,8 @@ export function CanvasColumn({ * one seam to the artifacts PUT route. Absent for a non-editable artifact. */ readonly onSaveArtifact?: (content: string) => void; }) { - // `inert` rather than `aria-hidden`: a collapsed column has to be out of - // both the accessibility tree and the tab order, and `aria-hidden` alone - // only does the first — a focusable descendant inside an `aria-hidden` - // subtree is an ARIA violation, and the browser moves focus out of an - // `inert` subtree for us when it closes. + // `inert`, not `aria-hidden` alone: `aria-hidden` doesn't remove a + // focusable descendant from the tab order. return (
@@ -265,11 +247,8 @@ function profileActions( ]; } - // No "Grants" hop here: settings-ui's Grants section has no deep-link - // filter to land on this person's rules specifically, and a profile - // card action that lands on the unfiltered, everyone's-rules list is - // worse than not offering it — same reasoning the agent branch above - // uses to drop "Edit agent". + // No "Grants" hop: no deep-link filter exists, and landing on the + // unfiltered everyone's-rules list is worse than not offering it. return [ message, mention, diff --git a/apps/web/src/shell/canvas-host-context.ts b/apps/web/src/shell/canvas-host-context.ts index 2985d1bee..0d6b51483 100644 --- a/apps/web/src/shell/canvas-host-context.ts +++ b/apps/web/src/shell/canvas-host-context.ts @@ -8,18 +8,8 @@ import type { ProfileSubject } from "@/chat"; import type { ArtifactRendererKind } from "@/library"; import type { CanvasColumnState } from "@/shell/layout"; -/** The canvas's typed-artifact pane: a title, the already-resolved - * renderer selection (see `@/library`'s `resolveArtifactRendererKind` - * / `resolveRendererKindFromMediaType`), and the content string those - * renderers read. - * - * `canEdit` (phase 2) opts a text-kind ("doc") artifact into - * `ArtifactTextEditor` instead of the read-only `ArtifactRenderer` — - * defaults to `false`/absent so every existing caller keeps rendering - * read-only with zero behavior change. The presence `/update` route's own - * `asset:*`/"write" grant check is the real security boundary regardless - * of this flag; `canEdit` only decides which component a capable viewer - * sees, never whether a write actually lands. */ +// `canEdit` only picks the component; the `/update` route's own grant +// check is the real security boundary regardless of this flag. export type CanvasArtifactContent = { readonly id: string; readonly title: string; @@ -27,56 +17,29 @@ export type CanvasArtifactContent = { readonly content: string; readonly unavailableReason?: string; readonly canEdit?: boolean; - /** The sandboxed preview route for an `"html"`-kind artifact — - * see `ArtifactRenderProps.previewSrc`. Absent for every other renderer - * kind, and for an HTML blob with no Library artifact id to preview. */ + // Absent for every renderer but html, and for a blob with no artifact id. readonly previewSrc?: string; }; -/** The canvas's routine pane subject: which routine to show, or `null` to - * start a brand-new one. Distinct from `CanvasArtifactContent` — the panel - * fetches and owns its own routine data (name, instruction, trigger, run - * history) from `routineId`, the same way `ProfileCanvasPane` fetches - * shared workbenches from a `ProfileSubject`'s address rather than being - * handed pre-resolved content. */ +// Distinct from `CanvasArtifactContent`: the panel fetches and owns its +// own routine data from `routineId` rather than being handed content. export type RoutinePanelSubject = { - /** Always opens the editor: a specific routine (`routineId` set) or a - * brand-new one (`routineId` omitted or `null`) — routines-page's own - * "New routine"/"Edit" actions, "Make this a routine", the composer's - * `/routine` command, and "New routine in this space" (: - * browsing/running existing routines moved to the global `/workflows` - * page, so this pane no longer has a list mode). */ readonly routineId?: string | null; - /** Seeds the Name/Instruction fields the instant a brand-new panel opens - * (`routineId: null` only) — "Make this a routine" (a completed task - * result) and similar callers with something worth pre-filling. The - * panel still autosaves on the person's own edits; this only seeds the - * initial draft. */ + // Seeds the Name/Instruction fields on a brand-new panel only; the panel + // still autosaves on the person's own edits after that. readonly initialName?: string; readonly initialInstruction?: string; - /** The conversation this routine belongs to — its own agent (the - * workbench's host participant; every workbench's host is Myra) backs the - * routine, and its own id is where the routine delivers. Carried through - * list mode too, so "New routine" picked from the list still binds to - * the workbench the panel was opened beside. Omitted only when there is no - * open conversation to bind to (e.g. a deliberate `/workflows` visit), - * in which case the panel falls back to this workbench's own default - * (Myra) workbench — never mints a new one. */ + // Omitted only with no open conversation to bind to, in which case the + // panel falls back to this workbench's own default (Myra) — never mints + // a new one. readonly workbenchId?: string; - /** Seeds the target picker's initial selection — the - * conversation's own single agent participant's definition asset id, - * when the opener could resolve exactly one. Shown visibly in - * `DefinitionTargetPicker` and freely replaceable/clearable by the - * person; only their final explicit pick is ever sent to the backend. - * Omitted whenever the opener found zero or several candidates, or has - * no conversation to derive one from at all. */ + // The opener's single resolved agent participant, when there was + // exactly one; freely replaceable, only the person's final pick is sent. readonly preselectedAssetId?: string; }; -/** Workbench's concrete instantiation of `@/shell/layout`'s generic - * canvas state — a `ProfileSubject` for the profile pane, this app's own - * `CanvasArtifactContent` for the artifact pane, `RoutinePanelSubject` for - * the routine pane. */ +// A `ProfileSubject` for the profile pane, `CanvasArtifactContent` for the +// artifact pane, `RoutinePanelSubject` for the routine pane. export type AppCanvasColumnState = CanvasColumnState< ProfileSubject, CanvasArtifactContent, diff --git a/apps/web/src/shell/chip.tsx b/apps/web/src/shell/chip.tsx index c6dc3b240..06e3ea4aa 100644 --- a/apps/web/src/shell/chip.tsx +++ b/apps/web/src/shell/chip.tsx @@ -1,14 +1,5 @@ -// Mock's `.chip[data-tone]` (workbench-flow-mock.html:262-273): a quiet -// status pill with three tones. Ambient machine activity ("working") and a -// resolved state ("ok") stay outlined; only a human-blocking ask -// ("needs-you") gets a filled, high-weight treatment — the one chip a -// person should never miss. -// -// No chip primitive exists yet in @corbits/react-ui (checked: `Badge` is -// the closest sibling, but it's an uppercase rounded-sm label for -// categorical tags, not this outlined/filled status-pill idiom with a -// radius-0 floor) — built locally. Worth extracting upstream once a -// second consumer needs it. +// Only "needs-you" is filled/high-weight — the one chip a person should +// never miss. Built locally: no chip primitive in @corbits/react-ui yet. import type { ReactNode } from "react"; export type ChipTone = "working" | "ok" | "needs-you"; diff --git a/apps/web/src/shell/composer-insertion.tsx b/apps/web/src/shell/composer-insertion.tsx index 202a422ee..e3ffea5a7 100644 --- a/apps/web/src/shell/composer-insertion.tsx +++ b/apps/web/src/shell/composer-insertion.tsx @@ -1,10 +1,5 @@ -// A shell-level seam between two independent surfaces: the canvas column's -// ProfileCard (this app) and the active workbench's composer (mounted deep -// inside `@/chat`'s `ChatWorkspace`, a sibling tree). The profile -// card's Mention action needs to land `@handle` in whatever -// composer is on screen without either side importing the other — the same -// "shell context exposing a callback hook" shape `canvas-availability.tsx` -// already uses for opening a profile. +// Lets the profile card's Mention action reach the composer without +// either side importing the other — same shape as `canvas-availability.tsx`. import { useContext, useMemo, useRef } from "react"; import type { ReactNode } from "react"; diff --git a/apps/web/src/shell/docks.tsx b/apps/web/src/shell/docks.tsx index 11d4097ff..41f6a160e 100644 --- a/apps/web/src/shell/docks.tsx +++ b/apps/web/src/shell/docks.tsx @@ -2,13 +2,7 @@ // switcher dock this file used to also export (`BenchDock`) is gone — see // `sidebar.tsx`'s header comment. -/** - * Initials for the identity dock's avatar, derived locally — the app is - * CSP-strict, so there is never a network fetch for an avatar image. - * Prefers the account name; an account with no usable name falls back - * to the email's local part, and "··" stands in when neither yields a - * letter (mirroring the reference chrome's placeholder). - */ +// Derived locally — CSP-strict, so never a network fetch for an avatar. export function initialsOf(name: string, email = ""): string { const source = name.trim().length > 0 ? name : (email.split("@")[0] ?? ""); const initials = source diff --git a/apps/web/src/shell/first-run-tour-store.ts b/apps/web/src/shell/first-run-tour-store.ts index fdeda615d..6dfdc2474 100644 --- a/apps/web/src/shell/first-run-tour-store.ts +++ b/apps/web/src/shell/first-run-tour-store.ts @@ -1,8 +1,5 @@ -// The first-run tour's open state, held outside the React tree like -// `command-palette-open-store.ts`: the tour must never auto-start itself on -// landing (it used to, and its overlay would land right over the chat the -// person was just redirected onto), so the only way in is an explicit call -// to `openFirstRunTour` from a command or menu action. +// Must never auto-start on landing (it used to, overlaying the chat the +// person was just redirected onto) — only an explicit call opens it. import { useSyncExternalStore } from "react"; diff --git a/apps/web/src/shell/routine-activity.test.ts b/apps/web/src/shell/routine-activity.test.ts index 3579ac9d3..b6d43d9bc 100644 --- a/apps/web/src/shell/routine-activity.test.ts +++ b/apps/web/src/shell/routine-activity.test.ts @@ -1,12 +1,5 @@ -// `listRoutineActivity` resolves no items without fetching. The -// `feed=fires` route it used to read is deleted with `@corbits/run-scope`, -// and the native `GET /workflows/runs` listing has no fires equivalent — -// its top-level-only predicate drops every routine fire by construction, -// and its rows carry no routine attribution to compose client-side from. -// So the shell's "Running" band and Mission Control's active-run count -// honestly report no routine activity until a native fires equivalent -// exists, instead of deriving routine activity from top-level deployment -// rows that are not routine fires. +// See `routine-activity.ts` for why this resolves no items until a native +// fires equivalent exists. import { afterEach, describe, expect, test } from "bun:test"; diff --git a/apps/web/src/shell/routine-activity.ts b/apps/web/src/shell/routine-activity.ts index 73a04bf7c..0c812aae0 100644 --- a/apps/web/src/shell/routine-activity.ts +++ b/apps/web/src/shell/routine-activity.ts @@ -1,17 +1,6 @@ -// deleted `@corbits/run-scope`'s `/top-level-runs?feed=fires` -// route — the only listing that kept a routine's fire despite it not -// being a top-level run — and the native `GET /workflows/runs` listing -// has no equivalent: its top-level-only predicate drops every routine -// fire by construction, and its rows carry no routine attribution to -// compose client-side from (matching fires to routines by definition -// name here would be exactly the slug guess the Insights feed refuses -// to do). So `listRoutineActivity` resolves no items without fetching: -// the shell's "Running" band and Mission Control's active-run count -// honestly report no routine activity until a native fires -// equivalent exists, instead of deriving routine activity from -// top-level deployment rows that are not routine fires. The -// `RoutineActivityItem` shape is kept so both consumers keep compiling -// against the seam in `./bench-activity.ts`. +// The native runs listing has no routine-fires equivalent, so this +// resolves no items rather than deriving activity from rows that aren't +// routine fires — an honest "none" until that equivalent exists. import type { ListingTurn } from "@corbits/workflows/client"; export type RoutineActivityItem = { diff --git a/apps/web/src/shell/shell-chrome-provider.tsx b/apps/web/src/shell/shell-chrome-provider.tsx index e53660245..8ba592c4c 100644 --- a/apps/web/src/shell/shell-chrome-provider.tsx +++ b/apps/web/src/shell/shell-chrome-provider.tsx @@ -1,12 +1,6 @@ -// Owns the shell chrome state that has to be visible above both the command -// palette and the shell frame: canvas state (open/profile/focus). -// CommandPaletteProvider and AppShell are siblings in app.tsx's Shell — a -// palette action that "closes the canvas" has to mutate the same state -// AppShell renders from, not a second copy scoped to AppShell's own -// subtree. This is the one place that state lives; AppShell consumes it -// through the same hooks page code already uses (`useCloseCanvas`, ...) -// plus the shell-only read (`useCanvasColumnOpen`) it needs for its own -// render. +// Sits above CommandPaletteProvider and AppShell (siblings) so a palette +// action that closes the canvas mutates the same state AppShell renders +// from, not a second copy scoped to its own subtree. import { useCallback, useState, type ReactNode } from "react"; @@ -59,10 +53,8 @@ export function ShellChromeProvider({ initialCanvasColumnState, ); - // The last scope and rail surface we applied, adjusted during render - // rather than from an effect — the canvas must never paint a frame - // holding another workbench's content. The initial null→ready tenant - // resolve is not a switch. + // Adjusted during render, not an effect: the canvas must never paint a + // frame holding another workbench's content. const [appliedTenantId, setAppliedTenantId] = useState(selectedTenantId); const routePrefix = inAppRoutePrefix(path); const [appliedRoutePrefix, setAppliedRoutePrefix] = useState(routePrefix); diff --git a/apps/web/src/shell/sidebar.tsx b/apps/web/src/shell/sidebar.tsx index 1b5329516..55d8a631c 100644 --- a/apps/web/src/shell/sidebar.tsx +++ b/apps/web/src/shell/sidebar.tsx @@ -1,23 +1,7 @@ -// The one sidebar. Header: the brand mark, then a create dropdown + search. -// Body: Agents and Channels — nothing page-scoped ever renders here. -// Footer: the primary rail is Artifacts, Skills, Tools, Workflows, Agents. -// Below the rail: a single Settings row (icon + label) — Insights, the -// account menu (avatar, name, sign out), and everything else now live -// inside Settings itself, not as separate sidebar affordances. -// Always present; there is no collapse affordance and no second nav column. -// Approvals belong in the conversation, not as a standing band here. -// -// Inbox is gone (owner decision: tasks + approvals don't flow -// into workbenches). Mission Control is gone too — its pending-approvals -// and activity panels now live inside each workbench's own workbench. -// -// No bench switcher: a workbench IS an agent conversation now, -// one per account, so there is nothing to switch between in the common -// case. A multi-bench install still resolves and routes correctly (see -// `bench-context.tsx`) — it just has no dedicated chrome slot. The one -// escape hatch is the command palette's hidden "Switch workbench" action -// (`command-palette-actions.ts`), which only appears once memberships -// resolve to more than one workbench. +// No bench switcher: a workbench IS an agent conversation now, one per +// account, so there's nothing to switch between in the common case. A +// multi-bench install still resolves (`bench-context.tsx`) via the command +// palette's hidden "Switch workbench" action. import { Button, diff --git a/apps/web/src/shell/stage-search.tsx b/apps/web/src/shell/stage-search.tsx index b733d5684..dc749c2ea 100644 --- a/apps/web/src/shell/stage-search.tsx +++ b/apps/web/src/shell/stage-search.tsx @@ -1,19 +1,8 @@ -// The stage top bar's per-page filter (DECISIONS.md → Search): a magnifier -// that morphs in place into a plain text input scoped to whatever the page -// is showing — Files filters files, Skills filters skills. It never reaches -// the global command palette; `Cmd+K` is a separate surface entirely -// (`command-palette-provider.tsx`), mounted on its own rather than out of -// this control. -// -// A page hands in the filter it already owns (`value`/`onChange`); this -// component only supplies the chrome — the button, the morph, and the input -// that drives that state directly. `StageTopBar` renders it only when a page -// passes a filter, so a page with nothing to filter shows no magnifier. -// -// Motion is the width transition authored on `.stage-search` in app.css -// (react-ui's `--duration-standard` and `--ease-in-out`). Reduced motion -// needs nothing here: react-ui's stylesheet already collapses every -// transition duration under `prefers-reduced-motion`. +// Never reaches the global command palette — `Cmd+K` is a separate +// surface, mounted on its own. + +// Chrome only: a page hands in the filter it already owns. `StageTopBar` +// renders this only when a page passes a filter. import { MagnifyingGlass } from "@/lib/icons"; import { useRef, useState } from "react"; From 7343973ad44d8cc6410eadb02094019d8eca0a09 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:51:17 -0700 Subject: [PATCH 07/53] chore(web): shell comments say why, not what, part 2 (CL-8378) --- apps/web/src/shell/app-shell.tsx | 20 +++--------- apps/web/src/shell/first-run-tour.tsx | 16 +++------ apps/web/src/shell/library-artifacts.ts | 43 ++++++------------------- apps/web/src/shell/stage-top-bar.tsx | 26 ++++----------- apps/web/src/shell/workbench-list.tsx | 6 ++-- 5 files changed, 26 insertions(+), 85 deletions(-) diff --git a/apps/web/src/shell/app-shell.tsx b/apps/web/src/shell/app-shell.tsx index 0ee4d5251..5f0aabe44 100644 --- a/apps/web/src/shell/app-shell.tsx +++ b/apps/web/src/shell/app-shell.tsx @@ -1,15 +1,5 @@ -// The app frame: one sidebar (the workbench list and shared chrome — see -// `sidebar.tsx`), the main pane a route renders into, and the optional -// canvas. Every route in `../routes.tsx` mounts inside this same frame — -// there is no per-route shell variant and no sidebar collapse. The -// conversation lives in the main stage; the canvas is auxiliary (profiles -// and similar) and opens on use, then closes internally. -// -// Canvas state is NOT owned here — it has to be visible to the command -// palette too (a sibling of this component, not a descendant — see -// `shell-chrome-provider.tsx`), so `ShellChromeProvider` owns it above both -// and this component only reads it through the same hooks page code -// already uses. +// Canvas state is NOT owned here — see `shell-chrome-provider.tsx` for why +// it has to be visible to the command palette too. import { lazy, Suspense, useRef, useState, type ReactNode, type RefObject } from "react"; import type { ArtifactSaveState } from "@/library"; @@ -92,10 +82,8 @@ export function AppShell({ const canvasFocus = useCanvasColumnFocus(); const { selectedTenantId: tenantId } = useBench(); - // A text-kind artifact's save state (single-user editing, no co-edit - // presence), carried with the id it belongs to: state for any other id is - // ignored during render, so a stale "Saved · v3" can never leak into a - // newly opened artifact and nothing has to be reset when one changes. + // Carried with the id it belongs to, so a stale "Saved · v3" can never + // leak into a newly opened artifact. const [savedFor, setSavedFor] = useState<{ readonly id: string; readonly state: ArtifactSaveState; diff --git a/apps/web/src/shell/first-run-tour.tsx b/apps/web/src/shell/first-run-tour.tsx index ca49a8b4d..eb06c7c10 100644 --- a/apps/web/src/shell/first-run-tour.tsx +++ b/apps/web/src/shell/first-run-tour.tsx @@ -1,10 +1,6 @@ -// A guided tour of the shell, started only by explicit user action (a -// command or menu item calling `openFirstRunTour`) — never automatically on -// landing, which used to drop its overlay right over the chat `/` redirects -// onto. "Seen" is a localStorage flag keyed by user id (mirrors -// `command-palette-recents.ts`'s defensive access) so a shared browser -// profile never re-shows it as "new" for the wrong account, and finishing or -// skipping both mark it seen — there is no "remind me later". +// "Seen" is a localStorage flag keyed by user id, so a shared browser +// profile never re-shows it as "new" for the wrong account. No "remind me +// later" — finishing or skipping both mark it seen. import Joyride, { ACTIONS, type CallBackProps, STATUS, type Step } from "react-joyride"; import { reportError } from "@corbits/error-sink"; @@ -62,10 +58,8 @@ const STEPS: readonly Step[] = [ export function FirstRunTour({ userId }: { readonly userId: string }) { const run = useFirstRunTourOpen(); - // Dismissing has to unmount Joyride, not just remember the dismissal: - // a running Joyride keeps two portals appended to `document.body` and - // an overlay over the app, and those portal containers are managed - // outside React's tree. + // Must unmount Joyride, not just remember the dismissal — its portal + // containers are managed outside React's tree. function handleCallback(data: CallBackProps) { // The tooltip's close (X) button fires action "close" without ever // moving status to FINISHED or SKIPPED, so it has to be treated as a diff --git a/apps/web/src/shell/library-artifacts.ts b/apps/web/src/shell/library-artifacts.ts index 9dbbd69ce..5d6edf416 100644 --- a/apps/web/src/shell/library-artifacts.ts +++ b/apps/web/src/shell/library-artifacts.ts @@ -48,11 +48,7 @@ export function artifactCountsPath(tenantId: string): string { return `/api/tenants/${tenantId}/artifacts/counts`; } -/** - * POST multipart upload against the tenant artifacts surface. Returns the - * created detail rows on 201; throws with status on non-2xx so the page can - * surface an honest failure. - */ +// Throws with status on non-2xx so the page can surface an honest failure. export async function uploadArtifactFiles( tenantId: string, files: readonly File[], @@ -88,12 +84,7 @@ export async function uploadArtifactFiles( return body.artifacts; } -/** - * PUT `/api/tenants/:tenantId/artifacts/:id` — the artifact - * editor's one save path now that co-edit presence is gone. Throws with - * status on non-2xx so the host can render an honest failed-save state - * instead of silently pretending the write landed. - */ +// Throws with status on non-2xx, never silently pretends the write landed. export async function saveArtifactContent( tenantId: string, artifactId: string, @@ -121,10 +112,8 @@ export async function saveArtifactContent( return (await response.json()) as ArtifactDetail; } -/** True when the hub answered "artifacts plane not configured" (503) — - * read off the query's own status field, never string-matched out of a - * rendered message (that copy is display-boundary plain by design and - * carries no status text to match against). */ +// Read off the query's status field, never string-matched out of the +// rendered message. export function isArtifactsUnavailableStatus(status: number | undefined): boolean { return status === 503; } @@ -136,17 +125,9 @@ export function artifactUploadToast(names: readonly string[]): string { : `Uploaded ${names.length} files`; } -/** - * The bulk/context-menu operation set this file adopts from the shared - * selection system — deliberately just the one real, already- - * shippable operation: every other candidate (delete, move, rename, - * download) has no backend route or store method behind it yet (see - * `packages/artifacts-hub/src/routes.ts` and `@corbits/artifacts`' - * `ArtifactStore`), so wiring a button for any of them would be exactly the - * dead/no-op control this adoption is required to avoid. `BulkActionBar` - * and the shell context menu's `artifact` target both read this same - * constant, which is what the parity test asserts against. - */ +// Deliberately just the one shippable operation: delete/move/rename/ +// download have no backend route yet, and a button with nothing behind +// it is exactly the dead control this adoption avoids. export const LIBRARY_BULK_OPERATION_IDS = ["copy-link"] as const; /** `/artifacts/a/:id` — the one canonical deep link a file has. */ @@ -172,14 +153,8 @@ export function copyArtifactLinksActionLabel(count: number): string { return count > 1 ? `Copy ${count} links` : "Copy link"; } -/** - * The MIME type of a file artifact's out-of-band upload blob, read off - * `source.upload.mimeType` — present on every artifact minted through - * `createFileArtifact` (`@corbits/artifacts`), regardless of whether the - * hub could inline its bytes into `content` as text. Null for an artifact - * with no upload backing at all (e.g. a co-edited doc), which is the only - * case where an empty `content` genuinely means "nothing here yet." - */ +// Null for an artifact with no upload backing at all — the only case +// where an empty `content` genuinely means "nothing here yet." export function uploadMimeTypeFromSource(source: Record): string | null { const upload = source.upload; if (typeof upload !== "object" || upload === null) return null; diff --git a/apps/web/src/shell/stage-top-bar.tsx b/apps/web/src/shell/stage-top-bar.tsx index dd9255fe7..7dd0af031 100644 --- a/apps/web/src/shell/stage-top-bar.tsx +++ b/apps/web/src/shell/stage-top-bar.tsx @@ -1,23 +1,9 @@ -// Every stage surface renders the same top bar, and every one of them -// titles itself with a breadcrumb trail: `crumbs` is the single way a page -// declares where it sits, and `actions` is the single home for its primary -// controls (a page never keeps a "New …" button in its body). A one-level -// page passes one crumb; a detail view passes its parent plus itself. -// -// Crumbs are routes, not callbacks: every level above the current page -// carries an `href`, so the trail is deep-linkable and a plain click -// navigates through the app's own `Link` instead of reloading the shell. -// -// `filter` is a page's own per-page filter (DECISIONS.md → Search), rendered -// through `StageSearch` ahead of `actions` when a page passes one. It is not -// shell chrome the way it used to be: a page with nothing to filter passes -// none and gets no magnifier at all. The global command palette (`Cmd+K`) -// is a separate surface entirely — see `command-palette-provider.tsx` — -// mounted on its own rather than out of this bar. -// -// `@corbits/react-ui`'s `TopBarBreadcrumbs` renders bare ``, which -// would drop the SPA out from under the click, so the trail lives here -// until react-ui takes a link-render slot. +// Crumbs are routes, not callbacks, so the trail is deep-linkable. The +// trail lives here (not `TopBarBreadcrumbs`) since react-ui's version +// renders bare ``, which would drop the SPA under the click. + +// `filter` is per-page (DECISIONS.md -> Search), not shell chrome — a page +// with nothing to filter gets no magnifier at all. import { Fragment, type ReactNode } from "react"; diff --git a/apps/web/src/shell/workbench-list.tsx b/apps/web/src/shell/workbench-list.tsx index 70e3c8468..79009de24 100644 --- a/apps/web/src/shell/workbench-list.tsx +++ b/apps/web/src/shell/workbench-list.tsx @@ -1,7 +1,5 @@ -// The sidebar's two labeled sections, in this order and never mixed: -// Workbenches (the bench's child tenants, a "#" glyph each) above Chats -// (one agent each, an initials circle each, newest first). Agent -// membership is a workbench concept; a chat has exactly one agent. +// Two sections, in this order and never mixed: Workbenches above Chats. +// Agent membership is a workbench concept; a chat has exactly one agent. import { EmptyState, Input, Skeleton } from "@corbits/react-ui"; import { Check, Hash, MagnifyingGlass } from "@/lib/icons"; From 609c0cf2b06959fc34e8c278450f0debf9a456bf Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:52:12 -0700 Subject: [PATCH 08/53] chore(web): shell/layout comments say why, not what (CL-8378) --- apps/web/src/shell/layout/breakpoints.ts | 6 ++---- .../src/shell/layout/canvas-column-state.ts | 19 +++---------------- apps/web/src/shell/layout/index.ts | 7 ++----- .../shell/layout/pending-dialog-request.ts | 9 ++------- apps/web/src/shell/layout/use-shell-layout.ts | 11 +++-------- 5 files changed, 12 insertions(+), 40 deletions(-) diff --git a/apps/web/src/shell/layout/breakpoints.ts b/apps/web/src/shell/layout/breakpoints.ts index be833cdf9..8af64da93 100644 --- a/apps/web/src/shell/layout/breakpoints.ts +++ b/apps/web/src/shell/layout/breakpoints.ts @@ -1,7 +1,5 @@ -// The shell's layout modes and the two widths that separate them, kept free -// of the DOM so the rules stay testable and so `use-shell-layout.ts` is the -// only place that touches `matchMedia`. The sidebar is always present; the -// canvas column is the one region that gives way as the viewport narrows. +// Kept free of the DOM so the rules stay testable; `use-shell-layout.ts` +// is the only place that touches `matchMedia`. export type ShellLayoutMode = "expanded" | "compact" | "narrow"; diff --git a/apps/web/src/shell/layout/canvas-column-state.ts b/apps/web/src/shell/layout/canvas-column-state.ts index 0ce41becc..30ddf4219 100644 --- a/apps/web/src/shell/layout/canvas-column-state.ts +++ b/apps/web/src/shell/layout/canvas-column-state.ts @@ -1,19 +1,6 @@ -// The canvas column's state as pure transitions, separate from -// `breakpoints.ts`'s allow/disallow rule — profile demand and the viewport's -// veto are independent inputs; `resolveCanvasVisibility` is the one place -// they combine. -// -// Canvas is auxiliary only (profiles and similar targeted surfaces). Primary -// channel conversation lives in the main stage via route (`/`, `/c`, `/c/:id`). -// There is no permanent toggle: canvas opens when auxiliary content is targeted -// and closes when that content is dismissed. -// -// `TProfile` is whatever subject the host app's canvas renders (a -// ProfileCard subject, or anything else worth targeting); `TArtifact` is -// whatever typed content pane the host renders alongside it (a document, a -// sheet, or anything else); `TRoutine` is a third, equally exclusive slot -// for a routine editor/detail pane. This module owns only the open/focus/ -// mutual-exclusion transitions, never the shape of what's shown. +// Pure transitions, separate from `breakpoints.ts`'s allow/disallow rule: +// profile demand and the viewport's veto are independent inputs; +// `resolveCanvasVisibility` is the one place they combine. export type CanvasColumnState = { readonly open: boolean; diff --git a/apps/web/src/shell/layout/index.ts b/apps/web/src/shell/layout/index.ts index b660214f1..be7fb5ee0 100644 --- a/apps/web/src/shell/layout/index.ts +++ b/apps/web/src/shell/layout/index.ts @@ -1,8 +1,5 @@ -// `@/shell/layout`: the generic shell mechanics a second Interchange -// deployment needs — the responsive breakpoint rules and the hook that reads -// them, the canvas column state machine, per-route scroll reset, and the -// pending-dialog-request pattern for cross-route dialog triggers. Route -// tables and app-specific policy stay in the consuming app. +// Generic shell mechanics a second Interchange deployment would need. +// Route tables and app-specific policy stay in the consuming app. export { canvasColumnAllowed, diff --git a/apps/web/src/shell/layout/pending-dialog-request.ts b/apps/web/src/shell/layout/pending-dialog-request.ts index 3e3b75e66..966bcb0c0 100644 --- a/apps/web/src/shell/layout/pending-dialog-request.ts +++ b/apps/web/src/shell/layout/pending-dialog-request.ts @@ -1,10 +1,5 @@ -// Generalizes the pending-flag pattern `library-upload.ts` already uses for -// cross-route dialog triggers: a caller may fire "open this dialog" from a -// page other than the one that owns it, before that page (and its -// window-event listener) has mounted. Dispatching the event immediately in -// that case is a race the listener always loses. Recording a one-shot -// pending flag and consuming it once the target page mounts avoids both the -// race and a setTimeout guess at when the listener will be ready. +// Avoids dispatching before the target page's listener mounts (a race it +// always loses) without a setTimeout guess at when it'll be ready. export type PendingDialogRequest = { /** On-route: dispatch immediately. Off-route: record the pending flag and diff --git a/apps/web/src/shell/layout/use-shell-layout.ts b/apps/web/src/shell/layout/use-shell-layout.ts index 2223b1c7a..00655524e 100644 --- a/apps/web/src/shell/layout/use-shell-layout.ts +++ b/apps/web/src/shell/layout/use-shell-layout.ts @@ -1,11 +1,6 @@ -// The shell's layout mode, read from the same media queries the stylesheet -// would use rather than from a resize listener: the browser evaluates the -// query and fires `change` only when the answer actually changes, so a drag -// across the whole viewport produces two state updates instead of hundreds. -// -// Rendering to static markup (the route tests) never runs effects and has no -// `window`, so it sees the initial "expanded" assumption — which is what a -// server-rendered shell should assume before it has a viewport to measure. +// Media queries, not a resize listener: `change` fires only when the +// answer changes, so a full-viewport drag produces two updates, not +// hundreds. import { useSyncExternalStore } from "react"; From baa17a1e71822d0a98612e3548cfabec36e87c5b Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:53:51 -0700 Subject: [PATCH 09/53] chore(web): shell context-menu comments say why, not what (CL-8378) --- apps/web/src/shell/context-menu/items.tsx | 14 ++++---------- .../shell/context-menu/lib/context-menu-view.tsx | 16 +++------------- .../src/shell/context-menu/lib/dialog-guard.ts | 14 +++----------- .../src/shell/context-menu/lib/focus-restore.ts | 7 ++----- apps/web/src/shell/context-menu/lib/index.ts | 11 ++--------- .../shell/context-menu/lib/target-resolver.ts | 11 ++--------- .../context-menu/lib/use-context-menu-state.ts | 5 +---- .../lib/use-document-context-menu-trigger.ts | 15 +++------------ apps/web/src/shell/context-menu/targets.ts | 14 ++++---------- 9 files changed, 24 insertions(+), 83 deletions(-) diff --git a/apps/web/src/shell/context-menu/items.tsx b/apps/web/src/shell/context-menu/items.tsx index 706cafea3..d45816c20 100644 --- a/apps/web/src/shell/context-menu/items.tsx +++ b/apps/web/src/shell/context-menu/items.tsx @@ -1,7 +1,5 @@ -// One item builder per target type. Every item here calls a real backend or -// a real, already-shipped shell affordance — nothing toast-only. A target -// whose mock counterpart had no working backend (mark-read, mute, archive, -// share) simply has no builder and so contributes no items. +// Nothing toast-only: a target with no working backend simply has no +// builder and contributes no items. import { profileSubjectFromParticipant } from "@/chat"; import type { ProfileSubject } from "@/chat"; @@ -173,12 +171,8 @@ function insightsRunMenu( }; } -/** - * Same operation set the Files bulk action bar offers: copy - * every acted-on file's canonical link. `target.ids` is either the single - * right-clicked row, or the whole active selection when the row is part of - * one — see `SHELL_CONTEXT_MENU_TARGETS`. - */ +// `target.ids` is either the single right-clicked row, or the whole active +// selection when the row is part of one. function artifactMenu(target: Extract): ContextMenu { const count = target.ids.length; return { diff --git a/apps/web/src/shell/context-menu/lib/context-menu-view.tsx b/apps/web/src/shell/context-menu/lib/context-menu-view.tsx index 155b3f6e1..f7e35b8f2 100644 --- a/apps/web/src/shell/context-menu/lib/context-menu-view.tsx +++ b/apps/web/src/shell/context-menu/lib/context-menu-view.tsx @@ -10,19 +10,9 @@ import { import { restoreFocus } from "./focus-restore"; import type { ContextMenu } from "./menu"; -/** - * Renders a `ContextMenu` at a fixed screen point using react-ui's - * `Menu` (Radix `DropdownMenu`) rather than a hand-rolled popover: Radix - * already owns anchored positioning that stays on screen, focus management, - * arrow-key navigation, and closing on Escape or an outside click, and it - * shares its dismissable-layer stack with react-ui's `Dialog` — the same - * stack the command palette's dialog uses — so a context menu never - * out-races a dialog on Escape. The only custom part is anchoring the - * (Radix-required) trigger to the click point instead of a visible button: - * that anchor is an inert, unfocusable point, so Radix's default - * close-focus-the-trigger behavior is overridden to focus `restoreFocusTo` - * (the row that was right-clicked) instead. - */ +// Uses react-ui's Radix-backed `Menu`, not a hand-rolled popover: it +// shares a dismissable-layer stack with the command palette's dialog, so +// a context menu never out-races it on Escape. export function ContextMenuView({ x, y, diff --git a/apps/web/src/shell/context-menu/lib/dialog-guard.ts b/apps/web/src/shell/context-menu/lib/dialog-guard.ts index 047cef436..1e310b786 100644 --- a/apps/web/src/shell/context-menu/lib/dialog-guard.ts +++ b/apps/web/src/shell/context-menu/lib/dialog-guard.ts @@ -1,14 +1,6 @@ -// A right-click that lands on (or behind) an open modal must not also pop -// a context menu — that's the "dialog beats context menu" half of the Esc -// precedence chain, enforced at open time rather than by racing keydown -// handlers. Every dialog in this codebase (react-ui's Dialog, and the -// command palette it's built from) renders Radix's `role="dialog"` while -// open and unmounts it on close, so presence in the DOM is exactly "is a -// modal open" — a stable ARIA contract, not an internal implementation -// detail. The `Escape`-closes-topmost-layer behavior itself needs no code -// here: Dialog and the context menu's own Menu are both Radix primitives -// sharing Radix's dismissable-layer stack, which already closes only the -// topmost one. +// Enforced at open time, not by racing keydown handlers: every dialog +// renders Radix's `role="dialog"` while open, so DOM presence is exactly +// "is a modal open." export function isBlockingOverlayOpen(doc?: Pick): boolean { const target = doc ?? globalThis.document; return target.querySelector('[role="dialog"]') !== null; diff --git a/apps/web/src/shell/context-menu/lib/focus-restore.ts b/apps/web/src/shell/context-menu/lib/focus-restore.ts index 7941dd92f..b9d4d561d 100644 --- a/apps/web/src/shell/context-menu/lib/focus-restore.ts +++ b/apps/web/src/shell/context-menu/lib/focus-restore.ts @@ -1,8 +1,5 @@ -// Radix's DropdownMenu restores focus to its trigger on close. Our trigger -// is an inert, unfocusable 1px anchor at the click point — restoring focus -// to it is a no-op that drops focus to ``. This module finds the real -// element worth focusing instead: the right-clicked row itself, or its -// nearest focusable ancestor. +// Our trigger is an inert 1px anchor, so Radix's default focus-restore +// drops to ``. This finds the real element worth focusing instead. const FOCUSABLE_SELECTOR = "a[href], button, input, select, textarea, [tabindex]"; diff --git a/apps/web/src/shell/context-menu/lib/index.ts b/apps/web/src/shell/context-menu/lib/index.ts index 1299b62c2..7a8d5b153 100644 --- a/apps/web/src/shell/context-menu/lib/index.ts +++ b/apps/web/src/shell/context-menu/lib/index.ts @@ -1,12 +1,5 @@ -// `@/shell/context-menu/lib`: the deployment-agnostic half of the global -// right-click context-menu system. Resolving a DOM event to a typed target -// (`target-resolver`), deciding whether a modal overlay should suppress it -// (`dialog-guard`), the open/position state machine -// (`use-context-menu-state`), the single document listener that drives it -// (`use-document-context-menu-trigger`), returning focus to the right-clicked -// row on close (`focus-restore`), and the react-ui-backed presentation -// (`context-menu-view`) all apply to any Interchange app with typed rows to -// right-click on. What counts as a target and which items it offers is +// The deployment-agnostic half of the global right-click context-menu +// system. What counts as a target and which items it offers is // product-specific and stays with the consumer. export { resolveTarget } from "./target-resolver"; diff --git a/apps/web/src/shell/context-menu/lib/target-resolver.ts b/apps/web/src/shell/context-menu/lib/target-resolver.ts index ad831a2bc..267475339 100644 --- a/apps/web/src/shell/context-menu/lib/target-resolver.ts +++ b/apps/web/src/shell/context-menu/lib/target-resolver.ts @@ -1,12 +1,5 @@ -// Turns the element a pointer event landed on into a typed target, the same -// way a router turns a path into a typed route: an ordered list of -// selector-scoped resolvers, each tried in turn against `origin.closest()` -// so a click anywhere inside a row still resolves to that row. The first -// *definition* to find any ancestor match wins — not the definition whose -// match is nearest — so a target nested inside another (e.g. a profile face -// inside a channel row) must be listed before its container, or the -// container's definition will win even though it matches farther away. -// Callers own what `T` is — this module only owns the walk. +// The first definition to find any ancestor match wins, not the nearest +// match — so a nested target must be listed before its container. export type TargetDefinition = { /** A CSS selector `Element.closest()` can match against. */ diff --git a/apps/web/src/shell/context-menu/lib/use-context-menu-state.ts b/apps/web/src/shell/context-menu/lib/use-context-menu-state.ts index ffaa54e29..c93ae53ef 100644 --- a/apps/web/src/shell/context-menu/lib/use-context-menu-state.ts +++ b/apps/web/src/shell/context-menu/lib/use-context-menu-state.ts @@ -14,10 +14,7 @@ export type ContextMenuState = { readonly hide: () => void; }; -/** Open/position state for a single context menu instance. Pure state — no - * DOM listeners here, so it composes with any trigger source (the document - * delegate in `use-document-context-menu-trigger`, or a row's own - * `onContextMenu`). */ +// Pure state, no DOM listeners, so it composes with any trigger source. export function useContextMenuState(): ContextMenuState { const [open, setOpen] = useState(false); const [position, setPosition] = useState({ x: 0, y: 0 }); diff --git a/apps/web/src/shell/context-menu/lib/use-document-context-menu-trigger.ts b/apps/web/src/shell/context-menu/lib/use-document-context-menu-trigger.ts index 4805c4f6f..519d542d1 100644 --- a/apps/web/src/shell/context-menu/lib/use-document-context-menu-trigger.ts +++ b/apps/web/src/shell/context-menu/lib/use-document-context-menu-trigger.ts @@ -13,18 +13,9 @@ export type ContextMenuTriggerOptions = { readonly onOpen: (x: number, y: number, menu: ContextMenu, origin: Element | null) => void; }; -/** - * Wires the single document-level `contextmenu` listener the whole app - * shares. A native right-click still wins over ours for text inputs, and an - * open dialog (including the command palette) always wins over opening a - * context menu underneath it — see `dialog-guard`. `resolve` returning an - * empty menu is the same as opting out: the native menu shows instead of an - * empty popover. - * - * Deliberately right-click only. This app's `Link` already gives Ctrl/Cmd - * click its native meaning (open in a new tab), so layering a second, - * conflicting meaning onto it here would cost more than it gives back. - */ +// An empty `resolve` result is the same as opting out: the native menu +// shows instead of an empty popover. Right-click only, deliberately — +// this app's `Link` already gives Ctrl/Cmd-click its native meaning. export function useDocumentContextMenuTrigger(options: ContextMenuTriggerOptions): void { const { resolve, onOpen } = options; useEffect(() => { diff --git a/apps/web/src/shell/context-menu/targets.ts b/apps/web/src/shell/context-menu/targets.ts index eb8bbd49a..ba0b6c078 100644 --- a/apps/web/src/shell/context-menu/targets.ts +++ b/apps/web/src/shell/context-menu/targets.ts @@ -23,12 +23,8 @@ export type ShellContextMenuTarget = | { readonly type: "artifact"; readonly id: string; - /** - * The ids this menu acts on: the row's own id alone, or — when the - * right-clicked row is part of a multi-row selection — every - * selected id, so a right-click inside an active selection offers - * the exact same operation set as the bulk action bar. - */ + // The row's own id alone, or every selected id when the row is part + // of a multi-row selection. readonly ids: readonly string[]; }; @@ -37,10 +33,8 @@ function attr(element: Element, name: string): string | null { return value === null || value === "" ? null : value; } -// Order matters: `resolveTarget` returns the first definition whose selector -// matches anywhere in the ancestor chain, not the nearest match overall — so -// a target nested inside another (the profile face inside a workbench row) -// must be listed before its container. +// Order matters: a nested target must be listed before its container, +// since `resolveTarget` returns the first match, not the nearest. export const SHELL_CONTEXT_MENU_TARGETS: readonly TargetDefinition[] = [ { selector: "[data-ctx-account]", From 1c3f683abd3a02fc1b7f84a32550124d92eda8c0 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:56:11 -0700 Subject: [PATCH 10/53] chore(web): settings comments say why, not what, part 1 (CL-8378) --- apps/web/src/settings-access.ts | 18 ++++---------- apps/web/src/settings-groups.tsx | 8 ++----- apps/web/src/settings/access.test.ts | 6 ++--- apps/web/src/settings/account-section.tsx | 18 ++++---------- apps/web/src/settings/credentials-section.tsx | 14 ++++------- apps/web/src/settings/grants-section.tsx | 7 ++---- apps/web/src/settings/identity.ts | 23 ++++-------------- .../src/settings/notifications-section.tsx | 16 +++---------- apps/web/src/settings/shell.tsx | 24 +++++-------------- 9 files changed, 33 insertions(+), 101 deletions(-) diff --git a/apps/web/src/settings-access.ts b/apps/web/src/settings-access.ts index e88249565..6476f1a8b 100644 --- a/apps/web/src/settings-access.ts +++ b/apps/web/src/settings-access.ts @@ -1,11 +1,6 @@ -// Settings section-nav gating, deduplicated: col2's nav band and the -// settings stage both mount independently and both need the same four -// tenancy probes (People/Roles/Grants/Credentials). Riding the app's shared -// QueryClient — instead of each mount calling `@/settings`'s bare -// `useTenancyAccess`, which fetches on every mount with no cache — means -// two mounted consumers share one in-flight request and one cached result. -// The package stays free of TanStack Query: this only injects the -// package's probe into the app's cache, the package never imports Query. +// Rides the app's shared QueryClient so col2's nav band and the settings +// stage share one in-flight request instead of each fetching on mount. +// The `@/settings` package itself stays free of TanStack Query. import { coalesceSectionAccess, probeSectionAccess } from "@/settings"; import type { TenancyAccess } from "@/settings"; @@ -33,11 +28,8 @@ function coalesceTenancyAccess( }; } -/** One shared probe per (tenant, principal), not one per mounted consumer. - * `null` ids report `loading` — the same "not shown yet, never disabled" - * contract `@/settings`'s own hook holds. A thrown evaluate - * (network/5xx) is `error`, not `denied`; a refetch failure keeps the - * last allow/deny so gated nav does not vanish as if unauthorized. */ +// A thrown evaluate is `error`, not `denied`; a refetch failure keeps the +// last allow/deny so gated nav doesn't vanish as if unauthorized. export function useSettingsAccess( tenantId: string | null, principalId: string | null, diff --git a/apps/web/src/settings-groups.tsx b/apps/web/src/settings-groups.tsx index 7371a36c3..b68cf924d 100644 --- a/apps/web/src/settings-groups.tsx +++ b/apps/web/src/settings-groups.tsx @@ -1,9 +1,5 @@ -// One resolver for the settings section groups this app actually shows — -// the package's own Account/Everyone registry, plus this app's one splice: -// Insights, pulled off the primary nav rail and reachable here instead. -// Both `settings-page.tsx` (stage) and `pages/settings-nav.tsx` call this -// rather than the package's bare `resolveSettingsSectionGroups` directly, -// so the two can never drift. +// Both the stage and the nav call this, not the package's bare resolver +// directly, so the two can never drift. import { ChartBar } from "@/lib/icons"; import { insertEveryoneSections, resolveSettingsSectionGroups } from "@/settings"; diff --git a/apps/web/src/settings/access.test.ts b/apps/web/src/settings/access.test.ts index 30f7c1a29..003128452 100644 --- a/apps/web/src/settings/access.test.ts +++ b/apps/web/src/settings/access.test.ts @@ -1,7 +1,5 @@ -// ACL evaluate probes decide whether People/Roles/Grants/Credentials belong -// in the settings nav. A 200 with effect !== allow is an authenticated deny; -// a thrown probe (network, 5xx) is not — it must not collapse to "denied" -// or those sections vanish as if the principal were unauthorized. +// A thrown probe (network, 5xx) must not collapse to "denied", or a +// section would vanish as if the principal were unauthorized. import { afterEach, describe, expect, test } from "bun:test"; diff --git a/apps/web/src/settings/account-section.tsx b/apps/web/src/settings/account-section.tsx index 218efef03..325404f7a 100644 --- a/apps/web/src/settings/account-section.tsx +++ b/apps/web/src/settings/account-section.tsx @@ -1,11 +1,6 @@ -// Personal Settings' opening section: a glanceable Account card (avatar, -// name, email — copyable — and Sign out), the same name/email/verified -// readout as before tucked below as a quieter subsection (still read-only — -// there is no native profile-update route; see `vendor/intx/hub-api/src/ -// routes` — only tenants and principals carry a PATCH), an Appearance card -// wired to `@corbits/react-ui`'s three-state ThemeProvider. The Agent card -// (a display-only Timezone row) was removed — nothing there could be -// changed until a hub preference store exists to write it to. +// Name/email readout is read-only: no native profile-update route exists. +// The Agent card was removed — nothing there could change until a hub +// preference store exists to write it to. import { Avatar, @@ -74,11 +69,8 @@ async function copyEmail(email: string): Promise { } } -/** - * The account panel's markup on its own, taking already-resolved display - * fields — kept separate from `AccountSection` for the same reason - * `BenchSectionView` is: directly renderable in tests without a fetch stub. - */ +// Kept separate from `AccountSection`, like `BenchSectionView`, so it's +// directly renderable in tests without a fetch stub. export function AccountSectionView({ id, name, diff --git a/apps/web/src/settings/credentials-section.tsx b/apps/web/src/settings/credentials-section.tsx index 96ce65a5c..8d78a616b 100644 --- a/apps/web/src/settings/credentials-section.tsx +++ b/apps/web/src/settings/credentials-section.tsx @@ -1,9 +1,5 @@ -// The "Credentials" settings section: a plain list-plus-create surface -// over Interchange's own stock credential routes -// (`vendor/intx/hub-api/src/routes/credentials.ts`) — the only way a key -// gets stored in this repo. No connector registry, no OAuth connect -// flow, no per-provider card: just the credentials this bench owns, -// named and typed, with a form to add one and a button to delete it. +// The stock credential routes are the only way a key gets stored in this +// repo — no connector registry, no OAuth flow, no per-provider card. import { Button, @@ -348,10 +344,8 @@ function readMetadataString(metadata: Credential["metadata"], key: string): stri return typeof value === "string" ? value : ""; } -/** Edits a credential through the stock `PATCH` route. `Base URL` and - * `Model` are this form's own metadata convention for a local, Ollama-style - * credential — they round-trip through the credential's opaque `metadata` - * field, nothing platform-specific. */ +// `Base URL`/`Model` are this form's own convention, round-tripped +// through the credential's opaque `metadata` field. function EditCredentialDialog({ credential, onOpenChange, diff --git a/apps/web/src/settings/grants-section.tsx b/apps/web/src/settings/grants-section.tsx index 8804d8c1c..eca875564 100644 --- a/apps/web/src/settings/grants-section.tsx +++ b/apps/web/src/settings/grants-section.tsx @@ -1,8 +1,5 @@ -// The "Grants" settings section: what each role or person is allowed, -// denied, or asked about, and on what — filterable, creatable, revocable -// over the native `/api/tenants/:tenantId/grants` route. The resource -// vocabulary has no listing endpoint of its own (see the tenancy -// inventory's gap list); `resource-vocabulary.ts` carries it as a constant. +// The resource vocabulary has no listing endpoint of its own; +// `resource-vocabulary.ts` carries it as a constant instead. import { Badge, diff --git a/apps/web/src/settings/identity.ts b/apps/web/src/settings/identity.ts index 7c7f00268..a9ecbb419 100644 --- a/apps/web/src/settings/identity.ts +++ b/apps/web/src/settings/identity.ts @@ -1,16 +1,6 @@ -// Pure helpers for the People section's one identity gap: `PrincipalResponse` -// gives agent principals no identity resolution, so `displayName` falls back -// server-side to the raw `refId` (see `resolveIdentities` in -// vendor/intx/hub-api/src/routes/principals.ts). The UI floor forbids ever -// showing that raw value as the label a person reads — this derives a -// humane stand-in from it and keeps the raw value available for a tooltip -// only, never as visible text. -// -// `PRINCIPAL_KIND_LABEL` and `PRINCIPAL_KIND_ORDER` live here too, shared by -// every picker that lists principals (Grants' target select and filter). -// Grants assign to people, agents, *and* workflows. Roles' assignment picker -// is scoped to user-kind principals only: agents/workflows are -// machine identities that belong on separate surfaces. +// An agent principal's `displayName` falls back server-side to a raw +// `refId`; the UI floor forbids showing that raw value, so this derives a +// humane stand-in and keeps the raw value for a tooltip only. import { SETTINGS_STRINGS } from "./strings"; @@ -29,12 +19,7 @@ function looksRaw(value: string): boolean { return RAW_LOOKING_PATTERN.test(value); } -/** - * Strips the plumbing off a raw agent address or id — scheme, host, - * underscore-prefixed type tags — down to whatever's left of a human name, - * title-cased. Falls back to a plain "Unnamed agent" when nothing - * recognizable survives. - */ +// Falls back to "Unnamed agent" when nothing recognizable survives. function derivePrincipalLabel(raw: string): string { const segment = raw diff --git a/apps/web/src/settings/notifications-section.tsx b/apps/web/src/settings/notifications-section.tsx index 797bc0565..cf44f0f98 100644 --- a/apps/web/src/settings/notifications-section.tsx +++ b/apps/web/src/settings/notifications-section.tsx @@ -1,16 +1,6 @@ -// Personal notification prefs. Delivery still routes through the product -// inbox / notify stack; these toggles are the operator-facing surface for -// which classes of activity should surface. No per-user preference store -// is mounted yet — draft state only, with an honesty note on save. -// -// REMOVED FROM THE SETTINGS REGISTRY (see section-registry.tsx): -// toggles that save nothing are fake controls, not a settings section — -// honest absence beats that. Re-add the "chat" section to -// `SETTINGS_SECTION_GROUPS`'s "account" group only once a per-user -// preference store exists for these fields and Save actually persists -// them, not before. (Workbench-scoped mute/mentions prefs live on -// `@/chat`'s workbench settings surface and already persist — -// that is a different surface.) +// Removed from the settings registry (see section-registry.tsx): toggles +// that save nothing are fake controls. Re-add once a per-user preference +// store exists and Save actually persists them. import { Button, SettingsPanel } from "@corbits/react-ui"; import { Check } from "@/lib/icons"; diff --git a/apps/web/src/settings/shell.tsx b/apps/web/src/settings/shell.tsx index f68bd4b43..b140533e2 100644 --- a/apps/web/src/settings/shell.tsx +++ b/apps/web/src/settings/shell.tsx @@ -10,10 +10,7 @@ import type { ReactElement } from "react"; import { SETTINGS_STRINGS } from "./strings"; -/** Whatever shared context a section needs to do its own fetching: the - * bench currently selected in the app's chrome, and the signed-in account's - * principal on that bench (for permission probes). A section with no use - * for either (Account, today) simply ignores the field. */ +// A section with no use for a field (Account, today) simply ignores it. export type SettingsContext = { readonly tenantId: string | null; readonly principalId: string | null; @@ -25,10 +22,8 @@ export type SettingsContext = { * `/settings/agents/:definitionId`), so a section with its own * master-detail can restore the right selection on a deep link. */ readonly entityId?: string | null; - /** Ends the signed-in session — the same callback the shell's account - * menu calls. Absent where the host has no sign-out concept of its own - * (a package test rendering a section standalone); a section that - * offers a Sign out action simply hides it when this is undefined. */ + // Absent where the host has no sign-out concept; a section with a + // Sign out action simply hides it when undefined. readonly onSignOut?: () => void; }; @@ -38,10 +33,7 @@ export type SettingsSection = { /** Leading icon for a host's own section nav (col2). */ readonly icon: Icon; readonly render: (ctx: SettingsContext) => ReactElement; - /** Tucks this section under a collapsed "Advanced" disclosure at the - * bottom of its group's nav, instead of listing it as a peer section — - * for sections whose mechanics (roles, grants, audit) nobody should have - * to parse just to find the thing they actually came for. */ + // Tucked under a collapsed "Advanced" disclosure, not a peer section. readonly advanced?: boolean; }; @@ -62,12 +54,8 @@ export function flattenSettingsSections( return groups.flatMap((group) => group.sections); } -/** - * The section a shell should treat as active: the requested id if it names - * a real section, otherwise the first section — never a crash, and never a - * blank nav. `sections` is validated non-empty by the caller; an empty - * registry is a distinct, deliberate empty state. - */ +// Falls back to the first section rather than crashing or showing a blank +// nav; an empty registry is a distinct, deliberate empty state. export function resolveActiveSection( sections: readonly SettingsSection[], requestedId: string | null, From 0259193f1c6519a6104406a43aeddb0d8c0fa680 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 12:59:41 -0700 Subject: [PATCH 11/53] chore(web): settings comments say why, not what, part 2 (CL-8378) --- apps/web/src/pages/settings-nav.tsx | 8 +-- apps/web/src/pages/settings-page.tsx | 19 ++---- apps/web/src/settings/access.ts | 12 +--- apps/web/src/settings/api-request.ts | 20 ++---- apps/web/src/settings/api.ts | 9 +-- apps/web/src/settings/credentials-api.ts | 7 +-- apps/web/src/settings/kind-cards.tsx | 6 +- apps/web/src/settings/models-section.tsx | 7 +-- apps/web/src/settings/people-section.tsx | 10 +-- apps/web/src/settings/resource-vocabulary.ts | 18 ++---- apps/web/src/settings/roles-section.tsx | 10 +-- apps/web/src/settings/section-registry.tsx | 64 ++++---------------- apps/web/src/settings/tenancy-api.ts | 19 ++---- docs/settings-sections.md | 17 ++++++ 14 files changed, 61 insertions(+), 165 deletions(-) create mode 100644 docs/settings-sections.md diff --git a/apps/web/src/pages/settings-nav.tsx b/apps/web/src/pages/settings-nav.tsx index 6747d9f47..bf5773de3 100644 --- a/apps/web/src/pages/settings-nav.tsx +++ b/apps/web/src/pages/settings-nav.tsx @@ -1,9 +1,5 @@ -// Settings section nav: the master list of the settings surface, rendered -// inside the stage beside the active section (master-detail — the list is -// never repeated in the section panel). Grouping, gating, and icons come -// from `@/settings`'s section registry -// (`resolveSettingsSectionGroups`); this component only adapts the app's -// scope context and router around it. +// Grouping, gating, and icons come from `@/settings`'s section registry; +// this component only adapts the app's scope context and router. import { SidebarItemRow } from "@corbits/react-ui"; import { SETTINGS_STRINGS } from "@/settings"; diff --git a/apps/web/src/pages/settings-page.tsx b/apps/web/src/pages/settings-page.tsx index 60cedc178..b31f9219c 100644 --- a/apps/web/src/pages/settings-page.tsx +++ b/apps/web/src/pages/settings-page.tsx @@ -1,11 +1,5 @@ -// Thin mount of `@/settings`'s shell: the package owns the section -// registry (Account / Everyone groups, icons, tenancy gates — see -// `resolveSettingsSectionGroups`); this file only adapts the app's -// bench-selection state (see ../bench-context.tsx) and the URL into the -// shape the package expects. `/settings` defaults to the first allowed -// section; `/settings/:section` deep-links directly to it. The section nav -// renders here in the stage, beside the active section (`settings-nav.tsx`) -// — master-detail, the list is never repeated in the section panel. +// Thin mount: this file only adapts the app's bench-selection state and +// the URL into the shape `@/settings`'s shell expects. import { flattenSettingsSections, resolveActiveSection, SettingsShell } from "@/settings"; import { PageShell } from "@corbits/react-ui"; @@ -41,13 +35,8 @@ export function SettingsRoute({ activeSection === undefined ? null : settingsEntityIdFromPath(path, activeSection.id); const requestedSectionExists = requestedId !== null && sections.some((section) => section.id === requestedId); - // A gated section (People/Roles/Grants/Credentials) is absent from - // `sections` while its probe is still resolving, same as when it's - // genuinely denied — wait for every gate to settle before treating a - // miss as final, or a deep link to an about-to-be-allowed section would - // bounce away before its probe finishes. `error` is settled: the - // registry withholds the section and the nav shows a couldn't-check - // state rather than pretending the principal is unauthorized. + // Wait for every gate to settle before treating a miss as final, or a + // deep link to an about-to-be-allowed section would bounce away early. const accessSettled = access.people !== "loading" && access.roles !== "loading" && diff --git a/apps/web/src/settings/access.ts b/apps/web/src/settings/access.ts index 5f48f8e2d..22979bff4 100644 --- a/apps/web/src/settings/access.ts +++ b/apps/web/src/settings/access.ts @@ -1,13 +1,5 @@ -// Whether the People/Roles/Grants/Credentials sections belong in the -// settings nav at all, decided the way the rest of this surface's og -// pages already gate access: never a disabled tab, just an absent one. -// There's no capability listing to read this off of, so this probes the -// one grant-checked route that requires no grant of its own — -// `evaluate` — for the resource each section is built on. -// A 200 whose effect is not `allow` is an authenticated deny. A thrown -// probe (network, 5xx) is `error`, never `denied` — collapsing those -// together made the gated nav vanish as if the principal were -// unauthorized. +// A thrown probe (network, 5xx) is `error`, never `denied` — collapsing +// those together made the gated nav vanish as if unauthorized. import { evaluate } from "./tenancy-api"; diff --git a/apps/web/src/settings/api-request.ts b/apps/web/src/settings/api-request.ts index 9edf4d232..3c50d667c 100644 --- a/apps/web/src/settings/api-request.ts +++ b/apps/web/src/settings/api-request.ts @@ -1,13 +1,6 @@ -// Shared fetch+parse wrapper for every settings-ui API seam -// (credentials-api.ts, access.ts, tenancy-api.ts): the same -// envelope-first error message on every -// non-2xx response, matching `apps/web/src/onboarding.ts`'s -// `readErrorEnvelope` — the hub's own `{error:{userMessage, refId}}` body -// wins when present, and the fallback names what was happening ("while -// loading credentials") rather than the raw route, which nobody reading a -// settings panel should ever have to see. Each seam keeps its own `Error` -// subclass so a catch site can still tell which API failed; only the -// request shape is shared here. +// Fallback message names what was happening, never the raw route, which +// nobody reading a settings panel should have to see. Each seam keeps its +// own `Error` subclass so a catch site can tell which API failed. import { type } from "arktype"; import type { ArkErrors } from "arktype"; @@ -16,11 +9,8 @@ const ErrorEnvelope = type({ error: { code: "string", userMessage: "string", refId: "string" }, }); -/** - * Resolves a non-2xx response's message: the hub's own envelope - * `userMessage` when the body carries one, otherwise a generic, path-free - * sentence naming the status and what the caller was doing. - */ +// The hub's own envelope `userMessage` wins when present, otherwise a +// generic, path-free sentence. export function readErrorEnvelope(status: number, body: unknown, verb: string): string { const envelope = ErrorEnvelope(body); return envelope instanceof type.errors diff --git a/apps/web/src/settings/api.ts b/apps/web/src/settings/api.ts index 9ecc63e51..9eacc2952 100644 --- a/apps/web/src/settings/api.ts +++ b/apps/web/src/settings/api.ts @@ -1,10 +1,5 @@ -// The settings surface's one seam of its own to Interchange's native hub -// routes: renaming a bench (`PATCH /api/tenants/:tenantId`, see -// `vendor/intx/hub-api/src/routes/tenants.ts`) and reading the signed-in -// account's profile (`GET /api/me`). Workbench settings and bench membership -// listings are not reimplemented here — they come straight from -// `@/chat` and `@/bench`, the packages that already own -// those seams. +// Workbench settings and bench membership listings aren't reimplemented +// here — they come straight from `@/chat` and `@/bench`. import { type } from "arktype"; import type { ArkErrors } from "arktype"; diff --git a/apps/web/src/settings/credentials-api.ts b/apps/web/src/settings/credentials-api.ts index 7c05e05f6..b5f2c0a64 100644 --- a/apps/web/src/settings/credentials-api.ts +++ b/apps/web/src/settings/credentials-api.ts @@ -50,11 +50,8 @@ export function listProviders(tenantId: string): Promise { ); } -/** Mints a plain, non-inference provider row named after the credential - * a person is about to store — the stock credentials route requires a - * `providerId`, and a bare "add a key" form has no provider of its own - * to point at yet. `plugin: "custom"` marks it as this form's own, - * generic kind rather than an inference adapter. */ +// The stock credentials route requires a `providerId`, and a bare "add a +// key" form has no provider of its own yet — this mints one. export function createProvider(tenantId: string, name: string): Promise { return request(`/api/tenants/${tenantId}/providers`, ProviderResponse, "creating that provider", { method: "POST", diff --git a/apps/web/src/settings/kind-cards.tsx b/apps/web/src/settings/kind-cards.tsx index 7cf295cae..2764ebe7a 100644 --- a/apps/web/src/settings/kind-cards.tsx +++ b/apps/web/src/settings/kind-cards.tsx @@ -1,7 +1,5 @@ -// Selectable card grid used by guided dialogs (grant effect, credential -// provider, workbench type, workbench kind). Mirrors the KindCardGrid shape -// from `@corbits/react-ui` so the dialogs keep working when the pin lags -// the component export — styling lives in styles.css. +// Mirrors react-ui's KindCardGrid shape so dialogs keep working when the +// pin lags the component export. import type { ReactNode } from "react"; diff --git a/apps/web/src/settings/models-section.tsx b/apps/web/src/settings/models-section.tsx index c165b7872..c6ae19836 100644 --- a/apps/web/src/settings/models-section.tsx +++ b/apps/web/src/settings/models-section.tsx @@ -1,9 +1,4 @@ -// The "Models" settings section: a read-only view of this workbench's -// credential providers (`GET /api/tenants/:t/providers`, the same stock -// route `credentials-section.tsx` reads) and its resolved model catalog -// (`GET /api/tenants/:t/models`, `createModelDiscoveryRoutes` — the same -// read `resolveModelSources` would act on at launch). No write path here; -// changing an offering's priority or restricting it belongs to the +// No write path here; changing an offering's priority belongs to the // catalog-management routes this section deliberately doesn't touch. import { diff --git a/apps/web/src/settings/people-section.tsx b/apps/web/src/settings/people-section.tsx index 93882f286..28c77e75f 100644 --- a/apps/web/src/settings/people-section.tsx +++ b/apps/web/src/settings/people-section.tsx @@ -1,11 +1,5 @@ -// The "People" settings section: every human (`kind: "user"`) principal on -// this bench, with suspend/reactivate/remove/role actions over the native -// `/api/tenants/:tenantId/principals` and `/roles` routes. Agent and -// workflow principals are machine identities, not people to manage here — -// Roles/Grants sections list every kind since those assign to machines -// too. Never renders a raw principal id or a raw agent refId — see -// `identity.ts`. New humans join only when an operator creates them -// through native APIs. +// Agent/workflow principals are machine identities, not people to manage +// here — Roles/Grants list every kind since those assign to machines too. import { Badge, diff --git a/apps/web/src/settings/resource-vocabulary.ts b/apps/web/src/settings/resource-vocabulary.ts index 8d411f7ed..5c2c5288a 100644 --- a/apps/web/src/settings/resource-vocabulary.ts +++ b/apps/web/src/settings/resource-vocabulary.ts @@ -1,9 +1,5 @@ -// The grant resource/action vocabulary: no hub route exposes this list (see -// the tenancy inventory's gap list, item 8), so the Grants section carries -// it here as a typed literal, sourced from -// `vendor/intx/hub-api/src/routes/grants.ts`'s own resource set. If the hub -// ever grows a capability-vocabulary endpoint, this constant is the one -// place to delete in favor of it. +// No hub route exposes this list, so it's carried here as a typed literal +// until a capability-vocabulary endpoint exists to delete this in favor of. export const GRANT_RESOURCES = [ "principal", @@ -29,14 +25,8 @@ export const GRANT_RESOURCES = [ ] as const; export type GrantResource = (typeof GRANT_RESOURCES)[number]; -/** - * Plain-language labels for `GRANT_RESOURCES`, written to read in a - * sentence ("Billing may read on {label}.") and in a table cell. Every - * consumer of a raw resource slug — the create-grant preview sentence, - * the grants table, the resource filter, the resource picker — reads - * from this one map so the copy never drifts between them. The raw slug - * still survives as a title/tooltip; it is never the visible text. - */ +// Every consumer of a raw resource slug reads from this one map so the +// copy never drifts between them. export const GRANT_RESOURCE_LABEL: Record = { principal: "accounts on this workbench", role: "roles", diff --git a/apps/web/src/settings/roles-section.tsx b/apps/web/src/settings/roles-section.tsx index d9b24a453..7c03a7043 100644 --- a/apps/web/src/settings/roles-section.tsx +++ b/apps/web/src/settings/roles-section.tsx @@ -1,7 +1,3 @@ -// The "Roles" settings section: this bench's roles (system roles marked and -// immutable), create/rename/delete for custom roles, and assigning or -// unassigning a role to a principal — all over the native -// `/api/tenants/:tenantId/roles` and `/principals/:id/roles/:id` routes. import { Badge, @@ -283,10 +279,8 @@ export function RoleAssignments({ const [principalId, setPrincipalId] = useState(""); const [roleId, setRoleId] = useState(""); - // Scope both picker and assignments to user-kind principals only. - // Agents and workflows are machine identities — the "Person" picker and - // its assignment table should match the People section's member roster, - // not the full tenant-wide principal list. + // The "Person" picker should match People's member roster, not the + // full tenant-wide principal list. const people = principals.filter((p) => p.kind === "user"); const assignments = people.flatMap((principal) => diff --git a/apps/web/src/settings/section-registry.tsx b/apps/web/src/settings/section-registry.tsx index 55cbfac67..658e2bfa0 100644 --- a/apps/web/src/settings/section-registry.tsx +++ b/apps/web/src/settings/section-registry.tsx @@ -1,22 +1,4 @@ -// The Personal Settings / Shared Settings section registry: the -// grouping, ordering, icons, and tenancy gates every Interchange -// deployment gets when it mounts this package's settings surface. The -// single-concept collapse folded Personal/Workspace into one account-scoped -// group and one shared group — there is one workbench per account now, so -// a "workspace-scoped" setting and an "account-scoped" one are the same -// tenant's settings. Shared Settings is the multiplayer-sharing surface: -// it leads with what everyone inherits (shared credentials, then -// People), and tucks the access-control mechanics (Roles, Grants, Audit) -// under a collapsed Advanced disclosure — nobody should have to parse -// grants and roles just to find where a shared API key lives. Bench dies -// outright — there is no longer a second thing to name, distinct from the -// account, so its rename/purpose/icon form and member list have no home to -// keep them separate in. Conversation-scoped settings (agent, capabilities, -// history) live on the workbench's own settings surface -// (`@/chat`'s `WorkbenchSettingsSurface`) — not here. -// Consuming apps compose bench context and routing around -// `resolveSettingsSectionGroups` — the domain model of "what settings -// exist and who can see them" lives here, not in an app. +// See docs/settings-sections.md for the grouping rationale. import { Cpu, Key, ListBullets, Shield, Star, User, Users } from "@/lib/icons"; @@ -47,12 +29,8 @@ const SETTINGS_SECTION_GROUPS: readonly SettingsSectionGroupDef[] = [ { id: "account", label: SETTINGS_STRINGS.groupAccountLabel, - // No "Your agent" section here: it has no preference store to back it. - // Re-add only once a hub preference store exists and save actually persists. - // No Notifications ("chat") section either: toggles with no per-user - // preference store are fake controls — see notifications-section.tsx - // for the re-add condition. Account (General) is the sole - // Account-group section until those stores exist. + // No "Your agent"/Notifications sections: no preference store backs + // them yet — see notifications-section.tsx for the re-add condition. sections: [ { id: "account", @@ -69,10 +47,8 @@ const SETTINGS_SECTION_GROUPS: readonly SettingsSectionGroupDef[] = [ label: SETTINGS_STRINGS.groupEveryoneLabel, sections: [ { - // Leads Shared Settings: a key added here is the thing everyone - // creating workbenches in this tenancy inherits. A plain list - // and create form over the stock credentials route — nothing - // provider-specific lives here. + // Leads Shared Settings: a key added here is what everyone + // creating workbenches in this tenancy inherits. id: "credentials", title: SETTINGS_STRINGS.credentialsSectionTitle, icon: Key, @@ -103,10 +79,8 @@ const SETTINGS_SECTION_GROUPS: readonly SettingsSectionGroupDef[] = [ render: (ctx) => , }, { - // Read-only: this workbench's providers and resolved model - // catalog, over the same stock routes credentials/inference - // already read. No gate — model:*/provider:* read is the same - // grant every member needs to chat at all. + // No gate — model:*/provider:* read is the same grant every + // member needs to chat at all. id: "models", title: SETTINGS_STRINGS.modelsSectionTitle, icon: Cpu, @@ -123,16 +97,9 @@ const SETTINGS_SECTION_GROUPS: readonly SettingsSectionGroupDef[] = [ }, ]; -/** - * The Personal Settings / Shared Settings groups, with a section dropped entirely — never - * rendered disabled — until its `access[gate]` probe resolves `allowed`. - * Loading and authenticated deny both withhold the section. A probe `error` - * withholds too (so gated sections never flash then hide) but marks the - * group `accessProbeFailed` so a host can show a couldn't-check state - * instead of looking like unauthorized. Both the settings stage and a - * host's own section nav (e.g. col2) should read from this single registry - * so they can never drift. - */ +// A section is dropped entirely, never rendered disabled, until its gate +// probe resolves `allowed`. A probe `error` withholds too but marks the +// group `accessProbeFailed` so a host can show a couldn't-check state. export function resolveSettingsSectionGroups( access: TenancyAccess, ): readonly SettingsSectionGroup[] { @@ -151,15 +118,8 @@ export function resolveSettingsSectionGroups( }); } -/** - * Splices host-supplied sections into the Everyone group, at its front — - * for domain sections that live outside this package (e.g. a host app's - * Agents/Skills directories) but still belong in the same account-wide - * nav. A host calling this must pass the same `extra` list to every - * consumer (settings stage and its own section nav / col2), the same - * discipline `resolveSettingsSectionGroups` itself documents, or the two - * surfaces drift. - */ +// A host calling this must pass the same `extra` list to every consumer +// (settings stage and its own section nav), or the two surfaces drift. export function insertEveryoneSections( groups: readonly SettingsSectionGroup[], extra: readonly SettingsSection[], diff --git a/apps/web/src/settings/tenancy-api.ts b/apps/web/src/settings/tenancy-api.ts index 50d9d63d4..553acf17f 100644 --- a/apps/web/src/settings/tenancy-api.ts +++ b/apps/web/src/settings/tenancy-api.ts @@ -1,8 +1,3 @@ -// The People/Roles/Grants sections' one seam to Interchange's native -// tenancy routes (see vendor/intx/hub-api/src/routes/{principals,roles, -// grants}.ts). Every fetch goes through a function here, and every response -// is parsed with an arktype schema from `@intx/types` at the boundary — the -// same convention `@/bench`'s api.ts already holds. import { type } from "arktype"; import type { ArkErrors } from "arktype"; @@ -93,10 +88,8 @@ export function removePrincipal(tenantId: string, principalId: string): Promise< export type InviteMemberInput = typeof InviteMember.infer; -/** Invites an existing platform user to this tenant by email over the - * stock `/members/invite` route — creates an `invited`-status principal - * and optionally assigns a role. The invited user must already have an - * account; there is no separate invite-by-email-only flow. */ +// The invited user must already have an account — no invite-by-email-only +// flow exists. export function inviteMember(tenantId: string, input: InviteMemberInput): Promise { return request(`/api/tenants/${tenantId}/members/invite`, PrincipalResponse, { method: "POST", @@ -204,12 +197,8 @@ export function revokeGrant(tenantId: string, grantId: string): Promise { }); } -/** - * Probes whether the signed-in principal can act on a resource — the one - * grant-checked route that itself requires no grant (see the tenancy - * inventory), so it doubles as the permission check that decides whether - * People/Roles/Grants show up in the settings nav at all. - */ +// The one grant-checked route that itself requires no grant, so it +// doubles as the check deciding whether People/Roles/Grants show up. export function evaluate( tenantId: string, principalId: string, diff --git a/docs/settings-sections.md b/docs/settings-sections.md new file mode 100644 index 000000000..3661d9bfe --- /dev/null +++ b/docs/settings-sections.md @@ -0,0 +1,17 @@ +# Settings section registry + +`apps/web/src/settings/section-registry.tsx` collapsed Personal/Workspace +into one account-scoped group and one shared group: there is one workbench +per account now, so "workspace-scoped" and "account-scoped" name the same +tenant's settings. + +Shared Settings leads with what everyone inherits (credentials, then +People) and tucks access-control mechanics (Roles, Grants, Audit) under a +collapsed Advanced disclosure — nobody should have to parse grants and +roles just to find where a shared API key lives. + +Bench dies outright: there's no longer a second thing to name, distinct +from the account, so its rename/purpose/icon form and member list have no +home to keep them separate in. Conversation-scoped settings (agent, +capabilities, history) live on the workbench's own settings surface +instead. From fda59b31681907feb57c3ae20466d208ad4e1b7e Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:01:17 -0700 Subject: [PATCH 12/53] chore(web): pages comments say why, not what, part 1 (CL-8378) --- apps/web/src/pages/chat-thread-page.tsx | 15 ++++----- apps/web/src/pages/diff-view.tsx | 8 ++--- .../web/src/pages/provisioning-error-page.tsx | 6 ++-- .../src/pages/run-status-tone-parity.test.ts | 21 +++---------- apps/web/src/pages/workbench-page.tsx | 31 ++++++------------- 5 files changed, 24 insertions(+), 57 deletions(-) diff --git a/apps/web/src/pages/chat-thread-page.tsx b/apps/web/src/pages/chat-thread-page.tsx index 38cbf5282..eacb4a5c9 100644 --- a/apps/web/src/pages/chat-thread-page.tsx +++ b/apps/web/src/pages/chat-thread-page.tsx @@ -171,10 +171,9 @@ function ChatTranscript({ // data, not a fetch of its own. if (chat !== undefined) markChatSeen(chatId, chat.lastMessageId); - // The inbox stream is the only signal that an agent has answered; it - // carries no chat identity, so it invalidates rather than patches. An - // agent that parks on an ask sends no mail, but the same stream ticks - // over its turn, so the approvals read is refreshed alongside. + // Invalidates rather than patches: the inbox stream carries no chat + // identity. Also refreshes approvals, since a parked ask sends no mail + // but still ticks the same stream. useEffect( () => subscribeToInbox(tenantId, () => { @@ -184,11 +183,9 @@ function ChatTranscript({ [tenantId, queryClient], ); - // Only this agent's asks belong in this transcript; the bench-wide list - // is filtered down to the run address this chat talks to. Polling only - // covers the same "agent still starting" window `chatQuery` polls for — - // once live, the inbox subscription's invalidation above is the only - // trigger, so a single mailbox event issues one read, not two. + // Polling only covers the same "agent still starting" window `chatQuery` + // polls for — once live, the inbox subscription's invalidation is the + // only trigger. const liveAddress = chat?.agent.liveAddress ?? null; const approvalsQuery = usePendingApprovals(tenantId, { refetchInterval: liveAddress === null ? 3000 : false, diff --git a/apps/web/src/pages/diff-view.tsx b/apps/web/src/pages/diff-view.tsx index 3cba1b817..d407824eb 100644 --- a/apps/web/src/pages/diff-view.tsx +++ b/apps/web/src/pages/diff-view.tsx @@ -1,9 +1,5 @@ -// One diff renderer for every surface that shows "what changed": the -// save-confirmation step and the version comparison on a detail page both -// mount this, so a diff always reads the same way. The line script comes -// from `@/lib/text-diff`; this file is only its presentation, and it -// computes the script exactly once per render — the change summary is read -// off the same result the rows come from. +// Computes the diff script exactly once per render — the change summary +// reads off the same result the rows come from. import { Badge, Table, TableBody, TableCell, TableRow } from "@corbits/react-ui"; import { diffText } from "@/lib/text-diff"; diff --git a/apps/web/src/pages/provisioning-error-page.tsx b/apps/web/src/pages/provisioning-error-page.tsx index 7d448618d..fd84c4bed 100644 --- a/apps/web/src/pages/provisioning-error-page.tsx +++ b/apps/web/src/pages/provisioning-error-page.tsx @@ -1,7 +1,5 @@ -// Shown when the first-login hook's provisioning call fails outright. -// A signed-in user with zero benches and a failed provisioning attempt has -// nothing useful to do anywhere else in the app, so this replaces the -// whole shell rather than a toast a user could miss or dismiss past. +// Replaces the whole shell, not a toast — a user with zero benches and a +// failed provisioning attempt has nothing useful to do elsewhere. import { Button, EmptyState, PageShell } from "@corbits/react-ui"; import { WarningCircle } from "@/lib/icons"; diff --git a/apps/web/src/pages/run-status-tone-parity.test.ts b/apps/web/src/pages/run-status-tone-parity.test.ts index 238319b5d..8f9394786 100644 --- a/apps/web/src/pages/run-status-tone-parity.test.ts +++ b/apps/web/src/pages/run-status-tone-parity.test.ts @@ -1,19 +1,8 @@ -// Guards against the exact bug this file was added for (design -// review): a page grows its own run-status → tone map/function instead of -// reading react-ui's `RUN_STATUS_TONE` (`workflow-run.ts`), and it quietly -// disagrees — a cancelled run reading neutral grey on Routines and amber -// warning on Insights, the same status meaning two different things in one -// product. `RUN_STATUS_TONE` is the one place a run-status tone is allowed -// to be decided; every surface below must normalize its own status -// vocabulary onto react-ui's `RunStatus` and read the tone from there. -// -// A static scan can't verify this: the divergence this catches was a -// `case "stopped": return "warning"` inside a switch, not an object -// literal, and re-deriving "is this tone value equal to canonical" from -// source text would mean parsing arbitrary JS — the general-purpose lint -// framework this ticket explicitly says not to build. Calling the real -// exported code and comparing its output to `RUN_STATUS_TONE` catches both -// shapes (switch or map) with no parser. +// Guards against a page growing its own run-status -> tone map that +// quietly disagrees with react-ui's `RUN_STATUS_TONE` — e.g. a cancelled +// run reading grey on Routines and amber on Insights. Calls the real +// exported code and compares output, since a static scan can't tell a +// switch's tone from an object literal's without parsing arbitrary JS. import { describe, expect, test } from "bun:test"; import { RUN_STATUS_TONE } from "@corbits/react-ui"; diff --git a/apps/web/src/pages/workbench-page.tsx b/apps/web/src/pages/workbench-page.tsx index 9128187e8..e6c956fca 100644 --- a/apps/web/src/pages/workbench-page.tsx +++ b/apps/web/src/pages/workbench-page.tsx @@ -1,10 +1,5 @@ -// A workbench is a child tenant, and its workbench is that tenant's mailbox: -// the timeline is the tenant's mail threads, participants are its -// principals, and a message is addressed to every agent in the workbench. The -// left info column folds in what Mission Control used to show for a -// bench overall — here scoped to this one workbench: latest activity, -// relevant artifacts, and pending approvals with approve/deny. Sub-threads -// are native in-reply-to chains, shown in a side panel. +// See docs/chat-mail-threading.md. The left info column folds in what +// Mission Control used to show, scoped to this one workbench. import { Button, @@ -137,10 +132,8 @@ function WorkbenchInfoColumn({ readonly latestMessage: WorkbenchMessage | undefined; readonly participants: readonly WorkbenchParticipant[]; }) { - // Poll only while an agent in this workbench is still starting (released, - // no live address yet) — the same window `participants` itself polls for. - // Once every agent is live, the inbox subscription's invalidation is the - // only trigger, so one mailbox event issues one read, not two. + // Poll only while an agent is still starting; once live, the inbox + // subscription's invalidation is the only trigger. const anyAgentStarting = participants.some((p) => p.kind === "agent" && p.address === ""); const approvalsQuery = usePendingApprovals(workbenchTenantId, { refetchInterval: anyAgentStarting ? 3000 : false, @@ -221,12 +214,8 @@ function WorkbenchInfoColumn({ ); } -/** Redeploys one released agent (asset present, no live run — a hub - * restart releases every process-provisioned deployment). Renders nothing; - * one instance per released agent, keyed by asset id, so a mount's own ref - * plus the mutation's `isPending`/`isSuccess` keep StrictMode's double - * render (and any refetch that finds the same agent still released) from - * firing it twice. */ +// Renders nothing; a mount's own ref plus the mutation's `isPending`/ +// `isSuccess` keep StrictMode's double render from firing it twice. function AgentRedeployer({ workbenchTenantId, agent, @@ -273,11 +262,9 @@ function Workbench({ workbenchTenantId }: { readonly workbenchTenantId: string } queryFn: () => readWorkbench(workbenchTenantId), }); - // The workbench mailbox stream is the only signal that an agent answered; it - // carries no thread identity, so it invalidates rather than patches. An - // agent that parks on an ask sends no mail, but the same stream ticks over - // its turn, so the approvals read invalidates here too instead of the info - // column polling it continuously while any agent is simply live. + // Invalidates rather than patches: the stream carries no thread + // identity. Also invalidates approvals, since a parked ask sends no + // mail but still ticks the stream. useEffect( () => subscribeToInbox(workbenchTenantId, () => { From 96852ad332f380019a66b1e27e83f2905c6ee31f Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:03:32 -0700 Subject: [PATCH 13/53] chore(web): pages comments say why, not what, part 2 (CL-8378) --- apps/web/src/pages/agents-page.tsx | 18 +++------ apps/web/src/pages/create-skill-dialog.tsx | 14 ++----- .../src/pages/insights-page-render.test.tsx | 18 ++------- apps/web/src/pages/onboarding-page.tsx | 40 ++++++------------- apps/web/src/pages/skills-page.tsx | 36 ++++------------- apps/web/src/pages/tools-page.tsx | 12 ++---- 6 files changed, 35 insertions(+), 103 deletions(-) diff --git a/apps/web/src/pages/agents-page.tsx b/apps/web/src/pages/agents-page.tsx index e7147e481..d4be89a27 100644 --- a/apps/web/src/pages/agents-page.tsx +++ b/apps/web/src/pages/agents-page.tsx @@ -1,9 +1,5 @@ -// Agents: a roster and nothing else — display name, live/starting status, -// and a Chat action per agent. Everything this page used to carry (a -// detail panel, run health/events/approvals, bulk archive) leaned on hub -// routes or state no longer worth a whole screen; a chat with the agent is -// the one action anyone actually used. Create-agent lives in the sidebar's -// "+" menu, not here. +// A roster and nothing else — a chat with the agent is the one action +// anyone actually used. Create-agent lives in the sidebar's "+" menu. import { Badge, @@ -32,13 +28,9 @@ import { Link } from "../navigation"; import { useTenantQuery } from "../routines-api"; import { StageTopBar } from "../shell/stage-top-bar"; -/** - * One agent's roster row: `Live` once it has a live run address, `starting` - * while a deploy is still landing one (mid-first-deploy or mid-redeploy), - * `not-running` once its latest deployment has gone terminal (a hub - * restart releases every prior allocation) — a restart is then the only - * way forward. - */ +// `not-running` once the latest deployment has gone terminal (a hub +// restart releases every prior allocation) — restart is the only way +// forward. export function agentRosterStatus( agent: Pick, ): "live" | "starting" | "not-running" { diff --git a/apps/web/src/pages/create-skill-dialog.tsx b/apps/web/src/pages/create-skill-dialog.tsx index b114d566c..da9690602 100644 --- a/apps/web/src/pages/create-skill-dialog.tsx +++ b/apps/web/src/pages/create-skill-dialog.tsx @@ -1,14 +1,6 @@ -// The create-skill dialog. -// -// this used to hand a full SKILL.md (name/description/body, or -// an uploaded file) to the workbench's own skill registry, which parsed -// and stored it. That registry is gone — skills are native `kind:"skill"` -// hub assets now, and the stock asset routes (`@intx/hub-api`'s -// `routes/assets.ts`) accept only a bare `{ kind, name, displayName }` on -// create: there is no stock route yet to write a skill's SKILL.md content -// in the same call. This dialog is scoped down to match: it names the -// asset only, and hands off; writing the skill's actual instructions -// happens through whatever surface eventually covers skill content. +// Scoped down to naming the asset only: the stock asset routes accept a +// bare `{ kind, name, displayName }`, with no stock route yet to write a +// skill's SKILL.md content in the same call. import { Button, Dialog, diff --git a/apps/web/src/pages/insights-page-render.test.tsx b/apps/web/src/pages/insights-page-render.test.tsx index db05feb8f..30b3a9828 100644 --- a/apps/web/src/pages/insights-page-render.test.tsx +++ b/apps/web/src/pages/insights-page-render.test.tsx @@ -1,12 +1,5 @@ -// A malformed percent-escape on an Insights deep link (`/insights/runs/%`) -// must render the same landing dashboard any other unrecognized Insights -// path gets — never a blank page (see `insights-path.ts`'s -// `parseInsightsPath`, which InsightsPage calls with the exact same `path` -// prop this test passes). deleted the usage/activity/tools/latency/ -// scope-switcher props `InsightsPage` used to take along with -// `packages/insights` itself — `InsightsWorkbenchPage`'s workbench-scoped -// mode now lives entirely in `InsightsRoute`, so a `/insights/workbench/...` -// path is out of scope for this component-level test. +// A malformed percent-escape on a deep link must render the same landing +// dashboard any other unrecognized path gets — never a blank page. import { afterEach, describe, expect, test } from "bun:test"; import { act } from "react"; @@ -112,11 +105,8 @@ describe("InsightsPage 'Running now' strip", () => { expect(el.textContent).toContain("Weekly digest"); }); - // Liveness is not a windowed property: a run that started long ago and is - // still running must not disappear from the strip or read 0 in the - // "Running now" KPI just because it started long before this page's - // recent-runs slice. Persist has not settled (`endedAt` absent), so the - // fire is live — not remapped to completed by the abandoned-fire window. + // Liveness is not a windowed property: a run started long ago must not + // disappear from the strip or the "Running now" KPI. test("a run started 8 days ago that is still running stays in the strip and the KPI", () => { const eightDaysAgo = new Date(Date.now() - 8 * 24 * 60 * 60 * 1000).toISOString(); const el = render("/insights", { diff --git a/apps/web/src/pages/onboarding-page.tsx b/apps/web/src/pages/onboarding-page.tsx index 112bd7248..0e54c30ed 100644 --- a/apps/web/src/pages/onboarding-page.tsx +++ b/apps/web/src/pages/onboarding-page.tsx @@ -1,23 +1,14 @@ -// The setup gate: the screen a signed-in -// session lands on when the hub reports setup-required. It reads the -// hub's native setup-status route, and a hub that already has tenants -// bounces straight into the shell (`/`). An empty hub drives the -// browser installer itself, as one converge loop over stock routes, -// asking the operator only where a human input is genuinely required: +// The setup gate: a hub with tenants bounces straight to the shell; an +// empty hub drives the browser installer itself, asking the operator only +// where a human input is genuinely required (see steps below). // -// 1. mint the account's primary tenant (stock `POST /api/tenants`) if -// it does not already own one; -// 2. resolve a catalog offering to deploy Myra against — if the -// tenant already resolves one (inherited, or a previous run of -// this step), skip straight past; otherwise ask the operator to -// connect exactly one provider credential (`ProviderConnectStep`); -// 3. push Myra's source tree and build its deploy input for that offering -// (`deployMyraSource`) and hand it to `bootstrapClientSession` as -// `myraDeploy`. +// 1. mint the account's primary tenant if it doesn't already own one; +// 2. resolve a catalog offering, asking to connect a provider credential +// only if none is already resolved; +// 3. push Myra's source tree and deploy it. // -// A converged install lands on `/`; a stock capability gap this loop did -// not anticipate, or a hard failure at any step, renders here with a -// retry — a gap is not "ready". +// A stock capability gap this loop didn't anticipate, or a hard failure, +// renders here with a retry — a gap is not "ready". import { Button, EmptyState } from "@corbits/react-ui"; import { WarningCircle } from "@/lib/icons"; import { WorkbenchLoadingState } from "@/chat"; @@ -86,11 +77,8 @@ export function OnboardingPage({ user }: { readonly user: SessionUser }) { checkStatus(); }, [checkStatus]); - // Step 1: mint the primary tenant if this account does not already - // own one, then move to the offering-resolution step. Stays a - // separate phase from "installing" (which still runs the full - // `bootstrapClientSession` converge for the rest of the needs list) - // because a credential connect needs a tenant id to write against. + // A separate phase from "installing" because a credential connect + // needs a tenant id to write against. useEffect(() => { if (state.phase !== "resolving-tenant") return; let cancelled = false; @@ -144,10 +132,8 @@ export function OnboardingPage({ user }: { readonly user: SessionUser }) { // out of this effect's dependency list. }, [state.phase, user]); - // Step 3: publish Myra's deploy input for the resolved offering, then - // hand off to the installing phase. Shared by both the - // already-resolved-offering path (above) and the operator-connected - // path (`ProviderConnectStep`'s `onConnected` below). + // Shared by both the already-resolved-offering path and the + // operator-connected path. function publishAndInstall( tenantId: string, tenantDomain: string, diff --git a/apps/web/src/pages/skills-page.tsx b/apps/web/src/pages/skills-page.tsx index 0f0cdeeb7..e330625d1 100644 --- a/apps/web/src/pages/skills-page.tsx +++ b/apps/web/src/pages/skills-page.tsx @@ -1,19 +1,8 @@ -// Skills: a standalone rail destination, over the workbench's -// real skill registry (`@corbits/skills`, via `../skills-api.ts`). Used to -// be a Settings section; the owner moved it back out to its own -// page — this is the only surface left, there is no Settings duplicate. -// This replaced the session-local store shipped before that: a -// skill now lives in a native `kind:"skill"` hub asset the moment it is -// created, and its version history is that asset's git history. +// A skill lives in a native `kind:"skill"` hub asset the moment it's +// created; its version history is that asset's git history. // -// Two states a skill can be in, both visible in the "Who can see it" -// column: -// private — visible only to the person who wrote it (the default) -// shared — visible to the whole workbench -// -// There is no external catalog: skills are authored in this workbench. -// This page lists them; a single skill — its editor, its versions, its -// visibility toggle — lives on its own page (`skill-detail-page.tsx`). +// Two states visible in "Who can see it": private (default) or shared +// with the whole workbench. No external catalog — skills are authored here. import { PageShell, @@ -55,14 +44,8 @@ function messageOf(cause: unknown): string { return cause instanceof Error ? cause.message : String(cause); } -/** - * The Skills roster over one workbench's skill registry, with its own - * top-nav contract: the trail says where the reader is and the - * top bar's action slot is the only home for "New skill". `tenantId` is the - * registry every read is scoped to; opening a row navigates to that skill's - * own page at `/skills/`, which is where editing, versions, - * and diffs live — this page never renders a skill inline. - */ +// Opening a row navigates to `/skills/`: editing, versions, and +// diffs live there, never rendered inline here. export function SkillsPage({ tenantId, navigate, @@ -238,12 +221,7 @@ export function SkillsPage({ ); } -/** - * Skills roster mount at `/skills`: a thin adapter that resolves - * which workbench's registry is listed. The stage chrome (breadcrumb trail, - * action slot) belongs to `SkillsPage`; a single skill has its own route - * (`/skills/`, `skill-detail-page.tsx`). - */ +// A thin adapter that resolves which workbench's registry is listed. export function SkillsRoute({ navigate }: { readonly navigate: (to: string) => void }) { const { selectedTenantId } = useBench(); diff --git a/apps/web/src/pages/tools-page.tsx b/apps/web/src/pages/tools-page.tsx index 923a822bf..1b85f1ee5 100644 --- a/apps/web/src/pages/tools-page.tsx +++ b/apps/web/src/pages/tools-page.tsx @@ -1,8 +1,5 @@ -// Tools: a standalone rail destination listing the tool packages the -// tenant's live agent deployments actually carry — read off each agent's -// deployed `definition.json` (plus Myra's bundled mail/posix, which never -// lands in that file). No stock route lists a tenant's MCP servers yet, so -// this page has nothing to show for those until one exists. +// No stock route lists a tenant's MCP servers yet, so this page has +// nothing to show for those until one exists. import { PageShell, @@ -21,10 +18,7 @@ import { useDeployedToolPackages } from "../tools/deployed-tool-packages"; import { useBench } from "../bench-context"; import { StageTopBar } from "../shell/stage-top-bar"; -/** - * The tool packages the tenant's live agent deployments carry. - * `tenantId` is the tenant every read is scoped to. - */ +// `tenantId` is the tenant every read is scoped to. export function ToolsPage({ tenantId }: { readonly tenantId: string | null }) { const query = useDeployedToolPackages(tenantId); const crumbs = [{ label: "Tools" }]; From 4fdfb858114ad37633e7ab3ae2d5523fd2c81825 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:04:45 -0700 Subject: [PATCH 14/53] chore(web): insights page comments say why, not what (CL-8378) --- apps/web/src/pages/insights-page.test.ts | 6 +-- apps/web/src/pages/insights-page.tsx | 59 ++++++------------------ 2 files changed, 17 insertions(+), 48 deletions(-) diff --git a/apps/web/src/pages/insights-page.test.ts b/apps/web/src/pages/insights-page.test.ts index c1eac7a18..7f133a15a 100644 --- a/apps/web/src/pages/insights-page.test.ts +++ b/apps/web/src/pages/insights-page.test.ts @@ -66,10 +66,8 @@ describe("isRunningNow", () => { ).toBe(false); }); - // Warm-keep: a routine's delivery agent stays deployed - // after it replies, so workflow_run.status lingers on `running`. Past the - // fire window that is not an in-flight job — Insights must not keep it in - // "Running now" forever. + // Warm-keep: status lingers on `running` after a reply, but past the + // fire window it's not in-flight — must not stay "Running now" forever. test("endedAt drops in-flight immediately, even while status is still running inside the window", () => { expect( isRunningNow( diff --git a/apps/web/src/pages/insights-page.tsx b/apps/web/src/pages/insights-page.tsx index 8d5308d9b..0812d4055 100644 --- a/apps/web/src/pages/insights-page.tsx +++ b/apps/web/src/pages/insights-page.tsx @@ -1,11 +1,5 @@ -// Insights over the native `GET /workflows/runs` top-level listing: KPI -// row, "running now" strip, recent purpose runs, and runs history grouped -// by definition. The stock observability routes -// (`vendor/intx/hub-api/src/routes/observability.ts`'s four GET routes) -// are unimplemented stubs — each returns 501 — so this page is entirely -// native `WorkflowRunResponse` data, already fetched the same way -// `mission-control-page.tsx` and `routines-api.ts` read runs elsewhere in -// this app. +// The stock observability routes are unimplemented 501 stubs, so this +// page is entirely native `WorkflowRunResponse` data. import { Badge, @@ -76,11 +70,8 @@ export function formatWhen(iso: string): string { }); } -/** A platform workflow run's status (`WorkflowRunStatus`) doesn't spell - * react-ui's `RunStatus` vocabulary the same way — normalize onto it here - * so the badge tone always comes from `RUN_STATUS_TONE`, the one source - * every run-status tone reads from, rather than a second opinion invented - * on this page. */ +// Normalizes onto react-ui's `RunStatus` vocabulary so the tone always +// comes from `RUN_STATUS_TONE`, never a second opinion invented here. const WORKFLOW_RUN_STATUS_ALIAS: Readonly> = { deployed: "completed", running: "running", @@ -174,10 +165,8 @@ function runsDetailLabel(stats: { readonly running: number; readonly errored: nu const ELAPSED_TICK_MS = 1_000; -/** Ticks once a second while `enabled` — the clock the elapsed label next to - * the pulsing `StatusDot` reads from, so it counts up like the live indicator - * beside it instead of freezing at whatever instant this component mounted - * or last re-rendered for an unrelated reason. */ +// So the elapsed label counts up like the live indicator, instead of +// freezing at whatever instant this component last rendered. function useTickingNow(enabled: boolean): number { const [now, setNow] = useState(() => Date.now()); useEffect(() => { @@ -188,13 +177,9 @@ function useTickingNow(enabled: boolean): number { return now; } -/** A run actually in flight right now (`status: running | updating`) — - * liveness is not a windowed property, so this filters the full run set, - * never the range-filtered one. A persisted `endedAt` means the fire - * already finished, even if `status` still reads `running`. A live fire - * with an in-flight turn stays in flight however old it is; without an - * explicit no-in-flight signal, missing `turns` is not abandonment. - */ +// Liveness is not a windowed property, so this filters the full run set, +// never the range-filtered one. A persisted `endedAt` means the fire +// already finished, even if `status` still reads `running`. export function isRunningNow(run: InsightsRun, now: number = Date.now()): boolean { const outcome = runOutcomeStatus(withListingAbandoned(run, now), now); return outcome === "running" || outcome === "updating"; @@ -204,22 +189,16 @@ function insightsRunStatus(run: InsightsRun, now: number = Date.now()): string { return runOutcomeStatus(withListingAbandoned(run, now), now) ?? run.status; } -/** Wall-clock time since a run started, in the same "2m 12s" form as the - * rest of this page (`durationLabel`) — never a fabricated live counter. */ +// Wall-clock time since start, never a fabricated live counter. export function elapsedLabel(createdAt: string, now: number): string { const startMs = Date.parse(createdAt); if (Number.isNaN(startMs)) return "—"; return durationLabel(Math.max(0, now - startMs)); } -/** - * "Running now" — a horizontally scrolling strip of the runs actually in - * flight this instant (`status: running | updating`), not a fabricated - * live-metrics ticker. Renders nothing when nothing is running, same - * convention as react-ui's `WorkflowDock`: an empty "nothing running" strip - * is a permanent fixture reporting the normal case, not an empty state worth - * showing. - */ +// Renders nothing when nothing is running — same convention as react-ui's +// `WorkflowDock`: an empty strip reports the normal case, not an empty +// state worth showing. function RunningNowStrip({ runs, onOpenRun, @@ -700,16 +679,8 @@ export function InsightsPage({ ); } -/** - * Insights scoped to one workbench — `/insights/workbench/:workbenchId` - * resolves the workbench's own workbench tenant (see - * `../insights-workbench-scope.ts`) and titles the page by the WORKBENCH name, - * never the tenant's. A true legacy workbench (tenancy `null`) and an id - * absent from the bench's own workbench list (a stale - * `/insights/workbench/:tenantId` link, or any other mis-wired id — that - * route is retired) both get an honest empty state instead of a doomed - * tenant-scoped fetch. - */ +// Titles the page by the workbench name, never the tenant's. A legacy or +// mis-wired id gets an honest empty state instead of a doomed fetch. function InsightsWorkbenchPage({ workbenchesLoading, resolution, From 2cdec66e5ae00e4deb81d0aea892e5bca50f6741 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:06:39 -0700 Subject: [PATCH 15/53] chore(web): skill/library page comments say why, not what (CL-8378) --- apps/web/src/pages/library-page.tsx | 88 +++++++----------------- apps/web/src/pages/skill-detail-page.tsx | 17 +---- 2 files changed, 28 insertions(+), 77 deletions(-) diff --git a/apps/web/src/pages/library-page.tsx b/apps/web/src/pages/library-page.tsx index 376ae469a..3d20ad912 100644 --- a/apps/web/src/pages/library-page.tsx +++ b/apps/web/src/pages/library-page.tsx @@ -91,10 +91,8 @@ function ArtifactRows({ const allSelected = artifacts.length > 0 && selection.selectedCount === artifacts.length; const headerChecked: SelectionCheckboxState = selection.selectedCount === 0 ? false : allSelected ? true : "indeterminate"; - // `useListSelection` hands back ids in toggle/insertion order, not row - // order — a bottom-up shift-select would otherwise join/copy links out of - // visible order. Sort against this row order before handing ids to any - // bulk operation (copy links here, the context menu's `ids` below). + // `useListSelection` hands back ids in toggle order, not row order — a + // bottom-up shift-select would otherwise copy links out of visible order. const visibleOrder = useMemo( () => new Map(artifacts.map((artifact, index) => [artifact.id, index])), [artifacts], @@ -171,13 +169,8 @@ function ArtifactRows({ ); } -/** - * The one cheap provenance fact worth surfacing: a link to the - * workflow run that produced this artifact, when `source` says so - * (`workflowRunIdFromSource`). Not a lineage system — every other origin - * (manual, agent, imported, unknown) renders nothing here rather than - * guessing. - */ +// Not a lineage system — every other origin renders nothing rather than +// guessing. function ProvenanceLine({ source }: { readonly source: Record }) { const runId = workflowRunIdFromSource(source); if (runId === null) return null; @@ -211,12 +204,9 @@ function PreviewPane({ detail !== null && rendererKind === "html" && tenantId !== null ? artifactPreviewPath(tenantId, detail.id) : undefined; - // Empty `content` on a file artifact is ambiguous on its own: it's the - // honest "nothing here" for an inline-content artifact, but it's also - // what a real upload's row carries when its bytes live out-of-band and - // aren't text-decodable (an image, a real PDF, a legacy `.docx`/`.xlsx`). - // `source.upload.mimeType` disambiguates — present only when this - // artifact really does have stored bytes behind it. + // Empty `content` is ambiguous alone (honest "nothing here" vs. a real + // upload whose bytes aren't text-decodable); `uploadMimeType` + // disambiguates. const uploadMimeType = detail !== null ? uploadMimeTypeFromSource(detail.source) : null; const contentUnavailable = detail !== null && @@ -277,18 +267,8 @@ function PreviewPane({ ); } -/** - * The Artifacts stage: a row list of everything this workbench owns, with an - * in-stage preview when a row is selected. Real data only. - * - * Every control the page owns — the workbench lens, the name filter, sort, - * the rows/grid toggle, Upload — lives in `StageTopBar`'s action slot - * (DESIGN.md → Pages & Routing: the top nav owns the page's actions, and a - * page body never floats its own). The name filter drives the stage top - * bar's own magnifier (`filter` prop) rather than a second input — the - * magnifier IS this page's filter, never the global palette (DECISIONS.md - * → Search). - */ +// Every control lives in `StageTopBar`'s action slot (DESIGN.md -> Pages +// & Routing) — a page body never floats its own. export function LibraryPage({ artifacts, now, @@ -354,18 +334,12 @@ export function LibraryPage({ ); const visibleIds = useMemo(() => visible.map((artifact) => artifact.id), [visible]); - // A row filtered out of `visibleIds` drops out of `selection.selectedIds` - // immediately (the hook reconciles against `ids` on every read) but - // `useListSelection` keeps it in its own internal state, so the row comes - // back selected if the filter that hid it is cleared. Deliberate: it - // matches Finder/Sheets ("clearing a filter doesn't lose your picks") and - // needs no bookkeeping here. + // Deliberate: matches Finder/Sheets — clearing a filter doesn't lose + // your picks, since `useListSelection` keeps them in internal state. const selection = useListSelection({ ids: visibleIds }); - // Rows and cards render selection differently — only rows has checkboxes - // — so a selection made in one view has nothing to anchor to in the - // other. Clearing on view change is simpler than teaching the card view - // its own checkboxes for a selection UI it doesn't otherwise need. + // Only rows has checkboxes, so a selection has nothing to anchor to in + // cards — clearing on view change is simpler than adding card checkboxes. const [selectionViewMode, setSelectionViewMode] = useState(viewMode); if (selectionViewMode !== viewMode) { setSelectionViewMode(viewMode); @@ -419,21 +393,15 @@ export function LibraryPage({ actions={ <> {selectedSummary !== null ? ( - // This clears the open file and returns to the list — an - // action, not a filter. It used to say bare "All", which read - // as a third option in the scope group right beside it ("All" - // vs. "All workbenches"); this label can't be mistaken for - // that. + // An action, not a filter. Used to say bare "All", which read + // as a third scope option next to "All workbenches". ) : null} {workbenchScope !== null && onScopeChange !== undefined ? ( - // One control, two states — answers exactly one question - // ("whose files"). At lg the bordered segmented group matches - // `ViewToggle` in this bar; below lg that group is hidden and - // the overflow menu in this same slot is the way to reach - // All workbenches. + // Below lg the segmented group is hidden; the overflow menu + // in this same slot is the way to reach All workbenches. <>
(null); const [searchQuery, setSearchQuery] = useState(""); - // `/artifacts/a/:id` — a chat artifact chip's "Open in Artifacts" - // deep link, distinct from the kind-nav segments below. It only ever - // sets the initial selection; the user's own clicks stay local state, - // the same way kind-nav selection already worked before this route - // existed. + // Only sets the initial selection; the user's own clicks stay local + // state, as kind-nav selection already worked before this route existed. const deepLinkedArtifactId = libraryArtifactIdFromPath(path); const [selectedId, setSelectedId] = useState(deepLinkedArtifactId); const [appliedDeepLink, setAppliedDeepLink] = useState(deepLinkedArtifactId); @@ -626,10 +591,9 @@ export function LibraryRoute({ path }: { readonly path: string }) { } const kindSegment = deepLinkedArtifactId === null ? libraryKindSegmentFromPath(path) : ""; - // Artifacts' workbench-first lens: the workbench the person just - // came from, if `last-workbench.ts` recorded one for this bench, resolved - // to its own tenant via the same sidebar-backed activity listing every - // other bench-scoped surface already fetches. + // The workbench the person just came from, if `last-workbench.ts` + // recorded one — resolved via the same activity listing other + // bench-scoped surfaces already fetch. const activity = useBenchActivity(selectedTenantId); const lastWorkbenchId = selectedTenantId === null ? null : readLastWorkbenchId(selectedTenantId); const workbenchScope = @@ -750,11 +714,9 @@ export function LibraryRoute({ path }: { readonly path: string }) { await queryClient.invalidateQueries({ queryKey: tenantKeys.artifacts(selectedTenantId), }); - // The confirmation names what the server actually stored - // (its own titles), never the local `File` picked — the - // two can differ (e.g. a collision rename), and a sibling - // fix for empty content read-back means this toast must - // only ever repeat the upload response, not assume it. + // Names what the server actually stored, never the local + // `File` picked — the two can differ (e.g. a collision + // rename). toast(artifactUploadToast(uploaded.map((artifact) => artifact.title))); } catch (err) { setUploadError(describeApiError(err, "uploading those files")); diff --git a/apps/web/src/pages/skill-detail-page.tsx b/apps/web/src/pages/skill-detail-page.tsx index 58410c2b4..a576d5cae 100644 --- a/apps/web/src/pages/skill-detail-page.tsx +++ b/apps/web/src/pages/skill-detail-page.tsx @@ -1,17 +1,6 @@ -// The skill detail page at `/skills/`. -// -// this used to be a full editor over `@corbits/skills`' own registry — -// description/body editing with diff review, restore-by-version off that -// asset's git history, a "pinned by" list, and a private/shared visibility -// toggle. That registry (and the workflow routes it served) was deleted: -// skills are native `kind:"skill"` hub assets now, and the stock asset -// routes (`@intx/hub-api`'s `routes/assets.ts`) carry only metadata — id, -// name, displayName, creator, timestamps. There is still no stock route -// for a skill's version history, pinned-by list, or scope/visibility -// flag, so those stay out of this page. Its `SKILL.md` content, though, -// is readable and writable the same way agent source is: over the -// asset's own smart-HTTP git remote with a short-lived token (see -// `skill-source.ts`). +// No stock route for version history, pinned-by list, or visibility flag, +// so those stay out of this page. SKILL.md content, though, is readable +// and writable the same way agent source is (see `skill-source.ts`). import { Button, PageShell, From 17f644b5300106eb74d2c70a9d0a2a3dcb6c7331 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:07:12 -0700 Subject: [PATCH 16/53] chore(web): remaining pages comments say why, not what (CL-8378) --- apps/web/src/pages/create-agent-panel.tsx | 8 ++------ apps/web/src/pages/routine-detail-page.tsx | 5 ----- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/apps/web/src/pages/create-agent-panel.tsx b/apps/web/src/pages/create-agent-panel.tsx index 99b6abc81..0b6606aca 100644 --- a/apps/web/src/pages/create-agent-panel.tsx +++ b/apps/web/src/pages/create-agent-panel.tsx @@ -1,9 +1,5 @@ -// The agent-create panel: name + system prompt, deployed through the stock -// workflow-deploy path (`../agent-deploy.ts`) the same way Myra deploys -// herself. There is no drafting assist and no per-agent model/skills pin -// here — the routes that backed those were removed from the hub; the -// agent's model resolves from the tenant's existing inference offering, -// exactly as Myra's own deploy does. +// No drafting assist or per-agent model/skills pin: the routes that backed +// those were removed from the hub. import { Button, diff --git a/apps/web/src/pages/routine-detail-page.tsx b/apps/web/src/pages/routine-detail-page.tsx index 69c8a100b..4db35bc25 100644 --- a/apps/web/src/pages/routine-detail-page.tsx +++ b/apps/web/src/pages/routine-detail-page.tsx @@ -1,8 +1,3 @@ -// `/workflows/` — a deployed definition's own page: name, -// pause, run now, and its runs (with expandable event logs, from stock's -// `GET /workflows/runs?definitionId=` and `GET -// /workflows/runs/:runId/events` — `vendor/intx/hub-api/src/routes/runs.ts`). -// The id is the definition id. import { Badge, Button, From 203481067633a166832b1ed7f880cd3ef89a62b6 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:08:19 -0700 Subject: [PATCH 17/53] chore(web): pages cleanup pass (CL-8378) --- apps/web/src/pages/insights-page.tsx | 6 ++---- apps/web/src/pages/onboarding-page.tsx | 15 ++++----------- apps/web/src/pages/run-status-tone-parity.test.ts | 7 ++----- apps/web/src/pages/skill-detail-page.tsx | 6 ++---- apps/web/src/pages/skills-page.tsx | 6 ++---- apps/web/src/pages/tools-page.tsx | 5 +---- 6 files changed, 13 insertions(+), 32 deletions(-) diff --git a/apps/web/src/pages/insights-page.tsx b/apps/web/src/pages/insights-page.tsx index 0812d4055..a88ab0527 100644 --- a/apps/web/src/pages/insights-page.tsx +++ b/apps/web/src/pages/insights-page.tsx @@ -291,10 +291,8 @@ function InsightsLanding({ onOpenRuns, }: { readonly runs: readonly InsightsRun[]; - /** The feed's own `nextCursor` (`limit=100` fetch, see - * `insightsTopLevelRunsPath`) — non-null means more runs exist than the - * 100 fetched, so the KPIs below disclose the cap instead of silently - * presenting a truncated series as complete. */ + // Non-null means more runs exist than fetched, so KPIs disclose the cap + // instead of presenting a truncated series as complete. readonly runsNextCursor: string | null; readonly routines: readonly ScheduledWorkflowDefinition[]; readonly loading: boolean; diff --git a/apps/web/src/pages/onboarding-page.tsx b/apps/web/src/pages/onboarding-page.tsx index 0e54c30ed..da19d2b11 100644 --- a/apps/web/src/pages/onboarding-page.tsx +++ b/apps/web/src/pages/onboarding-page.tsx @@ -1,14 +1,7 @@ -// The setup gate: a hub with tenants bounces straight to the shell; an -// empty hub drives the browser installer itself, asking the operator only -// where a human input is genuinely required (see steps below). -// -// 1. mint the account's primary tenant if it doesn't already own one; -// 2. resolve a catalog offering, asking to connect a provider credential -// only if none is already resolved; -// 3. push Myra's source tree and deploy it. -// -// A stock capability gap this loop didn't anticipate, or a hard failure, -// renders here with a retry — a gap is not "ready". +// The setup gate: an empty hub drives the installer itself (mint tenant, +// resolve an offering, deploy Myra), asking the operator only where input +// is genuinely required. A gap this loop can't cross renders here with a +// retry, not a silent "ready". import { Button, EmptyState } from "@corbits/react-ui"; import { WarningCircle } from "@/lib/icons"; import { WorkbenchLoadingState } from "@/chat"; diff --git a/apps/web/src/pages/run-status-tone-parity.test.ts b/apps/web/src/pages/run-status-tone-parity.test.ts index 8f9394786..9a844e660 100644 --- a/apps/web/src/pages/run-status-tone-parity.test.ts +++ b/apps/web/src/pages/run-status-tone-parity.test.ts @@ -1,8 +1,5 @@ -// Guards against a page growing its own run-status -> tone map that -// quietly disagrees with react-ui's `RUN_STATUS_TONE` — e.g. a cancelled -// run reading grey on Routines and amber on Insights. Calls the real -// exported code and compares output, since a static scan can't tell a -// switch's tone from an object literal's without parsing arbitrary JS. +// Calls the real exported code and compares output, since a static scan +// can't tell a switch's tone from an object literal's without parsing. import { describe, expect, test } from "bun:test"; import { RUN_STATUS_TONE } from "@corbits/react-ui"; diff --git a/apps/web/src/pages/skill-detail-page.tsx b/apps/web/src/pages/skill-detail-page.tsx index a576d5cae..a1798b28b 100644 --- a/apps/web/src/pages/skill-detail-page.tsx +++ b/apps/web/src/pages/skill-detail-page.tsx @@ -221,10 +221,8 @@ function SkillDraftEditor({ ); } -/** - * Mount at `/skills/:name`: resolves the workbench this skill is read from - * and the name the route carries. The page owns its own stage chrome. - */ +// Resolves the workbench this skill is read from and the name the route +// carries. export function SkillDetailRoute({ path }: { readonly path: string }) { const { selectedTenantId } = useBench(); const name = skillIdFromPath(path); diff --git a/apps/web/src/pages/skills-page.tsx b/apps/web/src/pages/skills-page.tsx index e330625d1..30732b95f 100644 --- a/apps/web/src/pages/skills-page.tsx +++ b/apps/web/src/pages/skills-page.tsx @@ -1,8 +1,6 @@ // A skill lives in a native `kind:"skill"` hub asset the moment it's -// created; its version history is that asset's git history. -// -// Two states visible in "Who can see it": private (default) or shared -// with the whole workbench. No external catalog — skills are authored here. +// created; its version history is that asset's git history. No external +// catalog — skills are authored here, private by default or shared. import { PageShell, diff --git a/apps/web/src/pages/tools-page.tsx b/apps/web/src/pages/tools-page.tsx index 1b85f1ee5..bb3fb7bac 100644 --- a/apps/web/src/pages/tools-page.tsx +++ b/apps/web/src/pages/tools-page.tsx @@ -83,10 +83,7 @@ export function ToolsPage({ tenantId }: { readonly tenantId: string | null }) { ); } -/** - * Tools roster mount at `/tools`: a thin adapter that resolves which - * workbench's registry is listed. The stage chrome lives on `ToolsPage`. - */ +// A thin adapter that resolves which workbench's registry is listed. export function ToolsRoute() { const { selectedTenantId } = useBench(); From 2150d928a60b432fd28c2ec93d9426f06d4fae81 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:11:34 -0700 Subject: [PATCH 18/53] chore(web): cleanup pass on chat/shell comment overflows (CL-8378) --- apps/web/src/chat/blocks/approval-actions.ts | 6 ++---- apps/web/src/chat/blocks/approve-card-state.ts | 5 +---- apps/web/src/chat/threads-api.ts | 7 ++----- apps/web/src/chat/tool-activity-view.tsx | 6 ++---- apps/web/src/chat/tool-activity.ts | 13 +++++-------- apps/web/src/chat/turn-activity.tsx | 7 ++----- apps/web/src/chat/wire/blocks.ts | 7 +------ .../src/chat/wire/consumer-inference-text.ts | 6 +----- apps/web/src/chat/wire/parts.ts | 6 +----- apps/web/src/settings/shell.tsx | 7 ++----- apps/web/src/shell/canvas-column.tsx | 18 ++++-------------- apps/web/src/shell/library-artifacts.ts | 8 -------- apps/web/src/shell/sidebar.tsx | 4 +--- 13 files changed, 24 insertions(+), 76 deletions(-) diff --git a/apps/web/src/chat/blocks/approval-actions.ts b/apps/web/src/chat/blocks/approval-actions.ts index c501bb3c8..5db723655 100644 --- a/apps/web/src/chat/blocks/approval-actions.ts +++ b/apps/web/src/chat/blocks/approval-actions.ts @@ -19,10 +19,8 @@ export type PlatformApprovalDetail = { * it"). Falls back to the generic "Approve" when the host's read doesn't * carry one. */ readonly actionVerb?: string; - /** One-line, platform-authored consequence of taking the action (e.g. - * "Merging goes further than posting a review -- it puts the change - * live."). Replaces a risk-level badge, which only ever repeated the - * agent's own framing back at the human deciding against it. */ + // Replaces a risk-level badge, which only repeated the agent's own + // framing back at the human deciding against it. readonly consequence?: string; readonly standingConsent?: StandingConsentOffer; }; diff --git a/apps/web/src/chat/blocks/approve-card-state.ts b/apps/web/src/chat/blocks/approve-card-state.ts index 0e699a5e8..1a987173c 100644 --- a/apps/web/src/chat/blocks/approve-card-state.ts +++ b/apps/web/src/chat/blocks/approve-card-state.ts @@ -1,7 +1,4 @@ -// Pure state mapping for the approve card: turns the live status read plus -// any in-flight decision into exactly what the view renders. Kept free of -// React so the branch logic (actable vs. spectator vs. resolved vs. the -// forbidden-read branch) is unit-testable without a DOM. +// Kept free of React so the branch logic is unit-testable without a DOM. import type { ApprovalLiveStatus, diff --git a/apps/web/src/chat/threads-api.ts b/apps/web/src/chat/threads-api.ts index 0e138bc85..9634e2351 100644 --- a/apps/web/src/chat/threads-api.ts +++ b/apps/web/src/chat/threads-api.ts @@ -34,11 +34,8 @@ export type ChatAgent = { /** The address of the agent's currently live run, or null when none is * live (mid-redeploy). */ readonly liveAddress: string | null; - /** The newest deployment's status for this agent's asset — `undefined` - * when it has never been deployed. `"pending"`/`"recovering"` mean a run - * is on the way up; a terminal status (`released`, `failed`, - * `destroy_failed`, `stopped`) means nothing is running and nothing is - * coming unless someone restarts it. */ + // A terminal status means nothing is running and nothing is coming + // unless someone restarts it. readonly latestStatus: string | undefined; }; diff --git a/apps/web/src/chat/tool-activity-view.tsx b/apps/web/src/chat/tool-activity-view.tsx index 84aea0f55..421b95b7f 100644 --- a/apps/web/src/chat/tool-activity-view.tsx +++ b/apps/web/src/chat/tool-activity-view.tsx @@ -164,10 +164,8 @@ export function ToolActivityGroup({ rows }: { readonly rows: readonly ToolActivi ); } -/** - * The mid-turn strip: the same rows, plus the two things that only exist - * while a turn is open — the model thinking, and a retried request. - */ +// Plus the two things that only exist while a turn is open: thinking and +// a retried request. export function LiveToolActivity({ rows, thinking, diff --git a/apps/web/src/chat/tool-activity.ts b/apps/web/src/chat/tool-activity.ts index c71ac23a9..695c2491c 100644 --- a/apps/web/src/chat/tool-activity.ts +++ b/apps/web/src/chat/tool-activity.ts @@ -1,7 +1,6 @@ -// The one place a tool call's raw identifier/args/result gets translated -// into a plain sentence, so no downstream surface reaches for -// `JSON.stringify`. Two tenses (running vs. settled) since both the live -// strip and the persisted transcript render through here. +// Translates a tool call into a plain sentence, so no downstream surface +// reaches for `JSON.stringify`. Two tenses since both the live strip and +// the persisted transcript render through here. import type { Part, ToolTracePart } from "./wire/parts"; @@ -103,10 +102,8 @@ export type ProviderTile = { readonly color: string; }; -/** Brand mark for the chip's leading tile — two letters and the provider's - * own color, the way the mock's `[Li #5E6AD2]` / `[GH #24292f]` read. Only - * providers a person would recognise on sight get a fixed brand color; - * anything else is not a brand — the chip uses an action glyph. */ +// Only providers a person would recognise on sight get a fixed brand +// color; anything else uses an action glyph. const PROVIDER_TILES: Record = { github: { initials: "GH", color: "#24292f" }, gitlab: { initials: "GL", color: "#fc6d26" }, diff --git a/apps/web/src/chat/turn-activity.tsx b/apps/web/src/chat/turn-activity.tsx index 3f0c1df21..c1c5dde64 100644 --- a/apps/web/src/chat/turn-activity.tsx +++ b/apps/web/src/chat/turn-activity.tsx @@ -304,11 +304,8 @@ export function useTurnActivity( setActivity(null); } - // Reset (clear + re-arm) on every event that actually changes the - // activity object — an ignored event never resets the clock, since - // nothing about the open turn changed. A dropped stream leaves - // `activity` referentially stable forever, so this timer is the only - // thing that ever clears it in that case. + // This timer is the only thing that clears `activity` if the stream + // drops and leaves it referentially stable forever. useEffect(() => { if (activity === null) return; const timer = clock.setTimeout(() => { diff --git a/apps/web/src/chat/wire/blocks.ts b/apps/web/src/chat/wire/blocks.ts index 97eeede9b..92e4923d5 100644 --- a/apps/web/src/chat/wire/blocks.ts +++ b/apps/web/src/chat/wire/blocks.ts @@ -78,12 +78,7 @@ export type BlockParseResult = | { readonly ok: true; readonly block: Block } | { readonly ok: false; readonly type: string; readonly summary: string }; -/** - * Parse a `BlockPart` envelope into a typed block at the render boundary. - * An unknown type or invalid data yields an `ok: false` result for the - * caller's fallback card — never a throw, so one malformed block can't take - * down a timeline. - */ +// Never throws, so one malformed block can't take down a timeline. export function parseBlock(envelope: BlockPart["block"]): BlockParseResult { switch (envelope.type) { case "approve": { diff --git a/apps/web/src/chat/wire/consumer-inference-text.ts b/apps/web/src/chat/wire/consumer-inference-text.ts index d7c816e41..7a6c57ef1 100644 --- a/apps/web/src/chat/wire/consumer-inference-text.ts +++ b/apps/web/src/chat/wire/consumer-inference-text.ts @@ -55,11 +55,7 @@ export function isClassifiedInferenceFailureText(text: string): boolean { return CLASSIFIED_INFERENCE_FAILURE_PREAMBLES.some((preamble) => text.startsWith(preamble)); } -/** - * Bench-list / sidebar preview copy: never the full failure - * paragraph, never HTTP/raw provider dumps — a short consumer sentence - * when the text is a classified failure. - */ +// Never the full failure paragraph or raw provider dumps. export function activityPreviewText(raw: string): string { const facing = consumerFacingInferenceText(raw); return facing; diff --git a/apps/web/src/chat/wire/parts.ts b/apps/web/src/chat/wire/parts.ts index f0738b8c7..47efb9008 100644 --- a/apps/web/src/chat/wire/parts.ts +++ b/apps/web/src/chat/wire/parts.ts @@ -83,11 +83,7 @@ export const Part = TextPart.or(ReasoningPart) .or(EventPart); export type Part = typeof Part.infer; -/** - * Parse untrusted data as a `Part`, throwing a precise error rather than - * returning malformed or partially-trusted data. The only supported way - * to bring external JSON into the `Part` type. - */ +// The only supported way to bring external JSON into the `Part` type. export function parsePart(data: unknown): Part { const result = Part(data); if (result instanceof type.errors) { diff --git a/apps/web/src/settings/shell.tsx b/apps/web/src/settings/shell.tsx index b140533e2..efb75527f 100644 --- a/apps/web/src/settings/shell.tsx +++ b/apps/web/src/settings/shell.tsx @@ -1,8 +1,5 @@ -// The settings surface's shell: renders the active section's panel only. -// Section nav is a master-detail list — it lives in the host's own col2 -// (see `resolveSettingsSectionGroups`), never repeated in the stage. -// Everything about what a section shows and how it saves lives in the -// section's own `render`, never here. +// Renders the active section's panel only — nav is master-detail, never +// repeated in the stage. import { EmptyState } from "@corbits/react-ui"; import type { Icon } from "@/lib/icons"; diff --git a/apps/web/src/shell/canvas-column.tsx b/apps/web/src/shell/canvas-column.tsx index a9d67463c..17d4c4d0e 100644 --- a/apps/web/src/shell/canvas-column.tsx +++ b/apps/web/src/shell/canvas-column.tsx @@ -117,14 +117,8 @@ function mentionAction( }; } -/** Shared header row for every canvas pane: an optional leading back - * control, an optional title, an optional pane-specific `trailing` slot, - * and — for the panes that use them — the mock's focus-cycle control and - * its explicit close. `onBack` and the focus/close controls are mutually - * exclusive in practice (a pane is either master-detail-driven, like the - * routine pane, or focus/close-driven, like profile and artifact), but - * both are optional so this one component covers every canvas pane's - * header rather than each pane hand-rolling its own. */ +// `onBack` and the focus/close controls are mutually exclusive in +// practice, but both stay optional so one component covers every pane. export function CanvasPaneHeader({ title, onBack, @@ -226,12 +220,8 @@ function profileActions( // pause endpoint exists anywhere in the hub) — omitted rather than left // as a no-op that pretends to do something. if (profile.kind === "agent") { - // No "Edit agent" hop here: `ProfileSubject` (chat-ui's - // `profile-subject.ts`) carries only address/handle/displayName, never - // a workbench id, so this card has no way to resolve the agent's own - // workbench settings. The global `/settings/agents` tab this used to - // target is gone — rather than hop to a dead route, the action is - // dropped until a subject carries enough context to land somewhere real. + // No "Edit agent" hop: `ProfileSubject` carries no workbench id, and + // the global `/settings/agents` tab this used to target is gone. return [ message, mention, diff --git a/apps/web/src/shell/library-artifacts.ts b/apps/web/src/shell/library-artifacts.ts index 5d6edf416..0e0611e75 100644 --- a/apps/web/src/shell/library-artifacts.ts +++ b/apps/web/src/shell/library-artifacts.ts @@ -1,11 +1,3 @@ -// The Library page's one seam to the hub's real artifacts plane: -// `GET /api/tenants/:tenantId/artifacts` (list) and -// `GET /api/tenants/:tenantId/artifacts/:id` (detail), plus -// `POST .../artifacts/upload` for file ingest and -// `GET .../artifacts/counts` for the kind nav's counts. -// -// This module owns pure mapping + upload helper so the page stays thin and -// the shape contract has its own tests. The old asset-shim path is gone. import type { ArtifactSummary } from "@/library"; import { ApiQueryError, UnauthenticatedError } from "@/lib/api-query"; diff --git a/apps/web/src/shell/sidebar.tsx b/apps/web/src/shell/sidebar.tsx index 55d8a631c..4d5fcacfc 100644 --- a/apps/web/src/shell/sidebar.tsx +++ b/apps/web/src/shell/sidebar.tsx @@ -1,6 +1,4 @@ -// No bench switcher: a workbench IS an agent conversation now, one per -// account, so there's nothing to switch between in the common case. A -// multi-bench install still resolves (`bench-context.tsx`) via the command +// No bench switcher: a multi-bench install still resolves via the command // palette's hidden "Switch workbench" action. import { From ad8a9fd671d279d13949df2a79dc94735c5d42ed Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:13:44 -0700 Subject: [PATCH 19/53] chore(web): lib/api-query comments say why, not what (CL-8378) --- apps/web/src/lib/api-query/envelope.ts | 49 ++++++------------- .../web/src/lib/api-query/query-view.test.tsx | 6 +-- apps/web/src/lib/api-query/query-view.tsx | 15 ++---- apps/web/src/lib/slug/slug.ts | 10 +--- apps/web/src/lib/text-diff/line-diff.ts | 37 +++----------- 5 files changed, 29 insertions(+), 88 deletions(-) diff --git a/apps/web/src/lib/api-query/envelope.ts b/apps/web/src/lib/api-query/envelope.ts index 545613eb9..00665b885 100644 --- a/apps/web/src/lib/api-query/envelope.ts +++ b/apps/web/src/lib/api-query/envelope.ts @@ -1,10 +1,6 @@ -// The hub-query envelope: every page's data fetch reports exactly one of -// these four outcomes, so a page never has to invent its own notion of -// "still loading" vs. "no session" vs. "failed". `toAPIQuery` adapts any -// TanStack-Query-shaped result onto it; `QueryView` (./query-view) renders -// it. Both halves are framework-agnostic about the fetch itself — neither -// imports `@tanstack/react-query` — so a host wires its own query hook to -// this contract instead of the package assuming one. +// Neither this file nor `QueryView` imports `@tanstack/react-query`, so a +// host wires its own query hook to this contract instead of one being +// assumed. export type APIQuery = | { readonly kind: "loading" } @@ -13,16 +9,14 @@ export type APIQuery = readonly kind: "error"; readonly message: string; readonly retry: () => void; - /** The response status when the failure was an HTTP error (absent for - * network failures) — lets a caller tell "404, genuinely not found" - * apart from "500, something is actually broken" instead of - * collapsing every failure into the same generic error state. */ + // Lets a caller tell "404, not found" from "500, broken" instead of + // one generic error state. readonly status?: number; } | { readonly kind: "ready"; readonly data: T }; -/** Thrown from a queryFn on HTTP 401 so a host's retry policy can stop - * retrying and `toAPIQuery` can map the failure to `kind: "unauthenticated"`. */ +// Thrown on HTTP 401 so a retry policy can stop retrying and `toAPIQuery` +// can map it to `kind: "unauthenticated"`. export class UnauthenticatedError extends Error { constructor(message = "unauthenticated") { super(message); @@ -30,10 +24,7 @@ export class UnauthenticatedError extends Error { } } -/** The one HTTP-query error shape every hub request throws: a human message - * plus the response status when one exists (absent for network failures), - * plus the request path for logs — never surfaced in user-facing copy — - * plus the envelope `refId` when the hub answered through the sink. */ +// `path` is for logs only, never surfaced in user-facing copy. export class ApiQueryError extends Error { constructor( message: string, @@ -45,11 +36,8 @@ export class ApiQueryError extends Error { } } -/** - * Human copy for a failed query, kept plain and actionable — the technical - * detail (status codes, hub URLs, schema mismatches) stays in `console` / - * devtools for debugging, never in the primary line a person reads. - */ +// Technical detail stays in console/devtools, never the primary line a +// person reads. export function describeQueryError(error: unknown): string { if (error instanceof TypeError) { return "Can't reach the server. Check your connection."; @@ -57,14 +45,8 @@ export function describeQueryError(error: unknown): string { return "Something went wrong. Try again."; } -/** - * Human copy for a failed request, one sentence per status class, named - * around what the caller was trying to do ("loading your benches", - * "uploading this file"). Reads only `error.status` (any error-like value - * carrying one, not just `ApiQueryError`) — never `error.message`, so a - * request path, tenant id, or raw status text baked into a thrown message - * can never reach this return value. - */ +// Reads only `error.status`, never `error.message`, so a request path or +// raw status text baked into a thrown message can never leak through. export function describeApiError(error: unknown, doing: string): string { const status = typeof error === "object" && @@ -82,11 +64,8 @@ export function describeApiError(error: unknown, doing: string): string { return `Something went wrong ${doing}. Try again.`; } -/** - * Map a TanStack-Query-shaped result onto `APIQuery`. `isLoading` (pending + - * fetching) is the loading state — bare `isPending` would flash skeletons - * when cached data exists. - */ +// `isLoading` (pending + fetching), not bare `isPending`, since the +// latter would flash skeletons when cached data exists. export function toAPIQuery(result: { readonly isLoading: boolean; readonly isError: boolean; diff --git a/apps/web/src/lib/api-query/query-view.test.tsx b/apps/web/src/lib/api-query/query-view.test.tsx index 19761ab69..84ca79840 100644 --- a/apps/web/src/lib/api-query/query-view.test.tsx +++ b/apps/web/src/lib/api-query/query-view.test.tsx @@ -1,7 +1,5 @@ -// QueryView is the shared translation of loading/unauthenticated/error/ -// ready — every host page's failure copy and recovery affordance flow -// through here, so this suite is the one place asserting a failed query -// never strands a person with raw error text and nothing to do about it. +// Asserts a failed query never strands a person with raw error text and +// nothing to do about it. import { describe, expect, test } from "bun:test"; import { renderToStaticMarkup } from "react-dom/server"; diff --git a/apps/web/src/lib/api-query/query-view.tsx b/apps/web/src/lib/api-query/query-view.tsx index 54cf38310..215d14d04 100644 --- a/apps/web/src/lib/api-query/query-view.tsx +++ b/apps/web/src/lib/api-query/query-view.tsx @@ -9,11 +9,8 @@ import type { ReactNode } from "react"; import type { APIQuery } from "./envelope"; import { describeApiError } from "./envelope"; -/** Which shape a loading `QueryView` should hint at, close enough to the - * real content's footprint to keep layout shift small — not a skeleton - * framework, just the handful of shapes host pages actually need. `"block"` - * (the default) is a fixed placeholder for surfaces that are neither a list - * nor a single record. */ +// Not a skeleton framework — just the handful of shapes host pages +// actually need. export type QuerySkeletonVariant = "block" | "rows" | "detail"; /** A handful of list-row placeholders, sized near a real row. */ @@ -80,12 +77,8 @@ export function QueryView({ * real content so it doesn't jump when data lands. Ignored when * `loadingContent` is set. */ readonly skeleton?: QuerySkeletonVariant; - /** Overrides the loading render entirely — a page-level wait (a whole - * stage or panel's primary content, not a row hint) should pass its own - * warm loader here rather than take the `"block"` skeleton slab, which - * this package can't render itself: `@/chat`'s - * `WorkbenchLoadingState` depends on this package, so `QueryView` can - * never import it back without a cycle. */ + // `@/chat`'s `WorkbenchLoadingState` depends on this package, so + // `QueryView` can never import it back without a cycle. readonly loadingContent?: ReactNode; readonly children: (data: T) => ReactNode; }) { diff --git a/apps/web/src/lib/slug/slug.ts b/apps/web/src/lib/slug/slug.ts index e92c249b4..aaa47b38f 100644 --- a/apps/web/src/lib/slug/slug.ts +++ b/apps/web/src/lib/slug/slug.ts @@ -5,14 +5,8 @@ export const SLUG_MAX_LENGTH = 64; const SLUG_PATTERN = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; -/** - * Derive a slug from a display name: accents folded to ASCII, everything - * that is not a letter or digit collapsed into a single hyphen, and the - * result capped at `SLUG_MAX_LENGTH` without leaving a trailing hyphen. A - * name with nothing sluggable in it (punctuation, emoji, a non-Latin - * script) yields the empty string — the caller decides what to do with a - * name that cannot name a URL. - */ +// A name with nothing sluggable (punctuation, emoji, non-Latin script) +// yields the empty string — the caller decides what to do with it. export function slugify(name: string): string { const ascii = name .normalize("NFKD") diff --git a/apps/web/src/lib/text-diff/line-diff.ts b/apps/web/src/lib/text-diff/line-diff.ts index 386a1fb72..a568fab2d 100644 --- a/apps/web/src/lib/text-diff/line-diff.ts +++ b/apps/web/src/lib/text-diff/line-diff.ts @@ -1,22 +1,7 @@ -// A line-level diff over two revisions of the same document, shaped the way -// a review surface renders it: one row per line, in reading order, each row -// knowing whether it was kept, added, or removed and which line number it -// carries on each side. -// -// Three things keep it cheap enough to run in a dialog: -// -// 1. Newlines are normalized first, so a CRLF document never reads as -// "every line changed" — and callers can write back exactly the text -// they diffed (`normalizeNewlines` is exported for that). -// 2. The identical head and tail are trimmed before any table is built, -// so a one-line edit in a 20k-line document costs a linear scan. -// 3. What remains is diffed with a longest-common-subsequence walk, whose -// table is quadratic — so a hard cap refuses the walk instead of -// allocating gigabytes, and the caller shows a summary instead. -// -// Long runs of unchanged lines between edits are collapsed into a single -// "skipped" row: a reader needs the neighbourhood of a change, not the -// thousands of lines that did not move. +// Kept cheap enough for a dialog: newlines normalize first (no CRLF +// false-positives), identical head/tail trim before the quadratic LCS +// walk (with a hard cap instead of allocating gigabytes on a huge diff), +// and long unchanged runs collapse into a single "skipped" row. export type DiffLineKind = "context" | "added" | "removed" | "skipped"; @@ -89,12 +74,7 @@ function characterCount(lines: readonly string[]): number { return total; } -/** - * Lengths of the longest common subsequence for every suffix pair, so the - * walk below can always take the branch that keeps more lines in common. - * One Int32Array per row: the table is the expensive part of a diff, and a - * typed array keeps it to four bytes a cell. - */ +// A typed array keeps the expensive table to four bytes a cell. function commonSuffixLengths( before: readonly string[], after: readonly string[], @@ -241,11 +221,8 @@ export function diffTotals(lines: readonly DiffLine[]): DiffTotals { return { added, removed }; } -/** - * The one entry point: a diff of two revisions, or an honest refusal when - * the changed region is too large to diff inline. Callers render whichever - * status comes back rather than computing the script a second time. - */ +// Callers render whichever status comes back rather than computing the +// script a second time. export function diffText( beforeRevision: string, afterRevision: string, From e44a55e9db380515782ede85d905c28e4f6a4ead Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:14:40 -0700 Subject: [PATCH 20/53] chore(web): lib icons/client-log comments say why, not what (CL-8378) --- apps/web/src/lib/client-log/index.ts | 13 ++----------- apps/web/src/lib/icons/index.test.tsx | 9 ++------- apps/web/src/lib/icons/index.tsx | 25 ++++++------------------- 3 files changed, 10 insertions(+), 37 deletions(-) diff --git a/apps/web/src/lib/client-log/index.ts b/apps/web/src/lib/client-log/index.ts index f89cc1585..a33675379 100644 --- a/apps/web/src/lib/client-log/index.ts +++ b/apps/web/src/lib/client-log/index.ts @@ -1,14 +1,5 @@ -// The one browser-side logger: every ad-hoc `console.*` call in -// apps/web and chat-ui routes through this instead, so a user's actions -// are traceable end-to-end through one category+level shape instead of -// scattered, differently-worded console lines. Two jobs: an in-memory -// ring buffer any surface can flush (a devtools panel, a future -// diagnostics beacon), and a console mirror gated by level so a -// production console isn't flooded with routine debug/info trace lines. -// -// This module is the sanctioned exception to the `no-console` lint rule -// — every other file under `src/` calls `getLogger` instead of -// `console.*` directly. +// The sanctioned exception to the `no-console` lint rule — every other +// file calls `getLogger` instead of `console.*` directly. export type LogLevel = "debug" | "info" | "warn" | "error"; diff --git a/apps/web/src/lib/icons/index.test.tsx b/apps/web/src/lib/icons/index.test.tsx index 961dea872..ca1ed374c 100644 --- a/apps/web/src/lib/icons/index.test.tsx +++ b/apps/web/src/lib/icons/index.test.tsx @@ -13,13 +13,8 @@ describe("@/lib/icons", () => { expect(typeof icons.BoldIconProvider).toBe("function"); }); - // Regression for the oversized right-click menu / search bar: Phosphor's - // IconContext.Provider fully replaces the context value rather than - // merging with it, so a bare `{ weight: "bold" }` silently drops the - // library's own `size: "1em"` default. Any glyph mounted without an - // ancestor CSS rule or an explicit `size=` prop then renders as a bare - // with no width/height, which the browser falls back to sizing as - // a 300x150 replaced element. + // Regression: a bare `{ weight: "bold" }` silently drops Phosphor's own + // `size` default, since the context value is replaced, not merged. test("BoldIconProvider preserves Phosphor's size default alongside bold weight", () => { expect(icons.boldIconContextValue).toEqual({ size: "1em", diff --git a/apps/web/src/lib/icons/index.tsx b/apps/web/src/lib/icons/index.tsx index 6153fa313..e1389589c 100644 --- a/apps/web/src/lib/icons/index.tsx +++ b/apps/web/src/lib/icons/index.tsx @@ -1,16 +1,6 @@ -// The one icon surface every app/package imports glyphs through — owner -// ruling (CL-icons-phosphor): Phosphor replaces lucide-react everywhere, -// bold is the only weight, and the Sparkle/Sparkles glyph is banned outright -// (it read as a generic "AI" cliché; every former sparkle spot now carries a -// glyph that means something specific to what it marks). This module is a -// curated re-export, not a full pass-through of `@phosphor-icons/react` — -// only the glyphs the product actually uses are named here, so a stray -// import can't reach for an off-list icon or tiptoe around the weight rule. -// -// Extraction-ready: this is deliberately just re-exports plus one context -// provider, no app-specific logic. If `@corbits/react-ui` grows its own -// icon surface, this file becomes the shim that re-points at it instead of -// every call site changing again. +// Sparkle/Sparkles is banned outright — it read as a generic "AI" cliché. +// A curated re-export, not a full pass-through, so a stray import can't +// reach for an off-list icon or tiptoe around the bold-weight rule. import { IconContext, type Icon, type IconProps } from "@phosphor-icons/react"; import type { ReactNode } from "react"; @@ -85,12 +75,9 @@ export { X, } from "@phosphor-icons/react"; -/** `IconContext.Provider` replaces Phosphor's whole context value rather than - * merging with it, so this must restate every default the library ships - * (`size: "1em"`) alongside the one we're overriding (`weight: "bold"`) — - * dropping `size` silently un-sizes every glyph that has no ancestor CSS - * rule and no explicit `size=` prop, which is why the right-click menu and - * the search bar rendered at the browsers' unsized- fallback. */ +// `IconContext.Provider` replaces Phosphor's whole context value rather +// than merging it, so every library default (`size`) must be restated +// alongside the override — dropping it silently un-sizes bare glyphs. export const boldIconContextValue = { size: "1em", weight: "bold" } as const; /** Wraps a subtree so every Phosphor icon under it defaults to bold weight From 321cb9b18ff861e715841de21dce63159aad78dc Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:16:56 -0700 Subject: [PATCH 21/53] chore(web): library comments say why, not what (CL-8378) --- apps/web/src/library/artifact-renderer.tsx | 64 +++++-------------- apps/web/src/library/artifact-text-editor.tsx | 23 ++----- apps/web/src/library/kind-filter.ts | 20 ++---- apps/web/src/library/provenance.ts | 8 +-- apps/web/src/library/renderer-kind.ts | 31 +++------ apps/web/src/library/save-state.ts | 7 +- 6 files changed, 39 insertions(+), 114 deletions(-) diff --git a/apps/web/src/library/artifact-renderer.tsx b/apps/web/src/library/artifact-renderer.tsx index ec4fb9c19..f0a0e36bb 100644 --- a/apps/web/src/library/artifact-renderer.tsx +++ b/apps/web/src/library/artifact-renderer.tsx @@ -1,11 +1,5 @@ -// Typed, read-only renderers for artifact content — one per -// `ArtifactRendererKind` (see `renderer-kind.ts`), dispatched by -// `ArtifactRenderer`. Every host (canvas pane, Library detail preview, an -// opened chat blob) renders through this one component so a kind's shape -// only has one implementation. -// -// Read-only phase: no editing affordances here at all — the -// multiplayer-editing half is substrate to build on top of this. +// Every host renders through this one component so a kind's shape only +// has one implementation. Read-only phase: no editing affordances here. import { CsvTable } from "@corbits/react-ui"; import { FileDashed } from "@/lib/icons"; @@ -14,32 +8,17 @@ import type { ArtifactRendererKind } from "./renderer-kind"; export type ArtifactRenderProps = { readonly rendererKind: ArtifactRendererKind; readonly title: string; - /** Empty string is a legitimate, honestly-rendered "nothing here" — - * never distinguished from "not fetched yet" (the host's own loading - * state handles that before this component ever mounts). */ + // Empty string is a legitimate "nothing here" — never distinguished + // from "not fetched yet", which the host's loading state handles first. readonly content: string; - /** - * True when `content` is empty NOT because the artifact is genuinely - * blank, but because its real bytes are stored out-of-band (a file - * upload) in a format this renderer can't decode as text — a binary - * `.docx`/`.xlsx`, an image, a real PDF. Empty `content` alone can't - * carry that distinction, so the host that fetched the artifact passes - * it explicitly. Swaps the per-kind "no content yet" copy for an honest - * "couldn't read this file" message — the artifact was NOT uploaded - * empty, its contents just can't be shown here. - */ + // True when `content` is empty because the real bytes are out-of-band + // and undecodable, not because the artifact is genuinely blank — swaps + // in an honest "couldn't read this file" message instead. readonly contentUnavailable?: boolean; - /** Overrides the default "unsupported" copy with something specific to - * why this content can't be shown (e.g. a binary MIME type). */ readonly unavailableReason?: string; - /** - * The sandboxed preview route (`GET .../artifacts/:id/preview`) for a - * `"html"`-kind artifact — the `` painted underneath as the no-JS fallback. - */ +// Local shell rather than react-ui's `blocks/login/auth-layout`, since that +// block hardcodes its own procedural `DitherCanvas` — this one needs the +// image-driven dither's hero `` as a no-JS fallback underneath. export function AuthLayout({ children }: { readonly children: ReactNode }) { return (
diff --git a/apps/web/src/auth/dither-background.tsx b/apps/web/src/auth/dither-background.tsx index d05deb457..40d9c7e82 100644 --- a/apps/web/src/auth/dither-background.tsx +++ b/apps/web/src/auth/dither-background.tsx @@ -22,20 +22,8 @@ const STRENGTH_LERP = 0.08; // cursor influence easing per frame const FRAME_MS = 1000 / 30; // animation cap (~30fps) to spare the CPU const ASSET = "/images/hero-dither.png"; // same-origin source image -/** - * Animated ordered-dither over a source image, rendered on a 2D canvas. A - * downscaled buffer is dithered each frame with a slow ambient sine warp plus a - * cursor-driven displacement, then upscaled with `image-rendering: pixelated`. - * - * This replaces the original WebGL port: a WebGL canvas promoted the auth - * panel to a GPU-composited layer that failed to paint (blank/white). A 2D - * canvas is CPU-rasterized, composites reliably, and retains its last frame - * when requestAnimationFrame is paused on a hidden tab. - * - * The loop is paused whenever the canvas is offscreen or the tab is hidden, is - * capped to ~30fps, and honours `prefers-reduced-motion` reactively (a single - * static frame, re-evaluated when the OS setting toggles). - */ +// Renders on a plain 2D canvas, not WebGL — see +// docs/auth-dither-background.md for why. export function DitherBackground({ className }: { className?: string }) { // The animation belongs to the canvas element, so it starts and stops with // it: a ref callback with a cleanup, never an effect reaching for a ref. @@ -111,9 +99,7 @@ export function DitherBackground({ className }: { className?: string }) { tStr = inside ? 1 : 0; }; // Listen on window, not the canvas: the canvas is painted behind the - // QuoteCard overlay, so canvas-scoped pointermove never fires. onMove - // already maps coordinates to the canvas rect and zeroes strength when - // the cursor is outside the panel, so the global listener is cheap. + // QuoteCard overlay, so canvas-scoped pointermove never fires. window.addEventListener("pointermove", onMove, { passive: true }); const reduceQuery = window.matchMedia("(prefers-reduced-motion: reduce)"); diff --git a/apps/web/src/auth/quote-card.tsx b/apps/web/src/auth/quote-card.tsx index 7071938ad..f3fe7936a 100644 --- a/apps/web/src/auth/quote-card.tsx +++ b/apps/web/src/auth/quote-card.tsx @@ -49,10 +49,8 @@ function nextIndex(): number { return (lastIndex() + 1) % QUOTES.length; } -/** - * Brand quote card. Advances to the next quote once per page load (persisted in - * localStorage) — it does not cycle while the page is open. - */ +// Advances to the next quote once per page load; does not cycle while +// the page is open. export function QuoteCard() { // Picked and persisted once, as the card mounts — the rotation advances // per page load, never while the page is open. diff --git a/docs/auth-dither-background.md b/docs/auth-dither-background.md new file mode 100644 index 000000000..c847502ac --- /dev/null +++ b/docs/auth-dither-background.md @@ -0,0 +1,14 @@ +# Auth dither background: 2D canvas, not WebGL + +`apps/web/src/auth/dither-background.tsx` renders an animated ordered-dither +over a source image on a plain 2D canvas: a downscaled buffer is dithered +each frame with a slow ambient sine warp plus a cursor-driven displacement, +then upscaled with `image-rendering: pixelated`. + +This replaces an original WebGL port: a WebGL canvas promoted the auth panel +to a GPU-composited layer that failed to paint (blank/white). A 2D canvas is +CPU-rasterized, composites reliably, and retains its last frame when +`requestAnimationFrame` is paused on a hidden tab. + +The loop pauses whenever the canvas is offscreen or the tab is hidden, caps +to ~30fps, and honors `prefers-reduced-motion` reactively. From 3b218370823cfc9c4f273715650212b1bd13df2d Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:53:05 -0700 Subject: [PATCH 51/53] chore(web): trim comments in shell/command-palette/lib/pages files (CL-8378) --- .../src/command-palette/use-entity-search.ts | 7 ++----- apps/web/src/lib/text-diff/line-diff.ts | 6 ++---- apps/web/src/pages/onboarding-page.tsx | 6 ++---- apps/web/src/shell/canvas-column.tsx | 8 +++----- apps/web/src/shell/first-run-tour.tsx | 19 ++++++------------- 5 files changed, 15 insertions(+), 31 deletions(-) diff --git a/apps/web/src/command-palette/use-entity-search.ts b/apps/web/src/command-palette/use-entity-search.ts index 4fbad7f6d..e983ec590 100644 --- a/apps/web/src/command-palette/use-entity-search.ts +++ b/apps/web/src/command-palette/use-entity-search.ts @@ -31,11 +31,8 @@ export type UseEntitySearchResult = { readonly loadMore: () => void; }; -// Debouncing lives here, not the app shell, because it's inseparable from -// the pagination it resets: a keystroke mid-debounce must restart the -// timer and the offset together, or a stale page would leak in. -// `loading` is derived so it's visible on the render the keystroke -// caused, not after a passive effect flushes. +// Debouncing lives here, inseparable from the pagination it resets: a +// keystroke mid-debounce must restart the timer and offset together. export function useEntitySearch({ query, enabled, diff --git a/apps/web/src/lib/text-diff/line-diff.ts b/apps/web/src/lib/text-diff/line-diff.ts index a568fab2d..445802545 100644 --- a/apps/web/src/lib/text-diff/line-diff.ts +++ b/apps/web/src/lib/text-diff/line-diff.ts @@ -1,7 +1,5 @@ -// Kept cheap enough for a dialog: newlines normalize first (no CRLF -// false-positives), identical head/tail trim before the quadratic LCS -// walk (with a hard cap instead of allocating gigabytes on a huge diff), -// and long unchanged runs collapse into a single "skipped" row. +// Head/tail trim before the quadratic LCS walk, with a hard cap, keeps +// this cheap enough for a dialog on a huge diff. export type DiffLineKind = "context" | "added" | "removed" | "skipped"; diff --git a/apps/web/src/pages/onboarding-page.tsx b/apps/web/src/pages/onboarding-page.tsx index da19d2b11..02846b3ac 100644 --- a/apps/web/src/pages/onboarding-page.tsx +++ b/apps/web/src/pages/onboarding-page.tsx @@ -1,7 +1,5 @@ -// The setup gate: an empty hub drives the installer itself (mint tenant, -// resolve an offering, deploy Myra), asking the operator only where input -// is genuinely required. A gap this loop can't cross renders here with a -// retry, not a silent "ready". +// A gap the install loop can't cross renders here with a retry, never a +// silent "ready". import { Button, EmptyState } from "@corbits/react-ui"; import { WarningCircle } from "@/lib/icons"; import { WorkbenchLoadingState } from "@/chat"; diff --git a/apps/web/src/shell/canvas-column.tsx b/apps/web/src/shell/canvas-column.tsx index 17d4c4d0e..0e694e8c6 100644 --- a/apps/web/src/shell/canvas-column.tsx +++ b/apps/web/src/shell/canvas-column.tsx @@ -293,11 +293,9 @@ function ProfileCanvasPane({ ); } -/** Whether this render shows `ArtifactTextEditor` instead of the static - * `ArtifactRenderer`: the artifact has to be a text kind. Whether the - * resulting pane is interactive is `artifact.canEdit`, checked separately: - * a viewer without write access still gets `ArtifactTextEditor` in its - * own `readOnly` mode, just with keystrokes ignored. */ +/** Whether the pane is interactive is `artifact.canEdit`, checked + * separately — a read-only viewer still gets the editor, just in + * `readOnly` mode. */ function showsTextEditor(artifact: CanvasArtifactContent): boolean { return artifact.rendererKind === "doc"; } diff --git a/apps/web/src/shell/first-run-tour.tsx b/apps/web/src/shell/first-run-tour.tsx index eb06c7c10..932d5725b 100644 --- a/apps/web/src/shell/first-run-tour.tsx +++ b/apps/web/src/shell/first-run-tour.tsx @@ -50,21 +50,16 @@ const STEPS: readonly Step[] = [ }, ]; -/** - * Mounted once from `AppShell`. Renders nothing until `openFirstRunTour` is - * called — never on its own, so a fresh landing on `/` never drops this - * overlay over the chat the person was just redirected onto. - */ +// Renders nothing until `openFirstRunTour` is called — never on its own, +// so landing on `/` never drops this overlay unprompted. export function FirstRunTour({ userId }: { readonly userId: string }) { const run = useFirstRunTourOpen(); // Must unmount Joyride, not just remember the dismissal — its portal // containers are managed outside React's tree. function handleCallback(data: CallBackProps) { - // The tooltip's close (X) button fires action "close" without ever - // moving status to FINISHED or SKIPPED, so it has to be treated as a - // dismissal in its own right — otherwise closing the tour this way - // never closes it. + // The close (X) button fires action "close" without moving status to + // FINISHED or SKIPPED, so it must be treated as dismissal here too. if ( data.status === STATUS.FINISHED || data.status === STATUS.SKIPPED || @@ -84,10 +79,8 @@ export function FirstRunTour({ userId }: { readonly userId: string }) { continuous showSkipButton disableOverlayClose - // The overlay otherwise swallows every click outside the spotlight — - // real app chrome (e.g. a roster row's Chat link) is reachable at - // any point in the shell, tour running or not, so clicks must pass - // through to it rather than land on the tour's own backdrop. + // Without this, the overlay swallows clicks outside the spotlight — + // real app chrome must stay reachable while the tour runs. spotlightClicks spotlightPadding={6} callback={handleCallback} From 1b13a7e347a595e2f9704ede27890a6eaa923e5a Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:53:59 -0700 Subject: [PATCH 52/53] chore(web): trim last remaining comment blocks (CL-8378) --- apps/web/src/client-bootstrap.ts | 7 ++----- apps/web/src/insights-api.ts | 6 ++---- apps/web/src/skill-version-author.ts | 6 ++---- apps/web/src/tools/deployed-tool-packages.ts | 6 ++---- 4 files changed, 8 insertions(+), 17 deletions(-) diff --git a/apps/web/src/client-bootstrap.ts b/apps/web/src/client-bootstrap.ts index fafb26677..3879db2db 100644 --- a/apps/web/src/client-bootstrap.ts +++ b/apps/web/src/client-bootstrap.ts @@ -1,8 +1,5 @@ -// Drives the client's needs-list against a stock Interchange hub using -// only stock routes — no workbench server proxies, tables, or mounts. -// Persists created child tenant ids scoped by hub origin and account, so a -// reinstall reclaims by id, never by slug. A missing stock capability -// carries a typed gap in the result instead of throwing. +// Uses only stock routes — no workbench server proxies, tables, or mounts. +// Persisted child tenant ids let a reinstall reclaim by id, never by slug. import { WORKFLOW_CATALOG } from "@corbits/workflows/catalog"; diff --git a/apps/web/src/insights-api.ts b/apps/web/src/insights-api.ts index 83f7edb9a..6abdb4a62 100644 --- a/apps/web/src/insights-api.ts +++ b/apps/web/src/insights-api.ts @@ -29,10 +29,8 @@ export const TopLevelRunsSchema = paginatedSchema(InsightsRunSchema); // `listTopLevelRuns` uses for this route. const TOP_LEVEL_RUNS_LIMIT = 100; -// The native listing's own predicate already excludes every non-top-level -// run, so this page never derives that exclusion itself. See -// docs/insights-native-runs.md for the accepted-loss differences from the -// deleted `feed=fires` feed. +// The native listing already excludes every non-top-level run. See +// docs/insights-native-runs.md for the accepted-loss differences. export function insightsTopLevelRunsPath(tenantId: string): string { return `/api/tenants/${tenantId}/workflows/runs?limit=${TOP_LEVEL_RUNS_LIMIT}`; } diff --git a/apps/web/src/skill-version-author.ts b/apps/web/src/skill-version-author.ts index 2962f59ed..3de303645 100644 --- a/apps/web/src/skill-version-author.ts +++ b/apps/web/src/skill-version-author.ts @@ -1,7 +1,5 @@ -// Every product-made save commits under the hub's fixed git identity, a -// machine account — shown as the product's own name, never that internal -// one, until per-principal attribution is plumbed through. A commit from -// outside the product keeps the real author name git recorded. +// Product-made saves commit under the hub's fixed machine identity, shown +// as the product's own name until per-principal attribution lands. const HUB_GIT_AUTHOR = "interchange-hub"; diff --git a/apps/web/src/tools/deployed-tool-packages.ts b/apps/web/src/tools/deployed-tool-packages.ts index fcfc21197..bcb6e6829 100644 --- a/apps/web/src/tools/deployed-tool-packages.ts +++ b/apps/web/src/tools/deployed-tool-packages.ts @@ -1,7 +1,5 @@ -// Read off what agents actually carry, rather than a registry that outlives -// the packages it once held. Myra's tools are bundled, not pinned in -// `definition.json`, so her package.json dependencies are the source of -// truth instead of a literal copy that could drift. +// Read off what agents actually carry, not a registry that outlives them. +// Myra's tools are bundled, so her package.json is the source of truth. import { useQuery } from "@tanstack/react-query"; import { reportError } from "@corbits/error-sink"; From 1b933710e79dcaec51038c788f908c917309f5d2 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 18 Sep 2026 13:54:58 -0700 Subject: [PATCH 53/53] chore(web): fix formatting after comment trims (CL-8378) --- apps/web/src/settings/roles-section.tsx | 1 - apps/web/src/settings/tenancy-api.ts | 1 - apps/web/src/shell/library-artifacts.ts | 1 - 3 files changed, 3 deletions(-) diff --git a/apps/web/src/settings/roles-section.tsx b/apps/web/src/settings/roles-section.tsx index 7c03a7043..eefc8019d 100644 --- a/apps/web/src/settings/roles-section.tsx +++ b/apps/web/src/settings/roles-section.tsx @@ -1,4 +1,3 @@ - import { Badge, Button, diff --git a/apps/web/src/settings/tenancy-api.ts b/apps/web/src/settings/tenancy-api.ts index 553acf17f..84586dc3b 100644 --- a/apps/web/src/settings/tenancy-api.ts +++ b/apps/web/src/settings/tenancy-api.ts @@ -1,4 +1,3 @@ - import { type } from "arktype"; import type { ArkErrors } from "arktype"; import { diff --git a/apps/web/src/shell/library-artifacts.ts b/apps/web/src/shell/library-artifacts.ts index 0e0611e75..ced68f2d8 100644 --- a/apps/web/src/shell/library-artifacts.ts +++ b/apps/web/src/shell/library-artifacts.ts @@ -1,4 +1,3 @@ - import type { ArtifactSummary } from "@/library"; import { ApiQueryError, UnauthenticatedError } from "@/lib/api-query";