From fa1e1715538f6f8ac076761f7821f68479f616ab Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sun, 20 Sep 2026 15:56:26 -0400 Subject: [PATCH 01/24] refactor(observability): hold OTLP export settings per signal (#12657) Signed-off-by: Yordis Prieto --- apps/server/src/bin.test.ts | 7 ++++--- apps/server/src/cli/config.test.ts | 19 +++++++++++------- apps/server/src/cli/config.ts | 20 +++++++++++++++---- apps/server/src/cli/pair.ts | 7 ++++--- apps/server/src/config.ts | 19 +++++++++++------- .../src/environment/ServerEnvironment.test.ts | 8 +++++--- apps/server/src/http.ts | 2 +- .../src/observability/Layers/Observability.ts | 16 +++++++++------ apps/server/src/server.test.ts | 12 +++++------ apps/server/src/serverLogger.test.ts | 17 ++++++++++------ apps/server/src/serverLogger.ts | 7 ++++--- packages/shared/src/observability.ts | 19 ++++++++++++++++++ 12 files changed, 104 insertions(+), 49 deletions(-) diff --git a/apps/server/src/bin.test.ts b/apps/server/src/bin.test.ts index 795bb05db148..9bc20fff84e1 100644 --- a/apps/server/src/bin.test.ts +++ b/apps/server/src/bin.test.ts @@ -15,6 +15,7 @@ import { } from "@t3tools/contracts"; import * as NetService from "@t3tools/shared/Net"; import { HostProcessEnvironment } from "@t3tools/shared/hostProcess"; +import { DEFAULT_SIGNAL_EXPORT } from "@t3tools/shared/observability"; import { assert, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as DateTime from "effect/DateTime"; @@ -101,10 +102,10 @@ const makeCliTestServerConfig = (baseDir: string) => otlpTracesUrl: undefined, otlpMetricsUrl: undefined, otlpLogsUrl: undefined, - otlpExportIntervalMs: 10_000, + otlpTracesExport: DEFAULT_SIGNAL_EXPORT, + otlpMetricsExport: DEFAULT_SIGNAL_EXPORT, + otlpLogsExport: DEFAULT_SIGNAL_EXPORT, otlpServiceName: "t3-server", - otlpHeaders: undefined, - otlpProtocol: "http/json", mode: "web", port: 0, host: "127.0.0.1", diff --git a/apps/server/src/cli/config.test.ts b/apps/server/src/cli/config.test.ts index aea753c3398f..34ea7f685364 100644 --- a/apps/server/src/cli/config.test.ts +++ b/apps/server/src/cli/config.test.ts @@ -17,6 +17,7 @@ import { type DesktopBackendBootstrap as DesktopBackendBootstrapValue, } from "@t3tools/contracts"; import * as NetService from "@t3tools/shared/Net"; +import { DEFAULT_SIGNAL_EXPORT } from "@t3tools/shared/observability"; import * as NodeServices from "@effect/platform-node/NodeServices"; import { deriveServerPaths } from "../config.ts"; import { resolveServerConfig } from "./config.ts"; @@ -51,10 +52,10 @@ it.layer(NodeServices.layer)("cli config resolution", (it) => { otlpTracesUrl: undefined, otlpMetricsUrl: undefined, otlpLogsUrl: undefined, - otlpExportIntervalMs: 10_000, + otlpTracesExport: DEFAULT_SIGNAL_EXPORT, + otlpMetricsExport: DEFAULT_SIGNAL_EXPORT, + otlpLogsExport: DEFAULT_SIGNAL_EXPORT, otlpServiceName: "t3-server", - otlpHeaders: undefined, - otlpProtocol: "http/json", devAllowedOrigins: [], } as const; @@ -764,7 +765,7 @@ it.layer(NodeServices.layer)("cli config resolution", (it) => { ), ); - expect(resolved.otlpHeaders).toEqual({ + expect(resolved.otlpTracesExport.headers).toEqual({ authorization: "Basic abc==", "x-tenant": "t3", }); @@ -808,7 +809,7 @@ it.layer(NodeServices.layer)("cli config resolution", (it) => { ), ); - expect(resolved.otlpHeaders).toEqual({ + expect(resolved.otlpTracesExport.headers).toEqual({ authorization: "Bearer abc==", "x-tenant": "t3", }); @@ -816,7 +817,7 @@ it.layer(NodeServices.layer)("cli config resolution", (it) => { }), ); - it.effect("reads the OTLP protocol from env", () => + it.effect("gives every signal the protocol named without one", () => Effect.gen(function* () { const { join } = yield* Path.Path; const baseDir = join(NodeOS.tmpdir(), "t3-cli-config-otlp-protocol-base"); @@ -848,7 +849,11 @@ it.layer(NodeServices.layer)("cli config resolution", (it) => { ), ); - expect(resolved.otlpProtocol).toBe("http/protobuf"); + expect([ + resolved.otlpTracesExport.protocol, + resolved.otlpMetricsExport.protocol, + resolved.otlpLogsExport.protocol, + ]).toEqual(["http/protobuf", "http/protobuf", "http/protobuf"]); }), ); diff --git a/apps/server/src/cli/config.ts b/apps/server/src/cli/config.ts index 32f465b6fb0a..09a30aeb19e7 100644 --- a/apps/server/src/cli/config.ts +++ b/apps/server/src/cli/config.ts @@ -1,5 +1,9 @@ import * as NetService from "@t3tools/shared/Net"; -import { OtlpHeadersFromString, OtlpProtocol } from "@t3tools/shared/observability"; +import { + OtlpHeadersFromString, + OtlpProtocol, + type SignalExport, +} from "@t3tools/shared/observability"; import { parsePersistedServerObservabilitySettings } from "@t3tools/shared/serverSettings"; import { DesktopBackendBootstrap, PortSchema } from "@t3tools/contracts"; import * as Config from "effect/Config"; @@ -382,6 +386,14 @@ export const resolveServerConfig = ( ); const logLevel = Option.getOrElse(cliLogLevel, () => env.logLevel); + // T3 Code's own OTLP variables name no signal, so the one answer they give + // is the answer for all three. + const signalExport: SignalExport = { + protocol: env.otlpProtocol, + headers: env.otlpHeaders, + exportIntervalMs: env.otlpExportIntervalMs, + }; + const config: ServerConfig.ServerConfig["Service"] = { logLevel, traceMinLevel: env.traceMinLevel, @@ -399,10 +411,10 @@ export const resolveServerConfig = ( persistedObservabilitySettings.otlpMetricsUrl, otlpLogsUrl: env.otlpLogsUrl ?? bootstrap?.otlpLogsUrl ?? persistedObservabilitySettings.otlpLogsUrl, - otlpExportIntervalMs: env.otlpExportIntervalMs, + otlpTracesExport: signalExport, + otlpMetricsExport: signalExport, + otlpLogsExport: signalExport, otlpServiceName: env.otlpServiceName, - otlpHeaders: env.otlpHeaders, - otlpProtocol: env.otlpProtocol, mode, port, cwd, diff --git a/apps/server/src/cli/pair.ts b/apps/server/src/cli/pair.ts index 9ff9a8e13b39..493e6b719416 100644 --- a/apps/server/src/cli/pair.ts +++ b/apps/server/src/cli/pair.ts @@ -15,6 +15,7 @@ import { PortSchema, } from "@t3tools/contracts"; import { resolveWorktreeT3Home } from "@t3tools/shared/devHome"; +import { DEFAULT_SIGNAL_EXPORT } from "@t3tools/shared/observability"; import { buildTailscaleHttpsBaseUrl, DEFAULT_TAILSCALE_SERVE_PORT, @@ -321,10 +322,10 @@ const makePairServerConfig = Effect.fn(function* (input: { otlpTracesUrl: undefined, otlpMetricsUrl: undefined, otlpLogsUrl: undefined, - otlpExportIntervalMs: 10_000, + otlpTracesExport: DEFAULT_SIGNAL_EXPORT, + otlpMetricsExport: DEFAULT_SIGNAL_EXPORT, + otlpLogsExport: DEFAULT_SIGNAL_EXPORT, otlpServiceName: "t3-server", - otlpHeaders: undefined, - otlpProtocol: "http/json", mode: "web", port: state.port, host: state.host, diff --git a/apps/server/src/config.ts b/apps/server/src/config.ts index 4835ebb40e44..5762ccdb6ff3 100644 --- a/apps/server/src/config.ts +++ b/apps/server/src/config.ts @@ -17,7 +17,7 @@ import type * as Redacted from "effect/Redacted"; import * as Schema from "effect/Schema"; import { sweepStalePendingAttachments } from "./attachmentStore.ts"; -import { OtlpProtocol } from "@t3tools/shared/observability"; +import { DEFAULT_SIGNAL_EXPORT, type SignalExport } from "@t3tools/shared/observability"; export const DEFAULT_PORT = 3773; @@ -73,10 +73,15 @@ export class ServerConfig extends Context.Service< readonly otlpTracesUrl: string | undefined; readonly otlpMetricsUrl: string | undefined; readonly otlpLogsUrl: string | undefined; - readonly otlpExportIntervalMs: number; + /** + * How each signal is exported. Read instead of a process-wide setting so + * the wire format, credential, and schedule travel with the endpoint they + * were configured beside. + */ + readonly otlpTracesExport: SignalExport; + readonly otlpMetricsExport: SignalExport; + readonly otlpLogsExport: SignalExport; readonly otlpServiceName: string; - readonly otlpHeaders: Readonly> | undefined; - readonly otlpProtocol: OtlpProtocol; readonly mode: RuntimeMode; readonly port: number; readonly host: string | undefined; @@ -212,10 +217,10 @@ const makeTest = Effect.fn("ServerConfig.makeTest")(function* ( otlpTracesUrl: undefined, otlpMetricsUrl: undefined, otlpLogsUrl: undefined, - otlpExportIntervalMs: 10_000, + otlpTracesExport: DEFAULT_SIGNAL_EXPORT, + otlpMetricsExport: DEFAULT_SIGNAL_EXPORT, + otlpLogsExport: DEFAULT_SIGNAL_EXPORT, otlpServiceName: "t3-server", - otlpHeaders: undefined, - otlpProtocol: "http/json", cwd, baseDir, ...derivedPaths, diff --git a/apps/server/src/environment/ServerEnvironment.test.ts b/apps/server/src/environment/ServerEnvironment.test.ts index 0ccf8de691eb..b4758e980065 100644 --- a/apps/server/src/environment/ServerEnvironment.test.ts +++ b/apps/server/src/environment/ServerEnvironment.test.ts @@ -10,6 +10,8 @@ import * as Option from "effect/Option"; import * as PlatformError from "effect/PlatformError"; import * as Schema from "effect/Schema"; +import { DEFAULT_SIGNAL_EXPORT } from "@t3tools/shared/observability"; + import * as ServerSecretStore from "../auth/ServerSecretStore.ts"; import { PUBLISH_AGENT_ACTIVITY_SECRET, @@ -54,10 +56,10 @@ const makeServerConfig = Effect.fn(function* (baseDir: string) { otlpTracesUrl: undefined, otlpMetricsUrl: undefined, otlpLogsUrl: undefined, - otlpExportIntervalMs: 10_000, + otlpTracesExport: DEFAULT_SIGNAL_EXPORT, + otlpMetricsExport: DEFAULT_SIGNAL_EXPORT, + otlpLogsExport: DEFAULT_SIGNAL_EXPORT, otlpServiceName: "t3-server", - otlpHeaders: undefined, - otlpProtocol: "http/json", cwd: process.cwd(), baseDir, mode: "web", diff --git a/apps/server/src/http.ts b/apps/server/src/http.ts index 879ca66ca60a..7533b8c1db19 100644 --- a/apps/server/src/http.ts +++ b/apps/server/src/http.ts @@ -322,7 +322,7 @@ export const otlpTracesProxyRouteLayer = HttpRouter.add( const request = yield* HttpServerRequest.HttpServerRequest; const config = yield* ServerConfig.ServerConfig; const otlpTracesUrl = config.otlpTracesUrl; - const otlpHeaders = config.otlpHeaders; + const otlpHeaders = config.otlpTracesExport.headers; const browserTraceCollector = yield* BrowserTraceCollector.BrowserTraceCollector; const httpClient = yield* HttpClient.HttpClient; const serialization = yield* OtlpSerialization.OtlpSerialization; diff --git a/apps/server/src/observability/Layers/Observability.ts b/apps/server/src/observability/Layers/Observability.ts index 0c9acdfb1ee3..8f7b607745f4 100644 --- a/apps/server/src/observability/Layers/Observability.ts +++ b/apps/server/src/observability/Layers/Observability.ts @@ -20,7 +20,11 @@ import * as BrowserTraceCollector from "../BrowserTraceCollector.ts"; export const ObservabilityLive = Layer.unwrap( Effect.gen(function* () { const config = yield* ServerConfig.ServerConfig; - const serializationLayer = otlpSerializationLayer(config.otlpProtocol); + const traces = config.otlpTracesExport; + const metrics = config.otlpMetricsExport; + // The trace serializer stays in the returned context because the browser + // trace forwarder exports on the same signal. + const serializationLayer = otlpSerializationLayer(traces.protocol); const resource = ServerConfig.otlpResource(config); const attribution = yield* ResourceAttribution.ResourceAttribution; @@ -51,8 +55,8 @@ export const ObservabilityLive = Layer.unwrap( ? undefined : yield* OtlpTracer.make({ url: config.otlpTracesUrl, - exportInterval: `${config.otlpExportIntervalMs} millis`, - headers: config.otlpHeaders, + exportInterval: `${traces.exportIntervalMs} millis`, + headers: traces.headers, resource, }); @@ -77,10 +81,10 @@ export const ObservabilityLive = Layer.unwrap( ? Layer.empty : OtlpMetrics.layer({ url: config.otlpMetricsUrl, - exportInterval: `${config.otlpExportIntervalMs} millis`, - headers: config.otlpHeaders, + exportInterval: `${metrics.exportIntervalMs} millis`, + headers: metrics.headers, resource, - }).pipe(Layer.provideMerge(serializationLayer)); + }).pipe(Layer.provide(otlpSerializationLayer(metrics.protocol))); return Layer.mergeAll(ServerLoggerLive, traceReferencesLayer, tracerLayer, metricsLayer); }), diff --git a/apps/server/src/server.test.ts b/apps/server/src/server.test.ts index 2618020d388d..5d46c866a165 100644 --- a/apps/server/src/server.test.ts +++ b/apps/server/src/server.test.ts @@ -218,7 +218,7 @@ import { transferBudgetViolations, } from "../integration/TransferBudgetReport.integration.ts"; import { symlinksSupported } from "@t3tools/shared/testing/symlinks"; -import { otlpSerializationLayer } from "@t3tools/shared/observability"; +import { DEFAULT_SIGNAL_EXPORT, otlpSerializationLayer } from "@t3tools/shared/observability"; const defaultProjectId = ProjectId.make("project-default"); const defaultThreadId = ThreadId.make("thread-default"); @@ -578,10 +578,10 @@ const buildAppUnderTest = (options?: { otlpTracesUrl: undefined, otlpMetricsUrl: undefined, otlpLogsUrl: undefined, - otlpExportIntervalMs: 10_000, + otlpTracesExport: DEFAULT_SIGNAL_EXPORT, + otlpMetricsExport: DEFAULT_SIGNAL_EXPORT, + otlpLogsExport: DEFAULT_SIGNAL_EXPORT, otlpServiceName: "t3-server", - otlpHeaders: undefined, - otlpProtocol: "http/json", mode: "desktop", port: 0, host: "127.0.0.1", @@ -1076,7 +1076,7 @@ const buildAppUnderTest = (options?: { ...options?.layers?.browserTraceCollector, }), ), - Layer.provide(otlpSerializationLayer(config.otlpProtocol)), + Layer.provide(otlpSerializationLayer(config.otlpTracesExport.protocol)), Layer.provide( Layer.mock(ServerLifecycleEvents.ServerLifecycleEvents)({ publish: (event) => Effect.succeed({ ...(event as any), sequence: 1 }), @@ -5299,7 +5299,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => { yield* buildAppUnderTest({ config: { otlpTracesUrl: collector.url, - otlpProtocol: "http/protobuf", + otlpTracesExport: { ...DEFAULT_SIGNAL_EXPORT, protocol: "http/protobuf" }, }, layers: { browserTraceCollector: { diff --git a/apps/server/src/serverLogger.test.ts b/apps/server/src/serverLogger.test.ts index 59ca908cb4ad..cbb5056ed314 100644 --- a/apps/server/src/serverLogger.test.ts +++ b/apps/server/src/serverLogger.test.ts @@ -8,6 +8,8 @@ import * as Tracer from "effect/Tracer"; import * as HttpClient from "effect/unstable/http/HttpClient"; import * as HttpClientResponse from "effect/unstable/http/HttpClientResponse"; +import { DEFAULT_SIGNAL_EXPORT } from "@t3tools/shared/observability"; + import * as ServerConfig from "./config.ts"; import { ServerLoggerLive } from "./serverLogger.ts"; @@ -51,10 +53,10 @@ const configLayer = (overrides: Partial) = otlpTracesUrl: undefined, otlpMetricsUrl: undefined, otlpLogsUrl: undefined, - otlpExportIntervalMs: 10_000, + otlpTracesExport: DEFAULT_SIGNAL_EXPORT, + otlpMetricsExport: DEFAULT_SIGNAL_EXPORT, + otlpLogsExport: DEFAULT_SIGNAL_EXPORT, otlpServiceName: "t3-server", - otlpHeaders: undefined, - otlpProtocol: "http/json", cwd: baseDir, baseDir, ...derivedPaths, @@ -155,12 +157,15 @@ describe("ServerLoggerLive", () => { }), ); - it.effect("sends the headers and wire format the rest of OTLP export already uses", () => + it.effect("sends the headers and wire format the log signal asked for", () => Effect.gen(function* () { const requests = yield* logThrough({ otlpLogsUrl: "https://collector.example.com/v1/logs", - otlpProtocol: "http/protobuf", - otlpHeaders: { "x-scope": "logs" }, + otlpLogsExport: { + ...DEFAULT_SIGNAL_EXPORT, + protocol: "http/protobuf", + headers: { "x-scope": "logs" }, + }, }); assert.lengthOf(requests, 1); diff --git a/apps/server/src/serverLogger.ts b/apps/server/src/serverLogger.ts index 389a9535efab..6d19907c20ca 100644 --- a/apps/server/src/serverLogger.ts +++ b/apps/server/src/serverLogger.ts @@ -12,13 +12,14 @@ export const ServerLoggerLive = Effect.gen(function* () { const config = yield* ServerConfig; const minimumLogLevelLayer = Layer.succeed(References.MinimumLogLevel, config.logLevel); + const logs = config.otlpLogsExport; const otlpLogger = config.otlpLogsUrl === undefined ? undefined : OtlpLogger.make({ url: config.otlpLogsUrl, - exportInterval: `${config.otlpExportIntervalMs} millis`, - headers: config.otlpHeaders, + exportInterval: `${logs.exportIntervalMs} millis`, + headers: logs.headers, resource: otlpResource(config), }); @@ -41,7 +42,7 @@ export const ServerLoggerLive = Effect.gen(function* () { { mergeWithExisting: false }, ).pipe( Layer.provide(OtlpExporter.layerFlusher), - Layer.provide(otlpSerializationLayer(config.otlpProtocol)), + Layer.provide(otlpSerializationLayer(logs.protocol)), ); return Layer.mergeAll(loggerLayer, minimumLogLevelLayer); diff --git a/packages/shared/src/observability.ts b/packages/shared/src/observability.ts index 62121399853b..6393ed0879db 100644 --- a/packages/shared/src/observability.ts +++ b/packages/shared/src/observability.ts @@ -16,6 +16,25 @@ export type OtlpProtocol = typeof OtlpProtocol.Type; export const otlpSerializationLayer = (protocol: OtlpProtocol) => protocol === "http/protobuf" ? OtlpSerialization.layerProtobuf : OtlpSerialization.layerJson; +/** + * How one signal is exported, once whichever source named that signal's + * endpoint has been resolved. Held per signal rather than per process, so a + * wire format or a credential cannot be paired by hand with an endpoint that + * came from somewhere else. + */ +export interface SignalExport { + readonly protocol: OtlpProtocol; + readonly headers: Readonly> | undefined; + readonly exportIntervalMs: number; +} + +/** What T3 Code exports with when nothing configured a signal. */ +export const DEFAULT_SIGNAL_EXPORT: SignalExport = { + protocol: "http/json", + headers: undefined, + exportIntervalMs: 10_000, +}; + const FLUSH_BUFFER_THRESHOLD = 256; const textEncoder = new TextEncoder(); From ead1dee22916b414906eafdae5f5a1293f16e637 Mon Sep 17 00:00:00 2001 From: Igor Makowski <56691628+Mnigos@users.noreply.github.com> Date: Sun, 20 Sep 2026 22:04:46 +0200 Subject: [PATCH 02/24] fix(web): explain what enabling network access means in its confirmation (#10098) Co-authored-by: shivamhwp <91240327+shivamhwp@users.noreply.github.com> --- .../settings/ConnectionsSettings.tsx | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/apps/web/src/components/settings/ConnectionsSettings.tsx b/apps/web/src/components/settings/ConnectionsSettings.tsx index 324361d7fb68..1410b3b5e0e7 100644 --- a/apps/web/src/components/settings/ConnectionsSettings.tsx +++ b/apps/web/src/components/settings/ConnectionsSettings.tsx @@ -3417,8 +3417,8 @@ export function ConnectionsSettings() { {pendingDesktopServerExposureMode === "network-accessible" - ? "T3 Code will restart to expose this environment over the network." - : "T3 Code will restart and limit this environment back to this machine."} + ? "Let your other devices connect to T3 Code over the network. Pair devices to give them access. T3 Code will restart." + : "Devices connected over your local network will disconnect. Existing tunnels, such as T3 Connect or Tailscale HTTPS, keep working. T3 Code will restart."} @@ -3426,27 +3426,23 @@ export function ConnectionsSettings() { disabled={isUpdatingDesktopServerExposure} render={ From 9f0c9f725d4553d34e3c91c13d67ebcc58be5b0f Mon Sep 17 00:00:00 2001 From: oliver <97427849+flamboh@users.noreply.github.com> Date: Sun, 20 Sep 2026 13:20:10 -0700 Subject: [PATCH 03/24] fix(web): reuse current PR status in the sidebar (#12545) --- .../pullRequest/PullRequestService.test.ts | 61 ++++ .../src/pullRequest/PullRequestService.ts | 31 +- .../pullRequest/gitHubPullRequestJson.test.ts | 8 + .../src/pullRequest/gitHubPullRequestJson.ts | 7 +- apps/web/src/components/RightPanelTabs.tsx | 28 +- .../src/components/ThreadStatusIndicators.tsx | 19 +- .../pullRequest/PullRequestChecksPopover.tsx | 10 +- .../PullRequestDetailPanel.test.tsx | 3 +- .../pullRequest/PullRequestDetailPanel.tsx | 300 +++++++++++------- .../pullRequest/PullRequestGhosts.tsx | 44 ++- .../pullRequest/PullRequestSummaryTab.tsx | 13 +- apps/web/src/state/pullRequests.test.ts | 83 +++++ apps/web/src/state/pullRequests.ts | 160 ++++++++-- apps/web/src/state/query.ts | 2 + packages/contracts/src/pullRequest.ts | 6 + 15 files changed, 604 insertions(+), 171 deletions(-) create mode 100644 apps/web/src/state/pullRequests.test.ts diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 9d563d3965c3..a0d111bd710f 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3404,6 +3404,67 @@ it.effect("a listing narrowed to some projects is its own cache entry", () => }), ); +it.effect( + "keeps listing freshness tied to read start when filtered reads finish out of order", + () => + Effect.gen(function* () { + const olderStarted = yield* Deferred.make(); + const releaseOlder = yield* Deferred.make(); + let reads = 0; + const updatedAt = "2026-07-02T00:00:00Z"; + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" }), + ], + providers: [ + fakeProvider("github", { + listChangeRequests: ({ filters }) => + Effect.gen(function* () { + reads += 1; + const older = filters?.checks === "failing"; + if (older) { + yield* Deferred.succeed(olderStarted, undefined); + yield* Deferred.await(releaseOlder); + } + return { + items: [ + { + ...changeRequest(1, updatedAt), + checksState: older ? ("failing" as const) : ("passing" as const), + mergeability: older ? ("mergeable" as const) : ("conflicting" as const), + }, + ], + truncated: false, + continues: false, + }; + }), + }), + ], + }); + const olderInput = { state: "open" as const, filters: { checks: "failing" as const } }; + const newerInput = { state: "open" as const, filters: { checks: "passing" as const } }; + + const olderRead = yield* service.list(olderInput).pipe(Effect.forkChild()); + yield* Deferred.await(olderStarted); + yield* TestClock.adjust("1 second"); + const newer = yield* service.list(newerInput); + yield* Deferred.succeed(releaseOlder, undefined); + const older = yield* Fiber.join(olderRead); + + assert.strictEqual(older.entries[0]?.checksState, "failing"); + assert.strictEqual(older.entries[0]?.mergeability, "mergeable"); + assert.strictEqual(newer.entries[0]?.checksState, "passing"); + assert.strictEqual(newer.entries[0]?.mergeability, "conflicting"); + assert.strictEqual(typeof older.entries[0]?.observedAt, "number"); + assert.strictEqual(typeof newer.entries[0]?.observedAt, "number"); + assert.isBelow(older.entries[0]!.observedAt!, newer.entries[0]!.observedAt!); + + const cachedOlder = yield* service.list(olderInput); + assert.strictEqual(cachedOlder.entries[0]?.observedAt, older.entries[0]?.observedAt); + assert.strictEqual(reads, 2); + }), +); + it.effect("keeps unrelated PRs warm after a mutation, explicit refresh, and project turn", () => Effect.gen(function* () { const calls: string[] = []; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index a653ef94c713..6b4b2bd8bfd5 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -611,6 +611,12 @@ function withRateLimitBackoff( Record, never>; } +// Capture before the provider read so a slow response keeps its original freshness through caches. +const observeRead = Effect.fnUntraced(function* (read: Effect.Effect) { + const observedAt = yield* Clock.currentTimeMillis; + return { value: yield* read, observedAt }; +}); + export const make = Effect.gen(function* () { const mergedPullRequests = yield* PubSub.sliding(64); const pullRequestRefreshes = yield* SubscriptionRef.make(0); @@ -1076,6 +1082,7 @@ export const make = Effect.gen(function* () { readonly project: SupportedProject; readonly item: ProviderChangeRequest; readonly viewer: string; + readonly observedAt: number; }): PullRequestListEntry => { const viewer = input.viewer.toLowerCase(); return { @@ -1098,6 +1105,7 @@ export const make = Effect.gen(function* () { deletions: input.item.deletions, createdAt: input.item.createdAt, updatedAt: input.item.updatedAt, + observedAt: input.observedAt, ...(input.item.checksState === undefined || input.item.checksState === null ? {} : { checksState: input.item.checksState }), @@ -1246,7 +1254,8 @@ export const make = Effect.gen(function* () { }), }) .pipe( - Effect.map((page): RepositoryBatch => { + observeRead, + Effect.map(({ value: page, observedAt }): RepositoryBatch => { // The boundary instant was asked for inclusively, so the rows already sent at it // come back with the slice. Dropping them here rather than asking for strictly // older is what keeps their neighbours at the same instant from being skipped. @@ -1262,7 +1271,7 @@ export const make = Effect.gen(function* () { key, entries: items .filter((item) => matchesRowFilters(item, input.filters, viewer)) - .map((item) => toEntry({ project, item, viewer })), + .map((item) => toEntry({ project, item, viewer, observedAt })), errors: [], truncated: page.truncated, nextCursor: @@ -1323,7 +1332,8 @@ export const make = Effect.gen(function* () { ? {} : { cursor: { updatedBefore: cursor.updatedBefore, delivered: cursor.delivered } }), }).pipe( - Effect.flatMap((page) => + observeRead, + Effect.flatMap(({ value: page, observedAt }) => Effect.flatMap(Clock.currentTimeMillis, (now) => { const rows = new Map>(); for (const [key, visibleAt] of searchVisibleAt) { @@ -1384,7 +1394,7 @@ export const make = Effect.gen(function* () { key: project.cursorKey, entries: items .filter((item) => matchesRowFilters(item, input.filters, viewer)) - .map((item) => toEntry({ project, item, viewer })), + .map((item) => toEntry({ project, item, viewer, observedAt })), errors: [], truncated: page.truncated, nextCursor: @@ -1551,7 +1561,8 @@ export const make = Effect.gen(function* () { : project.api.getChangeRequestSummary(providerInput); return read.pipe( Effect.mapError(toPullRequestError("summary")), - Effect.map((changeRequest): PullRequestSummary => ({ + observeRead, + Effect.map(({ value: changeRequest, observedAt }): PullRequestSummary => ({ provider: project.api.kind, projectId: project.project.id, repository: project.repository, @@ -1564,6 +1575,7 @@ export const make = Effect.gen(function* () { closedAt: changeRequest.closedAt ?? null, mergedAt: changeRequest.mergedAt ?? null, updatedAt: changeRequest.updatedAt, + observedAt, ...(changeRequest.isDraft === undefined ? {} : { isDraft: changeRequest.isDraft }), ...(changeRequest.author === undefined ? {} : { author: changeRequest.author }), ...(changeRequest.additions === undefined @@ -1634,12 +1646,12 @@ export const make = Effect.gen(function* () { host: project.host, number: input.number, }) - .pipe(Effect.mapError(toPullRequestError("detail"))), + .pipe(Effect.mapError(toPullRequestError("detail")), observeRead), viewerOf(project), ], { concurrency: 2 }, ).pipe( - Effect.map(([changeRequest, viewer]): PullRequestDetail => ({ + Effect.map(([{ value: changeRequest, observedAt }, viewer]): PullRequestDetail => ({ provider: project.api.kind, capabilities: project.api.capabilities, projectId: project.project.id, @@ -1664,6 +1676,7 @@ export const make = Effect.gen(function* () { baseBranch: changeRequest.baseBranch, createdAt: changeRequest.createdAt, updatedAt: changeRequest.updatedAt, + observedAt, mergedAt: changeRequest.mergedAt, closedAt: changeRequest.closedAt, reviewers: changeRequest.reviewers, @@ -2894,12 +2907,14 @@ export const make = Effect.gen(function* () { closedAt: detail.closedAt, mergedAt: detail.mergedAt, updatedAt: detail.updatedAt, + observedAt: detail.observedAt, }); const shouldReplaceHeldSummary = (key: string, next: PullRequestSummary) => { const current = lastGoodSummary.peek(key); if (current === undefined) return true; if (current.state === "merged" && next.state !== "merged") return false; - return next.updatedAt >= current.updatedAt; + if (next.updatedAt !== current.updatedAt) return next.updatedAt > current.updatedAt; + return (next.observedAt ?? -Infinity) >= (current.observedAt ?? -Infinity); }; const detail: PullRequestService["Service"]["detail"] = (input) => { const key = refCacheKey(input); diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts index 4591ac0437bf..05796283b9b5 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts @@ -121,6 +121,13 @@ describe("pull request list decoding", () => { { statusCheckRollup: [{ context: "ci/legacy", state: "ERROR" }] }, // Neither a pass, a failure nor a wait is no verdict rather than a green tick. { statusCheckRollup: [{ name: "lint", status: "COMPLETED", conclusion: "SKIPPED" }] }, + // Cancelled reads as failing here and in the detail header, so the two never flap. + { + statusCheckRollup: [ + { name: "lint", status: "COMPLETED", conclusion: "SUCCESS" }, + { name: "test", status: "COMPLETED", conclusion: "CANCELLED" }, + ], + }, { statusCheckRollup: [] }, {}, ]), @@ -132,6 +139,7 @@ describe("pull request list decoding", () => { "passing", "failing", null, + "failing", null, null, ]); diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index a011702d685e..8a1434224ad2 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -1447,8 +1447,9 @@ function toCheckEntries( * GitHub's own indicator reads: a run that has already gone red will not go green by finishing. * * Null rather than "passing" for a head commit with no checks at all, so a repository that runs - * none shows nothing instead of a green tick it never earned. Checks whose verdict is neither a - * pass, a failure nor a wait — skipped, cancelled, neutral — count towards neither. + * none shows nothing instead of a green tick it never earned. A cancelled run is a failure, as + * GitHub's own rollup and the client's detail rollup both read it; skipped and neutral count + * towards neither, so the row and the detail header never disagree about one head commit. * * Counted off the deduped checks rather than the raw rollup, so the word and the list under it * cannot disagree: the run a re-run replaced is not a verdict twice. A row with no name at all is @@ -1463,7 +1464,7 @@ function rollupChecksState( ...(raw ?? []).filter(isNamelessCheck).map((check) => toCheckStatus(check)), ]; if (statuses.length === 0) return null; - if (statuses.includes("failure")) return "failing"; + if (statuses.includes("failure") || statuses.includes("cancelled")) return "failing"; if (statuses.includes("pending") || statuses.includes("action-required")) return "pending"; return statuses.includes("success") ? "passing" : null; } diff --git a/apps/web/src/components/RightPanelTabs.tsx b/apps/web/src/components/RightPanelTabs.tsx index f234efff093e..e1bc3f151fd9 100644 --- a/apps/web/src/components/RightPanelTabs.tsx +++ b/apps/web/src/components/RightPanelTabs.tsx @@ -34,6 +34,7 @@ import { type ReactNode, useCallback, useEffect, + useMemo, useRef, useState, } from "react"; @@ -62,7 +63,11 @@ import { ScrollArea } from "~/components/ui/scroll-area"; import { PanelTabCloseButton } from "~/components/ui/panel-tab-close-button"; import { faviconUrlForOrigin } from "~/lib/favicon"; import { useTheme } from "~/hooks/useTheme"; -import { pullRequestEnvironment } from "~/state/pullRequests"; +import { + newestPullRequestSummary, + pullRequestEnvironment, + useSharedPullRequestSummary, +} from "~/state/pullRequests"; import { useEnvironmentQuery } from "~/state/query"; import { COLLAPSED_SIDEBAR_TITLEBAR_INSET_CLASS } from "~/workspaceTitlebar"; @@ -800,19 +805,26 @@ function PullRequestSurfaceIcon({ }, }), ).data; + const reference = useMemo( + () => ({ + projectId: surface.projectId as ProjectId, + repository: surface.repository, + number: surface.number, + }), + [surface.projectId, surface.repository, surface.number], + ); + const sharedSummary = useSharedPullRequestSummary(resolvedEnvironmentId, reference, null); // The compact tab intentionally shows lifecycle and draft state only. Conflict warnings have // their own presentation on surfaces that have mergeability, while this tab stays stable as // detail data arrives. - const status = - linkedSnapshot !== null - ? linkedSnapshot - : detail === null - ? (seed ?? null) - : { state: detail.state, isDraft: detail.isDraft }; + const status = linkedSnapshot ?? newestPullRequestSummary(detail, sharedSummary) ?? seed ?? null; if (status === null) { return ; } - const presentation = resolvePullRequestState({ state: status.state, isDraft: status.isDraft }); + const presentation = resolvePullRequestState({ + state: status.state, + isDraft: status.isDraft ?? detail?.isDraft ?? seed?.isDraft ?? false, + }); return ; } diff --git a/apps/web/src/components/ThreadStatusIndicators.tsx b/apps/web/src/components/ThreadStatusIndicators.tsx index 016a2c2e431d..5b2945042ac2 100644 --- a/apps/web/src/components/ThreadStatusIndicators.tsx +++ b/apps/web/src/components/ThreadStatusIndicators.tsx @@ -76,15 +76,24 @@ export function useLinkedThreadPullRequest( ); const fallback = current === null ? ((!supportsLinks ? linkedPullRequest : null) ?? branchPullRequest) : null; - const host = fallback == null ? undefined : parseChangeRequestUrl(fallback.url)?.host; - const reference = - fallback == null ? null : { ...fallback, ...(host === undefined ? {} : { host }) }; + // Stable per link: the shared summary effect keys on this object, and a sidebar row must not + // touch the cache on every render. + const reference = useMemo(() => { + if (fallback == null) return null; + const host = parseChangeRequestUrl(fallback.url)?.host; + return { ...fallback, ...(host === undefined ? {} : { host }) }; + }, [fallback]); const queried = useEnvironmentQuery( !enabled || environmentId === null || reference === null ? null : linkedPullRequestDetailAtom({ environmentId, input: reference }), - ).data; - const detail = useSharedPullRequestSummary(environmentId, reference, queried); + ); + const detail = useSharedPullRequestSummary( + environmentId, + reference, + queried.data, + queried.dataUpdatedAt, + ); return useMemo(() => { if (current !== null) return linkedPullRequestSnapshotStatus(current); diff --git a/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx b/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx index 20100339ecbb..e9797016aad7 100644 --- a/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx +++ b/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx @@ -109,6 +109,7 @@ function ChecksBody({ export function PullRequestChecksPopover({ checksState, checks, + stale = false, environmentId, reference, threadRef = null, @@ -117,6 +118,7 @@ export function PullRequestChecksPopover({ checksState: PullRequestChecksState; /** The checks already in hand, for the detail header. Absent on a listing row. */ checks?: ReadonlyArray; + stale?: boolean; environmentId?: EnvironmentId; reference?: PullRequestRef; /** Thread the popover sits beside; a listing row has none. */ @@ -125,7 +127,7 @@ export function PullRequestChecksPopover({ }) { const presentation = pullRequestChecksStatePresentation(checksState); // Counts beat the rollup's own wording where they are known, the way GitHub's own header reads. - const summary = checks === undefined ? null : summarizePullRequestChecks(checks); + const summary = checks === undefined || stale ? null : summarizePullRequestChecks(checks); return ( {/* A listing row is itself a button, so the trigger renders as a span: a nested button is @@ -148,7 +150,11 @@ export function PullRequestChecksPopover({

{presentation.label}

{summary === null ? null :

{summary}

} - {checks !== undefined ? ( + {stale ? ( +

+ Check details are out of date. Refresh the pull request to update them. +

+ ) : checks !== undefined ? ( ) : environmentId !== undefined && reference !== undefined ? ( ({ usePreparePullRequestThreadAction: () => ({ run: prepareThread }), })); vi.mock("~/state/use-atom-command", () => ({ useAtomCommand: () => vi.fn() })); -vi.mock("~/state/pullRequests", () => ({ +vi.mock("~/state/pullRequests", async (importOriginal) => ({ + ...(await importOriginal()), pullRequestEnvironment: { detail: () => "detail", activity: () => "activity" }, usePullRequestTurnRefresh: () => 0, useSharedPullRequestSummary: () => null, diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 31d81315c2e1..c605409b1801 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -78,8 +78,13 @@ import { useProjects, useServerConfigs } from "~/state/entities"; import { useEnvironments, usePrimaryEnvironmentId } from "~/state/environments"; import { useEnvironmentQuery } from "~/state/query"; import { useLiveRefresh } from "~/hooks/useLiveRefresh"; -import { pullRequestEnvironment } from "~/state/pullRequests"; -import { usePullRequestTurnRefresh, useSharedPullRequestSummary } from "~/state/pullRequests"; +import { + pullRequestEnvironment, + pullRequestListEntryToSummary, + newestPullRequestSummary, + usePullRequestTurnRefresh, + useSharedPullRequestSummary, +} from "~/state/pullRequests"; import { useAtomCommand } from "~/state/use-atom-command"; import { PullRequestStackMenu } from "./PullRequestStackMenu"; import { PullRequestThreadLinks } from "./PullRequestThreadLinks"; @@ -167,6 +172,7 @@ import { PullRequestMetaLine, PullRequestReviewOutcomeIcon, pullRequestChecksState, + pullRequestChecksStatePresentation, pullRequestReviewOutcomeToneClassName, resolvePullRequestState, summarizePullRequestChecks, @@ -521,10 +527,11 @@ export function PullRequestDetailPanel({ }) { const environmentConfigs = useServerConfigs(); const projects = useProjects(); - const repositoryIdentity = projects.find( + const project = projects.find( (project) => project.id === requestedReference.projectId && project.environmentId === environmentId, - )?.repositoryIdentity; + ); + const repositoryIdentity = project?.repositoryIdentity; const supportsThreadPullRequests = environmentConfigs.get(environmentId)?.environment.capabilities.threadPullRequests === true; const reference = useMemo( @@ -542,6 +549,8 @@ export function PullRequestDetailPanel({ const matchingListEntry = listEntry?.projectId === reference.projectId && listEntry.repository.toLowerCase() === reference.repository.toLowerCase() && + (reference.host === undefined || + listEntry.host.toLowerCase() === reference.host.toLowerCase()) && listEntry.number === reference.number ? listEntry : null; @@ -675,14 +684,47 @@ export function PullRequestDetailPanel({ cached: cachedDetail, reference, }); - const sharedSummary = useSharedPullRequestSummary(environmentId, reference, resolvedCoreDetail); + const listSummary = useMemo( + () => (matchingListEntry === null ? null : pullRequestListEntryToSummary(matchingListEntry)), + [matchingListEntry], + ); + const detailSummary = useMemo( + () => + detailQuery.data === null + ? null + : { + ...detailQuery.data, + checksState: pullRequestChecksState(detailQuery.data.checks), + }, + [detailQuery.data], + ); + const observedSummary = useSharedPullRequestSummary( + environmentId, + reference, + detailSummary, + detailQuery.dataUpdatedAt, + ); + // The list row is also published to the shared cache, but only after this commit's layout + // effects run, so it is compared directly rather than trusted to be there already. + const sharedSummary = useMemo( + () => + newestPullRequestSummary( + resolvedCoreDetail, + newestPullRequestSummary(observedSummary, listSummary), + ), + [resolvedCoreDetail, observedSummary, listSummary], + ); const coreDetail = useMemo( () => resolvedCoreDetail === null || sharedSummary === null || sharedSummary === resolvedCoreDetail ? resolvedCoreDetail : { ...resolvedCoreDetail, - ...sharedSummary, + title: sharedSummary.title, + state: sharedSummary.state, + headBranch: sharedSummary.headBranch, + baseBranch: sharedSummary.baseBranch, + updatedAt: sharedSummary.updatedAt, author: sharedSummary.author ?? resolvedCoreDetail.author, additions: sharedSummary.additions ?? resolvedCoreDetail.additions, deletions: sharedSummary.deletions ?? resolvedCoreDetail.deletions, @@ -720,6 +762,7 @@ export function PullRequestDetailPanel({ }, [activity, coreDetail], ); + const handoffSummary = detail ?? sharedSummary; const keybindings = useAtomValue(primaryServerKeybindingsAtom); const { copyToClipboard: copyReference } = useCopyToClipboard({ target: "pull request reference", @@ -945,9 +988,11 @@ export function PullRequestDetailPanel({ const acting = pickableEnvironments.find((entry) => entry.environmentId === chosenEnvironmentId) ?? null; const actingEnvironmentId = acting?.environmentId ?? environmentId; + const checkoutRoot = + acting?.workspaceRoot ?? detail?.workspaceRoot ?? project?.workspaceRoot ?? null; const prepareThread = usePreparePullRequestThreadAction({ environmentId: actingEnvironmentId, - cwd: acting?.workspaceRoot ?? detail?.workspaceRoot ?? null, + cwd: checkoutRoot, }); const finishAction = async ( @@ -1178,7 +1223,7 @@ export function PullRequestDetailPanel({ // already work — and it moves the branch under everything else that is open there. mode: "worktree" | "local" = "worktree", ) => { - if (!detail || handoff !== null) return; + if (!handoffSummary || handoff !== null) return; if (attachTarget !== null && task !== null) { writeTaskToComposer(attachTarget, task); toastManager.add({ @@ -1188,6 +1233,7 @@ export function PullRequestDetailPanel({ }); return; } + if (checkoutRoot === null) return; setHandoff(kind); // The menu closes on the press and takes its "Preparing..." label with it, so this is the // only thing answering for the checkout. It carries no timeout of its own: a loading toast @@ -1198,7 +1244,10 @@ export function PullRequestDetailPanel({ }); // Wherever the reader chose to act: the thread, the checkout it is pointed at and the composer // the task lands in are all one server's, and picking another one moves all three. - const projectRef = scopeProjectRef(actingEnvironmentId, acting?.projectId ?? detail.projectId); + const projectRef = scopeProjectRef( + actingEnvironmentId, + acting?.projectId ?? handoffSummary.projectId, + ); // The thread is opened before the checkout rather than after it, because the project's setup // script only runs for a checkout that knows which thread it is for — and a worktree with no // dependencies installed is not something anyone can test. @@ -1219,7 +1268,7 @@ export function PullRequestDetailPanel({ return; } const prepared = await prepareThread.run({ - reference: detail.url, + reference: handoffSummary.url, mode, threadId: opened.threadId, }); @@ -1348,7 +1397,7 @@ export function PullRequestDetailPanel({ }; const startCheckout = (mode: "worktree" | "local") => { - if (!detail) return; + if (!handoffSummary) return; void startHandoff(`checkout:${mode}`, null, mode); }; @@ -1380,20 +1429,20 @@ export function PullRequestDetailPanel({ baseBranch: detail.baseBranch, reviewThreads: detail.reviewThreads, comments: detail.comments, - checks: detail.checks, + checks: checksStale ? [] : detail.checks, commentsTruncated: detail.commentsTruncated, }), ); }; const startResolveConflicts = () => { - if (!detail) return; + if (!handoffSummary) return; void startHandoff("conflicts", { prompt: buildResolveConflictsPrompt({ - number: detail.number, - url: detail.url, - headBranch: detail.headBranch, - baseBranch: detail.baseBranch, + number: handoffSummary.number, + url: handoffSummary.url, + headBranch: handoffSummary.headBranch, + baseBranch: handoffSummary.baseBranch, }), }); }; @@ -1443,7 +1492,17 @@ export function PullRequestDetailPanel({ const can = (action: PullRequestAction) => detail?.capabilities.actions.includes(action) === true && detail.viewerPermissions.actions.includes(action); - const checksState = detail ? pullRequestChecksState(detail.checks) : null; + const detailChecksState = detail ? pullRequestChecksState(detail.checks) : null; + const latestChecksState = + sharedSummary?.checksState === undefined ? detailChecksState : sharedSummary.checksState; + // List rollups can omit workflows awaiting approval. Only refreshed detail can clear those. + const checksState = + latestChecksState !== "failing" && + detail?.checks.some((check) => check.status === "action-required") + ? "pending" + : latestChecksState; + // A newer rollup cannot tell us which runs changed or how many passed. + const checksStale = checksState !== detailChecksState; // The merge state remains in one stable slot from waiting through completion. Conflicts take // the slot while they need a person; the armed badge remains beside them so that state is not lost. const primaryAction = detail @@ -1494,7 +1553,13 @@ export function PullRequestDetailPanel({ const statePresentation = detail ? resolvePullRequestState({ state: detail.state, isDraft: detail.isDraft }) : null; - const checksSummary = detail ? summarizePullRequestChecks(detail.checks) : null; + const checksSummary = checksStale + ? checksState === null + ? "No checks reported" + : pullRequestChecksStatePresentation(checksState).label + : detail + ? summarizePullRequestChecks(detail.checks) + : null; // Approvals that still stand, and only those. A superseded one is dimmed beside the reviewer // who gave it, so counting it here would have the header assert in a number what the row next // to it has just qualified. @@ -1509,10 +1574,108 @@ export function PullRequestDetailPanel({ ).length : 0; + const checkoutControl = + context === "page" ? ( + + + + + + {handoff?.startsWith("checkout") ? "Checking out..." : "Check out"} + + + + } + /> + } + /> + Check out this pull request + + + startCheckout("worktree")}> + + + In a separate worktree + + Its own folder and thread. Nothing you have open moves. + + + + startCheckout("local")}> + + + In this repository + + Switches the branch you are working in, like `gh pr checkout`. + + + + {pickableEnvironments.length > 0 ? ( + setActingScope({ pullRequestKey, environmentId: next })} + disabled={handoff !== null} + /> + ) : null} + + + ) : null; + + const resolveConflictsControl = ( + + + + + } + /> + + {handoff === "conflicts" ? "Preparing..." : "Resolve conflicts"} + + + ); + // The list already has the pull request's identity and summary. Keep them on screen // and let the richer detail read replace the remaining placeholders in place. if (detailQuery.isPending && !detail) { - return ; + return ( + + {checkoutControl} + {handoffSummary.state === "open" && handoffSummary.mergeability === "conflicting" + ? resolveConflictsControl + : null} + + ) : undefined + } + /> + ); } return ( @@ -1720,71 +1883,7 @@ export function PullRequestDetailPanel({ threadRef={null} /> ) : null} - {/* Checking a pull request out is the reason to open one here at all, so it is a - button of its own rather than a side effect of asking an agent for something. - It asks where, because the two answers are not interchangeable: one leaves your - work where it is, the other moves the repository you are standing in. Only on - the page: beside a thread the branch is already checked out right there. */} - {context === "page" ? ( - - - - - - {handoff?.startsWith("checkout") ? "Checking out..." : "Check out"} - - - - } - /> - } - /> - Check out this pull request - - - startCheckout("worktree")}> - - - In a separate worktree - - Its own folder and thread. Nothing you have open moves. - - - - startCheckout("local")}> - - - In this repository - - Switches the branch you are working in, like `gh pr checkout`. - - - - {pickableEnvironments.length > 0 ? ( - setActingScope({ pullRequestKey, environmentId: next })} - disabled={handoff !== null} - /> - ) : null} - - - ) : null} + {checkoutControl} {/* Said where the Merge button is, because it is the answer to why nobody has pressed it: the merge is already asked for, and the host is holding it. */} {autoMergeArmed && primaryAction !== "auto-merge-armed" ? ( @@ -1809,31 +1908,7 @@ export function PullRequestDetailPanel({ ) : null} {primaryAction === "resolve" ? ( - - - - - } - /> - - {handoff === "conflicts" ? "Preparing..." : "Resolve conflicts"} - - + resolveConflictsControl ) : primaryAction === "ready" ? ( {tab === "summary" ? ( - {workflowApprovalsRequired > 0 && can("approve-workflows") ? ( + {workflowApprovalsRequired > 0 && !checksStale && can("approve-workflows") ? ( @@ -2649,12 +2725,14 @@ export function PullRequestDetailPanel({ reference={reference} detail={detail} activityPending={activityPending} + checksStale={checksStale} activityError={activityError} pendingFinding={handoff} fixFindingLabel={handoffLabels.fixFinding} fixCheckLabel={handoffLabels.fixCheck} onFixFinding={startFixFinding} onRefresh={refreshDetail} + onRefreshChecks={refreshFromHost} /> ) : null} diff --git a/apps/web/src/components/pullRequest/PullRequestGhosts.tsx b/apps/web/src/components/pullRequest/PullRequestGhosts.tsx index f8c922356548..c82129df13b1 100644 --- a/apps/web/src/components/pullRequest/PullRequestGhosts.tsx +++ b/apps/web/src/components/pullRequest/PullRequestGhosts.tsx @@ -7,8 +7,9 @@ * both themes) and the single `animate-skeleton` pulse, applied once on the container so any * number of bars costs one opacity animation. */ -import type { PullRequestListEntry } from "@t3tools/contracts"; +import type { PullRequestListEntry, PullRequestSummary } from "@t3tools/contracts"; import { ArrowLeftIcon } from "lucide-react"; +import type { ReactNode } from "react"; import { cn } from "~/lib/utils"; import { formatRelativeTimeLabel } from "~/timestampFormat"; @@ -72,16 +73,33 @@ export function PullRequestListGhost({ * boundaries in the ghost prevents the loaded pull request from replacing one layout with * another a moment later. */ -export function PullRequestDetailGhost({ seed }: { seed?: PullRequestListEntry | null }) { +export function PullRequestDetailGhost({ + seed: entry, + summary, + actions, +}: { + seed?: PullRequestListEntry | null; + summary?: PullRequestSummary | null; + actions?: ReactNode; +}) { + const seed = summary + ? { + ...entry, + ...summary, + isDraft: summary.isDraft ?? entry?.isDraft, + } + : entry; const statePresentation = seed ? resolvePullRequestState({ state: seed.state, - isDraft: seed.isDraft, + isDraft: seed.isDraft ?? false, }) : null; - const checksPresentation = seed?.checksState - ? pullRequestChecksStatePresentation(seed.checksState) - : null; + // Passing list rollups can omit workflows awaiting approval; wait for detail to claim success. + const checksPresentation = + seed?.checksState === "failing" || seed?.checksState === "pending" + ? pullRequestChecksStatePresentation(seed.checksState) + : null; return (
-
+
{seed && statePresentation ? ( @@ -114,8 +132,8 @@ export function PullRequestDetailGhost({ seed }: { seed?: PullRequestListEntry | )}
- - + {actions ?? } +
@@ -129,7 +147,7 @@ export function PullRequestDetailGhost({ seed }: { seed?: PullRequestListEntry | {seed ? ( <> @@ -165,8 +183,8 @@ export function PullRequestDetailGhost({ seed }: { seed?: PullRequestListEntry | {seed ? ( ) : ( @@ -217,7 +235,7 @@ export function PullRequestDetailGhost({ seed }: { seed?: PullRequestListEntry |
- {seed ? ( + {seed?.labels ? ( seed.labels.slice(0, 3).map((label) => { const color = pullRequestLabelColor(label.color); return ( diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index e89b38c28e1e..b2096c87ded9 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -461,18 +461,21 @@ export function PullRequestSummaryTab({ reference, detail, activityPending, + checksStale = false, activityError, pendingFinding, fixFindingLabel = "Fix in a thread", fixCheckLabel = "Fix", onFixFinding, onRefresh, + onRefreshChecks = onRefresh, }: { environmentId: EnvironmentId; threadRef: ScopedThreadRef | null; reference: PullRequestRef; detail: PullRequestDetailView; activityPending: boolean; + checksStale?: boolean; activityError: string | null; /** The hand-off currently preparing, if any, so only the finding it belongs to says so. */ pendingFinding?: string | null; @@ -480,6 +483,7 @@ export function PullRequestSummaryTab({ fixCheckLabel?: string; onFixFinding?: (finding: PullRequestFinding) => void; onRefresh: () => void; + onRefreshChecks?: () => void; }) { // Keyed by the pull request, so opening another one starts at the end of its conversation // rather than wherever the last one had been read back to. @@ -872,7 +876,14 @@ export function PullRequestSummaryTab({
- {detail.checks.length === 0 ? ( + {checksStale ? ( +
+ Check details are out of date. + +
+ ) : detail.checks.length === 0 ? (

No checks reported.

) : ( detail.checks.map((check, index) => { diff --git a/apps/web/src/state/pullRequests.test.ts b/apps/web/src/state/pullRequests.test.ts new file mode 100644 index 000000000000..48ee5b51b884 --- /dev/null +++ b/apps/web/src/state/pullRequests.test.ts @@ -0,0 +1,83 @@ +import { ProjectId, type PullRequestSummary } from "@t3tools/contracts"; +import { describe, expect, it } from "vite-plus/test"; + +import { newestPullRequestObservation, newestPullRequestSummary } from "./pullRequests"; + +function summary(overrides: Partial = {}): PullRequestSummary { + return { + provider: "github", + projectId: ProjectId.make("pull-request-cache-test"), + repository: "acme/widget", + number: 7, + title: "Improve widget", + url: "https://github.com/acme/widget/pull/7", + state: "open", + isDraft: false, + headBranch: "improve-widget", + baseBranch: "main", + updatedAt: "2026-09-10T00:00:00Z", + author: { login: "oliver", name: null, avatarUrl: null }, + mergeability: "mergeable", + checksState: "passing", + ...overrides, + }; +} + +const observed = (value: PullRequestSummary, observedAt: number) => ({ + summary: value, + observedAt, +}); + +describe("pull request summary cache", () => { + it("orders same-dated snapshots by the server's read time, not by arrival", () => { + const stale = observed(summary({ observedAt: 200 }), 500); + const held = observed( + summary({ mergeability: "conflicting", checksState: "failing", observedAt: 300 }), + 100, + ); + expect(newestPullRequestObservation(stale, held)?.summary).toMatchObject({ + mergeability: "conflicting", + checksState: "failing", + observedAt: 300, + }); + // An older filtered or server-cached response finishing last must not roll status back. + expect(newestPullRequestObservation(held, stale)).toBe(held); + // The same read seen again is not a new observation. + expect(newestPullRequestObservation(held, { ...held, observedAt: 900 })).toBe(held); + }); + + it("uses arrival order only when neither snapshot carries a server read time", () => { + const first = observed(summary(), 100); + const later = observed(summary({ checksState: "failing" }), 200); + expect(newestPullRequestObservation(first, later)).toMatchObject({ observedAt: 200 }); + expect(newestPullRequestObservation(later, first)).toBe(later); + // A stamped read beats an unstamped one regardless of which arrived last. + const stamped = observed(summary({ observedAt: 50 }), 1); + expect(newestPullRequestObservation(later, stamped)?.summary.observedAt).toBe(50); + expect(newestPullRequestObservation(stamped, later)).toBe(stamped); + }); + + it("keeps known status when a newer snapshot omits it, and clears it when told to", () => { + const list = observed(summary({ reviewDecision: "approved", observedAt: 100 }), 100); + const sparse = observed( + summary({ checksState: undefined, reviewDecision: undefined, observedAt: 200 }), + 200, + ); + expect(newestPullRequestObservation(list, sparse)?.summary).toMatchObject({ + checksState: "passing", + reviewDecision: "approved", + }); + const cleared = observed(summary({ checksState: null, observedAt: 300 }), 300); + expect(newestPullRequestObservation(list, cleared)?.summary.checksState).toBeNull(); + }); + + it("treats merged as final and otherwise prefers the later host update", () => { + const merged = summary({ state: "merged", updatedAt: "2026-09-01T00:00:00Z" }); + const reopened = summary({ updatedAt: "2026-09-12T00:00:00Z", observedAt: 900 }); + expect(newestPullRequestSummary(merged, reopened)).toBe(merged); + expect(newestPullRequestSummary(reopened, merged)).toBe(merged); + const older = summary({ updatedAt: "2026-09-11T00:00:00Z", observedAt: 999 }); + expect(newestPullRequestSummary(older, reopened)).toBe(reopened); + expect(newestPullRequestSummary(reopened, older)).toBe(reopened); + }); +}); diff --git a/apps/web/src/state/pullRequests.ts b/apps/web/src/state/pullRequests.ts index e2e840c89c49..365c6812d096 100644 --- a/apps/web/src/state/pullRequests.ts +++ b/apps/web/src/state/pullRequests.ts @@ -8,6 +8,7 @@ import type { EnvironmentId, PullRequestListInput, PullRequestListStatsInput, + PullRequestListEntry, PullRequestRef, PullRequestSummary, } from "@t3tools/contracts"; @@ -30,49 +31,147 @@ export const linkedPullRequestDetailAtom = createLinkedPullRequestSummaryAtomFam pullRequestEnvironment.refreshes, ); +export interface ObservedPullRequestSummary { + readonly summary: PullRequestSummary; + /** Client arrival time, the only ordering older servers leave us for same-dated snapshots. */ + readonly observedAt: number; +} + const observedPullRequestSummaryAtom = Atom.family((key: string) => - Atom.make(null).pipe( + Atom.make(null).pipe( Atom.setIdleTTL(5 * 60_000), Atom.withLabel(`web-pull-requests:observed-summary:${key}`), ), ); +/** + * Positive when `incoming` is the newer snapshot. Merged is final. Then the host's own update + * time, then the server's read-start time, which survives its caches; a snapshot without one + * never beats a stamped read. Zero when neither side carries a read time. + */ +function compareSummaries(current: PullRequestSummary, incoming: PullRequestSummary): number { + const merged = Number(incoming.state === "merged") - Number(current.state === "merged"); + if (merged !== 0) return merged; + const updated = Date.parse(incoming.updatedAt) - Date.parse(current.updatedAt); + if (updated !== 0) return updated; + if (current.observedAt === undefined && incoming.observedAt === undefined) return 0; + return (incoming.observedAt ?? -Infinity) - (current.observedAt ?? -Infinity); +} + export function newestPullRequestSummary( current: PullRequestSummary | null, observed: PullRequestSummary | null, ): PullRequestSummary | null { if (current === null) return observed; if (observed === null) return current; - if (current.state === "merged") return current; - if (observed.state === "merged") return observed; - return Date.parse(observed.updatedAt) >= Date.parse(current.updatedAt) ? observed : current; + return compareSummaries(current, observed) >= 0 ? observed : current; +} + +/** Reuse list status without treating its deferred line-count placeholders as real stats. */ +export function pullRequestListEntryToSummary(entry: PullRequestListEntry): PullRequestSummary { + return { + provider: entry.provider, + projectId: entry.projectId, + repository: entry.repository, + number: entry.number, + title: entry.title, + url: entry.url, + state: entry.state, + isDraft: entry.isDraft, + headBranch: entry.headBranch, + baseBranch: entry.baseBranch, + updatedAt: entry.updatedAt, + ...(entry.observedAt === undefined ? {} : { observedAt: entry.observedAt }), + author: entry.author, + ...(entry.reviewDecision === undefined ? {} : { reviewDecision: entry.reviewDecision }), + ...(entry.checksState === undefined ? {} : { checksState: entry.checksState }), + mergeability: entry.mergeability, + }; +} + +// A project has one remote, so its id already pins the host. Leaving the host out lets a list +// row, a hostless legacy reference and a URL-derived thread reference share one entry. +function pullRequestSummaryKey(environmentId: EnvironmentId, reference: PullRequestRef): string { + return JSON.stringify([ + environmentId, + reference.projectId, + reference.repository.toLowerCase(), + reference.number, + ]); +} + +/** The observation to hold after `incoming` arrives. Returns `current` itself on a tie. */ +export function newestPullRequestObservation( + current: ObservedPullRequestSummary | null, + incoming: ObservedPullRequestSummary | null, +): ObservedPullRequestSummary | null { + if (current === null) return incoming; + if (incoming === null) return current; + let order = compareSummaries(current.summary, incoming.summary); + // Client clocks only break ties between snapshots that both lack a server read time. + if ( + order === 0 && + current.summary.observedAt === undefined && + incoming.summary.observedAt === undefined + ) { + order = incoming.observedAt - current.observedAt; + } + if (!(order > 0)) return current; + // A sparse summary must not erase known status, or carry old detail stats into a new list read. + return { + ...incoming, + summary: { + ...incoming.summary, + isDraft: incoming.summary.isDraft ?? current.summary.isDraft, + mergeability: incoming.summary.mergeability ?? current.summary.mergeability, + reviewDecision: + incoming.summary.reviewDecision === undefined + ? current.summary.reviewDecision + : incoming.summary.reviewDecision, + checksState: + incoming.summary.checksState === undefined + ? current.summary.checksState + : incoming.summary.checksState, + }, + }; +} + +function observePullRequestSummary( + environmentId: EnvironmentId, + reference: PullRequestRef, + summary: PullRequestSummary, + observedAt: number, +): void { + const atom = observedPullRequestSummaryAtom(pullRequestSummaryKey(environmentId, reference)); + appAtomRegistry.modify(atom, (previous) => { + const next = newestPullRequestObservation(previous, { summary, observedAt }); + return next === previous ? [false, previous] : [true, next]; + }); } export function useSharedPullRequestSummary( environmentId: EnvironmentId | null, reference: PullRequestRef | null, current: PullRequestSummary | null, + observedAt: number | null = null, ): PullRequestSummary | null { const key = environmentId === null || reference === null ? "none" - : JSON.stringify([ - environmentId, - reference.projectId, - reference.host?.toLowerCase() ?? null, - reference.repository.toLowerCase(), - reference.number, - ]); + : pullRequestSummaryKey(environmentId, reference); const atom = observedPullRequestSummaryAtom(key); const observed = useAtomValue(atom); useLayoutEffect(() => { - if (environmentId === null || current === null) return; - appAtomRegistry.modify(atom, (previous) => { - const next = newestPullRequestSummary(previous, current); - return next === previous ? [false, previous] : [true, next]; - }); - }, [atom, current, environmentId]); - return newestPullRequestSummary(current, observed); + if (environmentId === null || reference === null || current === null || observedAt === null) + return; + observePullRequestSummary(environmentId, reference, current, observedAt); + }, [current, environmentId, reference, observedAt]); + return ( + newestPullRequestObservation( + observed, + current === null || observedAt === null ? null : { summary: current, observedAt }, + )?.summary ?? current + ); } export const pullRequestStackAtom = createPullRequestStackAtomFamily( connectionAtomRuntime, @@ -90,6 +189,7 @@ interface MergedEnvironmentQueryView { /** The first environment that failed. Others may still have answered — this is not fatal. */ readonly error: string | null; readonly isPending: boolean; + readonly observations: ReadonlyArray; } /** @@ -110,6 +210,7 @@ function createMergedEnvironmentQuery( Atom.make((get): MergedEnvironmentQueryView => { const targets = JSON.parse(key) as ReadonlyArray>; const values: Array = []; + const observations: Array = []; let error: string | null = null; let isPending = false; for (const target of targets) { @@ -120,12 +221,16 @@ function createMergedEnvironmentQuery( } const value = Option.getOrNull(AsyncResult.value(result)); if (value !== null) values.push([target.environmentId, value]); + if (result._tag === "Success") { + observations.push([target.environmentId, result.value, result.timestamp]); + } } - return { values, error, isPending }; + return { values, error, isPending, observations }; }).pipe(Atom.withLabel(`${label}:${key}`)), ); const empty = Atom.make>({ values: [], + observations: [], error: null, isPending: false, }).pipe(Atom.withLabel(`${label}:empty`)); @@ -187,6 +292,23 @@ export function usePullRequestList( targets: ReadonlyArray>, ): MergedPullRequestListView { const query = usePullRequestListsQuery(targets); + useLayoutEffect(() => { + for (const [environmentId, answer, observedAt] of query.observations) { + for (const entry of answer.entries) { + observePullRequestSummary( + environmentId, + { + projectId: entry.projectId, + host: entry.host, + repository: entry.repository, + number: entry.number, + }, + pullRequestListEntryToSummary(entry), + observedAt, + ); + } + } + }, [query.observations]); const data = useMemo(() => mergePullRequestLists(query.values), [query.values]); return { data, error: query.error, isPending: query.isPending, refresh: query.refresh }; } diff --git a/apps/web/src/state/query.ts b/apps/web/src/state/query.ts index b2823a51e332..f5cb765a2479 100644 --- a/apps/web/src/state/query.ts +++ b/apps/web/src/state/query.ts @@ -9,6 +9,7 @@ const EMPTY_ASYNC_RESULT_ATOM = Atom.make(AsyncResult.initial(fals export interface EnvironmentQueryView { readonly data: A | null; + readonly dataUpdatedAt: number | null; readonly error: string | null; readonly isPending: boolean; readonly isSuccess: boolean; @@ -30,6 +31,7 @@ export function useEnvironmentQuery( const refresh = useAtomRefresh(selectedAtom); return { data: Option.getOrNull(AsyncResult.value(result)), + dataUpdatedAt: result._tag === "Success" ? result.timestamp : null, error: result._tag === "Failure" ? formatEnvironmentQueryError(result.cause) : null, isPending: atom !== null && result.waiting, isSuccess: result._tag === "Success", diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index b146810a8aec..1a3c2d50b209 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -525,6 +525,8 @@ export const PullRequestListEntry = Schema.Struct({ deletions: NonNegativeInt, createdAt: IsoDateTime, updatedAt: IsoDateTime, + /** Server epoch milliseconds when the provider read started; preserved on cache hits. */ + observedAt: Schema.optional(Schema.Finite), viewerReviewRequested: Schema.Boolean, labels: Schema.Array(PullRequestLabel), /** Absent where the host does not summarise its reviews, which is every host but GitHub. */ @@ -736,6 +738,8 @@ export const PullRequestSummary = Schema.Struct({ closedAt: Schema.optional(Schema.NullOr(Schema.String)), mergedAt: Schema.optional(Schema.NullOr(Schema.String)), updatedAt: IsoDateTime, + /** Server epoch milliseconds when the provider read started; preserved on cache hits. */ + observedAt: Schema.optional(Schema.Finite), author: Schema.optional(Schema.NullOr(PullRequestActor)), additions: Schema.optional(NonNegativeInt), deletions: Schema.optional(NonNegativeInt), @@ -841,6 +845,8 @@ export const PullRequestDetail = Schema.Struct({ baseBranch: TrimmedNonEmptyString, createdAt: IsoDateTime, updatedAt: IsoDateTime, + /** Server epoch milliseconds when the provider read started; preserved on cache hits. */ + observedAt: Schema.optional(Schema.Finite), mergedAt: Schema.NullOr(IsoDateTime), closedAt: Schema.NullOr(IsoDateTime), reviewers: Schema.Array(PullRequestActor), From 6a699f0f2fbd8847d7ec2df8d9245ce8c2eb8707 Mon Sep 17 00:00:00 2001 From: oliver <97427849+flamboh@users.noreply.github.com> Date: Sun, 20 Sep 2026 13:25:46 -0700 Subject: [PATCH 04/24] fix(web): stabilize pull request loading layout (#12721) Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> --- .../pullRequest/PullRequestCopyableCode.tsx | 66 +++ .../pullRequest/PullRequestDetailPanel.tsx | 113 ++--- .../pullRequest/PullRequestGhosts.tsx | 390 ++++++++++++------ .../pullRequest/PullRequestSummaryTab.tsx | 2 +- .../pullRequestDetail.logic.test.ts | 46 +++ .../pullRequest/pullRequestDetail.logic.ts | 16 + 6 files changed, 419 insertions(+), 214 deletions(-) create mode 100644 apps/web/src/components/pullRequest/PullRequestCopyableCode.tsx diff --git a/apps/web/src/components/pullRequest/PullRequestCopyableCode.tsx b/apps/web/src/components/pullRequest/PullRequestCopyableCode.tsx new file mode 100644 index 000000000000..4c9d14b53734 --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestCopyableCode.tsx @@ -0,0 +1,66 @@ +import { useCopyToClipboard } from "~/hooks/useCopyToClipboard"; +import { cn } from "~/lib/utils"; + +import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; + +export function PullRequestCopyableCode({ + value, + target, + copyLabel, + copiedLabel, + className, + tooltipSide = "top", + onError, +}: { + readonly value: string; + readonly target: string; + readonly copyLabel: string; + readonly copiedLabel: string; + readonly className?: string; + readonly tooltipSide?: "top" | "bottom"; + readonly onError?: (error: Error) => void; +}) { + const { copyToClipboard, isCopied } = useCopyToClipboard({ + target, + timeout: 1600, + ...(onError ? { onError } : {}), + }); + return ( + + copyToClipboard(value)} + /> + } + > + + {value} + + + + + {`${isCopied ? "Copied" : copyLabel}: ${value}`} + + + ); +} diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index c605409b1801..98d172f53eb5 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -121,6 +121,7 @@ import { Popover, PopoverPopup, PopoverTrigger } from "../ui/popover"; import { toastManager } from "../ui/toast"; import { Tooltip, TooltipPopup, TooltipProvider, TooltipTrigger } from "../ui/tooltip"; import { PullRequestDetailGhost, PullRequestTimelineGhost } from "./PullRequestGhosts"; +import { PullRequestCopyableCode } from "./PullRequestCopyableCode"; import { PullRequestActivityUnavailableState } from "./PullRequestActivityUnavailableState"; import { DiffPanelLoadingState } from "../DiffPanelShell"; import { PullRequestsUnavailableState } from "./PullRequestsUnavailableState"; @@ -140,6 +141,7 @@ import { handoffPrompt, handoffReviewComments, latestPullRequestReviewOutcomes, + loadingPullRequestCheckoutCommand, isStackedPullRequestBase, pullRequestActionMenuHasGroup, pullRequestActionNeedsHostRefresh, @@ -326,68 +328,6 @@ const openNumberContextMenu = ( }); }; -function PullRequestCopyableCode({ - value, - target, - copyLabel, - copiedLabel, - className, - tooltipSide = "top", - onError, -}: { - readonly value: string; - readonly target: string; - readonly copyLabel: string; - readonly copiedLabel: string; - readonly className?: string; - readonly tooltipSide?: "top" | "bottom"; - readonly onError?: (error: Error) => void; -}) { - const { copyToClipboard, isCopied } = useCopyToClipboard({ - target, - timeout: 1600, - ...(onError ? { onError } : {}), - }); - return ( - - copyToClipboard(value)} - /> - } - > - - {value} - - - - - {`${isCopied ? "Copied" : copyLabel}: ${value}`} - - - ); -} - /** * The stale-branch warning, said beside the branch it is about rather than as a bar of its own. * The banner this replaces held a row of chrome open across the top of every pull request that @@ -804,15 +744,22 @@ export function PullRequestDetailPanel({ repositoryUrl !== null ? new URL(`/${encodeURIComponent(detail.author.login)}`, repositoryUrl).toString() : null; - const checkoutCommand = detail + const checkoutCommand = handoffSummary ? pullRequestCheckoutCommand( - detail.provider, - detail.number, - detail.headBranch, - detail.headRepositoryNameWithOwner, - repositoryUrl, + handoffSummary.provider, + handoffSummary.number, + handoffSummary.headBranch, + detail?.headRepositoryNameWithOwner, + changeRequestRepositoryUrl(handoffSummary.url), ) - : null; + : loadingPullRequestCheckoutCommand(reference, repositoryIdentity); + const onCheckoutCommandError = useCallback((error: Error) => { + toastManager.add({ + type: "error", + title: "Could not copy checkout command", + description: error.message, + }); + }, []); const branchRefsQuery = useEnvironmentQuery( detail === null ? null @@ -1475,8 +1422,9 @@ export function PullRequestDetailPanel({ // Out of date with the base, and still cleanly mergeable — the one pairing an update button // exists for. Null everywhere else, including hosts that cannot compare at all. const freshness = detail === null ? null : resolveBaseFreshness(detail); - // A host that cannot produce a patch has no Code tab to open. The tabs themselves stay hidden - // until the detail arrives, so the loading ghost is the panel's only unfinished UI. + // A host that cannot produce a patch has no Code tab to open. While detail is loading the ghost + // uses this optimistic tab set to reserve the same chrome; a host without a patch removes Code + // when its capabilities arrive. const visibleTabs = TABS.filter( (item) => item.value !== "code" || detail === null || detail.capabilities.diff, ); @@ -1664,6 +1612,13 @@ export function PullRequestDetailPanel({ @@ -2371,7 +2326,7 @@ export function PullRequestDetailPanel({ {detail ? (
{titleDraft === null ? ( -
+
)} -
+
- toastManager.add({ - type: "error", - title: "Could not copy checkout command", - description: error.message, - }) - } + onError={onCheckoutCommandError} /> ) : null}
-
+
- + {detail.changedFiles.toLocaleString()}{" "} {detail.changedFiles === 1 ? "file" : "files"} diff --git a/apps/web/src/components/pullRequest/PullRequestGhosts.tsx b/apps/web/src/components/pullRequest/PullRequestGhosts.tsx index c82129df13b1..90f88da93668 100644 --- a/apps/web/src/components/pullRequest/PullRequestGhosts.tsx +++ b/apps/web/src/components/pullRequest/PullRequestGhosts.tsx @@ -8,16 +8,31 @@ * number of bars costs one opacity animation. */ import type { PullRequestListEntry, PullRequestSummary } from "@t3tools/contracts"; -import { ArrowLeftIcon } from "lucide-react"; +import { + ArrowLeftIcon, + ChevronRightIcon, + EllipsisIcon, + ExternalLinkIcon, + FileDiffIcon, + PanelRightIcon, + TagIcon, + UserPlusIcon, + UsersIcon, +} from "lucide-react"; import type { ReactNode } from "react"; +import { readLocalApi } from "~/localApi"; import { cn } from "~/lib/utils"; import { formatRelativeTimeLabel } from "~/timestampFormat"; +import { Button, InlineButton } from "../ui/button"; +import { Toggle, ToggleGroup } from "../ui/toggle-group"; +import { PullRequestCopyableCode } from "./PullRequestCopyableCode"; import { pullRequestLabelColor } from "./pullRequestList.logic"; import { PullRequestActorLabel, PullRequestDiffStat, + PullRequestMetaLine, pullRequestChecksStatePresentation, resolvePullRequestState, } from "./pullRequestPresentation"; @@ -29,6 +44,11 @@ function GhostBar({ className }: { className?: string | undefined }) { /** Widths cycle rather than randomize, so the ghost renders the same on every pass. */ const TITLE_WIDTHS = ["w-3/5", "w-2/5", "w-1/2", "w-2/3", "w-2/5", "w-3/5", "w-1/2"]; const META_WIDTHS = ["w-2/5", "w-1/3", "w-2/5", "w-1/4", "w-1/3", "w-2/5", "w-1/3"]; +const DEFAULT_DETAIL_TABS = [ + { value: "summary", label: "Summary" }, + { value: "timeline", label: "Timeline" }, + { value: "code", label: "Code" }, +] as const; /** Rows in the list's own grid — glyph, title over meta, time over diffstat. */ export function PullRequestListGhost({ @@ -77,10 +97,25 @@ export function PullRequestDetailGhost({ seed: entry, summary, actions, + checkoutCommand, + tabs = DEFAULT_DETAIL_TABS, + activeTab, + number, + onBack, + onClose, + onCheckoutError, }: { seed?: PullRequestListEntry | null; summary?: PullRequestSummary | null; actions?: ReactNode; + checkoutCommand?: string | null; + tabs?: ReadonlyArray<{ value: string; label: string }>; + /** The panel's current tab, so the highlight does not jump when the detail arrives. */ + activeTab?: string; + number?: number; + onBack?: (() => void) | undefined; + onClose?: (() => void) | undefined; + onCheckoutError?: ((error: Error) => void) | undefined; }) { const seed = summary ? { @@ -100,6 +135,10 @@ export function PullRequestDetailGhost({ seed?.checksState === "failing" || seed?.checksState === "pending" ? pullRequestChecksStatePresentation(seed.checksState) : null; + const checkout = checkoutCommand ?? null; + const changedFiles = summary?.changedFiles ?? null; + const selectedTab = + tabs.find((item) => item.value === activeTab)?.value ?? tabs[0]?.value ?? "summary"; return (
-
-
-
- {seed && statePresentation ? ( +
+
+
+ {onBack ? ( + + ) : null} + {seed ? ( <> - - {seed.repository} - - {seed.repository} + void readLocalApi()?.shell.openExternal(seed.url)} + className={cn( + "font-medium underline-offset-2 hover:underline", + statePresentation?.toneClassName, + )} + aria-label={`Open pull request #${seed.number} on host`} > #{seed.number} - + + ) : ( <> - + #{number ?? "…"} )}
-
- {actions ?? } - -
+
+
+ {actions ?? } + + {onClose ? ( + + ) : null}
-
- {seed ? ( -

{seed.title}

- ) : ( - - )} -
- {seed ? ( - <> - - - updated {formatRelativeTimeLabel(seed.updatedAt)} - - - ) : ( - <> - - - - )} -
-
- {seed ? ( - - {seed.baseBranch} - - {seed.headBranch} - - ) : ( - <> - - - - - )} -
- +
+
+
{seed ? ( - +
+

{seed.title}

+
) : ( - +
+ +
)} +
+ {seed ? ( + + + updated {formatRelativeTimeLabel(seed.updatedAt)} + + ) : ( + + + + + + + + )} + {checkout ? ( + + ) : null} +
+ +
+ + {seed ? ( + + {seed.baseBranch} + + ) : ( + + + + )} + + {seed ? ( + + ) : ( + + )} + + + + + {changedFiles === null ? ( + + ) : ( + `${changedFiles.toLocaleString()} ${changedFiles === 1 ? "file" : "files"}` + )} + + {seed ? ( + + ) : ( + + )} + +
-
-
- - - -
+
+
-
-
-
- - -
-
- - - -
-
-
-
- - -
-
- {seed?.labels ? ( - seed.labels.slice(0, 3).map((label) => { - const color = pullRequestLabelColor(label.color); - return ( - - - {label.name} - - ); - }) - ) : ( - <> - - - - )} +
+
+
+ + + Reviewers + + + + +
-
-
-
- - +
+ + + Labels + + + {entry?.labels ? ( + entry.labels.length > 0 ? ( + entry.labels.map((label) => { + const color = pullRequestLabelColor(label.color); + return ( + + + {label.name} + + ); + }) + ) : ( + None + ) + ) : ( + <> + + + + )} + +
-
-
-
- - +
+
+
+ Description + +
-
- - - - +
+ +
+ + + + +
diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index b2096c87ded9..4470e6c307c0 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -275,7 +275,7 @@ function MetaRow({ children: ReactNode; }) { return ( -
+
{icon} {label} diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index 5e75851f08ad..ac0c9d01d17e 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -7,7 +7,9 @@ import { type PullRequestComment, type PullRequestDetail, type PullRequestDetailView, + type PullRequestRef, type PullRequestReviewThread, + type RepositoryIdentity, type ThreadPullRequestLink, } from "@t3tools/contracts"; import { describe, expect, it } from "vite-plus/test"; @@ -27,6 +29,7 @@ import { stripPullRequestHandoffReferences, isPullRequestVerdictStale, isStackedPullRequestBase, + loadingPullRequestCheckoutCommand, pullRequestPanelContext, latestPullRequestReviewOutcomes, newestPullRequestCommitAt, @@ -94,6 +97,49 @@ describe("pull request checkout commands", () => { "git fetch 'https://forgejo.local/maria/repo'\\''$(echo nope)' refs/pull/42/head && git checkout -B pulls/42 FETCH_HEAD", ); }); + + const reference = (host?: string): PullRequestRef => ({ + projectId: ProjectId.make("project-1"), + ...(host === undefined ? {} : { host }), + repository: "acme/web", + number: 42, + }); + const identity = (provider: string, canonicalKey: string): RepositoryIdentity => ({ + canonicalKey, + locator: { + source: "git-remote", + remoteName: "origin", + remoteUrl: "git@github.com:acme/web.git", + }, + provider, + }); + + it("uses a public host when no repository identity is available", () => { + expect(loadingPullRequestCheckoutCommand(reference("github.com"), undefined)).toBe( + "gh pr checkout 42", + ); + expect(loadingPullRequestCheckoutCommand(reference("gitlab.com"), null)).toBe( + "glab mr checkout 42", + ); + }); + + it("uses a matching enterprise identity and rejects an explicit host mismatch", () => { + const enterprise = identity("github", "github.example.test/acme/web"); + expect(loadingPullRequestCheckoutCommand(reference("github.example.test"), enterprise)).toBe( + "gh pr checkout 42", + ); + expect(loadingPullRequestCheckoutCommand(reference("github.com"), enterprise)).toBeNull(); + }); + + it("does not infer a number-only command without a trusted provider", () => { + expect(loadingPullRequestCheckoutCommand(reference(), undefined)).toBeNull(); + expect( + loadingPullRequestCheckoutCommand( + reference("github.com"), + identity("gitlab", "gitlab.com/acme/web"), + ), + ).toBeNull(); + }); }); const TIMELINE_SOURCE: Pick< diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 733e314b770f..2ec4863072dc 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -140,6 +140,22 @@ export function pullRequestCheckoutCommand( } } +/** Build a checkout command from identity metadata while the detail request is still pending. */ +export function loadingPullRequestCheckoutCommand( + reference: PullRequestRef, + identity: RepositoryIdentity | null | undefined, +): string | null { + const host = reference.host?.trim().toLowerCase(); + const provider = + identity?.provider ?? + (host === "github.com" ? "github" : host === "gitlab.com" ? "gitlab" : null); + if (provider !== "github" && provider !== "gitlab" && provider !== "azure-devops") return null; + if (identity?.provider !== undefined && host && pullRequestHostOf(identity, provider) !== host) { + return null; + } + return pullRequestCheckoutCommand(provider, reference.number, ""); +} + /** Activity changes only when the same host resource reports a newer revision. */ export function shouldRefreshPullRequestActivity( previous: { readonly key: string; readonly updatedAt: string } | null, From 55c24273a025874cdb8a96a25ee0c59eeb4e3051 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sun, 20 Sep 2026 13:48:27 -0700 Subject: [PATCH 05/24] fix(desktop): align preview recording cursors and show input feedback (#12779) --- apps/desktop/src/ipc/channels.ts | 2 + apps/desktop/src/ipc/methods/preview.ts | 25 +- apps/desktop/src/preload.ts | 10 + apps/desktop/src/preview/GuestProtocol.ts | 5 + apps/desktop/src/preview/Manager.test.ts | 352 ++++++++++++++---- apps/desktop/src/preview/Manager.ts | 154 +++++++- apps/desktop/src/preview/PickPreload.ts | 82 ++++ apps/desktop/src/preview/RecordingCursor.ts | 197 ++++++++++ .../src/preview/RecordingInput.test.ts | 51 +++ apps/desktop/src/preview/RecordingInput.ts | 58 +++ apps/web/src/browser/browserRecording.ts | 25 +- .../src/browser/recordingCompositor.test.ts | 179 +++++++++ apps/web/src/browser/recordingCompositor.ts | 190 ++++++++++ .../components/preview/AgentBrowserCursor.tsx | 8 +- .../components/preview/PreviewView.test.tsx | 43 ++- .../src/components/preview/PreviewView.tsx | 11 +- .../settings/IntegrationsSettings.tsx | 39 ++ .../settings/SettingsPanels.logic.test.ts | 4 + .../settings/SettingsPanels.logic.ts | 4 + .../components/settings/SettingsPanels.tsx | 4 + .../src/components/settings/settingsSearch.ts | 12 + packages/contracts/src/ipc.ts | 25 ++ packages/contracts/src/settings.test.ts | 17 + packages/contracts/src/settings.ts | 8 + 24 files changed, 1398 insertions(+), 107 deletions(-) create mode 100644 apps/desktop/src/preview/RecordingCursor.ts create mode 100644 apps/desktop/src/preview/RecordingInput.test.ts create mode 100644 apps/desktop/src/preview/RecordingInput.ts create mode 100644 apps/web/src/browser/recordingCompositor.test.ts create mode 100644 apps/web/src/browser/recordingCompositor.ts diff --git a/apps/desktop/src/ipc/channels.ts b/apps/desktop/src/ipc/channels.ts index 226793657848..f49b6dbd968e 100644 --- a/apps/desktop/src/ipc/channels.ts +++ b/apps/desktop/src/ipc/channels.ts @@ -113,3 +113,5 @@ export const PREVIEW_POINTER_EVENT_CHANNEL = "desktop:preview-pointer-event"; export const MAC_PERMISSION_HELPER_CHANNEL = "desktop:mac-permission-helper"; export const CHECK_SYSTEM_PERMISSION_CHANNEL = "desktop:check-system-permission"; + +export const PREVIEW_RECORDING_INPUT_CHANNEL = "desktop:preview-recording-input"; diff --git a/apps/desktop/src/ipc/methods/preview.ts b/apps/desktop/src/ipc/methods/preview.ts index 5fb7eff99fc6..36cf7c9abe15 100644 --- a/apps/desktop/src/ipc/methods/preview.ts +++ b/apps/desktop/src/ipc/methods/preview.ts @@ -30,11 +30,13 @@ import { } from "@t3tools/contracts"; import * as Effect from "effect/Effect"; import * as Schema from "effect/Schema"; +import * as Option from "effect/Option"; import * as NodeURL from "node:url"; import * as ElectronWindow from "../../electron/ElectronWindow.ts"; import * as BrowserImport from "../../preview/BrowserImport/BrowserImport.ts"; import * as PreviewManager from "../../preview/Manager.ts"; +import * as DesktopClientSettings from "../../settings/DesktopClientSettings.ts"; import { PREVIEW_WEBVIEW_PREFERENCES } from "../../preview/WebviewPreferences.ts"; import * as IpcChannels from "../channels.ts"; import * as DesktopIpc from "../DesktopIpc.ts"; @@ -50,6 +52,9 @@ export const installPreviewEventForwarding = Effect.fn( yield* manager.subscribeRecordingFrames((frame) => electronWindow.sendAll(IpcChannels.PREVIEW_RECORDING_FRAME_CHANNEL, frame), ); + yield* manager.subscribeRecordingInputs((event) => + electronWindow.sendAll(IpcChannels.PREVIEW_RECORDING_INPUT_CHANNEL, event), + ); yield* manager.subscribePointerEvents((event) => electronWindow.sendAll(IpcChannels.PREVIEW_POINTER_EVENT_CHANNEL, event), ); @@ -180,11 +185,21 @@ export const cancelPickElement = tabMethod( "desktop.ipc.preview.cancelPickElement", (manager, tabId) => manager.cancelPickElement(tabId), ); -export const startRecording = tabMethod( - IpcChannels.PREVIEW_RECORDING_START_CHANNEL, - "desktop.ipc.preview.startRecording", - (manager, tabId) => manager.startRecording(tabId), -); +export const startRecording = DesktopIpc.makeIpcMethod({ + channel: IpcChannels.PREVIEW_RECORDING_START_CHANNEL, + payload: DesktopPreviewTabInputSchema, + result: Schema.Void, + handler: Effect.fn("desktop.ipc.preview.startRecording")(function* ({ tabId }) { + const manager = yield* PreviewManager.PreviewManager; + const store = yield* DesktopClientSettings.DesktopClientSettings; + const settings = yield* store.get; + const options = Option.map(settings, (value) => ({ + showKeyPresses: value.browserRecordingShowKeyPresses, + showMousePresses: value.browserRecordingShowMousePresses, + })); + yield* manager.startRecording(tabId, Option.getOrUndefined(options)); + }), +}); export const stopRecording = tabMethod( IpcChannels.PREVIEW_RECORDING_STOP_CHANNEL, "desktop.ipc.preview.stopRecording", diff --git a/apps/desktop/src/preload.ts b/apps/desktop/src/preload.ts index 885b31a64d6b..7256f610fbeb 100644 --- a/apps/desktop/src/preload.ts +++ b/apps/desktop/src/preload.ts @@ -1,6 +1,7 @@ import type { DesktopBridge, DesktopPreviewPointerEvent, + DesktopPreviewRecordingInputEvent, DesktopPreviewRecordingFrame, DesktopPreviewTabState, DesktopSnapShotEvent, @@ -327,6 +328,15 @@ contextBridge.exposeInMainWorld("desktopBridge", { ipcRenderer.invoke(IpcChannels.PREVIEW_PICTURE_IN_PICTURE_CLOSE_CHANNEL, { tabId }), }, recording: { + onInput: (listener) => { + const wrappedListener = (_event: Electron.IpcRendererEvent, event: unknown) => { + if (typeof event !== "object" || event === null) return; + listener(event as DesktopPreviewRecordingInputEvent); + }; + ipcRenderer.on(IpcChannels.PREVIEW_RECORDING_INPUT_CHANNEL, wrappedListener); + return () => + ipcRenderer.removeListener(IpcChannels.PREVIEW_RECORDING_INPUT_CHANNEL, wrappedListener); + }, startScreencast: (tabId) => ipcRenderer.invoke(IpcChannels.PREVIEW_RECORDING_START_CHANNEL, { tabId }), stopScreencast: (tabId) => diff --git a/apps/desktop/src/preview/GuestProtocol.ts b/apps/desktop/src/preview/GuestProtocol.ts index e63597b71efc..1a73bb30f29e 100644 --- a/apps/desktop/src/preview/GuestProtocol.ts +++ b/apps/desktop/src/preview/GuestProtocol.ts @@ -5,3 +5,8 @@ export const ANNOTATION_CAPTURED_CHANNEL = "preview:annotation-captured"; export const ANNOTATION_THEME_CHANNEL = "preview:annotation-theme"; export const HUMAN_INPUT_CHANNEL = "preview:human-input"; export const MOUSE_NAVIGATE_CHANNEL = "preview:mouse-navigate"; +export const RECORDING_CURSOR_CHANNEL = "preview:recording-cursor"; +export const RECORDING_POINTER_CHANNEL = "preview:recording-pointer"; +export const RECORDING_KEY_CHANNEL = "preview:recording-key"; +export const RECORDING_INPUT_CHANNEL = "preview:recording-input"; +export const RECORDING_CONTROLLER_CHANNEL = "preview:recording-controller"; diff --git a/apps/desktop/src/preview/Manager.test.ts b/apps/desktop/src/preview/Manager.test.ts index 7b76a1b8003a..304efeafefe9 100644 --- a/apps/desktop/src/preview/Manager.test.ts +++ b/apps/desktop/src/preview/Manager.test.ts @@ -1,7 +1,10 @@ import * as NodeVM from "node:vm"; import { it as effectIt } from "@effect/vitest"; import { DESKTOP_PREVIEW_RECORDING_CAPTURE_TRIGGER } from "@t3tools/contracts"; -import type { DesktopPreviewRecordingFrame } from "@t3tools/contracts"; +import type { + DesktopPreviewRecordingFrame, + DesktopPreviewRecordingInputEvent, +} from "@t3tools/contracts"; import { HostProcessPlatform } from "@t3tools/shared/hostProcess"; import * as Cause from "effect/Cause"; import * as Deferred from "effect/Deferred"; @@ -2885,6 +2888,165 @@ describe("PreviewManager", () => { ), ); + effectIt.effect( + "restores the native cursor when recording startup fails, then allows a retry", + () => + withManager((manager) => + Effect.gen(function* () { + const host = makeTestHostWebContents(); + host.executeJavaScript.mockResolvedValueOnce(false); + let cursorActive = false; + const cursorAtCapture: boolean[] = []; + const contents = Object.assign( + makeTestPreviewWebContents( + async () => { + cursorAtCapture.push(cursorActive); + return { + toJPEG: () => Buffer.from("frame"), + getSize: () => ({ width: 800, height: 600 }), + }; + }, + 42, + host, + ), + { + send: (channel: string, active: unknown) => { + if (channel === "preview:recording-cursor") cursorActive = active === true; + }, + }, + ); + fromId.mockReturnValue(contents as never); + yield* manager.createTab("tab_cursor"); + yield* manager.registerWebview("tab_cursor", 42); + const failed = yield* Effect.exit(manager.startRecording("tab_cursor")); + expect(Exit.isFailure(failed)).toBe(true); + expect(cursorAtCapture).toEqual([true]); + expect(cursorActive).toBe(false); + + yield* manager.startRecording("tab_cursor"); + expect(cursorAtCapture).toEqual([true, true]); + expect(cursorActive).toBe(true); + yield* manager.stopRecording("tab_cursor"); + expect(cursorActive).toBe(false); + }), + ), + ); + + effectIt.effect("restores the recording cursor after navigation only while recording", () => + withManager((manager) => + Effect.gen(function* () { + const listeners = new Map void>(); + let cursorActive = false; + let cursorUpdated: (() => void) | undefined; + let inputOptions: unknown; + const options = { showKeyPresses: true, showMousePresses: false }; + const contents = Object.assign( + makeTestPreviewWebContents(async () => ({ + toJPEG: () => Buffer.from("frame"), + getSize: () => ({ width: 800, height: 600 }), + })), + { + on: (event: string, listener: () => void) => listeners.set(event, listener), + send: (channel: string, active: unknown, recordingOptions: unknown) => { + if (channel !== "preview:recording-cursor") return; + cursorActive = active === true; + inputOptions = recordingOptions; + cursorUpdated?.(); + }, + }, + ); + fromId.mockReturnValue(contents as never); + yield* manager.createTab("tab_cursor_reload"); + yield* manager.registerWebview("tab_cursor_reload", 42); + yield* manager.startRecording("tab_cursor_reload", options); + for (const recording of [true, false]) { + if (!recording) yield* manager.stopRecording("tab_cursor_reload"); + // A new document has lost the previous preload's cursor overlay. + cursorActive = false; + const restored = new Promise((resolve) => { + cursorUpdated = resolve; + }); + listeners.get("dom-ready")?.(); + yield* Effect.promise(() => restored); + cursorUpdated = undefined; + expect(cursorActive).toBe(recording); + expect(inputOptions).toEqual(recording ? options : undefined); + } + }), + ), + ); + + effectIt.effect("gates recording decorations and isolates failed subscribers", () => + withManager((manager) => + Effect.gen(function* () { + const callbacks = new Map< + string, + (event: unknown, input: unknown) => Fiber.Fiber | undefined + >(); + const contents = Object.assign( + makeTestPreviewWebContents(async () => ({ + toJPEG: () => Buffer.from("frame"), + getSize: () => ({ width: 800, height: 600 }), + })), + { + ipc: { + on: ( + channel: string, + callback: (event: unknown, input: unknown) => Fiber.Fiber | undefined, + ) => callbacks.set(channel, callback), + off: vi.fn(), + }, + }, + ); + fromId.mockReturnValue(contents as never); + yield* manager.createTab("tab_recording_input"); + yield* manager.registerWebview("tab_recording_input", 42); + const received: DesktopPreviewRecordingInputEvent[] = []; + yield* manager.subscribeRecordingInputs(() => Effect.die("renderer unavailable")); + yield* manager.subscribeRecordingInputs((event) => + Effect.sync(() => { + received.push(event); + }), + ); + const send = (input: unknown) => + Effect.gen(function* () { + const fiber = callbacks.get("preview:recording-input")?.(null, input); + if (fiber) yield* Fiber.join(fiber); + }); + const key = { type: "key", label: "⌘C", held: true, width: 800 }; + const pointer = { + type: "pointer", + phase: "down", + x: 120, + y: 80, + width: 800, + height: 600, + }; + yield* send(key); + expect(received).toEqual([]); + yield* manager.startRecording("tab_recording_input", { + showKeyPresses: true, + showMousePresses: false, + }); + yield* send(key); + yield* send(pointer); + yield* send({ ...key, width: 0 }); + expect(received).toEqual([{ tabId: "tab_recording_input", input: key }]); + yield* manager.stopRecording("tab_recording_input"); + yield* send(key); + expect(received).toHaveLength(1); + yield* manager.startRecording("tab_recording_input", { + showKeyPresses: false, + showMousePresses: true, + }); + yield* send(key); + yield* send(pointer); + expect(received.at(-1)).toEqual({ tabId: "tab_recording_input", input: pointer }); + expect(received).toHaveLength(2); + }), + ), + ); + effectIt.effect("continues native recording when the source warmup fails", () => withManager((manager) => Effect.gen(function* () { @@ -3853,88 +4015,108 @@ describe("PreviewManager", () => { ), ); - effectIt.effect("emits the resolved pointer target before dispatching an automation click", () => - withManager((manager) => - Effect.gen(function* () { - let humanInput: ((_event: unknown, signal: unknown) => void) | undefined; - const activity: string[] = []; - const sendCommand = vi.fn(async (method: string, params?: Record) => { - if (method === "Runtime.evaluate") { - return { - result: { - value: { width: 800, height: 600 }, - }, - }; - } - if (method === "Input.dispatchMouseEvent" && params?.type === "mousePressed") { - activity.push("mousePressed"); - humanInput?.({}, { kind: "pointer", x: params.x, y: params.y, button: 0 }); - } - return undefined; - }); - fromId.mockReturnValue({ - id: 42, - isDestroyed: () => false, - getType: () => "webview", - getURL: () => "https://example.com", - getTitle: () => "Example", - isLoading: () => false, - isDevToolsOpened: () => false, - getZoomFactor: () => 1, - setZoomFactor: vi.fn(), - setAudioMuted: vi.fn(), - isCurrentlyAudible: () => false, - on: vi.fn(), - off: vi.fn(), - ipc: { - on: vi.fn((channel: string, listener: typeof humanInput) => { - if (channel === "preview:human-input") humanInput = listener; - }), - off: vi.fn(), - }, - send: webviewSend, - navigationHistory: { canGoBack: () => false, canGoForward: () => false }, - setIgnoreMenuShortcuts: vi.fn(), - setWindowOpenHandler: vi.fn(), - debugger: { - isAttached: () => false, - attach: vi.fn(), - sendCommand, + effectIt.effect( + "records the resolved pointer target before dispatching an automation click", + () => + withManager((manager) => + Effect.gen(function* () { + let humanInput: ((_event: unknown, signal: unknown) => void) | undefined; + const activity: string[] = []; + const sendCommand = vi.fn(async (method: string, params?: Record) => { + if (method === "Runtime.evaluate") { + return { + result: { + value: { width: 800, height: 600 }, + }, + }; + } + if (method === "Input.dispatchMouseEvent" && params?.type === "mousePressed") { + activity.push("mousePressed"); + humanInput?.({}, { kind: "pointer", x: params.x, y: params.y, button: 0 }); + } + return undefined; + }); + fromId.mockReturnValue({ + id: 42, + hostWebContents: makeTestHostWebContents(), + capturePage: vi.fn(async () => ({ toPNG: () => Buffer.from("frame") })), + setBackgroundThrottling: vi.fn(), + isDestroyed: () => false, + getType: () => "webview", + getURL: () => "https://example.com", + getTitle: () => "Example", + isLoading: () => false, + isDevToolsOpened: () => false, + getZoomFactor: () => 1, + setZoomFactor: vi.fn(), + setAudioMuted: vi.fn(), + isCurrentlyAudible: () => false, on: vi.fn(), off: vi.fn(), - }, - } as never); - - yield* manager.subscribePointerEvents((event) => - Effect.sync(() => { - activity.push(event.phase); - }), - ); - yield* manager.createTab("tab_1"); - yield* manager.registerWebview("tab_1", 42); - const click = yield* manager - .automationClick("tab_1", { x: 120, y: 80 }) - .pipe(Effect.forkChild({ startImmediately: true })); - yield* TestClock.adjust(200); - yield* Fiber.join(click); + ipc: { + on: vi.fn((channel: string, listener: typeof humanInput) => { + if (channel === "preview:human-input") humanInput = listener; + }), + off: vi.fn(), + }, + send: webviewSend, + navigationHistory: { canGoBack: () => false, canGoForward: () => false }, + setIgnoreMenuShortcuts: vi.fn(), + setWindowOpenHandler: vi.fn(), + debugger: { + isAttached: () => false, + attach: vi.fn(), + sendCommand, + on: vi.fn(), + off: vi.fn(), + }, + } as never); - expect(activity).toEqual(["move", "click", "mousePressed"]); - expect(sendCommand).toHaveBeenCalledWith("Input.dispatchMouseEvent", { - type: "mousePressed", - x: 120, - y: 80, - button: "left", - clickCount: 1, - }); - expect(sendCommand).toHaveBeenCalledWith("Input.dispatchMouseEvent", { - type: "mouseReleased", - x: 120, - y: 80, - button: "left", - clickCount: 1, - }); - }), - ), + yield* manager.subscribePointerEvents((event) => + Effect.sync(() => { + activity.push(event.phase); + }), + ); + yield* manager.createTab("tab_1"); + yield* manager.registerWebview("tab_1", 42); + yield* manager.startRecording("tab_1"); + const click = yield* manager + .automationClick("tab_1", { x: 120, y: 80 }) + .pipe(Effect.forkChild({ startImmediately: true })); + yield* TestClock.adjust(200); + yield* Fiber.join(click); + + expect(activity).toEqual(["move", "click", "mousePressed"]); + expect( + webviewSend.mock.calls + .filter(([channel]) => channel === "preview:recording-controller") + .map(([, controller]) => controller), + ).toEqual(["agent", "none"]); + + const recordedPointer = webviewSend.mock.calls + .filter(([channel]) => channel === "preview:recording-pointer") + .map(([, event]) => event); + expect(recordedPointer).toEqual([ + expect.objectContaining({ phase: "move", x: 120, y: 80 }), + expect.objectContaining({ phase: "click", x: 120, y: 80 }), + ]); + expect(sendCommand).toHaveBeenCalledWith("Input.dispatchMouseEvent", { + type: "mousePressed", + x: 120, + y: 80, + button: "left", + clickCount: 1, + }); + yield* manager.stopRecording("tab_1"); + expect(sendCommand).toHaveBeenCalledWith("Input.dispatchMouseEvent", { + type: "mouseReleased", + x: 120, + y: 80, + button: "left", + clickCount: 1, + }); + }), + ), ); effectIt.effect("types in background webviews and enables native key input", () => @@ -4230,6 +4412,9 @@ describe("PreviewManager", () => { }); fromId.mockReturnValue({ id: 42, + hostWebContents: makeTestHostWebContents(), + capturePage: vi.fn(async () => ({ toPNG: () => Buffer.from("frame") })), + setBackgroundThrottling: vi.fn(), isDestroyed: () => false, getType: () => "webview", getURL: () => "https://example.com", @@ -4263,6 +4448,7 @@ describe("PreviewManager", () => { yield* manager.createTab("tab_1"); yield* manager.registerWebview("tab_1", 42); + yield* manager.startRecording("tab_1"); const click = yield* manager .automationClick("tab_1", { x: 120, y: 80 }) @@ -4270,6 +4456,12 @@ describe("PreviewManager", () => { yield* TestClock.adjust(200); const exit = yield* Fiber.await(click); expect(Exit.isFailure(exit)).toBe(true); + expect( + webviewSend.mock.calls + .filter(([channel]) => channel === "preview:recording-controller") + .map(([, controller]) => controller), + ).toEqual(["agent", "human", "none"]); + yield* manager.stopRecording("tab_1"); if (Exit.isSuccess(exit)) return; const error = Option.getOrThrow(Cause.findErrorOption(exit.cause)); expect(error).toMatchObject({ diff --git a/apps/desktop/src/preview/Manager.ts b/apps/desktop/src/preview/Manager.ts index a2e34fe54736..468c47065315 100644 --- a/apps/desktop/src/preview/Manager.ts +++ b/apps/desktop/src/preview/Manager.ts @@ -6,7 +6,10 @@ * here). Single layer-scoped browser session partition. */ import * as NodeCrypto from "node:crypto"; -import { DESKTOP_PREVIEW_RECORDING_CAPTURE_TRIGGER } from "@t3tools/contracts"; +import { + DesktopPreviewRecordingInputSchema, + DESKTOP_PREVIEW_RECORDING_CAPTURE_TRIGGER, +} from "@t3tools/contracts"; import type { DesktopPreviewAnnotationTheme, DesktopPreviewAutomationStatus, @@ -18,6 +21,7 @@ import type { PreviewAnnotationSubmissionResult, DesktopPreviewRecordingArtifact, DesktopPreviewRecordingFrame, + DesktopPreviewRecordingInputEvent, DesktopPreviewScreenshotArtifact, DesktopPreviewTabDefaults, PreviewAutomationClickInput, @@ -71,6 +75,11 @@ import { ELEMENT_PICKED_CHANNEL, HUMAN_INPUT_CHANNEL, MOUSE_NAVIGATE_CHANNEL, + RECORDING_CURSOR_CHANNEL, + RECORDING_POINTER_CHANNEL, + RECORDING_KEY_CHANNEL, + RECORDING_INPUT_CHANNEL, + RECORDING_CONTROLLER_CHANNEL, START_PICK_CHANNEL, } from "./GuestProtocol.ts"; import { isPreviewAnnotationPayload } from "./PickedElementPayload.ts"; @@ -81,6 +90,7 @@ import { previewAutomationEditingCommandExpression, } from "./PreviewKeyboard.ts"; import { captureFavicon, safeHttpOrigin, selectFaviconCandidates } from "./FaviconCapture.ts"; +import { DEFAULT_RECORDING_INPUT_OPTIONS, type RecordingInputOptions } from "./RecordingInput.ts"; export type PreviewNavStatus = | { kind: "Idle" } @@ -437,6 +447,7 @@ interface ManagedListeners { type FrameCaptureConsumer = "picture-in-picture" | "recording"; interface FrameCaptureSession { + readonly recordingInputOptions?: RecordingInputOptions; readonly scope: Scope.Closeable | null; readonly consumers: ReadonlySet; readonly unthrottledWebContentsIds: ReadonlySet; @@ -485,6 +496,10 @@ interface BrowserDiagnostics { readonly requests: ReadonlyMap; } +const isRecordingInput = Schema.is(DesktopPreviewRecordingInputSchema); + +type RecordingInputListener = (event: DesktopPreviewRecordingInputEvent) => Effect.Effect; + type PointerEventListener = (event: DesktopPreviewPointerEvent) => Effect.Effect; interface ExpectedAgentInput { @@ -636,6 +651,9 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function const attachedRef = yield* Ref.make>(new Map()); const listenersRef = yield* Ref.make>(new Set()); const pointerEventListenersRef = yield* Ref.make>(new Set()); + const recordingInputListenersRef = yield* Ref.make>( + new Set(), + ); const recordingFrameListenersRef = yield* Ref.make>( new Set(), ); @@ -821,6 +839,20 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function return Effect.succeed([undefined, sessions] as const); } return setFrameCaptureWebContentsBackgroundThrottling(wc, false).pipe( + Effect.tap(() => + Effect.gen(function* () { + if (!current.consumers.has("recording")) return; + const tab = (yield* SynchronizedRef.get(tabsRef)).get(tabId); + yield* attempt({ operation: "recording.cursor", tabId, webContentsId: wc.id }, () => + wc.send( + RECORDING_CURSOR_CHANNEL, + true, + current.recordingInputOptions, + tab?.controller, + ), + ); + }), + ), Effect.map( () => [ @@ -846,6 +878,15 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function if (!current || !current.consumers.has(consumer)) { return [undefined, sessions] as const; } + if (consumer === "recording") { + yield* Effect.forEach(current.unthrottledWebContentsIds, (id) => + attempt({ operation: "recording.cursor", tabId, webContentsId: id }, () => { + const contents = webContents.fromId(id); + if (contents && !contents.isDestroyed()) + contents.send(RECORDING_CURSOR_CHANNEL, false); + }).pipe(Effect.ignore), + ); + } const consumers = new Set(current.consumers); consumers.delete(consumer); if (consumers.size > 0) { @@ -896,7 +937,7 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function }); const deliverEvent = ( - eventKind: "state-change" | "recording-frame" | "pointer-event", + eventKind: "state-change" | "recording-frame" | "recording-input" | "pointer-event", tabId: string, delivery: () => Effect.Effect, ) => @@ -933,6 +974,7 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function const update = Effect.fn("PreviewManager.update")(function* ( tabId: string, patch: Partial, + humanPoint?: { readonly x: number; readonly y: number }, ) { const updatedAt = yield* currentIso; const next = yield* SynchronizedRef.modify(tabsRef, (tabs) => { @@ -950,7 +992,20 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function // can commit between the modify above and here, and republishing this // snapshot would roll the UI back to a value that writer will not send // again because it suppresses unchanged audibility. - if (Option.isSome(next)) yield* emitIfCurrent(tabId, next.value); + if (Option.isSome(next)) { + if (patch.controller !== undefined && next.value.webContentsId != null) { + const capture = (yield* SynchronizedRef.get(frameCaptureSessionsRef)).get(tabId); + const webContentsId = next.value.webContentsId; + if (capture?.consumers.has("recording")) { + yield* attempt({ operation: "recording.controller", tabId }, () => { + const contents = webContents.fromId(webContentsId); + if (contents && !contents.isDestroyed()) + contents.send(RECORDING_CONTROLLER_CHANNEL, patch.controller, humanPoint); + }).pipe(Effect.ignore); + } + } + yield* emitIfCurrent(tabId, next.value); + } }); /** @@ -1728,6 +1783,21 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function const sync = () => runFork(syncState(true)); const syncNavigation = () => runFork(syncState(false, true)); const syncInPageNavigation = () => runFork(syncState(false)); + const restoreRecordingCursor = () => + runFork( + Effect.gen(function* () { + const session = (yield* SynchronizedRef.get(frameCaptureSessionsRef)).get(tabId); + if (!wc.isDestroyed()) { + const tab = (yield* SynchronizedRef.get(tabsRef)).get(tabId); + wc.send( + RECORDING_CURSOR_CHANNEL, + session?.consumers.has("recording") ?? false, + session?.recordingInputOptions, + tab?.controller, + ); + } + }), + ); const navigationStarted = ( event: Electron.Event, ) => { @@ -1860,13 +1930,37 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function copy.set(tabId, (epochs.get(tabId) ?? 0) + 1); }), ); - yield* update(tabId, { controller: "human" }); + yield* update( + tabId, + { controller: "human" }, + isPreviewInputSignal(rawSignal) && rawSignal.kind === "pointer" + ? { x: rawSignal.x, y: rawSignal.y } + : undefined, + ); yield* Effect.sleep(750); const tabs = yield* SynchronizedRef.get(tabsRef); if (tabs.get(tabId)?.controller === "human") { yield* update(tabId, { controller: "none" }); } }); + const recordingInput = (_event: unknown, input: unknown) => { + if (!isRecordingInput(input)) return; + return runFork( + Effect.gen(function* () { + const tab = (yield* SynchronizedRef.get(tabsRef)).get(tabId); + const capture = (yield* SynchronizedRef.get(frameCaptureSessionsRef)).get(tabId); + if (tab?.webContentsId !== wc.id || !capture?.consumers.has("recording")) return; + if (input.type === "key" && !capture.recordingInputOptions?.showKeyPresses) return; + if (input.type === "pointer" && !capture.recordingInputOptions?.showMousePresses) return; + const listeners = yield* Ref.get(recordingInputListenersRef); + yield* Effect.forEach( + listeners, + (listener) => deliverEvent("recording-input", tabId, () => listener({ tabId, input })), + { discard: true }, + ); + }), + ); + }; const humanInput = (_event: unknown, rawSignal?: unknown): void => { runFork(handleHumanInput(rawSignal)); }; @@ -1928,11 +2022,13 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function wc.off("page-favicon-updated", faviconUpdated as never); wc.off("did-start-loading", sync); wc.off("did-stop-loading", sync); + wc.off("dom-ready", restoreRecordingCursor); wc.off("did-fail-load", failed as never); wc.off("audio-state-changed", audioStateChanged); wc.off("did-create-window", windowCreated); wc.off("before-input-event", beforeInput); wc.ipc.off(HUMAN_INPUT_CHANNEL, humanInput); + wc.ipc.off(RECORDING_INPUT_CHANNEL, recordingInput); wc.ipc.off(MOUSE_NAVIGATE_CHANNEL, mouseNavigate); }).pipe(Effect.ignore), ); @@ -1948,9 +2044,11 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function wc.on("page-favicon-updated", faviconUpdated as never); wc.on("did-start-loading", sync); wc.on("did-stop-loading", sync); + wc.on("dom-ready", restoreRecordingCursor); wc.on("did-fail-load", failed as never); wc.on("audio-state-changed", audioStateChanged); wc.ipc.on(HUMAN_INPUT_CHANNEL, humanInput); + wc.ipc.on(RECORDING_INPUT_CHANNEL, recordingInput); wc.ipc.on(MOUSE_NAVIGATE_CHANNEL, mouseNavigate); wc.setWindowOpenHandler((details) => { if (previewWindowOpenAction(details) === "popup") { @@ -3405,7 +3503,10 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function }); }; - const startRecording = Effect.fn("PreviewManager.startRecording")(function* (tabId: string) { + const startRecording = Effect.fn("PreviewManager.startRecording")(function* ( + tabId: string, + options: RecordingInputOptions = DEFAULT_RECORDING_INPUT_OPTIONS, + ) { if ((yield* Ref.get(closingTabIdsRef)).has(tabId)) { return yield* new PreviewTabNotFoundError({ tabId }); } @@ -3413,11 +3514,21 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function tabId, Effect.gen(function* () { yield* startFrameCapture(tabId, "recording"); + yield* SynchronizedRef.update(frameCaptureSessionsRef, (sessions) => + replaceMap(sessions, (copy) => { + const current = copy.get(tabId); + if (current) copy.set(tabId, { ...current, recordingInputOptions: options }); + }), + ); const wc = yield* requireWebContents(tabId); const requestWebContents = wc.hostWebContents; if (requestWebContents === null) { return yield* new PreviewMainWindowClosedError({ tabId }); } + const tab = (yield* SynchronizedRef.get(tabsRef)).get(tabId); + yield* attempt({ operation: "recording.cursor", tabId, webContentsId: wc.id }, () => + wc.send(RECORDING_CURSOR_CHANNEL, true, options, tab?.controller), + ); yield* attemptPromise( { operation: "recording.warmSource", @@ -3720,6 +3831,15 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function const emitPointerEvent = Effect.fn("PreviewManager.emitPointerEvent")(function* ( event: DesktopPreviewPointerEvent, ) { + const recording = (yield* SynchronizedRef.get(frameCaptureSessionsRef)).get(event.tabId); + const tab = (yield* SynchronizedRef.get(tabsRef)).get(event.tabId); + const webContentsId = tab?.webContentsId; + if (recording?.consumers.has("recording") && webContentsId != null) { + yield* attempt({ operation: "recording.pointer", tabId: event.tabId }, () => { + const contents = webContents.fromId(webContentsId); + if (contents && !contents.isDestroyed()) contents.send(RECORDING_POINTER_CHANNEL, event); + }); + } const listeners = yield* Ref.get(pointerEventListenersRef); yield* Effect.forEach( listeners, @@ -4110,6 +4230,18 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function const keySequence = makePreviewAutomationNativeKeySequence(input, { isMac: hostPlatform === "darwin", }); + const recording = (yield* SynchronizedRef.get(frameCaptureSessionsRef)).get(tabId); + if (recording?.consumers.has("recording") && recording.recordingInputOptions?.showKeyPresses) { + yield* attempt({ operation: "recording.key", tabId, webContentsId: wc.id }, () => + wc.send(RECORDING_KEY_CHANNEL, { + key: keySequence.signal.key || input.key, + metaKey: input.modifiers?.includes("Meta") ?? false, + ctrlKey: input.modifiers?.includes("Control") ?? false, + altKey: input.modifiers?.includes("Alt") ?? false, + shiftKey: input.modifiers?.includes("Shift") ?? false, + }), + ); + } // CDP keyboard dispatch follows the embedder's focused renderer, and // WebContents.focus() is a no-op for webview guests. Native input targets // this guest's widget directly, so Enter cannot submit the host composer. @@ -4470,6 +4602,7 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function Ref.set(expectedAgentInputsRef, new Map()), Ref.set(pointerEventListenersRef, new Set()), Ref.set(recordingFrameListenersRef, new Set()), + Ref.set(recordingInputListenersRef, new Set()), ], { discard: true }, ); @@ -4513,6 +4646,8 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function stopRecording, subscribePointerEvents: (listener: PointerEventListener) => subscribe(pointerEventListenersRef, listener), + subscribeRecordingInputs: (listener: RecordingInputListener) => + subscribe(recordingInputListenersRef, listener), subscribeRecordingFrames: (listener: RecordingFrameListener) => subscribe(recordingFrameListenersRef, listener), subscribeStateChanges: (listener: Listener) => subscribe(listenersRef, listener), @@ -4877,7 +5012,10 @@ export class PreviewManager extends Context.Service< readonly copyArtifactToClipboard: (path: string) => Effect.Effect; readonly openPictureInPicture: (tabId: string) => Effect.Effect; readonly closePictureInPicture: (tabId: string) => Effect.Effect; - readonly startRecording: (tabId: string) => Effect.Effect; + readonly startRecording: ( + tabId: string, + options?: RecordingInputOptions, + ) => Effect.Effect; readonly stopRecording: (tabId: string) => Effect.Effect; readonly saveRecording: ( tabId: string, @@ -4918,6 +5056,9 @@ export class PreviewManager extends Context.Service< readonly subscribePointerEvents: ( listener: PointerEventListener, ) => Effect.Effect; + readonly subscribeRecordingInputs: ( + listener: RecordingInputListener, + ) => Effect.Effect; readonly subscribeRecordingFrames: ( listener: RecordingFrameListener, ) => Effect.Effect; @@ -5011,6 +5152,7 @@ export const make = Effect.gen(function* PreviewManagerMake() { subscribeStateChanges: operations.subscribeStateChanges, subscribePointerEvents: operations.subscribePointerEvents, subscribeRecordingFrames: operations.subscribeRecordingFrames, + subscribeRecordingInputs: operations.subscribeRecordingInputs, }); }).pipe(Effect.withSpan("PreviewManager.make")); diff --git a/apps/desktop/src/preview/PickPreload.ts b/apps/desktop/src/preview/PickPreload.ts index 6155c4119ec8..78351e9d79c3 100644 --- a/apps/desktop/src/preview/PickPreload.ts +++ b/apps/desktop/src/preview/PickPreload.ts @@ -16,6 +16,8 @@ import type { import { resolveAnnotationSubmission } from "./AnnotationKeyboard.ts"; import { previewAnnotationStyles } from "./AnnotationStyles.generated.ts"; +import { installRecordingCursor } from "./RecordingCursor.ts"; +import { DEFAULT_RECORDING_INPUT_OPTIONS } from "./RecordingInput.ts"; import { ANNOTATION_CAPTURED_CHANNEL, ANNOTATION_THEME_CHANNEL, @@ -23,6 +25,11 @@ import { ELEMENT_PICKED_CHANNEL, HUMAN_INPUT_CHANNEL, MOUSE_NAVIGATE_CHANNEL, + RECORDING_CURSOR_CHANNEL, + RECORDING_POINTER_CHANNEL, + RECORDING_KEY_CHANNEL, + RECORDING_INPUT_CHANNEL, + RECORDING_CONTROLLER_CHANNEL, START_PICK_CHANNEL, } from "./GuestProtocol.ts"; const OVERLAY_ATTRIBUTE = "data-t3code-annotation-ui"; @@ -35,6 +42,80 @@ const ELEMENT_CONTEXT_TIMEOUT_MS = 5_000; const CONTENT_LAYER_Z_INDEX = 1; const CHROME_LAYER_Z_INDEX = 10; +let recordingCursor: ReturnType | null = null; +ipcRenderer.on( + RECORDING_CURSOR_CHANNEL, + (_event, active: unknown, inputOptions: unknown, controller: unknown) => { + if (active === true) { + const options = + typeof inputOptions === "object" && inputOptions !== null + ? { + showKeyPresses: + "showKeyPresses" in inputOptions && inputOptions.showKeyPresses === true, + showMousePresses: + "showMousePresses" in inputOptions && inputOptions.showMousePresses === true, + } + : DEFAULT_RECORDING_INPUT_OPTIONS; + recordingCursor ??= installRecordingCursor(document, window, options, (input) => + ipcRenderer.send(RECORDING_INPUT_CHANNEL, input), + ); + recordingCursor.setTheme(annotationTheme); + if (controller === "agent" || controller === "human" || controller === "none") + recordingCursor.setController(controller); + } else { + recordingCursor?.dispose(); + recordingCursor = null; + } + }, +); +ipcRenderer.on(RECORDING_CONTROLLER_CHANNEL, (_event, controller: unknown, point: unknown) => { + const humanPoint = + typeof point === "object" && + point !== null && + "x" in point && + typeof point.x === "number" && + Number.isFinite(point.x) && + "y" in point && + typeof point.y === "number" && + Number.isFinite(point.y) + ? { x: point.x, y: point.y } + : undefined; + if (controller === "agent" || controller === "human" || controller === "none") + recordingCursor?.setController(controller, humanPoint); +}); +ipcRenderer.on(RECORDING_KEY_CHANNEL, (_event, input: unknown) => { + if ( + typeof input !== "object" || + input === null || + !("key" in input) || + typeof input.key !== "string" + ) + return; + recordingCursor?.keyPress({ + key: input.key, + metaKey: "metaKey" in input && input.metaKey === true, + ctrlKey: "ctrlKey" in input && input.ctrlKey === true, + altKey: "altKey" in input && input.altKey === true, + shiftKey: "shiftKey" in input && input.shiftKey === true, + }); +}); +ipcRenderer.on(RECORDING_POINTER_CHANNEL, (_event, point: unknown) => { + if ( + typeof point === "object" && + point !== null && + "x" in point && + typeof point.x === "number" && + Number.isFinite(point.x) && + "y" in point && + typeof point.y === "number" && + Number.isFinite(point.y) + ) + recordingCursor?.move( + { x: point.x, y: point.y }, + "phase" in point && point.phase === "click" ? "click" : "move", + ); +}); + type AnnotationTool = "select" | "marquee" | "draw" | "erase"; interface SelectedElement { @@ -1361,6 +1442,7 @@ ipcRenderer.on(START_PICK_CHANNEL, (_event, theme: DesktopPreviewAnnotationTheme }); ipcRenderer.on(ANNOTATION_THEME_CHANNEL, (_event, theme: DesktopPreviewAnnotationTheme) => { annotationTheme = theme; + recordingCursor?.setTheme(theme); activeSession?.applyTheme(theme); }); ipcRenderer.on(CANCEL_PICK_CHANNEL, () => activeSession?.teardown(false)); diff --git a/apps/desktop/src/preview/RecordingCursor.ts b/apps/desktop/src/preview/RecordingCursor.ts new file mode 100644 index 000000000000..70b0a43be43a --- /dev/null +++ b/apps/desktop/src/preview/RecordingCursor.ts @@ -0,0 +1,197 @@ +import type { + DesktopPreviewAnnotationTheme, + DesktopPreviewRecordingInput, +} from "@t3tools/contracts"; + +import { + DEFAULT_RECORDING_INPUT_OPTIONS, + recordingKeyLabel, + recordingKeysAreSensitive, + type RecordingInputOptions, + type RecordingKeyPress, +} from "./RecordingInput.ts"; + +/** + * Chromium's capture cursor uses native window bounds, which do not follow a + * webview's CSS placement or scale. Draw it in the guest's coordinate space + * while recording, and make the native cursor transparent to avoid two cursors. + */ +export function installRecordingCursor( + document: Document, + window: Window, + options: RecordingInputOptions = DEFAULT_RECORDING_INPUT_OPTIONS, + emit: (input: DesktopPreviewRecordingInput) => void = () => {}, +) { + const style = document.createElement("style"); + style.textContent = + "html, html * { cursor: none !important; } @media (prefers-reduced-motion: reduce) { [data-t3code-recording-agent-cursor] { transition: none !important; } }"; + const cursor = document.createElement("div"); + cursor.setAttribute("aria-hidden", "true"); + cursor.setAttribute("data-t3code-recording-cursor", ""); + cursor.style.cssText = + "position:fixed;left:0;top:0;width:16px;height:24px;pointer-events:none;z-index:2147483647;display:none;"; + cursor.innerHTML = + ''; + const agentCursor = document.createElement("div"); + agentCursor.setAttribute("aria-hidden", "true"); + agentCursor.setAttribute("data-t3code-recording-agent-cursor", ""); + agentCursor.style.cssText = + "position:fixed;left:0;top:0;width:20px;height:20px;pointer-events:none;z-index:2147483647;display:none;filter:drop-shadow(0 1px 2px #0003);transition:transform 150ms ease-out,opacity 150ms ease-out;"; + // Match the MousePointer2 icon used by the live AgentBrowserCursor. + agentCursor.innerHTML = + ''; + document.documentElement.append(style, cursor, agentCursor); + let controller: "human" | "agent" | "none" = "none"; + let humanPoint: { readonly x: number; readonly y: number } | null = null; + const drawHuman = () => { + if (!humanPoint) return; + cursor.style.transform = `translate(${humanPoint.x}px, ${humanPoint.y}px)`; + cursor.style.display = "block"; + }; + let agentActive = false; + let agentTimer: number | undefined; + const setController = ( + next: typeof controller, + point?: { readonly x: number; readonly y: number }, + ) => { + if (point) humanPoint = point; + controller = next; + if (next === "agent") cursor.style.display = "none"; + if (next === "human") drawHuman(); + if (!agentActive) agentCursor.style.opacity = next === "human" ? "0.18" : "0.35"; + }; + const setTheme = ( + theme: Pick | null, + ) => { + agentCursor.style.setProperty("--recording-cursor-primary", theme?.primary ?? "#2563eb"); + agentCursor.style.setProperty("--recording-cursor-background", theme?.background ?? "white"); + }; + let lastKeyLabel: string | null = null; + let pointerHeld = false; + let pointerFrame: number | undefined; + let pendingPointer: DesktopPreviewRecordingInput | undefined; + const cancelPendingPointer = () => { + if (pointerFrame !== undefined) window.cancelAnimationFrame(pointerFrame); + pointerFrame = undefined; + pendingPointer = undefined; + }; + const keyPress = (input: RecordingKeyPress, held = false) => { + if (!options.showKeyPresses) return; + const label = recordingKeysAreSensitive(document) + ? null + : recordingKeyLabel(input, /Mac/.test(window.navigator.platform)); + lastKeyLabel = label; + emit({ type: "key", label, held, width: window.innerWidth }); + }; + const pointer = ( + point: { readonly x: number; readonly y: number }, + phase: "move" | "down" | "up" | "click", + ) => { + if (!options.showMousePresses || (phase === "move" && !pointerHeld)) return; + if (phase === "down") pointerHeld = true; + if (phase === "up") pointerHeld = false; + const input: DesktopPreviewRecordingInput = { + type: "pointer", + phase, + ...point, + width: window.innerWidth, + height: window.innerHeight, + }; + if (phase === "move") { + pendingPointer = input; + pointerFrame ??= window.requestAnimationFrame(() => { + pointerFrame = undefined; + if (pendingPointer) emit(pendingPointer); + pendingPointer = undefined; + }); + } else { + cancelPendingPointer(); + emit(input); + } + }; + const move = ( + point: { readonly x: number; readonly y: number }, + phase: "move" | "click" = "move", + ) => { + agentCursor.style.transform = `translate(${point.x}px, ${point.y}px)`; + agentCursor.style.display = "block"; + agentCursor.style.opacity = "1"; + agentActive = true; + window.clearTimeout(agentTimer); + agentTimer = window.setTimeout(() => { + agentActive = false; + agentCursor.style.opacity = controller === "human" ? "0.18" : "0.35"; + }, 700); + pointer(point, phase); + }; + const moveHuman = (point: { readonly x: number; readonly y: number }) => { + if (controller === "agent") return; + humanPoint = point; + drawHuman(); + }; + const pointerMove = (event: PointerEvent) => { + if (event.pointerType === "touch") return; + const point = { x: event.clientX, y: event.clientY }; + moveHuman(point); + pointer(point, "move"); + }; + const pointerDown = (event: PointerEvent) => { + if (event.pointerType === "touch") return; + moveHuman({ x: event.clientX, y: event.clientY }); + pointer({ x: event.clientX, y: event.clientY }, "down"); + }; + const pointerUp = (event: PointerEvent) => { + if (event.pointerType !== "touch") pointer({ x: event.clientX, y: event.clientY }, "up"); + }; + const keyDown = (event: KeyboardEvent) => { + if (event.isComposing || event.repeat) return; + keyPress(event, true); + }; + const keyUp = () => { + if (!options.showKeyPresses) return; + emit({ + type: "key", + label: recordingKeysAreSensitive(document) ? null : lastKeyLabel, + held: false, + width: window.innerWidth, + }); + }; + const hide = () => { + cursor.style.display = "none"; + pointerHeld = false; + cancelPendingPointer(); + if (options.showKeyPresses || options.showMousePresses) emit({ type: "clear" }); + }; + const leave = (event: PointerEvent) => { + if (event.relatedTarget === null) hide(); + }; + window.addEventListener("pointermove", pointerMove, true); + window.addEventListener("pointerdown", pointerDown, true); + window.addEventListener("pointerup", pointerUp, true); + window.addEventListener("pointercancel", pointerUp, true); + window.addEventListener("keydown", keyDown, true); + window.addEventListener("keyup", keyUp, true); + window.addEventListener("pointerout", leave, true); + window.addEventListener("blur", hide); + return { + move, + keyPress, + setController, + setTheme, + dispose: () => { + window.removeEventListener("pointermove", pointerMove, true); + window.removeEventListener("pointerdown", pointerDown, true); + window.removeEventListener("pointerup", pointerUp, true); + window.removeEventListener("pointercancel", pointerUp, true); + window.removeEventListener("keydown", keyDown, true); + window.removeEventListener("keyup", keyUp, true); + window.removeEventListener("pointerout", leave, true); + window.removeEventListener("blur", hide); + cancelPendingPointer(); + window.clearTimeout(agentTimer); + cursor.remove(); + agentCursor.remove(); + style.remove(); + }, + }; +} diff --git a/apps/desktop/src/preview/RecordingInput.test.ts b/apps/desktop/src/preview/RecordingInput.test.ts new file mode 100644 index 000000000000..062685ec2934 --- /dev/null +++ b/apps/desktop/src/preview/RecordingInput.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from "vite-plus/test"; +import { recordingKeyLabel, recordingKeysAreSensitive } from "./RecordingInput.ts"; + +const key = ( + value: string, + modifiers: Partial<{ + metaKey: boolean; + ctrlKey: boolean; + altKey: boolean; + shiftKey: boolean; + }> = {}, +) => ({ + key: value, + metaKey: false, + ctrlKey: false, + altKey: false, + shiftKey: false, + ...modifiers, +}); + +describe("recording key labels", () => { + it("formats macOS and other-platform shortcuts", () => { + expect(recordingKeyLabel(key("c", { metaKey: true }), true)).toBe("⌘C"); + expect(recordingKeyLabel(key("c", { ctrlKey: true }), false)).toBe("Ctrl + C"); + expect(recordingKeyLabel(key("Tab", { altKey: true, shiftKey: true }), true)).toBe("⌥⇧⇥"); + }); + it("shows held modifiers once and labels navigation keys", () => { + expect(recordingKeyLabel(key("Meta", { metaKey: true }), true)).toBe("⌘"); + expect(recordingKeyLabel(key("Shift", { shiftKey: true }), false)).toBe("Shift"); + expect(recordingKeyLabel(key("ArrowLeft"), true)).toBe("←"); + expect(recordingKeyLabel(key(" "), false)).toBe("Space"); + }); + it.each(["Dead", "Unidentified", "Process", ""])("excludes composition key %s", (value) => { + expect(recordingKeyLabel(key(value), true)).toBeNull(); + }); +}); + +describe("recording key privacy", () => { + const field = (type: string) => ({ tagName: "INPUT", getAttribute: () => type }); + const sensitive = (activeElement: unknown) => + recordingKeysAreSensitive({ activeElement } as Document); + it("excludes password fields and their shadow-root focus", () => { + expect(sensitive(field("password"))).toBe(true); + expect(sensitive({ shadowRoot: { activeElement: field("password") } })).toBe(true); + expect(sensitive(field("text"))).toBe(false); + }); + it("excludes iframe focus whose field cannot be inspected", () => { + expect(sensitive({ tagName: "IFRAME" })).toBe(true); + expect(sensitive({ tagName: "SECRET-FIELD" })).toBe(true); + }); +}); diff --git a/apps/desktop/src/preview/RecordingInput.ts b/apps/desktop/src/preview/RecordingInput.ts new file mode 100644 index 000000000000..a69102121d4e --- /dev/null +++ b/apps/desktop/src/preview/RecordingInput.ts @@ -0,0 +1,58 @@ +export interface RecordingInputOptions { + readonly showKeyPresses: boolean; + readonly showMousePresses: boolean; +} + +export const DEFAULT_RECORDING_INPUT_OPTIONS: RecordingInputOptions = { + showKeyPresses: false, + showMousePresses: false, +}; + +export interface RecordingKeyPress { + readonly key: string; + readonly metaKey: boolean; + readonly ctrlKey: boolean; + readonly altKey: boolean; + readonly shiftKey: boolean; +} + +/** Formats a single chord without duplicating a modifier pressed on its own. */ +export function recordingKeyLabel(input: RecordingKeyPress, isMac: boolean): string | null { + if (["Dead", "Process", "Unidentified", ""].includes(input.key)) return null; + const modifiers = [ + input.ctrlKey || input.key === "Control" ? (isMac ? "⌃" : "Ctrl") : null, + input.altKey || input.key === "Alt" ? (isMac ? "⌥" : "Alt") : null, + input.shiftKey || input.key === "Shift" ? (isMac ? "⇧" : "Shift") : null, + input.metaKey || input.key === "Meta" ? (isMac ? "⌘" : "Win") : null, + ].filter((value) => value !== null); + const labels: Record = { + Enter: "↵", + Tab: "⇥", + Backspace: "⌫", + Delete: "⌦", + Escape: "Esc", + ArrowUp: "↑", + ArrowDown: "↓", + ArrowLeft: "←", + ArrowRight: "→", + " ": "Space", + Space: "Space", + }; + if (!["Control", "Alt", "Shift", "Meta"].includes(input.key)) { + modifiers.push( + labels[input.key] ?? (input.key.length === 1 ? input.key.toUpperCase() : input.key), + ); + } + return modifiers.join(isMac ? "" : " + "); +} + +/** Unknown iframe or closed-shadow focus is excluded because its field type cannot be checked. */ +export function recordingKeysAreSensitive(document: Document): boolean { + let element = document.activeElement; + while (element?.shadowRoot?.activeElement) element = element.shadowRoot.activeElement; + return ( + element?.tagName === "IFRAME" || + element?.tagName.includes("-") === true || + element?.getAttribute("type")?.toLowerCase() === "password" + ); +} diff --git a/apps/web/src/browser/browserRecording.ts b/apps/web/src/browser/browserRecording.ts index 5ea10c2b05d5..0bbe4bd7bf18 100644 --- a/apps/web/src/browser/browserRecording.ts +++ b/apps/web/src/browser/browserRecording.ts @@ -8,6 +8,8 @@ import { previewBridge } from "~/components/preview/previewBridge"; import { ensureClientSettingsHydrated, getClientSettings } from "~/hooks/useSettings"; import { appAtomRegistry } from "~/rpc/atomRegistry"; +import { createRecordingCompositor } from "./recordingCompositor"; + import { acquireBrowserSurfaceActivity } from "./browserSurfaceStore"; export class BrowserRecordingUnavailableError extends Schema.TaggedError()( @@ -123,6 +125,7 @@ interface ActiveRecording { releaseSurfaceActivity: (() => void) | null; stream: MediaStream | null; recorder: MediaRecorder | null; + compositor: Awaited>; savedBlob?: Blob; uploadPromise?: Promise; lifecycle: BrowserRecordingLifecycle; @@ -381,6 +384,8 @@ const captureTabMediaStreamWithTimeout = async ( }; const clearActiveRecording = (recording: ActiveRecording): void => { + recording.compositor?.dispose(); + recording.compositor = null; recording.releaseSurfaceActivity?.(); recording.releaseSurfaceActivity = null; if (activeRecordings.get(recording.tabId) !== recording) return; @@ -525,6 +530,7 @@ export async function startBrowserRecording( releaseSurfaceActivity, stream: null, recorder: null, + compositor: null, lifecycle: startingLifecycle, }; activeRecordings.set(tabId, recording); @@ -534,7 +540,8 @@ export async function startBrowserRecording( clearActiveRecording(recording); throw cause; }); - const frameRate = getClientSettings().browserRecordingFrameRate; + const settings = getClientSettings(); + const frameRate = settings.browserRecordingFrameRate; await waitForBrowserRecordingPaint(); const throwIfStartupCancelled = async (): Promise => { // Once a grant starts, a stop lets startup finish so the caller receives an artifact. @@ -614,7 +621,19 @@ export async function startBrowserRecording( let recorder: MediaRecorder; try { - recorder = createMediaRecorder(stream); + recording.compositor = await createRecordingCompositor( + stream, + { + showKeyPresses: settings.browserRecordingShowKeyPresses, + showMousePresses: settings.browserRecordingShowMousePresses, + frameRate, + }, + (listener) => + bridge.recording.onInput((event) => { + if (event.tabId === tabId) listener(event.input); + }), + ); + recorder = createMediaRecorder(recording.compositor?.stream ?? stream); recording.recorder = recorder; recorder.addEventListener("dataavailable", (event) => { if (event.data.size > 0) chunks.push(event.data); @@ -694,6 +713,8 @@ const finalizeBrowserRecording = async ( cause, }); } + recording.compositor?.dispose(); + recording.compositor = null; // Encoding has flushed; release native capture before materializing and saving the file. stopMediaStream(recording.stream); recording.stream = null; diff --git a/apps/web/src/browser/recordingCompositor.test.ts b/apps/web/src/browser/recordingCompositor.test.ts new file mode 100644 index 000000000000..d10e69d24663 --- /dev/null +++ b/apps/web/src/browser/recordingCompositor.test.ts @@ -0,0 +1,179 @@ +import type { DesktopPreviewRecordingInput } from "@t3tools/contracts"; +import { afterEach, describe, expect, it, vi } from "vite-plus/test"; + +import { createRecordingCompositor, RecordingDecorations } from "./recordingCompositor"; + +const primaryColor = "oklch(0.65 0.2 310)"; +vi.mock("./annotationTheme", () => ({ + readPreviewAnnotationTheme: () => ({ primary: "oklch(0.65 0.2 310)" }), +})); + +const options = { showKeyPresses: true, showMousePresses: true, frameRate: 30 }; +const pointer = ( + phase: "move" | "down" | "up" | "click", + x = 100, +): DesktopPreviewRecordingInput => ({ + type: "pointer", + phase, + x, + y: 80, + width: 800, + height: 600, +}); +const context = () => ({ + save: vi.fn(), + restore: vi.fn(), + beginPath: vi.fn(), + fill: vi.fn(), + stroke: vi.fn(), + ellipse: vi.fn(), + roundRect: vi.fn(), + fillText: vi.fn(), + drawImage: vi.fn(), + measureText: () => ({ width: 40 }), + globalAlpha: 1, + strokeStyle: "", + fillStyle: "", +}); + +describe("recording decorations", () => { + it("keeps rings aligned through dragging and stops following the cursor after release", () => { + const decorations = new RecordingDecorations(options, primaryColor); + const ctx = context(); + decorations.apply(pointer("down"), 0); + decorations.apply(pointer("move", 120), 10); + decorations.draw(ctx as unknown as CanvasRenderingContext2D, 1600, 1200, 10); + expect(ctx.ellipse.mock.calls[0]?.slice(0, 4)).toEqual([240, 160, 40, 40]); + expect(ctx.strokeStyle).toBe(primaryColor); + expect(ctx.fillStyle).toBe(primaryColor); + expect(decorations.nextRedraw(10)).toBeNull(); + decorations.apply(pointer("up", 130), 20); + decorations.apply(pointer("move", 300), 30); + decorations.draw(ctx as unknown as CanvasRenderingContext2D, 1600, 1200, 320); + expect(ctx.ellipse.mock.calls[1]?.slice(0, 4)).toEqual([260, 160, 50, 50]); + ctx.ellipse.mockClear(); + decorations.draw(ctx as unknown as CanvasRenderingContext2D, 1600, 1200, 620); + expect(ctx.ellipse).not.toHaveBeenCalled(); + expect(decorations.nextRedraw(620)).toBeNull(); + }); + + it("pulses agent clicks and clears decorations on blur or navigation", () => { + const decorations = new RecordingDecorations(options, primaryColor); + const ctx = context(); + decorations.apply(pointer("click"), 0); + decorations.draw(ctx as unknown as CanvasRenderingContext2D, 800, 600, 300); + expect(ctx.ellipse).toHaveBeenCalledOnce(); + decorations.apply({ type: "clear" }, 301); + ctx.ellipse.mockClear(); + decorations.draw(ctx as unknown as CanvasRenderingContext2D, 800, 600, 302); + expect(ctx.ellipse).not.toHaveBeenCalled(); + expect(decorations.nextRedraw(302)).toBeNull(); + }); + + it("holds shortcut badges until release, then expires them even on a static page", () => { + const decorations = new RecordingDecorations(options, primaryColor); + const ctx = context(); + const key = { type: "key" as const, label: "⌘C", held: true, width: 800 }; + decorations.apply(key, 0); + decorations.draw(ctx as unknown as CanvasRenderingContext2D, 1600, 1200, 5000); + expect(ctx.fillText.mock.calls[0]?.slice(0, 3)).toEqual(["⌘C", 800, 1098]); + decorations.apply({ ...key, held: false }, 5000); + expect(decorations.nextRedraw(5500)).toBe(400); + ctx.fillText.mockClear(); + decorations.draw(ctx as unknown as CanvasRenderingContext2D, 1600, 1200, 5900); + expect(ctx.fillText).not.toHaveBeenCalled(); + }); + + it("removes the previous key badge on password focus", () => { + const decorations = new RecordingDecorations(options, primaryColor); + const ctx = context(); + decorations.apply({ type: "key", label: "A", held: true, width: 800 }, 0); + decorations.apply({ type: "key", label: null, held: true, width: 800 }, 1); + decorations.draw(ctx as unknown as CanvasRenderingContext2D, 800, 600, 2); + expect(ctx.fillText).not.toHaveBeenCalled(); + }); + + it("honors independent opt-in flags", () => { + const decorations = new RecordingDecorations( + { ...options, showMousePresses: false }, + primaryColor, + ); + const ctx = context(); + decorations.apply(pointer("down"), 0); + decorations.apply({ type: "key", label: "⌘C", held: true, width: 800 }, 0); + decorations.draw(ctx as unknown as CanvasRenderingContext2D, 800, 600, 1); + expect(ctx.ellipse).not.toHaveBeenCalled(); + expect(ctx.fillText).toHaveBeenCalledOnce(); + }); +}); + +describe("detached recording compositor", () => { + afterEach(() => vi.unstubAllGlobals()); + + it("keeps native capture when both decorations are off", async () => { + vi.stubGlobal("document", { + createElement: () => { + throw new Error("must not allocate"); + }, + }); + expect( + await createRecordingCompositor( + {} as MediaStream, + { + ...options, + showKeyPresses: false, + showMousePresses: false, + }, + () => { + throw new Error("must not subscribe"); + }, + ), + ).toBeNull(); + }); + + it.each([false, true])( + "releases the detached output on disposal or playback failure (%s)", + async (failPlayback) => { + const ctx = context(); + const stop = vi.fn(); + const unsubscribe = vi.fn(); + const cancelFrame = vi.fn(); + const source = { + getVideoTracks: () => [{ getSettings: () => ({ width: 800, height: 600 }) }], + } as unknown as MediaStream; + const stream = { getTracks: () => [{ stop }] } as unknown as MediaStream; + const canvas = { width: 0, height: 0, getContext: () => ctx, captureStream: () => stream }; + const video = { + muted: false, + playsInline: false, + srcObject: null as MediaStream | null, + readyState: 2, + videoWidth: 800, + videoHeight: 600, + pause: vi.fn(), + play: async () => { + if (failPlayback) throw new Error("play failed"); + }, + requestVideoFrameCallback: () => 1, + cancelVideoFrameCallback: cancelFrame, + }; + vi.stubGlobal("document", { + createElement: (tag: string) => (tag === "canvas" ? canvas : video), + }); + vi.stubGlobal("window", { clearTimeout: vi.fn(), setTimeout: vi.fn() }); + const compositor = createRecordingCompositor(source, options, () => unsubscribe); + if (failPlayback) await expect(compositor).rejects.toThrow("play failed"); + else { + const result = await compositor; + expect(result?.stream).toBe(stream); + expect(ctx.drawImage).toHaveBeenCalledOnce(); + result?.dispose(); + result?.dispose(); + } + expect(stop).toHaveBeenCalledOnce(); + expect(unsubscribe).toHaveBeenCalledOnce(); + expect(cancelFrame).toHaveBeenCalledWith(1); + expect(video.srcObject).toBeNull(); + }, + ); +}); diff --git a/apps/web/src/browser/recordingCompositor.ts b/apps/web/src/browser/recordingCompositor.ts new file mode 100644 index 000000000000..352154bd9aef --- /dev/null +++ b/apps/web/src/browser/recordingCompositor.ts @@ -0,0 +1,190 @@ +import type { DesktopPreviewRecordingInput } from "@t3tools/contracts"; + +import { readPreviewAnnotationTheme } from "./annotationTheme"; + +interface RecordingDecorationOptions { + readonly showKeyPresses: boolean; + readonly showMousePresses: boolean; + readonly frameRate: number; +} + +/** Decorates a detached canvas; no recording UI is inserted into the preview page. */ +export async function createRecordingCompositor( + source: MediaStream, + options: RecordingDecorationOptions, + subscribe: (listener: (input: DesktopPreviewRecordingInput) => void) => () => void, +) { + if (!options.showKeyPresses && !options.showMousePresses) return null; + const canvas = document.createElement("canvas"); + const context = canvas.getContext("2d", { alpha: false }); + if (!context) throw new Error("Recording canvas is unavailable."); + const video = document.createElement("video"); + video.muted = true; + video.playsInline = true; + video.srcObject = source; + const settings = source.getVideoTracks()[0]?.getSettings(); + canvas.width = settings?.width ?? 1920; + canvas.height = settings?.height ?? 1080; + const decorations = new RecordingDecorations(options, readPreviewAnnotationTheme().primary); + let disposed = false; + let frameId: number | undefined; + let timer: number | undefined; + const draw = () => { + if (disposed || video.readyState < 2) return; + const width = video.videoWidth || canvas.width; + const height = video.videoHeight || canvas.height; + if (canvas.width !== width) canvas.width = width; + if (canvas.height !== height) canvas.height = height; + context.drawImage(video, 0, 0, width, height); + const now = performance.now(); + decorations.draw(context, width, height, now); + window.clearTimeout(timer); + const next = decorations.nextRedraw(now); + if (next !== null) timer = window.setTimeout(draw, next); + }; + const frame = () => { + if (disposed) return; + draw(); + frameId = video.requestVideoFrameCallback(frame); + }; + const output = canvas.captureStream(options.frameRate); + let unsubscribe: (() => void) | undefined; + const dispose = () => { + if (disposed) return; + disposed = true; + unsubscribe?.(); + window.clearTimeout(timer); + if (frameId !== undefined) video.cancelVideoFrameCallback(frameId); + video.pause(); + video.srcObject = null; + for (const track of output.getTracks()) track.stop(); + }; + try { + unsubscribe = subscribe((input) => { + decorations.apply(input, performance.now()); + draw(); + }); + frameId = video.requestVideoFrameCallback(frame); + await video.play(); + draw(); + return { stream: output, dispose }; + } catch (error) { + dispose(); + throw error; + } +} + +/** Keeps input timing and coordinates independent of native video frame delivery. */ +export class RecordingDecorations { + private ring: { + x: number; + y: number; + width: number; + height: number; + held: boolean; + releasedAt: number | null; + } | null = null; + private key: { label: string; width: number; expiresAt: number | null } | null = null; + + constructor( + private readonly options: RecordingDecorationOptions, + private readonly primaryColor: string, + ) {} + + apply(input: DesktopPreviewRecordingInput, now: number) { + if (input.type === "clear") { + this.ring = null; + this.key = null; + } else if (input.type === "key" && this.options.showKeyPresses) { + this.key = input.label + ? { label: input.label, width: input.width, expiresAt: input.held ? null : now + 900 } + : null; + } else if (input.type === "pointer" && this.options.showMousePresses) { + if (input.phase === "down" || input.phase === "click") { + this.ring = { + x: input.x, + y: input.y, + width: input.width, + height: input.height, + held: input.phase === "down", + releasedAt: input.phase === "click" ? now : null, + }; + } else if (this.ring?.held) { + this.ring = { + ...this.ring, + x: input.x, + y: input.y, + width: input.width, + height: input.height, + held: input.phase !== "up", + releasedAt: input.phase === "up" ? now : null, + }; + } + } + } + + nextRedraw(now: number): number | null { + if ( + this.ring?.releasedAt !== null && + this.ring?.releasedAt !== undefined && + now < this.ring.releasedAt + 600 + ) { + return 1000 / this.options.frameRate; + } + return this.key?.expiresAt !== null && + this.key?.expiresAt !== undefined && + now < this.key.expiresAt + ? this.key.expiresAt - now + : null; + } + + draw(context: CanvasRenderingContext2D, width: number, height: number, now: number) { + // Guest coordinates are CSS pixels; native frames include zoom and display scale. + const scale = width / (this.key?.width ?? this.ring?.width ?? 1280); + const ring = this.ring; + if (ring && (ring.held || (ring.releasedAt !== null && now < ring.releasedAt + 600))) { + const progress = ring.releasedAt === null ? 0 : Math.min(1, (now - ring.releasedAt) / 600); + context.save(); + const opacity = 0.9 * (1 - progress); + context.strokeStyle = this.primaryColor; + context.fillStyle = this.primaryColor; + context.lineWidth = 2 * scale; + context.beginPath(); + context.ellipse( + (ring.x * width) / ring.width, + (ring.y * height) / ring.height, + ((20 * width) / ring.width) * (1 + progress * 0.5), + ((20 * height) / ring.height) * (1 + progress * 0.5), + 0, + 0, + Math.PI * 2, + ); + context.globalAlpha = opacity * 0.15; + context.fill(); + context.globalAlpha = opacity; + context.stroke(); + context.restore(); + } + const key = this.key; + if (key && (key.expiresAt === null || now < key.expiresAt)) { + context.save(); + context.font = `500 ${26 * scale}px system-ui, sans-serif`; + const badgeWidth = Math.min( + width - 32 * scale, + context.measureText(key.label).width + 36 * scale, + ); + const badgeHeight = 54 * scale; + const left = (width - badgeWidth) / 2; + const top = height - 24 * scale - badgeHeight; + context.fillStyle = "rgba(32,32,34,.86)"; + context.beginPath(); + context.roundRect(left, top, badgeWidth, badgeHeight, 14 * scale); + context.fill(); + context.fillStyle = "white"; + context.textAlign = "center"; + context.textBaseline = "middle"; + context.fillText(key.label, width / 2, top + badgeHeight / 2, badgeWidth - 24 * scale); + context.restore(); + } + } +} diff --git a/apps/web/src/components/preview/AgentBrowserCursor.tsx b/apps/web/src/components/preview/AgentBrowserCursor.tsx index bc89daee4595..408f268fa0c4 100644 --- a/apps/web/src/components/preview/AgentBrowserCursor.tsx +++ b/apps/web/src/components/preview/AgentBrowserCursor.tsx @@ -24,7 +24,6 @@ export function AgentBrowserCursor(props: { return ( (null); + const active = inactiveSequence !== event.sequence; useEffect(() => { - const timeout = window.setTimeout(() => setActive(false), CURSOR_ACTIVE_MS); + const timeout = window.setTimeout(() => setInactiveSequence(event.sequence), CURSOR_ACTIVE_MS); return () => window.clearTimeout(timeout); - }, []); + }, [event.sequence]); return (
({ pictureInPicture: false, showEmptyState: false, loading: false, + serverEpoch: null as string | null, + recordingTabIds: new Set(), + recordingRuntimeTabId: null as string | null, recordVisitForThread: vi.fn(), })); @@ -98,6 +101,7 @@ vi.mock("~/previewStateStore", () => ({ updatePreviewServerSnapshot: vi.fn(), useThreadPreviewState: () => ({ activeTabId: "tab-1", + serverEpoch: mocks.serverEpoch, desktopByTabId: { "tab-1": { hasWebContents: true, @@ -146,11 +150,11 @@ vi.mock("~/state/use-atom-command", () => ({ })); vi.mock("~/browser/browserRecording", () => ({ - findActiveBrowserRecordingRuntimeTabId: vi.fn(() => null), + findActiveBrowserRecordingRuntimeTabId: () => mocks.recordingRuntimeTabId, isBrowserRecordingStartCancelledError: vi.fn(() => false), startBrowserRecording: vi.fn(), stopBrowserRecording: vi.fn(), - useActiveBrowserRecordingTabIds: () => new Set(), + useActiveBrowserRecordingTabIds: () => mocks.recordingTabIds, })); vi.mock("~/browser/browserSurfaceStore", () => ({ @@ -244,7 +248,9 @@ vi.mock("./PreviewMoreMenu", () => ({ })); vi.mock("./PreviewUnreachable", () => ({ PreviewUnreachable: () => null })); vi.mock("./ZoomIndicator", () => ({ ZoomIndicator: () => null })); -vi.mock("./AgentBrowserCursor", () => ({ AgentBrowserCursor: () => null })); +vi.mock("./AgentBrowserCursor", () => ({ + AgentBrowserCursor: () => createElement("agent-cursor"), +})); vi.mock("~/browser/BrowserSurfaceSlot", () => ({ BrowserSurfaceSlot: () => null })); vi.mock("./usePreviewSession", () => ({ usePreviewSession: vi.fn() })); @@ -344,9 +350,38 @@ describe("PreviewView navigation", () => { mocks.pictureInPicture = false; mocks.showEmptyState = false; mocks.loading = false; + mocks.serverEpoch = null; + mocks.recordingTabIds = new Set(); + mocks.recordingRuntimeTabId = null; mocks.recordVisitForThread.mockClear(); }); + it("shows the cursor in a replacement browser while the old instance still records", async () => { + const document = installTestDom(); + const { createRoot } = await import("react-dom/client"); + const container = document.createElement("div"); + const root = createRoot(container as unknown as Element); + const hasCursor = (node: TestNode): boolean => + node.nodeName === "AGENT-CURSOR" || node.childNodes.some(hasCursor); + mocks.recordingTabIds.add(TEST_RUNTIME_TAB_ID); + mocks.recordingRuntimeTabId = TEST_RUNTIME_TAB_ID; + try { + await act(() => { + root.render(); + }); + expect(hasCursor(container)).toBe(false); + mocks.serverEpoch = "replacement-server"; + await act(() => { + root.render(); + }); + expect(hasCursor(container)).toBe(true); + expect(mocks.recordingTabIds.has(TEST_RUNTIME_TAB_ID)).toBe(true); + } finally { + await act(() => root.unmount()); + vi.unstubAllGlobals(); + } + }); + it("does not rerender while loading time passes", async () => { vi.useFakeTimers(); mocks.loading = true; diff --git a/apps/web/src/components/preview/PreviewView.tsx b/apps/web/src/components/preview/PreviewView.tsx index d190a78d4825..810e6d502804 100644 --- a/apps/web/src/components/preview/PreviewView.tsx +++ b/apps/web/src/components/preview/PreviewView.tsx @@ -798,18 +798,17 @@ export function PreviewView({ {snapshot && desktopOverlay ? ( ) : null} - {runtimeTabId && desktopOverlay && !showEmptyState && !isUnreachable ? ( + {runtimeTabId && + desktopOverlay && + !showEmptyState && + !isUnreachable && + !activeRecordingTabIds.has(runtimeTabId) ? ( ) : null} - {controller !== "none" ? ( -
- {controller === "agent" ? "Agent controlling browser" : "Human control"} -
- ) : null} {navStatus._tag === "LoadFailed" ? (
settings.browserRecordingShowKeyPresses); + const showMouse = useClientSettings((settings) => settings.browserRecordingShowMousePresses); + const updateSettings = useUpdatePrimarySettings(); + return ( + <> + + updateSettings({ browserRecordingShowKeyPresses: Boolean(checked) }) + } + /> + } + /> + + updateSettings({ browserRecordingShowMousePresses: Boolean(checked) }) + } + /> + } + /> + + ); +} + function BrowserRecordingFrameRateSetting({ disabled }: { readonly disabled: boolean }) { const frameRate = useClientSettings((settings) => settings.browserRecordingFrameRate); const updateSettings = useUpdatePrimarySettings(); @@ -1326,6 +1364,7 @@ export function IntegrationsSettingsPanel() { + diff --git a/apps/web/src/components/settings/SettingsPanels.logic.test.ts b/apps/web/src/components/settings/SettingsPanels.logic.test.ts index d93db8d970b1..3f9d233ed278 100644 --- a/apps/web/src/components/settings/SettingsPanels.logic.test.ts +++ b/apps/web/src/components/settings/SettingsPanels.logic.test.ts @@ -268,6 +268,8 @@ describe("getChangedBrowserSettingLabels", () => { browserDefaultZoomFactor: 1.5, browserDefaultAppearance: "dark", browserRecordingFrameRate: 60, + browserRecordingShowKeyPresses: true, + browserRecordingShowMousePresses: true, browserLinkTarget: "app", browserAutoShowFloatingPreview: !DEFAULT_UNIFIED_SETTINGS.browserAutoShowFloatingPreview, }), @@ -276,6 +278,8 @@ describe("getChangedBrowserSettingLabels", () => { "Browser zoom", "Browser appearance", "Recording frame rate", + "Recording key presses", + "Recording mouse presses", "Open links in", "Floating preview", ]); diff --git a/apps/web/src/components/settings/SettingsPanels.logic.ts b/apps/web/src/components/settings/SettingsPanels.logic.ts index 5cbcb190a97b..d5c6359c1c9f 100644 --- a/apps/web/src/components/settings/SettingsPanels.logic.ts +++ b/apps/web/src/components/settings/SettingsPanels.logic.ts @@ -115,6 +115,8 @@ export type BrowserDefaultSettings = Pick< | "browserDefaultZoomFactor" | "browserDefaultAppearance" | "browserRecordingFrameRate" + | "browserRecordingShowKeyPresses" + | "browserRecordingShowMousePresses" | "browserLinkTarget" | "browserAutoShowFloatingPreview" >; @@ -156,6 +158,8 @@ export function getChangedBrowserSettingLabels(settings: BrowserDefaultSettings) ...(settings.browserRecordingFrameRate !== DEFAULT_UNIFIED_SETTINGS.browserRecordingFrameRate ? ["Recording frame rate"] : []), + ...(settings.browserRecordingShowKeyPresses ? ["Recording key presses"] : []), + ...(settings.browserRecordingShowMousePresses ? ["Recording mouse presses"] : []), ...(settings.browserLinkTarget !== DEFAULT_UNIFIED_SETTINGS.browserLinkTarget ? ["Open links in"] : []), diff --git a/apps/web/src/components/settings/SettingsPanels.tsx b/apps/web/src/components/settings/SettingsPanels.tsx index 85e1de588f94..180ab45361df 100644 --- a/apps/web/src/components/settings/SettingsPanels.tsx +++ b/apps/web/src/components/settings/SettingsPanels.tsx @@ -635,6 +635,8 @@ export function useSettingsRestore(onRestored?: () => void) { settings.browserDefaultZoomFactor, settings.browserDefaultAppearance, settings.browserRecordingFrameRate, + settings.browserRecordingShowKeyPresses, + settings.browserRecordingShowMousePresses, settings.browserLinkTarget, settings.browserAutoShowFloatingPreview, settings.appearanceContrast, @@ -798,6 +800,8 @@ export function useSettingsRestore(onRestored?: () => void) { browserDefaultZoomFactor: DEFAULT_UNIFIED_SETTINGS.browserDefaultZoomFactor, browserDefaultAppearance: DEFAULT_UNIFIED_SETTINGS.browserDefaultAppearance, browserRecordingFrameRate: DEFAULT_UNIFIED_SETTINGS.browserRecordingFrameRate, + browserRecordingShowKeyPresses: DEFAULT_UNIFIED_SETTINGS.browserRecordingShowKeyPresses, + browserRecordingShowMousePresses: DEFAULT_UNIFIED_SETTINGS.browserRecordingShowMousePresses, browserLinkTarget: DEFAULT_UNIFIED_SETTINGS.browserLinkTarget, browserAutoShowFloatingPreview: DEFAULT_UNIFIED_SETTINGS.browserAutoShowFloatingPreview, // Re-granted like any other default. The confirmation dialog lists it by diff --git a/apps/web/src/components/settings/settingsSearch.ts b/apps/web/src/components/settings/settingsSearch.ts index 4532bd5f72fb..88f0326ea9e0 100644 --- a/apps/web/src/components/settings/settingsSearch.ts +++ b/apps/web/src/components/settings/settingsSearch.ts @@ -615,6 +615,18 @@ export const SETTINGS_SEARCH_ITEMS = [ title: "Browser recording frame rate", to: "/settings/integrations", }, + { + id: "browser-recording-key-presses", + title: "Show key presses in recordings", + to: "/settings/integrations", + searchTerms: ["browser preview keyboard shortcuts keystrokes overlay capture"], + }, + { + id: "browser-recording-mouse-presses", + title: "Show mouse presses in recordings", + to: "/settings/integrations", + searchTerms: ["browser preview clicks buttons drag overlay capture"], + }, { id: "browser-link-target", title: "Open links in", diff --git a/packages/contracts/src/ipc.ts b/packages/contracts/src/ipc.ts index 7e738b97786b..c67ecd27c0e1 100644 --- a/packages/contracts/src/ipc.ts +++ b/packages/contracts/src/ipc.ts @@ -658,6 +658,30 @@ export interface DesktopPreviewPointerEvent { createdAt: string; } +/** Recording decorations are forwarded separately from the captured page pixels. */ +export const DesktopPreviewRecordingInputSchema = Schema.Union([ + Schema.Struct({ + type: Schema.Literal("pointer"), + phase: Schema.Literals(["move", "down", "up", "click"]), + x: Schema.Finite, + y: Schema.Finite, + width: Schema.Finite.check(Schema.isGreaterThan(0)), + height: Schema.Finite.check(Schema.isGreaterThan(0)), + }), + Schema.Struct({ + type: Schema.Literal("key"), + label: Schema.NullOr(Schema.String.check(Schema.isMaxLength(100))), + held: Schema.Boolean, + width: Schema.Finite.check(Schema.isGreaterThan(0)), + }), + Schema.Struct({ type: Schema.Literal("clear") }), +]); +export type DesktopPreviewRecordingInput = typeof DesktopPreviewRecordingInputSchema.Type; +export interface DesktopPreviewRecordingInputEvent { + readonly tabId: string; + readonly input: DesktopPreviewRecordingInput; +} + /** * Static config a renderer needs to mount a preview ``. Returned * atomically by `DesktopPreviewBridge.getPreviewConfig()` so the renderer @@ -1295,6 +1319,7 @@ export interface DesktopPreviewBridge { close: (tabId: string) => Promise; }; recording: { + onInput: (listener: (event: DesktopPreviewRecordingInputEvent) => void) => () => void; startScreencast: (tabId: string) => Promise; stopScreencast: (tabId: string) => Promise; save: ( diff --git a/packages/contracts/src/settings.test.ts b/packages/contracts/src/settings.test.ts index 3a9fc35c4c82..ff54b6c0aa53 100644 --- a/packages/contracts/src/settings.test.ts +++ b/packages/contracts/src/settings.test.ts @@ -471,6 +471,23 @@ describe("ClientSettings browser recording frame rate", () => { }); }); +describe("ClientSettings recording input overlays", () => { + it("defaults both overlays off and accepts independent opt-ins", () => { + const settings = decodeClientSettings({}); + expect(settings.browserRecordingShowKeyPresses).toBe(false); + expect(settings.browserRecordingShowMousePresses).toBe(false); + expect( + decodeClientSettingsPatch({ + browserRecordingShowKeyPresses: true, + browserRecordingShowMousePresses: false, + }), + ).toMatchObject({ + browserRecordingShowKeyPresses: true, + browserRecordingShowMousePresses: false, + }); + }); +}); + describe("ClientSettings glass opacity", () => { it("defaults to a readable translucent surface", () => { expect(decodeClientSettings({}).glassOpacity).toBe(80); diff --git a/packages/contracts/src/settings.ts b/packages/contracts/src/settings.ts index 42e91a76ecaf..73a2a2517d05 100644 --- a/packages/contracts/src/settings.ts +++ b/packages/contracts/src/settings.ts @@ -317,6 +317,12 @@ export const ClientSettingsSchema = Schema.Struct({ browserRecordingFrameRate: BrowserRecordingFrameRate.pipe( Schema.withDecodingDefault(Effect.succeed(DEFAULT_BROWSER_RECORDING_FRAME_RATE)), ), + browserRecordingShowKeyPresses: Schema.Boolean.pipe( + Schema.withDecodingDefault(Effect.succeed(false)), + ), + browserRecordingShowMousePresses: Schema.Boolean.pipe( + Schema.withDecodingDefault(Effect.succeed(false)), + ), /** * Where links clicked in a thread (chat markdown, terminal output) open. * Only the desktop app has an in-app browser, so other clients ignore "app". @@ -1525,6 +1531,8 @@ export const ClientSettingsPatch = Schema.Struct({ browserDefaultZoomFactor: Schema.optionalKey(PreviewZoomFactor), browserDefaultAppearance: Schema.optionalKey(PreviewAppearancePreference), browserRecordingFrameRate: Schema.optionalKey(BrowserRecordingFrameRate), + browserRecordingShowKeyPresses: Schema.optionalKey(Schema.Boolean), + browserRecordingShowMousePresses: Schema.optionalKey(Schema.Boolean), browserLinkTarget: Schema.optionalKey(BrowserLinkTarget), browserAutoShowFloatingPreview: Schema.optionalKey(Schema.Boolean), browserProfiles: Schema.optionalKey(Schema.Array(BrowserProfile)), From 7ade2d2c6b37df6f1bc879bfcbf64658b7af2554 Mon Sep 17 00:00:00 2001 From: Igor Makowski <56691628+Mnigos@users.noreply.github.com> Date: Sun, 20 Sep 2026 23:24:33 +0200 Subject: [PATCH 06/24] fix(web): the Run on / Workspace menu closes after a pick (#12685) --- apps/web/src/components/BranchToolbar.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apps/web/src/components/BranchToolbar.tsx b/apps/web/src/components/BranchToolbar.tsx index 3330f40a090f..cdf3aeffbea1 100644 --- a/apps/web/src/components/BranchToolbar.tsx +++ b/apps/web/src/components/BranchToolbar.tsx @@ -233,6 +233,7 @@ const MobileRunContextSelector = memo(function MobileRunContextSelector({ { if (autoEnvironmentLabel) onAutoEnvironment?.(); }} @@ -250,6 +251,7 @@ const MobileRunContextSelector = memo(function MobileRunContextSelector({ key={env.environmentId} disabled={envLocked} value={env.environmentId} + closeOnClick > @@ -274,7 +276,7 @@ const MobileRunContextSelector = memo(function MobileRunContextSelector({ onEnvModeChange(value as EnvMode); }} > - + {activeWorktreePath ? ( @@ -286,14 +288,14 @@ const MobileRunContextSelector = memo(function MobileRunContextSelector({ - + {resolveEnvModeLabel("worktree")} {previousWorktreeLabel ? ( - + {previousWorktreeLabel} From 3cfebf4aa3ce7410806c0ba4cc4d991cc1ce8dbb Mon Sep 17 00:00:00 2001 From: maria Date: Sun, 20 Sep 2026 18:24:52 -0300 Subject: [PATCH 07/24] fix(web): keep portaled menus clickable over Electron drag regions (#12527) --- apps/web/src/components/ui/menu.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/apps/web/src/components/ui/menu.tsx b/apps/web/src/components/ui/menu.tsx index 3563b0ed01ac..8beed75880ae 100644 --- a/apps/web/src/components/ui/menu.tsx +++ b/apps/web/src/components/ui/menu.tsx @@ -56,6 +56,10 @@ function MenuPopup({ Date: Sun, 20 Sep 2026 18:25:03 -0300 Subject: [PATCH 08/24] fix(web): render citations in queued messages (#12403) --- apps/web/src/components/chat/MessagesTimeline.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/web/src/components/chat/MessagesTimeline.tsx b/apps/web/src/components/chat/MessagesTimeline.tsx index 42a45b991028..17e8fb1f56ca 100644 --- a/apps/web/src/components/chat/MessagesTimeline.tsx +++ b/apps/web/src/components/chat/MessagesTimeline.tsx @@ -1778,7 +1778,7 @@ function QueuedMessageTimelineRow({
{text.length > 0 ? ( -
{text}
+ ) : null} {attachmentCount > 0 || contextCount > 0 ? (
0 && "mt-1.5")}> @@ -4067,7 +4067,7 @@ const CollapsibleUserMessageBody = memo(function CollapsibleUserMessageBody(prop const UserMessageBody = memo(function UserMessageBody(props: { text: string; - renderContextReference: (reference: ChatMarkdownContextReference) => ReactNode; + renderContextReference?: (reference: ChatMarkdownContextReference) => ReactNode; skills: ReadonlyArray>; markdownCwd: string | undefined; }) { From 2d8f9a8f55d2195ac820c91f0f1e8f66a055f841 Mon Sep 17 00:00:00 2001 From: oliver <97427849+flamboh@users.noreply.github.com> Date: Sun, 20 Sep 2026 14:31:38 -0700 Subject: [PATCH 09/24] fix(web): keep the timeline still when the resting composer expands (#12771) --- .../components/ComposerPromptEditorTiptap.tsx | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/apps/web/src/components/ComposerPromptEditorTiptap.tsx b/apps/web/src/components/ComposerPromptEditorTiptap.tsx index 5126246dd05c..c108207304c4 100644 --- a/apps/web/src/components/ComposerPromptEditorTiptap.tsx +++ b/apps/web/src/components/ComposerPromptEditorTiptap.tsx @@ -725,6 +725,19 @@ function ComposerPromptEditorTiptapInner(props: ComposerPromptEditorProps) { ); }, []); + const editorAttributes = useMemo( + () => ({ + class: cn( + "composer-tiptap block max-h-50 min-h-17.5 w-full overflow-y-auto whitespace-pre-wrap wrap-break-word bg-transparent leading-relaxed text-foreground focus:outline-none", + className, + ), + "data-testid": "composer-editor", + "data-composer-rich-text": richText ? "true" : "false", + "aria-placeholder": placeholder, + }), + [className, placeholder, richText], + ); + const editor = useEditor( { extensions: [ @@ -787,15 +800,7 @@ function ComposerPromptEditorTiptapInner(props: ComposerPromptEditorProps) { ), editable: !disabled, editorProps: { - attributes: { - class: cn( - "composer-tiptap block max-h-50 min-h-17.5 w-full overflow-y-auto whitespace-pre-wrap wrap-break-word bg-transparent leading-relaxed text-foreground focus:outline-none", - className, - ), - "data-testid": "composer-editor", - "data-composer-rich-text": richText ? "true" : "false", - "aria-placeholder": placeholder, - }, + attributes: editorAttributes, handleKeyDown: (view, event) => { if ( isMacPlatform(navigator.platform) && @@ -995,6 +1000,17 @@ function ComposerPromptEditorTiptapInner(props: ComposerPromptEditorProps) { editorHolder.current = editor; }, [editor]); + // Tiptap forwards option changes to the view from a passive effect, so a + // class change here would reach the ProseMirror element one tick after + // React commits. The chat composer measures its resting and expanded + // geometry in layout effects that run first, and it clamps the prompt + // through `className`, so the attributes are pushed to the view here for + // those measurements to see the layout they are about to reserve for. + useLayoutEffect(() => { + if (!editor?.isInitialized) return; + editor.view.setProps({ attributes: editorAttributes }); + }, [editor, editorAttributes]); + const readSnapshot = useCallback(() => { const snapshot = snapshotRef.current; if (!editor) return snapshot; From 45e06f48a03354b600782259010ae6222241aff0 Mon Sep 17 00:00:00 2001 From: "Khai Shern, Toh" <55418374+Leos-Khai@users.noreply.github.com> Date: Mon, 21 Sep 2026 05:46:08 +0800 Subject: [PATCH 10/24] fix: composer hero reads project name to screen readers (#12397) --- .../features/threads/NewTaskDraftScreen.tsx | 2 +- .../src/components/chat/DraftHeroHeadline.tsx | 24 +++++++++++++++---- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/apps/mobile/src/features/threads/NewTaskDraftScreen.tsx b/apps/mobile/src/features/threads/NewTaskDraftScreen.tsx index 9732ed6865f5..681f6c5ee52d 100644 --- a/apps/mobile/src/features/threads/NewTaskDraftScreen.tsx +++ b/apps/mobile/src/features/threads/NewTaskDraftScreen.tsx @@ -1455,7 +1455,7 @@ export function NewTaskDraftScreen(props: { in + // The trigger's accessible name comes from its visible text (the + // project title) so the hero sentence reads naturally: an + // aria-label here would replace the title with an action phrase + // mid-sentence and baffle screen-reader users. + } > {activeProjectDisplayName ?? "Choose a project"} @@ -233,8 +234,21 @@ export function DraftHeroHeadline({ ); + // The composer hero is a sentence, so the heading's accessible name must be + // a complete sentence too. The project picker is a control rendered inline + // in the h1; without an explicit label its widget state bleeds into the + // announced phrase. + const headingLabel = hasResolvedProject + ? `What should we build in ${activeProjectDisplayName}?` + : canChooseProject + ? `${activeProjectDisplayName ?? "Choose a project"} to start` + : "Add a project to start"; + return ( -

+

{hasResolvedProject ? ( <>What should we build in {projectSelector}? ) : canChooseProject ? ( From f6cc6bc7e9cbe1feeb7811ea92d519ad77c76596 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Sun, 20 Sep 2026 18:04:01 -0400 Subject: [PATCH 11/24] fix(mobile): respect word wrap in diffs (#12590) Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Julius Marminge --- .../t3-review-diff/android/build.gradle | 12 + .../t3reviewdiff/ReviewDiffCanvasDrawing.kt | 108 ++++++- .../t3reviewdiff/ReviewDiffCodeLayout.kt | 169 +++++++++++ .../modules/t3reviewdiff/T3ReviewDiffView.kt | 53 ++-- .../t3reviewdiff/ReviewDiffCodeLayoutTest.kt | 150 +++++++++ .../ios/ReviewDiffCodeLayout.swift | 123 ++++++++ .../t3-review-diff/ios/T3ReviewDiffView.swift | 284 ++++++++++++++---- .../t3-review-diff/tests/ios/main.swift | 60 ++++ .../modules/t3-review-diff/tests/run-ios.sh | 12 + .../src/features/review/ReviewCommentCard.tsx | 3 +- .../review/nativeReviewDiffAdapter.ts | 7 +- .../reviewDiffHighlightScheduler.test.ts | 74 +++++ .../review/reviewDiffHighlightScheduler.ts | 51 ++++ .../review/useNativeReviewDiffHighlighting.ts | 41 +-- .../appearance/useAppearanceCodeSurface.ts | 4 +- 15 files changed, 1032 insertions(+), 119 deletions(-) create mode 100644 apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCodeLayout.kt create mode 100644 apps/mobile/modules/t3-review-diff/android/src/test/java/expo/modules/t3reviewdiff/ReviewDiffCodeLayoutTest.kt create mode 100644 apps/mobile/modules/t3-review-diff/ios/ReviewDiffCodeLayout.swift create mode 100644 apps/mobile/modules/t3-review-diff/tests/ios/main.swift create mode 100644 apps/mobile/modules/t3-review-diff/tests/run-ios.sh create mode 100644 apps/mobile/src/features/review/reviewDiffHighlightScheduler.test.ts create mode 100644 apps/mobile/src/features/review/reviewDiffHighlightScheduler.ts diff --git a/apps/mobile/modules/t3-review-diff/android/build.gradle b/apps/mobile/modules/t3-review-diff/android/build.gradle index 22bb070b3b81..d360d1580f1d 100644 --- a/apps/mobile/modules/t3-review-diff/android/build.gradle +++ b/apps/mobile/modules/t3-review-diff/android/build.gradle @@ -8,6 +8,10 @@ android { namespace 'expo.modules.t3reviewdiff' compileSdk rootProject.ext.compileSdkVersion + testOptions { + unitTests.includeAndroidResources = true + } + defaultConfig { minSdkVersion rootProject.ext.minSdkVersion targetSdkVersion rootProject.ext.targetSdkVersion @@ -16,4 +20,12 @@ android { dependencies { implementation project(':expo-modules-core') + testImplementation 'junit:junit:4.13.2' + testImplementation 'org.robolectric:robolectric:4.16.1' +} + +tasks.withType(Test).configureEach { + javaLauncher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.of(21) + } } diff --git a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCanvasDrawing.kt b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCanvasDrawing.kt index 6782e6894d99..80d0410643ff 100644 --- a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCanvasDrawing.kt +++ b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCanvasDrawing.kt @@ -9,6 +9,7 @@ import android.graphics.Path import android.graphics.RectF import android.graphics.Shader import android.graphics.Typeface +import android.text.TextPaint import kotlin.math.max import kotlin.math.min @@ -177,6 +178,39 @@ internal class ReviewDiffCanvasDrawing(context: Context) { textPaint.isUnderlineText = fontStyle and 4 != 0 } + var codeLayouts = CodeLayoutCache() + + /** Capture paint on the UI thread; the decode worker owns the new cache until publication. */ + fun prepareRows( + tokens: Map>, + style: DiffStyle, + width: Int + ): (List) -> CodeLayoutCache { + configureCodePaint(theme.text, 0, style) + val paint = TextPaint(textPaint) + val colors = theme + val cache = codeLayouts.copyForPreparation() + val availableWidth = ( + width - style.changeBarWidthPx - style.gutterWidthPx - + style.codePaddingPx * 2f + ).toInt() + return { rows -> + cache.apply { layout(rows, tokens, paint, style, colors, availableWidth) } + } + } + + fun codeWrapLayout( + rows: List, + tokens: Map>, + style: DiffStyle, + width: Int + ): CodeWrapLayout { + configureCodePaint(theme.text, 0, style) + val availableWidth = width - style.changeBarWidthPx - style.gutterWidthPx - + style.codePaddingPx * 2f + return codeLayouts.layout(rows, tokens, textPaint, style, theme, availableWidth.toInt()) + } + fun lineNumberColor(change: String): Int = when (change) { "add" -> theme.addText "delete" -> theme.deleteText @@ -198,13 +232,17 @@ internal class ReviewDiffCanvasDrawing(context: Context) { } } + /** Highlights word diffs; [top]..[bottom] is the row's first visual line. */ + @Suppress("LongParameterList") fun drawWordDiffRanges( canvas: Canvas, row: DiffRow, codeX: Float, top: Int, - bottom: Int + bottom: Int, + lines: CodeLines ) { + if (lines.nativeLayout != null) return if (row.wordDiffRanges.isEmpty() || (row.change != "add" && row.change != "delete")) return val color = if (row.change == "add") theme.addBar else theme.deleteBar backgroundPaint.color = withAlpha(color, 71) @@ -213,14 +251,66 @@ internal class ReviewDiffCanvasDrawing(context: Context) { val highlightHeight = max(4f * density, min(bottom - top - 4f * density, fontHeight)) val highlightTop = (top + bottom - highlightHeight) / 2f row.wordDiffRanges.forEach { range -> - val left = codeX + range.start * characterWidth - val right = max(left + 2f * density, codeX + range.end * characterWidth) - canvas.drawRoundRect( - RectF(left, highlightTop, right, highlightTop + highlightHeight), - 3f * density, - 3f * density, - backgroundPaint, - ) + // A wrapped row splits the highlight at each visual line boundary. + lines.starts.forEachIndexed { line, lineStart -> + val start = max(range.start, lineStart) + val end = min(range.end, lines.end(line, Int.MAX_VALUE)) + if (end <= start) return@forEachIndexed + val left = codeX + (start - lineStart) * characterWidth + val right = max(left + 2f * density, left + (end - start) * characterWidth) + val lineTop = highlightTop + line * lines.height + canvas.drawRoundRect( + RectF(left, lineTop, right, lineTop + highlightHeight), + 3f * density, + 3f * density, + backgroundPaint, + ) + } + } + } + + /** Draws a code row's text, or its syntax [tokens] when present, one visual line per start. */ + @Suppress("LongParameterList") + fun drawCode( + canvas: Canvas, + content: String, + tokens: List?, + codeX: Float, + baseline: Float, + style: DiffStyle, + lines: CodeLines + ) { + val nativeLayout = lines.nativeLayout + if (nativeLayout != null) { + canvas.save() + canvas.translate(codeX, baseline - nativeLayout.getLineBaseline(0)) + nativeLayout.draw(canvas) + canvas.restore() + return + } + val runs = if (tokens.isNullOrEmpty()) listOf(DiffToken(content, null, 0)) else tokens + var line = 0 + var x = codeX + var column = 0 + runs.forEach { run -> + configureCodePaint(run.color ?: theme.text, run.fontStyle, style) + var start = 0 + while (start < run.content.length) { + while (line + 1 < lines.starts.size && lines.starts[line + 1] <= column + start) { + line += 1 + x = codeX + } + val end = min(run.content.length, lines.end(line, Int.MAX_VALUE) - column) + val lineBaseline = baseline + line * lines.height + if (lineBaseline + textPaint.fontMetrics.descent >= canvas.clipBounds.top && + lineBaseline + textPaint.fontMetrics.ascent <= canvas.clipBounds.bottom + ) { + canvas.drawText(run.content, start, end, x, lineBaseline, textPaint) + x += textPaint.measureText(run.content, start, end) + } + start = end + } + column += run.content.length } } diff --git a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCodeLayout.kt b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCodeLayout.kt new file mode 100644 index 000000000000..dc77a8467033 --- /dev/null +++ b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/ReviewDiffCodeLayout.kt @@ -0,0 +1,169 @@ +package expo.modules.t3reviewdiff + +import android.graphics.Color +import android.graphics.Paint +import android.graphics.Typeface +import android.text.Layout +import android.text.SpannableString +import android.text.Spanned +import android.text.StaticLayout +import android.text.TextPaint +import android.text.style.BackgroundColorSpan +import android.text.style.ForegroundColorSpan +import android.text.style.StyleSpan +import android.text.style.UnderlineSpan +import kotlin.math.ceil +import kotlin.math.max + +/** Text layout is independent of comment heights and vertical row offsets. */ +internal class CodeLines( + val starts: IntArray, + val height: Int, + val nativeLayout: StaticLayout? = null +) { + fun end(line: Int, length: Int): Int = if (line + 1 < starts.size) starts[line + 1] else length + + fun firstHeight(base: Int): Int = max(base, nativeLayout?.getLineBottom(0) ?: 0) + + fun baseline(top: Int, bottom: Int, paint: Paint): Float = nativeLayout?.let { + top + (bottom - top - it.getLineBottom(0)) / 2f + it.getLineBaseline(0) + } ?: ((top + bottom - paint.fontMetrics.ascent - paint.fontMetrics.descent) / 2f) + + val extraHeight: Int + get() = nativeLayout?.let { it.height - it.getLineBottom(0) } ?: ((starts.size - 1) * height) +} + +internal class CodeWrapLayout( + val enabled: Boolean, + private val linesByRowId: Map +) { + fun lines(rowId: String): CodeLines = linesByRowId[rowId] ?: SINGLE_LINE + fun extraHeight(rowId: String): Int = lines(rowId).extraHeight + fun rowHeight(rowId: String, base: Int): Int = lines(rowId).let { + it.firstHeight(base) + + it.extraHeight + } + + companion object { + private val SINGLE_LINE = CodeLines(intArrayOf(0), 0) + val NONE = CodeWrapLayout(false, emptyMap()) + } +} + +/** ASCII is fixed-pitch; other text needs the same shaping for measurement and drawing. */ +internal fun createCodeLines(text: CharSequence, paint: TextPaint, width: Int): CodeLines { + val characterWidth = paint.measureText("M") + val lineHeight = ceil(paint.fontMetrics.run { descent - ascent }).toInt() + if (text.all { it in ' '..'~' }) { + val columns = max(1, (width / characterWidth).toInt()) + return CodeLines( + IntArray(max(1, (text.length + columns - 1) / columns)) { + it * columns + }, + lineHeight + ) + } + val layout = StaticLayout.Builder.obtain(text, 0, text.length, paint, max(1, width)) + .setAlignment(Layout.Alignment.ALIGN_NORMAL) + .setIncludePad(false) + .setBreakStrategy(Layout.BREAK_STRATEGY_SIMPLE) + .setHyphenationFrequency(Layout.HYPHENATION_FREQUENCY_NONE) + .build() + return CodeLines(IntArray(layout.lineCount) { layout.getLineStart(it) }, lineHeight, layout) +} + +internal class CodeLayoutCache { + private data class Entry(val row: DiffRow, val tokens: List?, val lines: CodeLines) + private var entries = emptyMap() + private var previousStyle: DiffStyle? = null + private var previousTheme: DiffTheme? = null + private var previousWidth = 0 + + /** Entries are immutable; a worker can reuse them without changing the displayed cache. */ + fun copyForPreparation(): CodeLayoutCache = CodeLayoutCache().also { + it.entries = entries + it.previousStyle = previousStyle + it.previousTheme = previousTheme + it.previousWidth = previousWidth + } + + @Suppress("LongParameterList") + fun layout( + rows: List, + tokens: Map>, + paint: Paint, + style: DiffStyle, + theme: DiffTheme, + width: Int + ): CodeWrapLayout { + if (!style.wordWrap || width < paint.measureText("M")) { + entries = emptyMap() + return CodeWrapLayout.NONE + } + if (previousStyle != style || previousTheme != theme || previousWidth != width) { + entries = emptyMap() + previousStyle = style + previousTheme = theme + previousWidth = width + } + val next = HashMap() + val layouts = HashMap() + for (row in rows) { + if (row.kind != "line") continue + val rowTokens = tokens[row.id] + val cached = entries[row.id] + val entry = if (cached?.row == row && cached.tokens == rowTokens) { + cached + } else { + val text = styledCode(row, rowTokens, theme) + Entry(row, rowTokens, createCodeLines(text, TextPaint(paint), width)) + } + next[row.id] = entry + layouts[row.id] = entry.lines + } + entries = next + return CodeWrapLayout(true, layouts) + } + + private fun styledCode(row: DiffRow, tokens: List?, theme: DiffTheme): CharSequence { + // The ASCII path uses the existing token drawing and rounded highlight rectangles. + if (row.content.all { it in ' '..'~' }) return row.content + val text = SpannableString(row.content) + var offset = 0 + for (token in tokens.orEmpty()) { + val end = (offset + token.content.length).coerceAtMost(text.length) + if (end > offset) { + token.color?.let { + text.setSpan(ForegroundColorSpan(it), offset, end, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE) + } + val fontStyle = (if (token.fontStyle and 2 != 0) Typeface.BOLD else 0) or + (if (token.fontStyle and 1 != 0) Typeface.ITALIC else 0) + if (fontStyle != + 0 + ) { + text.setSpan(StyleSpan(fontStyle), offset, end, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE) + } + if (token.fontStyle and 4 != + 0 + ) { + text.setSpan(UnderlineSpan(), offset, end, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE) + } + } + offset = end + } + if (row.change == "add" || row.change == "delete") { + val bar = if (row.change == "add") theme.addBar else theme.deleteBar + val color = Color.argb(71, Color.red(bar), Color.green(bar), Color.blue(bar)) + for (range in row.wordDiffRanges) { + val start = range.start.coerceIn(0, text.length) + val end = range.end.coerceIn(start, text.length) + if (end > + start + ) { + text.setSpan(BackgroundColorSpan(color), start, end, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE) + } + } + } + return text + } +} diff --git a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt index 97e9f696db90..37fee1cb3613 100644 --- a/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt +++ b/apps/mobile/modules/t3-review-diff/android/src/main/java/expo/modules/t3reviewdiff/T3ReviewDiffView.kt @@ -143,10 +143,13 @@ class T3ReviewDiffView(context: Context, appContext: AppContext) : ExpoView(cont fun setRowsJson(value: String) { rowsDecodeGeneration += 1 val generation = rowsDecodeGeneration + val prepareLayout = canvasView.prepareRows() payloadDecodeExecutor.execute { val decodedRows = parseRows(value) + val codeLayouts = prepareLayout(decodedRows) post { if (generation != rowsDecodeGeneration) return@post + canvasView.useCodeLayouts(codeLayouts) rows = decodedRows lastVisibleFileId = null rebuildVisibleRows() @@ -460,7 +463,7 @@ internal data class DiffWordDiffRange( val end: Int ) -private data class DiffToken( +internal data class DiffToken( val content: String, val color: Int?, val fontStyle: Int @@ -543,6 +546,7 @@ internal data class DiffTheme( } internal data class DiffStyle( + val wordWrap: Boolean, val rowHeightPx: Float, val gutterWidthPx: Float, val codePaddingPx: Float, @@ -564,6 +568,7 @@ internal data class DiffStyle( ) { companion object { fun defaults(density: Float): DiffStyle = DiffStyle( + wordWrap = false, rowHeightPx = 20f * density, gutterWidthPx = 72f * density, codePaddingPx = 10f * density, @@ -587,6 +592,7 @@ internal data class DiffStyle( fun fromJson(value: String, fallback: DiffStyle, density: Float): DiffStyle = try { val json = JSONObject(value) DiffStyle( + wordWrap = json.optBoolean("wordWrap", fallback.wordWrap), rowHeightPx = json.floatDp("rowHeight", fallback.rowHeightPx, density), gutterWidthPx = json.floatDp("gutterWidth", fallback.gutterWidthPx, density), codePaddingPx = json.floatDp("codePadding", fallback.codePaddingPx, density), @@ -684,6 +690,8 @@ private class DiffCanvasView(context: Context) : View(context) { }, ) private var rowOffsets = intArrayOf(0) + + private var codeWrap = CodeWrapLayout.NONE private var verticalOffset = 0 private var horizontalOffset = 0 private val headerPathOffsetsByFileId = mutableMapOf() @@ -699,6 +707,7 @@ private class DiffCanvasView(context: Context) : View(context) { var tokensByRowId: Map> = emptyMap() set(value) { field = value + if (style.wordWrap) rebuildOffsets() invalidate() } var viewedFileIds: Set = emptySet() @@ -725,6 +734,7 @@ private class DiffCanvasView(context: Context) : View(context) { set(value) { field = value drawing.theme = value + if (style.wordWrap) rebuildOffsets() invalidate() } var style: DiffStyle = DiffStyle.defaults(density) @@ -742,6 +752,11 @@ private class DiffCanvasView(context: Context) : View(context) { var onRowTap: ((DiffRow, String, RowTapTarget) -> Unit)? = null var onVisibleRowsChanged: ((Int, Int) -> Unit)? = null + fun prepareRows() = drawing.prepareRows(tokensByRowId, style, width) + fun useCodeLayouts(layouts: CodeLayoutCache) { + drawing.codeLayouts = layouts + } + override fun onMeasure(widthMeasureSpec: Int, heightMeasureSpec: Int) { setMeasuredDimension( MeasureSpec.getSize(widthMeasureSpec), @@ -751,6 +766,8 @@ private class DiffCanvasView(context: Context) : View(context) { override fun onSizeChanged(width: Int, height: Int, oldWidth: Int, oldHeight: Int) { super.onSizeChanged(width, height, oldWidth, oldHeight) + // Wrapped rows take their height from the width, so a new width is a new layout. + if (style.wordWrap && width != oldWidth) layoutRows() setVerticalOffset(verticalOffset) setHorizontalOffset(horizontalOffset) clampHeaderPathOffsets() @@ -818,7 +835,8 @@ private class DiffCanvasView(context: Context) : View(context) { fun horizontalOffset(): Int = horizontalOffset - fun maxHorizontalOffset(): Int = max(0, contentWidthPx - width) + fun maxHorizontalOffset(): Int = + if (codeWrap.enabled) 0 else max(0, contentWidthPx - width) fun maxHorizontalOffset(target: HorizontalPanTarget): Int = if (target.kind == HorizontalPanKind.FILE_HEADER_PATH) { @@ -844,14 +862,20 @@ private class DiffCanvasView(context: Context) : View(context) { } private fun rebuildOffsets() { + layoutRows() + requestLayout() + invalidate() + } + + private fun layoutRows() { + codeWrap = drawing.codeWrapLayout(rows, tokensByRowId, style, width) rowOffsets = IntArray(rows.size + 1) rows.forEachIndexed { index, row -> rowOffsets[index + 1] = rowOffsets[index] + rowHeight(row) } setVerticalOffset(verticalOffset) + setHorizontalOffset(horizontalOffset) clampHeaderPathOffsets() - requestLayout() - invalidate() } private fun rowHeight(row: DiffRow): Int = when (row.kind) { @@ -862,6 +886,7 @@ private class DiffCanvasView(context: Context) : View(context) { } else { (124 * density).toInt() } + "line" -> codeWrap.rowHeight(row.id, style.rowHeightPx.toInt()) else -> style.rowHeightPx.toInt() }.coerceAtLeast(1) @@ -1191,23 +1216,17 @@ private class DiffCanvasView(context: Context) : View(context) { ) } - val tokens = tokensByRowId[row.id] + // Wrapped rows keep the line number and first code line in the first row-height band. + val lines = codeWrap.lines(row.id) + val firstLineBottom = top + lines.firstHeight(style.rowHeightPx.toInt()) drawScrollableCode(canvas, top, bottom) { codeX -> drawing.configureCodePaint(theme.text, 0, style) - drawing.drawWordDiffRanges(canvas, row, codeX, top, bottom) - if (tokens.isNullOrEmpty()) { - canvas.drawText(row.content, codeX, centeredBaseline(top, bottom, textPaint), textPaint) - } else { - var x = codeX - tokens.forEach { token -> - drawing.configureCodePaint(token.color ?: theme.text, token.fontStyle, style) - canvas.drawText(token.content, x, centeredBaseline(top, bottom, textPaint), textPaint) - x += textPaint.measureText(token.content) - } - } + drawing.drawWordDiffRanges(canvas, row, codeX, top, firstLineBottom, lines) + val baseline = lines.baseline(top, firstLineBottom, textPaint) + drawing.drawCode(canvas, row.content, tokensByRowId[row.id], codeX, baseline, style, lines) } - drawLineNumber(canvas, row, top, bottom) + drawLineNumber(canvas, row, top, firstLineBottom) } private fun drawLineNumber(canvas: Canvas, row: DiffRow, top: Int, bottom: Int) { diff --git a/apps/mobile/modules/t3-review-diff/android/src/test/java/expo/modules/t3reviewdiff/ReviewDiffCodeLayoutTest.kt b/apps/mobile/modules/t3-review-diff/android/src/test/java/expo/modules/t3reviewdiff/ReviewDiffCodeLayoutTest.kt new file mode 100644 index 000000000000..1f4234b7175f --- /dev/null +++ b/apps/mobile/modules/t3-review-diff/android/src/test/java/expo/modules/t3reviewdiff/ReviewDiffCodeLayoutTest.kt @@ -0,0 +1,150 @@ +package expo.modules.t3reviewdiff + +import android.graphics.Bitmap +import android.graphics.Canvas +import android.graphics.Color +import android.graphics.Typeface +import android.text.Spanned +import android.text.TextPaint +import android.text.style.BackgroundColorSpan +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotSame +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.robolectric.annotation.GraphicsMode + +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [35], manifest = Config.NONE) +@GraphicsMode(GraphicsMode.Mode.NATIVE) +class ReviewDiffCodeLayoutTest { + private val paint = TextPaint().apply { + color = Color.WHITE + textSize = 24f + typeface = Typeface.MONOSPACE + } + + @Test + fun unicodeAndTabsFitWithoutSplittingClusters() { + val fixtures = listOf("漢字表示", "e\u0301", "👨‍👩‍👧‍👦", "مرحبا بالعالم ", "\tvalue ") + for (fixture in fixtures) { + val text = fixture.repeat(40) + for (width in listOf(180, 280, 420)) { + val layout = requireNotNull(createCodeLines(text, paint, width).nativeLayout) + assertInkFits(layout, width, fixture) + assertLinesFit(layout, fixture, width) + } + } + } + + private fun assertLinesFit(layout: android.text.StaticLayout, fixture: String, width: Int) { + val text = layout.text + for (line in 0 until layout.lineCount) { + if (!fixture.contains('\t')) { + assertTrue("$fixture line $line at $width", layout.getLineMax(line) <= width + 1) + } + val start = layout.getLineStart(line) + assertTrue(start == 0 || !Character.isLowSurrogate(text[start])) + if (fixture == "👨‍👩‍👧‍👦" || fixture == "e\u0301") { + assertEquals(0, start % fixture.length) + } + } + } + + private fun assertInkFits(layout: android.text.StaticLayout, width: Int, fixture: String) { + val bitmap = Bitmap.createBitmap(width + 40, layout.height, Bitmap.Config.ARGB_8888) + layout.draw(Canvas(bitmap)) + for (x in width + 1 until bitmap.width) { + for (y in 0 until bitmap.height) { + assertEquals("$fixture ink outside width $width", 0, Color.alpha(bitmap.getPixel(x, y))) + } + } + bitmap.recycle() + } + + @Test + fun asciiSegmentsCoverTheWholeLineAndFit() { + val text = "const value = 123; ".repeat(100) + val lines = createCodeLines(text, paint, 280) + val pieces = lines.starts.indices.map { + text.substring(lines.starts[it], lines.end(it, text.length)) + } + assertEquals(text, pieces.joinToString("")) + assertTrue(pieces.all { paint.measureText(it) <= 280 }) + } + + @Test + fun changingCommentHeightReusesCodeButWidthAndContentInvalidateIt() { + val cache = CodeLayoutCache() + val row = row("漢字".repeat(100)) + val comment = row.copy(kind = "comment", id = "comment", content = "", commentText = "Before") + val style = DiffStyle.defaults(1f).copy(wordWrap = true) + val theme = DiffTheme.fallback("light") + val first = cache.layout( + listOf(row, comment), + emptyMap(), + paint, + style, + theme, + 280 + ).lines(row.id) + val second = cache.layout( + listOf(row, comment.copy(commentText = "After")), + emptyMap(), + paint, + style, + theme, + 280, + ).lines(row.id) + assertSame(first, second) + val narrow = cache.layout(listOf(row), emptyMap(), paint, style, theme, 180).lines(row.id) + assertNotSame(first, narrow) + assertTrue(narrow.extraHeight > first.extraHeight) + val edited = cache.layout( + listOf(row.copy(content = "短い")), + emptyMap(), + paint, + style, + theme, + 180 + ).lines(row.id) + assertTrue(edited.extraHeight < narrow.extraHeight) + assertEquals( + 0, + cache.layout( + listOf(row), + emptyMap(), + paint, + style.copy(wordWrap = false), + theme, + 180 + ).extraHeight(row.id) + ) + } + + @Test + fun highlightsUseNativeTextRangesAndSurviveSyntaxArrival() { + val cache = CodeLayoutCache() + val row = row("漢字".repeat(30)).copy(wordDiffRanges = listOf(DiffWordDiffRange(3, 21))) + val style = DiffStyle.defaults(1f).copy(wordWrap = true) + val theme = DiffTheme.fallback("light") + val initial = cache.layout(listOf(row), emptyMap(), paint, style, theme, 180).lines(row.id) + val tokens = mapOf(row.id to listOf(DiffToken(row.content, 0xff008800.toInt(), 2))) + val highlighted = cache.layout(listOf(row), tokens, paint, style, theme, 180).lines(row.id) + assertNotSame(initial, highlighted) + val text = requireNotNull(highlighted.nativeLayout).text as Spanned + val span = text.getSpans(0, text.length, BackgroundColorSpan::class.java).single() + assertEquals(3, text.getSpanStart(span)) + assertEquals(21, text.getSpanEnd(span)) + } + + private fun row(content: String) = DiffRow( + kind = "line", id = "line", fileId = "file", filePath = "test.ts", previousPath = null, + changeType = "modified", additions = 1, deletions = 0, text = "", content = content, + change = "add", oldLineNumber = null, newLineNumber = 1, wordDiffRanges = emptyList(), + commentText = "", commentRangeLabel = "", commentSectionTitle = "", + ) +} diff --git a/apps/mobile/modules/t3-review-diff/ios/ReviewDiffCodeLayout.swift b/apps/mobile/modules/t3-review-diff/ios/ReviewDiffCodeLayout.swift new file mode 100644 index 000000000000..1d8b3e8ea8d9 --- /dev/null +++ b/apps/mobile/modules/t3-review-diff/ios/ReviewDiffCodeLayout.swift @@ -0,0 +1,123 @@ +import UIKit + +/// ASCII uses fixed-pitch columns. TextKit handles shaping, tabs, and Unicode highlights. +final class ReviewDiffCodeLayout: NSObject { + // Measurement reuses one engine; only recently drawn rows retain a full TextKit layout. + private static var measurer: ReviewDiffTextLayout { + let key = "T3ReviewDiff.textMeasurer" + if let layout = Thread.current.threadDictionary[key] as? ReviewDiffTextLayout { return layout } + let layout = ReviewDiffTextLayout() + Thread.current.threadDictionary[key] = layout + return layout + } + private static let drawnLayouts: NSCache = { + let cache = NSCache() + cache.countLimit = 128 + return cache + }() + let text: String + let starts: [Int] + let lineHeight: CGFloat + let firstLineHeight: CGFloat + let extraHeight: CGFloat + private let font: UIFont + private let width: CGFloat + private let characterWidth: CGFloat + let usesNativeLayout: Bool + + init(text: String, font: UIFont, width: CGFloat, characterWidth: CGFloat) { + self.text = text + self.font = font + self.width = width + self.characterWidth = characterWidth + lineHeight = ceil(font.lineHeight) + if text.utf8.allSatisfy({ $0 >= 32 && $0 <= 126 }) { + let columns = max(1, Int(width / characterWidth)) + starts = Array(stride(from: 0, to: max(1, text.utf8.count), by: columns)) + firstLineHeight = font.lineHeight + extraHeight = CGFloat(starts.count - 1) * lineHeight + usesNativeLayout = false + } else { + let layout = Self.measurer + layout.configure(text: text, font: font, width: width, characterWidth: characterWidth) + let manager = layout.manager + let container = layout.container + usesNativeLayout = true + starts = [0] + firstLineHeight = manager.numberOfGlyphs > 0 + ? manager.lineFragmentRect(forGlyphAt: 0, effectiveRange: nil).height : font.lineHeight + extraHeight = max(0, manager.usedRect(for: container).height - firstLineHeight) + } + } + + private func nativeLayout() -> ReviewDiffTextLayout { + if let cached = Self.drawnLayouts.object(forKey: self) { return cached } + let layout = ReviewDiffTextLayout() + layout.configure(text: text, font: font, width: width, characterWidth: characterWidth) + Self.drawnLayouts.setObject(layout, forKey: self) + return layout + } + + /// Only colors change when syntax tokens arrive; the measured text and font stay intact. + func decorate(text: NSAttributedString, highlights: [NSRange], color: UIColor, version: Int) { + guard usesNativeLayout else { return } + let layout = nativeLayout() + guard layout.decorationVersion != version else { return } + let storage = layout.storage + let fullRange = NSRange(location: 0, length: storage.length) + storage.beginEditing() + storage.removeAttribute(.foregroundColor, range: fullRange) + storage.removeAttribute(.backgroundColor, range: fullRange) + text.enumerateAttribute(.foregroundColor, in: NSRange(location: 0, length: text.length)) { value, range, _ in + let intersection = NSIntersectionRange(range, fullRange) + if let value, intersection.length > 0 { + storage.addAttribute(.foregroundColor, value: value, range: intersection) + } + } + for range in highlights { + let intersection = NSIntersectionRange(range, fullRange) + if intersection.length > 0 { + storage.addAttribute(.backgroundColor, value: color, range: intersection) + } + } + storage.endEditing() + layout.decorationVersion = version + } + + func draw(at origin: CGPoint, clip: CGRect) { + guard usesNativeLayout else { return } + let layout = nativeLayout() + let manager = layout.manager + let container = layout.container + let visible = clip.offsetBy(dx: -origin.x, dy: -origin.y) + let range = manager.glyphRange(forBoundingRect: visible, in: container) + manager.drawBackground(forGlyphRange: range, at: origin) + manager.drawGlyphs(forGlyphRange: range, at: origin) + } +} + +private final class ReviewDiffTextLayout { + let storage = NSTextStorage() + let manager = NSLayoutManager() + let container = NSTextContainer(size: .zero) + var decorationVersion = -1 + + init() { + container.lineFragmentPadding = 0 + container.lineBreakMode = .byCharWrapping + manager.addTextContainer(container) + storage.addLayoutManager(manager) + } + + func configure(text: String, font: UIFont, width: CGFloat, characterWidth: CGFloat) { + let paragraph = NSMutableParagraphStyle() + paragraph.lineBreakMode = .byCharWrapping + paragraph.tabStops = [] + paragraph.defaultTabInterval = characterWidth * 4 + container.size = CGSize(width: max(1, width), height: .greatestFiniteMagnitude) + storage.setAttributedString(NSAttributedString(string: text, attributes: [ + .font: font, .ligature: 0, .paragraphStyle: paragraph, + ])) + manager.ensureLayout(for: container) + } +} diff --git a/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift b/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift index 74111988f150..e2400e0a9393 100644 --- a/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift +++ b/apps/mobile/modules/t3-review-diff/ios/T3ReviewDiffView.swift @@ -137,6 +137,7 @@ private struct ReviewDiffNativeTheme { } private struct ReviewDiffNativeStylePayload: Decodable { + let wordWrap: Bool? let rowHeight: Double? let contentWidth: Double? let changeBarWidth: Double? @@ -170,6 +171,7 @@ private struct ReviewDiffNativeStylePayload: Decodable { } private struct ReviewDiffNativeStyle { + let wordWrap: Bool let rowHeight: CGFloat let contentWidth: CGFloat let changeBarWidth: CGFloat @@ -203,6 +205,7 @@ private struct ReviewDiffNativeStyle { static func resolve(_ payload: ReviewDiffNativeStylePayload?) -> ReviewDiffNativeStyle { ReviewDiffNativeStyle( + wordWrap: payload?.wordWrap ?? false, rowHeight: metric(payload?.rowHeight, fallback: 24), contentWidth: metric(payload?.contentWidth, fallback: 2800), changeBarWidth: nonNegativeMetric(payload?.changeBarWidth, fallback: 4), @@ -277,6 +280,7 @@ private struct ReviewDiffNativeStyle { func applyingOverrides(rowHeight: CGFloat?, contentWidth: CGFloat?) -> ReviewDiffNativeStyle { ReviewDiffNativeStyle( + wordWrap: wordWrap, rowHeight: rowHeight ?? self.rowHeight, contentWidth: contentWidth ?? self.contentWidth, changeBarWidth: changeBarWidth, @@ -434,16 +438,21 @@ public final class T3ReviewDiffView: ExpoView, UIScrollViewDelegate { guard let self, generation == self.rowsDecodeGeneration else { return } - self.rows = decodedRows - self.contentView.rows = decodedRows - self.hasAppliedInitialRowIndex = false - self.lastVisibleFileId = nil - self.emitDebug("rows-decoded", [ - "rows": decodedRows.count, - "firstKind": decodedRows.first?.kind ?? "none", - ]) - self.updateContentMetrics() - self.applyPendingScrollIfNeeded() + self.contentView.prepareRows(decodedRows, on: self.payloadDecodeQueue, isCurrent: { [weak self] in + generation == self?.rowsDecodeGeneration + }, completion: { [weak self] in + guard let self, generation == self.rowsDecodeGeneration else { return } + self.rows = decodedRows + self.contentView.rows = decodedRows + self.hasAppliedInitialRowIndex = false + self.lastVisibleFileId = nil + self.emitDebug("rows-decoded", [ + "rows": decodedRows.count, + "firstKind": decodedRows.first?.kind ?? "none", + ]) + self.updateContentMetrics() + self.applyPendingScrollIfNeeded() + }) } } catch { let message = error.localizedDescription @@ -652,6 +661,7 @@ public final class T3ReviewDiffView: ExpoView, UIScrollViewDelegate { private func updateContentMetrics() { let style = contentView.style + contentView.viewportWidth = bounds.width let height = max(bounds.height, contentView.contentHeight) let width = bounds.width scrollView.contentSize = CGSize(width: bounds.width, height: height) @@ -661,7 +671,6 @@ public final class T3ReviewDiffView: ExpoView, UIScrollViewDelegate { width: max(width, 1), height: max(bounds.height, 1) ) - contentView.viewportWidth = bounds.width contentView.verticalOffset = scrollView.contentOffset.y contentView.invalidateVisibleViewport() contentView.setNeedsDisplay() @@ -929,6 +938,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { headerPathOffsetsByFileId.removeAll() activePanFileId = nil activePanKind = nil + codeDecorationVersion += 1 tokenAttributedStringsByRowId.removeAll() rebuildRowLayout() setNeedsDisplayForVisibleBounds() @@ -936,6 +946,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { } var tokensByRowId: [String: [ReviewDiffNativeToken]] = [:] { didSet { + codeDecorationVersion += 1 tokenAttributedStringsByRowId.removeAll() clampHorizontalOffsets() setNeedsDisplayForVisibleBounds() @@ -976,6 +987,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { } var style = ReviewDiffNativeStyle.resolve(nil) { didSet { + codeDecorationVersion += 1 tokenAttributedStringsByRowId.removeAll() rebuildRowLayout() clampHorizontalOffsets() @@ -984,6 +996,10 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { } var viewportWidth: CGFloat = 0 { didSet { + // Wrapped rows take their height from the width, so a new width is a new layout. + if style.wordWrap, viewportWidth != oldValue { + rebuildRowLayout() + } clampHorizontalOffsets() setNeedsDisplayForVisibleBounds() } @@ -992,6 +1008,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { var theme = ReviewDiffNativeTheme.resolve("light") { didSet { tokenColorsByHex.removeAll() + codeDecorationVersion += 1 tokenAttributedStringsByRowId.removeAll() setNeedsDisplayForVisibleBounds() } @@ -1004,6 +1021,13 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { private var tokenColorsByHex: [String: UIColor] = [:] private var tokenAttributedStringsByRowId: [String: NSAttributedString] = [:] private var codeCharacterWidth: CGFloat = 8 + /// Columns per visual line while word wrap is on; nil while code rows pan horizontally. + private var codeWrapColumns: Int? + /// Text geometry survives comment height changes; width, font, and content invalidate it. + private var codeLayoutsByRowId: [String: ReviewDiffCodeLayout] = [:] + private var codeLayoutWidth: CGFloat = 0 + private var codeLayoutFont: UIFont? + private var codeDecorationVersion = 0 private var panStartHorizontalOffset: CGFloat = 0 private var activePanFileId: String? private var activePanKind: ReviewDiffHorizontalPanKind? @@ -1029,6 +1053,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { stickyWidth + style.codePadding } + /// Height before word wrap. Laid-out rows use height(at:), which includes wrapped lines. private func height(for row: ReviewDiffNativeRow) -> CGFloat { if row.kind == "file" { return style.fileHeaderHeight @@ -1045,6 +1070,16 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { return style.rowHeight } + /// Laid-out height, including wrapped lines. Requires a layout built from the current rows. + private func height(at index: Int) -> CGFloat { + let nextOffset = index + 1 < rowOffsets.count ? rowOffsets[index + 1] : contentHeight + return nextOffset - rowOffsets[index] + } + + private var codeWrapLineHeight: CGFloat { + ceil(codeFont.lineHeight) + } + func frameForRow(at index: Int) -> CGRect? { guard rows.indices.contains(index), rowOffsets.indices.contains(index) else { return nil @@ -1054,43 +1089,110 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { x: 0, y: rowOffsets[index], width: max(viewportWidth, 1), - height: height(for: rows[index]) + height: height(at: index) ) } + /// Shape new content on the existing decode worker before publishing rows to the UI. + func prepareRows( + _ rows: [ReviewDiffNativeRow], + on queue: DispatchQueue, + isCurrent: @escaping () -> Bool, + completion: @escaping () -> Void + ) { + let font = codeFont + let width = viewportWidth - codeStartX - style.codePadding + let characterWidth = monospaceCharacterWidth(font: font) + guard style.wordWrap, width >= characterWidth, characterWidth > 0 else { + completion() + return + } + let cached = codeLayoutWidth == width && codeLayoutFont == font ? codeLayoutsByRowId : [:] + queue.async { [weak self] in + var layouts: [String: ReviewDiffCodeLayout] = [:] + for row in rows where row.kind == "line" { + guard let text = row.content else { continue } + if let previous = cached[row.id], previous.text == text { + layouts[row.id] = previous + } else { + layouts[row.id] = ReviewDiffCodeLayout(text: text, font: font, width: width, characterWidth: characterWidth) + } + } + DispatchQueue.main.async { [weak self] in + guard let self, isCurrent() else { return } + if self.codeFont != font || self.viewportWidth - self.codeStartX - self.style.codePadding != width { + self.prepareRows(rows, on: queue, isCurrent: isCurrent, completion: completion) + return + } + self.codeLayoutWidth = width + self.codeLayoutFont = font + self.codeLayoutsByRowId = layouts + completion() + } + } + } + private func rebuildRowLayout() { var nextOffsets: [CGFloat] = [] var nextFileHeaderRowIndices: [Int] = [] nextOffsets.reserveCapacity(rows.count) var maxColumnCountsByFileId: [String: Int] = [:] + var nextCodeLayouts: [String: ReviewDiffCodeLayout] = [:] var offset: CGFloat = 0 + let font = codeFont + let characterWidth = monospaceCharacterWidth(font: font) + let wrapAvailableWidth = viewportWidth - codeStartX - style.codePadding + let wrapColumns = style.wordWrap && characterWidth > 0 && wrapAvailableWidth >= characterWidth + ? Int(wrapAvailableWidth / characterWidth) + : nil + if codeLayoutWidth != wrapAvailableWidth || codeLayoutFont != font { + codeLayoutsByRowId.removeAll() + codeLayoutWidth = wrapAvailableWidth + codeLayoutFont = font + } for (index, row) in rows.enumerated() { nextOffsets.append(offset) if row.kind == "file" { nextFileHeaderRowIndices.append(index) } - offset += height(for: row) + var rowHeight = height(for: row) let fileId = resolvedFileId(for: row) switch row.kind { case "line": - maxColumnCountsByFileId[fileId] = max( - maxColumnCountsByFileId[fileId] ?? 0, - row.content?.count ?? 0 - ) + // UTF-16 columns match the word diff ranges and the segments drawCodeLines draws. + let columnCount = row.content?.utf16.count ?? 0 + maxColumnCountsByFileId[fileId] = max(maxColumnCountsByFileId[fileId] ?? 0, columnCount) + if wrapColumns != nil, let content = row.content { + let cached = codeLayoutsByRowId[row.id] + let layout: ReviewDiffCodeLayout + if let cached, cached.text == content { + layout = cached + } else { + layout = ReviewDiffCodeLayout( + text: content, font: font, width: wrapAvailableWidth, characterWidth: characterWidth + ) + } + nextCodeLayouts[row.id] = layout + if rowHeight > 0 { + rowHeight = max(rowHeight, layout.firstLineHeight) + layout.extraHeight + } + } case "hunk": maxColumnCountsByFileId[fileId] = max( maxColumnCountsByFileId[fileId] ?? 0, - row.text?.count ?? 0 + row.text?.utf16.count ?? 0 ) default: - continue + break } + offset += rowHeight } - let characterWidth = monospaceCharacterWidth(font: codeFont) codeCharacterWidth = characterWidth + codeWrapColumns = wrapColumns + codeLayoutsByRowId = nextCodeLayouts contentWidthsByFileId = maxColumnCountsByFileId.mapValues { maxColumnCount in let measuredWidth = ceil(CGFloat(maxColumnCount) * characterWidth) + style.codePadding * 2 return max(0, min(style.contentWidth, measuredWidth)) @@ -1498,7 +1600,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { while lowerBound <= upperBound { let midpoint = (lowerBound + upperBound) / 2 let rowStart = rowOffsets[midpoint] - let rowEnd = rowStart + height(for: rows[midpoint]) + let rowEnd = rowStart + height(at: midpoint) if absoluteY < rowStart { upperBound = midpoint - 1 @@ -1521,7 +1623,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { var upperBound = rows.count while lowerBound < upperBound { let midpoint = (lowerBound + upperBound) / 2 - let rowEnd = rowOffsets[midpoint] + height(for: rows[midpoint]) + let rowEnd = rowOffsets[midpoint] + height(at: midpoint) if rowEnd < absoluteY { lowerBound = midpoint + 1 @@ -1609,6 +1711,9 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { let row = rows.first(where: { resolvedFileId(for: $0) == target.fileId && $0.kind == "file" }) { return maxHeaderPathOffset(for: row) } + if codeWrapColumns != nil { + return 0 + } return max(0, contentWidth(for: target.fileId) - max(0, viewportWidth - codeStartX)) } @@ -1728,7 +1833,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { var drawnRowCount = 0 for rowIndex in firstRowIndex...lastRowIndex { let rowStart = rowOffsets[rowIndex] - let rowHeight = height(for: rows[rowIndex]) + let rowHeight = height(at: rowIndex) if rowHeight <= 0 { continue } @@ -1786,7 +1891,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { private func drawRow(_ row: ReviewDiffNativeRow, rowIndex: Int, context: CGContext) { let rowY = rowOffsets[rowIndex] - verticalOffset - let fullRect = CGRect(x: 0, y: rowY, width: max(bounds.width, viewportWidth), height: height(for: row)) + let fullRect = CGRect(x: 0, y: rowY, width: max(bounds.width, viewportWidth), height: height(at: rowIndex)) switch row.kind { case "file": @@ -2172,15 +2277,21 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { let horizontalOffset = horizontalOffset(for: fileId) let contentWidth = contentWidth(for: fileId) let change = row.change ?? "context" + // Wrapped rows keep the line number and first code line in the first row-height band. + let layout = codeLayoutsByRowId[row.id] + let firstLineRect = CGRect( + x: rect.minX, y: rect.minY, width: rect.width, + height: max(style.rowHeight, layout?.firstLineHeight ?? 0) + ) rowBackground(for: change).setFill() context.fill(rect) if change == "add" { theme.addBar.setFill() - context.fill(CGRect(x: 0, y: rect.minY, width: style.changeBarWidth, height: style.rowHeight)) + context.fill(CGRect(x: 0, y: rect.minY, width: style.changeBarWidth, height: rect.height)) } else if change == "delete" { drawDeleteStripes( - rect: CGRect(x: 0, y: rect.minY, width: style.changeBarWidth, height: style.rowHeight), + rect: CGRect(x: 0, y: rect.minY, width: style.changeBarWidth, height: rect.height), context: context ) } @@ -2193,7 +2304,7 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { "\(lineNumber)", rect: CGRect( x: style.changeBarWidth, - y: centeredTextY(in: rect, font: lineNumberFont), + y: centeredTextY(in: firstLineRect, font: lineNumberFont), width: style.gutterWidth - style.codePadding, height: lineNumberFont.lineHeight ), @@ -2203,28 +2314,85 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { } context.saveGState() - context.clip(to: CGRect(x: stickyWidth, y: rect.minY, width: max(0, viewportWidth - stickyWidth), height: style.rowHeight)) + context.clip(to: CGRect(x: stickyWidth, y: rect.minY, width: max(0, viewportWidth - stickyWidth), height: rect.height)) let codeTextRect = CGRect( x: codeStartX - horizontalOffset, - y: centeredTextY(in: rect, font: codeFont), + y: centeredTextY(in: firstLineRect, font: codeFont), width: contentWidth, height: codeFont.lineHeight ) - drawWordDiffRanges(row, rowRect: rect, context: context, horizontalOffset: horizontalOffset) + if let layout, layout.usesNativeLayout { + let text = tokensByRowId[row.id].map { + tokenAttributedString(rowId: row.id, tokens: $0, fallbackColor: theme.text, font: codeFont) + } ?? NSAttributedString(string: row.content ?? "", attributes: [.foregroundColor: theme.text]) + let highlights = (change == "add" || change == "delete") ? (row.wordDiffRanges ?? []) : [] + layout.decorate( + text: text, + highlights: highlights.filter { $0.start >= 0 && $0.end > $0.start }.map { + NSRange(location: $0.start, length: $0.end - $0.start) + }, + color: (change == "add" ? theme.addBar : theme.deleteBar).withAlphaComponent(0.28), + version: codeDecorationVersion + ) + layout.draw( + at: CGPoint(x: codeStartX, y: rect.minY + max(0, (firstLineRect.height - layout.firstLineHeight) / 2)), + clip: context.boundingBoxOfClipPath + ) + context.restoreGState() + return + } + let lineStarts = layout?.starts ?? [0] + drawWordDiffRanges( + row, + lineStarts: lineStarts, + firstLineRect: firstLineRect, + context: context, + horizontalOffset: horizontalOffset + ) if let tokens = tokensByRowId[row.id], !tokens.isEmpty { - drawTokenText( + let attributedText = tokenAttributedString( rowId: row.id, - tokens, - rect: codeTextRect, + tokens: tokens, fallbackColor: theme.text, font: codeFont ) + drawCodeLines(length: attributedText.length, lineStarts: lineStarts, firstLineRect: codeTextRect) { range, lineRect in + let segment = range.length == attributedText.length + ? attributedText + : attributedText.attributedSubstring(from: range) + segment.draw(in: lineRect) + } } else { - drawText(row.content ?? "", rect: codeTextRect, color: theme.text, font: codeFont) + let content = (row.content ?? "") as NSString + drawCodeLines(length: content.length, lineStarts: lineStarts, firstLineRect: codeTextRect) { range, lineRect in + let segment = range.length == content.length ? content as String : content.substring(with: range) + drawText(segment, rect: lineRect, color: theme.text, font: codeFont) + } } context.restoreGState() } + /// Draws the segment starting at each of the row's line starts on its own visual line. + private func drawCodeLines( + length: Int, + lineStarts: [Int], + firstLineRect: CGRect, + draw: (NSRange, CGRect) -> Void + ) { + var lineRect = firstLineRect + let clip = UIGraphicsGetCurrentContext()?.boundingBoxOfClipPath ?? bounds + let first = max(0, Int(floor((clip.minY - firstLineRect.minY) / codeWrapLineHeight))) + let last = min(lineStarts.count, Int(ceil((clip.maxY - firstLineRect.minY) / codeWrapLineHeight))) + guard first < last else { return } + lineRect.origin.y += CGFloat(first) * codeWrapLineHeight + for line in first.. start else { + continue + } + let highlightRect = CGRect( + x: codeStartX - horizontalOffset + CGFloat(start - lineStart) * codeCharacterWidth, + y: highlightY + CGFloat(line) * codeWrapLineHeight, + width: max(2, CGFloat(end - start) * codeCharacterWidth), + height: highlightHeight + ) + UIBezierPath(roundedRect: highlightRect, cornerRadius: 3).fill() + } } } @@ -2448,22 +2624,6 @@ private final class ReviewDiffContentView: UIView, UIGestureRecognizerDelegate { return (sample as NSString).size(withAttributes: attributes).width / CGFloat(sampleLength) } - private func drawTokenText( - rowId: String, - _ tokens: [ReviewDiffNativeToken], - rect: CGRect, - fallbackColor: UIColor, - font: UIFont - ) { - let attributedText = tokenAttributedString( - rowId: rowId, - tokens: tokens, - fallbackColor: fallbackColor, - font: font - ) - attributedText.draw(in: rect) - } - private func tokenAttributedString( rowId: String, tokens: [ReviewDiffNativeToken], diff --git a/apps/mobile/modules/t3-review-diff/tests/ios/main.swift b/apps/mobile/modules/t3-review-diff/tests/ios/main.swift new file mode 100644 index 000000000000..baeeaae77d9a --- /dev/null +++ b/apps/mobile/modules/t3-review-diff/tests/ios/main.swift @@ -0,0 +1,60 @@ +import UIKit + +func check(_ passed: Bool, _ message: String = "Failed layout check") { + if !passed { + FileHandle.standardError.write(Data((message + "\n").utf8)) + exit(1) + } +} + +// Runs the production layout against UIKit through Mac Catalyst, without launching an app. +let font = UIFont.monospacedSystemFont(ofSize: 14, weight: .regular) +let characterWidth = ("M" as NSString).size(withAttributes: [.font: font]).width +let fixtures = ["漢字表示", "e\u{301}", "👨‍👩‍👧‍👦", "مرحبا بالعالم ", "\tvalue "] +var cases = 0 +for fixture in fixtures { + let text = String(repeating: fixture, count: 40) + var previousHeight = CGFloat.greatestFiniteMagnitude + for width: CGFloat in [180, 280, 420] { + let layout = ReviewDiffCodeLayout(text: text, font: font, width: width, characterWidth: characterWidth) + let height = layout.firstLineHeight + layout.extraHeight + check(height <= previousHeight, "Wider text must not require more height") + previousHeight = height + let fullRange = NSRange(location: 0, length: text.utf16.count) + let attributed = NSAttributedString(string: text, attributes: [.foregroundColor: UIColor.black]) + layout.decorate(text: attributed, highlights: [], color: .clear, version: 0) + let format = UIGraphicsImageRendererFormat() + format.scale = 1 + format.opaque = false + format.preferredRange = .standard + let size = CGSize(width: width + 40, height: ceil(height)) + let image = UIGraphicsImageRenderer(size: size, format: format).image { _ in + layout.draw(at: .zero, clip: CGRect(origin: .zero, size: size)) + } + let bitmap = image.cgImage! + let data = bitmap.dataProvider!.data! + let bytes = CFDataGetBytePtr(data)! + // Render without a viewport clip so an overflowing glyph cannot hide behind clipping. + for y in 0.. JSON.stringify(nativeReviewDiffTheme), [nativeReviewDiffTheme], ); + // The card's height is sized from its row count, so its snippet stays unwrapped. const nativeStyleJson = useMemo( - () => JSON.stringify(nativeReviewDiffStyle), + () => JSON.stringify({ ...nativeReviewDiffStyle, wordWrap: false }), [nativeReviewDiffStyle], ); const nativeDiffHeight = useMemo( diff --git a/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts b/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts index 8bea04c524dd..3056cc2136ca 100644 --- a/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts +++ b/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts @@ -63,8 +63,13 @@ function opaqueNativeHexColor(color: string, background: string): string { return `#${channels.map((channel) => channel.toString(16).padStart(2, "0")).join("")}`; } -export function createNativeReviewDiffStyle(codeSurface: ResolvedMobileCodeSurface) { +/** `wordWrap` wraps line rows at the view width instead of panning them horizontally. */ +export function createNativeReviewDiffStyle( + codeSurface: ResolvedMobileCodeSurface, + wordWrap: boolean, +) { return { + wordWrap, rowHeight: codeSurface.rowHeight, contentWidth: NATIVE_REVIEW_DIFF_CONTENT_WIDTH, changeBarWidth: 4, diff --git a/apps/mobile/src/features/review/reviewDiffHighlightScheduler.test.ts b/apps/mobile/src/features/review/reviewDiffHighlightScheduler.test.ts new file mode 100644 index 000000000000..3792e49f54c7 --- /dev/null +++ b/apps/mobile/src/features/review/reviewDiffHighlightScheduler.test.ts @@ -0,0 +1,74 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; + +import { createReviewDiffHighlightScheduler } from "./reviewDiffHighlightScheduler"; + +describe("review diff highlighting while scrolling", () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it("keeps requesting new rows during gradual scrolling through a large diff", () => { + const request = vi.fn(); + const scheduler = createReviewDiffHighlightScheduler(request); + for (let firstRowIndex = 1; firstRowIndex <= 1_674; firstRowIndex++) { + scheduler.update({ firstRowIndex, lastRowIndex: firstRowIndex + 80 }); + vi.advanceTimersByTime(16); + } + expect(request.mock.calls.length).toBeGreaterThan(100); + vi.advanceTimersByTime(150); + expect(request).toHaveBeenLastCalledWith({ firstRowIndex: 1_674, lastRowIndex: 1_754 }); + }); + + it("highlights the settled viewport even below the movement threshold", () => { + const request = vi.fn(); + const scheduler = createReviewDiffHighlightScheduler(request); + scheduler.update({ firstRowIndex: 2, lastRowIndex: 82 }); + vi.advanceTimersByTime(100); + scheduler.update({ firstRowIndex: 3, lastRowIndex: 83 }); + vi.advanceTimersByTime(100); + expect(request).not.toHaveBeenCalled(); + vi.advanceTimersByTime(50); + expect(request).toHaveBeenCalledExactlyOnceWith({ firstRowIndex: 3, lastRowIndex: 83 }); + }); + + it("does not let repeated draw events starve the settled refresh", () => { + const request = vi.fn(); + const scheduler = createReviewDiffHighlightScheduler(request); + for (let i = 0; i < 10; i++) { + scheduler.update({ firstRowIndex: 1, lastRowIndex: 81 }); + vi.advanceTimersByTime(30); + } + expect(request).toHaveBeenCalledExactlyOnceWith({ firstRowIndex: 1, lastRowIndex: 81 }); + }); + + it("requests large jumps and reverse scrolling immediately without stale timers", () => { + const request = vi.fn(); + const scheduler = createReviewDiffHighlightScheduler(request); + scheduler.update({ firstRowIndex: 1, lastRowIndex: 81 }); + scheduler.update({ firstRowIndex: 1_000, lastRowIndex: 1_080 }); + scheduler.update({ firstRowIndex: 0, lastRowIndex: 80 }); + vi.runAllTimers(); + expect(request.mock.calls).toEqual([ + [{ firstRowIndex: 1_000, lastRowIndex: 1_080 }], + [{ firstRowIndex: 0, lastRowIndex: 80 }], + ]); + }); + + it("cancels pending work on disposal and resets the range for a new diff", () => { + const request = vi.fn(); + const scheduler = createReviewDiffHighlightScheduler(request); + scheduler.update({ firstRowIndex: 1, lastRowIndex: 81 }); + scheduler.cancel(); + vi.runAllTimers(); + expect(request).not.toHaveBeenCalled(); + scheduler.update({ firstRowIndex: 1_000, lastRowIndex: 1_080 }); + request.mockClear(); + scheduler.update({ firstRowIndex: 1_001, lastRowIndex: 1_081 }); + scheduler.reset(); + vi.runAllTimers(); + expect(request).not.toHaveBeenCalled(); + scheduler.update({ firstRowIndex: 1, lastRowIndex: 81 }); + expect(request).not.toHaveBeenCalled(); + vi.advanceTimersByTime(150); + expect(request).toHaveBeenCalledExactlyOnceWith({ firstRowIndex: 1, lastRowIndex: 81 }); + }); +}); diff --git a/apps/mobile/src/features/review/reviewDiffHighlightScheduler.ts b/apps/mobile/src/features/review/reviewDiffHighlightScheduler.ts new file mode 100644 index 000000000000..4cededa52954 --- /dev/null +++ b/apps/mobile/src/features/review/reviewDiffHighlightScheduler.ts @@ -0,0 +1,51 @@ +export interface NativeReviewVisibleRange { + readonly firstRowIndex: number; + readonly lastRowIndex: number; +} + +export function createReviewDiffHighlightScheduler( + request: (range: NativeReviewVisibleRange) => void, +) { + let requestedRange: NativeReviewVisibleRange = { firstRowIndex: 0, lastRowIndex: 80 }; + let visibleRange = requestedRange; + let timer: ReturnType | undefined; + + const cancel = () => { + clearTimeout(timer); + timer = undefined; + }; + const flush = () => { + cancel(); + requestedRange = visibleRange; + request(visibleRange); + }; + + return { + update(nextRange: NativeReviewVisibleRange) { + if ( + nextRange.firstRowIndex === visibleRange.firstRowIndex && + nextRange.lastRowIndex === visibleRange.lastRowIndex + ) { + return; + } + visibleRange = nextRange; + cancel(); + // Accumulate small scroll events relative to the last request, not each other. + const movedRows = + Math.abs(nextRange.firstRowIndex - requestedRange.firstRowIndex) + + Math.abs(nextRange.lastRowIndex - requestedRange.lastRowIndex); + if (movedRows >= 20) { + flush(); + } else if (movedRows > 0) { + // Cover the final viewport even when scrolling stops below the threshold. + timer = setTimeout(flush, 150); + } + }, + reset() { + cancel(); + requestedRange = { firstRowIndex: 0, lastRowIndex: 80 }; + visibleRange = requestedRange; + }, + cancel, + }; +} diff --git a/apps/mobile/src/features/review/useNativeReviewDiffHighlighting.ts b/apps/mobile/src/features/review/useNativeReviewDiffHighlighting.ts index 35f06c263666..61a205f9d917 100644 --- a/apps/mobile/src/features/review/useNativeReviewDiffHighlighting.ts +++ b/apps/mobile/src/features/review/useNativeReviewDiffHighlighting.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { highlightNativeReviewDiffVisibleRows, @@ -8,10 +8,10 @@ import { import type { NativeReviewDiffRow } from "../diffs/nativeReviewDiffSurface"; import type { NativeReviewDiffFile } from "../diffs/nativeReviewDiffTypes"; -interface NativeReviewVisibleRange { - readonly firstRowIndex: number; - readonly lastRowIndex: number; -} +import { + createReviewDiffHighlightScheduler, + type NativeReviewVisibleRange, +} from "./reviewDiffHighlightScheduler"; function createEmptyTokenPatch(resetKey: string): string { return JSON.stringify({ resetKey, tokensByRowId: {} }); @@ -43,23 +43,22 @@ export function useNativeReviewDiffHighlighting(input: { }) { const { enabled, files, resetKey, rows, scheme } = input; const highlightedRowIdsRef = useRef>(new Set()); - const visibleRangeRef = useRef({ + const [visibleRange, setVisibleRange] = useState({ firstRowIndex: 0, lastRowIndex: 80, }); const visibleChunkIndexRef = useRef(0); const [tokensPatchJson, setTokensPatchJson] = useState(() => createEmptyTokenPatch(resetKey)); - const [visibleHighlightRequest, setVisibleHighlightRequest] = useState(0); + const [scheduler] = useState(() => createReviewDiffHighlightScheduler(setVisibleRange)); useEffect(() => { + scheduler.reset(); highlightedRowIdsRef.current = new Set(); visibleChunkIndexRef.current = 0; - visibleRangeRef.current = { firstRowIndex: 0, lastRowIndex: 80 }; + setVisibleRange({ firstRowIndex: 0, lastRowIndex: 80 }); setTokensPatchJson(createEmptyTokenPatch(resetKey)); - if (enabled && rows.length > 0) { - setVisibleHighlightRequest((request) => request + 1); - } - }, [enabled, resetKey, rows.length]); + return () => scheduler.cancel(); + }, [enabled, resetKey, rows.length, scheduler]); useEffect(() => { if (!enabled || rows.length === 0) { @@ -67,7 +66,7 @@ export function useNativeReviewDiffHighlighting(input: { } const abortController = new AbortController(); - const requestRange = visibleRangeRef.current; + const requestRange = visibleRange; const engine: NativeReviewDiffHighlightEngine = "native"; void (async () => { @@ -119,22 +118,10 @@ export function useNativeReviewDiffHighlighting(input: { })(); return () => abortController.abort(); - }, [enabled, files, resetKey, rows, scheme, visibleHighlightRequest]); - - const updateVisibleRange = useCallback((nextRange: NativeReviewVisibleRange) => { - const previousRange = visibleRangeRef.current; - const movedRows = - Math.abs(nextRange.firstRowIndex - previousRange.firstRowIndex) + - Math.abs(nextRange.lastRowIndex - previousRange.lastRowIndex); - - visibleRangeRef.current = nextRange; - if (movedRows >= 20) { - setVisibleHighlightRequest((request) => request + 1); - } - }, []); + }, [enabled, files, resetKey, rows, scheme, visibleRange]); return { tokensPatchJson, - updateVisibleRange, + updateVisibleRange: scheduler.update, }; } diff --git a/apps/mobile/src/features/settings/appearance/useAppearanceCodeSurface.ts b/apps/mobile/src/features/settings/appearance/useAppearanceCodeSurface.ts index 62760f1e43fa..5bd7bb469827 100644 --- a/apps/mobile/src/features/settings/appearance/useAppearanceCodeSurface.ts +++ b/apps/mobile/src/features/settings/appearance/useAppearanceCodeSurface.ts @@ -21,8 +21,8 @@ export function useAppearanceCodeSurface(): { ); const nativeSourceStyle = useMemo(() => createNativeSourceStyle(codeSurface), [codeSurface]); const nativeReviewDiffStyle = useMemo( - () => createNativeReviewDiffStyle(codeSurface), - [codeSurface], + () => createNativeReviewDiffStyle(codeSurface, appearance.codeWordBreak), + [appearance.codeWordBreak, codeSurface], ); return { From adcd90858c3bfece59095465d2f49860ba3dcea9 Mon Sep 17 00:00:00 2001 From: maria Date: Sun, 20 Sep 2026 19:10:16 -0300 Subject: [PATCH 12/24] fix(web): allow full contrast in assistant replies (#12405) --- apps/web/src/components/ChatMarkdown.tsx | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/apps/web/src/components/ChatMarkdown.tsx b/apps/web/src/components/ChatMarkdown.tsx index abebe4841cbd..8da35ee35cee 100644 --- a/apps/web/src/components/ChatMarkdown.tsx +++ b/apps/web/src/components/ChatMarkdown.tsx @@ -863,7 +863,10 @@ function MarkdownDetails({ {summary} -
+
{content}
@@ -3332,7 +3335,7 @@ function ChatMarkdown({
Date: Sun, 20 Sep 2026 15:51:32 -0700 Subject: [PATCH 13/24] fix(web): pull request chips share the link hover preview (#12719) --- apps/web/src/components/ChatMarkdown.test.tsx | 1 + apps/web/src/components/ChatMarkdown.tsx | 35 ++------- .../ChatMarkdown.workspace-images.test.tsx | 1 + apps/web/src/components/chat/ChatComposer.tsx | 1 + .../src/components/chat/MessagesTimeline.tsx | 3 +- .../composerContextPresentation.tsx | 4 + apps/web/src/components/contextChipParts.tsx | 58 +++++++++------ .../pullRequest/PullRequestLinkPreview.tsx | 74 +++++++++++-------- apps/web/src/lib/openPullRequestLink.ts | 51 ++++++++++++- 9 files changed, 146 insertions(+), 82 deletions(-) diff --git a/apps/web/src/components/ChatMarkdown.test.tsx b/apps/web/src/components/ChatMarkdown.test.tsx index 4e22e68ca902..3e1222e0ebb9 100644 --- a/apps/web/src/components/ChatMarkdown.test.tsx +++ b/apps/web/src/components/ChatMarkdown.test.tsx @@ -55,6 +55,7 @@ vi.mock("../editorPreferences", () => ({ vi.mock("~/lib/openPullRequestLink", () => ({ findProjectOnChangeRequestHost: () => undefined, parseChangeRequestUrl: () => null, + resolvePullRequestPreviewTarget: () => null, useOpenChangeRequestLink: () => vi.fn(), })); diff --git a/apps/web/src/components/ChatMarkdown.tsx b/apps/web/src/components/ChatMarkdown.tsx index 8da35ee35cee..3b398dd5100e 100644 --- a/apps/web/src/components/ChatMarkdown.tsx +++ b/apps/web/src/components/ChatMarkdown.tsx @@ -53,7 +53,6 @@ import { inlineCodeFilePathCandidate } from "@t3tools/client-runtime/markdown-li import { mediaFileReference, mediaUrlReference } from "@t3tools/client-runtime/media-reference"; import { mediaKindFromPath, mediaMimeTypeFromExtension } from "@t3tools/shared/filePreview"; import * as Cause from "effect/Cause"; -import { sourceControlRepositorySelector } from "@t3tools/shared/sourceControl"; import { AsyncResult } from "effect/unstable/reactivity"; import React, { Children, @@ -178,9 +177,9 @@ import { WORKSPACE_BASENAME_LOOKUP_LIMIT, } from "../workspaceBasenameLookup"; import { - findProjectForChangeRequest, parseChangeRequestUrl, pullRequestCandidateUrlFromReferenceAutolink, + resolvePullRequestPreviewTarget, useOpenChangeRequestLink, } from "~/lib/openPullRequestLink"; import { useOpenLink } from "../browser/useOpenLink"; @@ -2889,32 +2888,14 @@ const CHAT_MARKDOWN_COMPONENTS = { const confirmBeforeOpen = pullRequestAutolink === "reference"; const pullRequestCandidateUrl = confirmBeforeOpen && href ? pullRequestCandidateUrlFromReferenceAutolink(href) : href; - const pullRequestCandidate = pullRequestCandidateUrl - ? parseChangeRequestUrl(pullRequestCandidateUrl) + const pullRequestPreviewTarget = pullRequestCandidateUrl + ? resolvePullRequestPreviewTarget({ + environmentId, + projects, + pullRequestsEnabled: serverConfig?.environment.capabilities.pullRequests === true, + url: pullRequestCandidateUrl, + }) : null; - const pullRequestProject = - environmentId !== null && - serverConfig?.environment.capabilities.pullRequests === true && - pullRequestCandidate !== null - ? findProjectForChangeRequest( - projects.filter((project) => project.environmentId === environmentId), - pullRequestCandidate, - ) - : undefined; - const pullRequestPreviewTarget = - environmentId === null || pullRequestProject === undefined || pullRequestCandidate === null - ? null - : { - environmentId, - input: { - projectId: pullRequestProject.id, - host: pullRequestCandidate.authority ?? pullRequestCandidate.host, - repository: - sourceControlRepositorySelector(pullRequestProject.repositoryIdentity) ?? - pullRequestCandidate.repository, - number: pullRequestCandidate.number, - }, - }; const isSameDocumentLink = href?.startsWith("#") ?? false; const onClick = props.onClick; const canOpenInPreview = Boolean(threadRef) && isPreviewSupportedInRuntime(); diff --git a/apps/web/src/components/ChatMarkdown.workspace-images.test.tsx b/apps/web/src/components/ChatMarkdown.workspace-images.test.tsx index 344eca250ce9..bab751c74816 100644 --- a/apps/web/src/components/ChatMarkdown.workspace-images.test.tsx +++ b/apps/web/src/components/ChatMarkdown.workspace-images.test.tsx @@ -44,6 +44,7 @@ vi.mock("../editorPreferences", () => ({ vi.mock("~/lib/openPullRequestLink", () => ({ findProjectOnChangeRequestHost: () => undefined, parseChangeRequestUrl: () => null, + resolvePullRequestPreviewTarget: () => null, useOpenChangeRequestLink: () => vi.fn(), })); diff --git a/apps/web/src/components/chat/ChatComposer.tsx b/apps/web/src/components/chat/ChatComposer.tsx index 255f61a0c752..6ae4d6fe390e 100644 --- a/apps/web/src/components/chat/ChatComposer.tsx +++ b/apps/web/src/components/chat/ChatComposer.tsx @@ -1626,6 +1626,7 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) const previewFile = composerFiles.find((file) => file.id === previewFileId); const composerContextActions = useMemo( () => ({ + environmentId, expandImage: (imageId: string) => { const preview = buildExpandedImagePreview(composerImages, imageId); if (preview) onExpandImage(preview); diff --git a/apps/web/src/components/chat/MessagesTimeline.tsx b/apps/web/src/components/chat/MessagesTimeline.tsx index 17e8fb1f56ca..8517ff6c5114 100644 --- a/apps/web/src/components/chat/MessagesTimeline.tsx +++ b/apps/web/src/components/chat/MessagesTimeline.tsx @@ -3508,12 +3508,13 @@ function UserMessagePullRequestContextChip(props: { copyMarkdown: string; toneClassName: string; }) { - const { openPullRequest } = use(TimelineRowCtx); + const { activeThreadEnvironmentId, openPullRequest } = use(TimelineRowCtx); const metadata = props.record.pullRequest; if (metadata === undefined) return null; return ( void; expandVideo: (fileId: string) => void; openFile: (fileId: string) => void; @@ -74,6 +76,7 @@ export interface ComposerContextActions { } export const ComposerContextActionsContext = createContext({ + environmentId: null, expandImage: () => {}, expandVideo: () => {}, openFile: () => {}, @@ -247,6 +250,7 @@ function PullRequestContextChip(props: { record: ReviewCommentContext; toneClass return ( , url: string) => void; }) { + const previewTarget = usePullRequestPreviewTarget(props.environmentId, props.metadata.url); + const button = ( + + ); + if (previewTarget !== null) { + return ( + } + /> + ); + } return ( - props.onOpen(event, props.metadata.url)} - > - - {props.label} - - } - /> + diff --git a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx index bbdbd9e16f99..1ed759eaa80e 100644 --- a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx +++ b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx @@ -1,6 +1,13 @@ import { isAtomCommandInterrupted } from "@t3tools/client-runtime/state/runtime"; import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; -import { cloneElement, useState, type ComponentPropsWithoutRef, type ReactElement } from "react"; +import { + cloneElement, + useState, + type ComponentPropsWithoutRef, + type MouseEvent, + type ReactElement, + type ReactNode, +} from "react"; import { formatRelativeTimeLabel } from "~/timestampFormat"; import { pullRequestEnvironment } from "~/state/pullRequests"; @@ -15,7 +22,9 @@ interface PullRequestLinkPreviewTarget { readonly input: PullRequestRef; } -type PullRequestLinkElement = ReactElement>; +type PullRequestLinkElement = ReactElement< + ComponentPropsWithoutRef<"a"> | ComponentPropsWithoutRef<"button"> +>; export function PullRequestLinkPreview({ link, @@ -24,13 +33,15 @@ export function PullRequestLinkPreview({ confirmBeforeOpen, onOpenPullRequest, onOpenFallback, + fallback, }: { link: PullRequestLinkElement; originalUrl: string; target: PullRequestLinkPreviewTarget; - confirmBeforeOpen: boolean; - onOpenPullRequest: (url: string) => boolean; - onOpenFallback: (url: string) => Promise; + confirmBeforeOpen?: boolean; + onOpenPullRequest?: (url: string) => boolean; + onOpenFallback?: (url: string) => Promise; + fallback?: ReactNode; }) { const [open, setOpen] = useState(false); const [resolvingClick, setResolvingClick] = useState(false); @@ -46,28 +57,29 @@ export function PullRequestLinkPreview({ reportFailure: false, reportDefect: false, }); - const trigger = confirmBeforeOpen - ? cloneElement(link, { - onClick: (event) => { - if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) return; - event.preventDefault(); - event.stopPropagation(); - if (resolvingClick) return; - setOpen(false); - setResolvingClick(true); - void readPreview(target) - .then(async (result) => { - if (isAtomCommandInterrupted(result)) return; - if (result._tag === "Success" && onOpenPullRequest(result.value.url)) return; - await onOpenFallback(originalUrl); - }) - .catch((error: unknown) => { - console.error("[pull-request-link-preview] failed to open link", error); - }) - .finally(() => setResolvingClick(false)); - }, - }) - : link; + const trigger = + confirmBeforeOpen === true + ? cloneElement(link, { + onClick: (event: MouseEvent) => { + if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) return; + event.preventDefault(); + event.stopPropagation(); + if (resolvingClick) return; + setOpen(false); + setResolvingClick(true); + void readPreview(target) + .then(async (result) => { + if (isAtomCommandInterrupted(result)) return; + if (result._tag === "Success" && onOpenPullRequest?.(result.value.url)) return; + await onOpenFallback?.(originalUrl); + }) + .catch((error: unknown) => { + console.error("[pull-request-link-preview] failed to open link", error); + }) + .finally(() => setResolvingClick(false)); + }, + }) + : link; const detail = detailQuery.data; const state = detail === null @@ -86,9 +98,11 @@ export function PullRequestLinkPreview({ {detail !== null || detailQuery.error !== null ? ( {detail === null ? ( -

- {originalUrl} -

+ (fallback ?? ( +

+ {originalUrl} +

+ )) ) : (
diff --git a/apps/web/src/lib/openPullRequestLink.ts b/apps/web/src/lib/openPullRequestLink.ts index b691aa58cce3..da67d0b043a4 100644 --- a/apps/web/src/lib/openPullRequestLink.ts +++ b/apps/web/src/lib/openPullRequestLink.ts @@ -1,6 +1,7 @@ -import type { EnvironmentId, ScopedThreadRef } from "@t3tools/contracts"; +import type { EnvironmentId, PullRequestRef, ScopedThreadRef } from "@t3tools/contracts"; +import { useAtomValue } from "@effect/atom-react"; import { useNavigate } from "@tanstack/react-router"; -import { type MouseEvent, useCallback } from "react"; +import { type MouseEvent, useCallback, useMemo } from "react"; import { pullRequestHostOf, type SourceControlProviderKind } from "@t3tools/contracts"; import { parseChangeRequestUrl, type ChangeRequestLink } from "@t3tools/shared/changeRequestUrl"; @@ -15,6 +16,7 @@ import { useRightPanelStore } from "../rightPanelStore"; import type { EnvironmentProject } from "@t3tools/client-runtime/state/shell"; import { useProjects, useServerConfigs } from "../state/entities"; +import { serverEnvironment } from "../state/server"; import { usePrimaryEnvironmentId } from "../state/environments"; export { @@ -93,6 +95,51 @@ export function findProjectForChangeRequest( }); } +export function resolvePullRequestPreviewTarget({ + environmentId, + projects, + pullRequestsEnabled, + url, +}: { + environmentId: EnvironmentId | null; + projects: ReadonlyArray; + pullRequestsEnabled: boolean; + url: string; +}): { environmentId: EnvironmentId; input: PullRequestRef } | null { + if (!pullRequestsEnabled || environmentId === null) return null; + const parsed = parseChangeRequestUrl(url); + if (parsed === null) return null; + const project = findProjectForChangeRequest( + projects.filter((candidate) => candidate.environmentId === environmentId), + parsed, + ); + if (project === undefined) return null; + return { + environmentId, + input: { + projectId: project.id, + host: parsed.authority ?? parsed.host, + repository: sourceControlRepositorySelector(project.repositoryIdentity) ?? parsed.repository, + number: parsed.number, + }, + }; +} + +export function usePullRequestPreviewTarget(environmentId: EnvironmentId | null, url: string) { + const projects = useProjects(); + const serverConfig = useAtomValue(serverEnvironment.configValueAtom(environmentId)); + return useMemo( + () => + resolvePullRequestPreviewTarget({ + environmentId, + projects, + pullRequestsEnabled: serverConfig?.environment.capabilities.pullRequests === true, + url, + }), + [environmentId, projects, serverConfig, url], + ); +} + /** * Any project checked out from the link's host. Thread links are host-level, so a pull request * from a repository nobody has checked out is still linkable as long as one project on that From 584450a1f198c8592d28947a0a7a060d28013298 Mon Sep 17 00:00:00 2001 From: maria Date: Sun, 20 Sep 2026 20:12:43 -0300 Subject: [PATCH 14/24] fix(web): compact the worktree setup glass popover (#12802) --- apps/web/src/components/chat/MessagesTimeline.tsx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/web/src/components/chat/MessagesTimeline.tsx b/apps/web/src/components/chat/MessagesTimeline.tsx index 8517ff6c5114..161adc8be00d 100644 --- a/apps/web/src/components/chat/MessagesTimeline.tsx +++ b/apps/web/src/components/chat/MessagesTimeline.tsx @@ -2570,7 +2570,12 @@ function BackgroundWorktreeSetupChip({ snapshot }: { snapshot: WorktreeSetupSnap {scriptName} - + Date: Sun, 20 Sep 2026 20:32:35 -0300 Subject: [PATCH 15/24] fix(web): route keyboard submit through the primary worktree action (#12526) --- .../BranchToolbarBranchSelector.tsx | 53 ++++++++++++++----- .../components/PullRequestThreadDialog.tsx | 5 +- 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/apps/web/src/components/BranchToolbarBranchSelector.tsx b/apps/web/src/components/BranchToolbarBranchSelector.tsx index d958f2f76625..a7fa1ce99724 100644 --- a/apps/web/src/components/BranchToolbarBranchSelector.tsx +++ b/apps/web/src/components/BranchToolbarBranchSelector.tsx @@ -545,6 +545,7 @@ export function BranchToolbarBranchSelector({ setIsBranchMenuOpen(open); if (!open) { setBranchQuery(""); + highlightedBranchValueRef.current = null; } }, []); @@ -594,6 +595,9 @@ export function BranchToolbarBranchSelector({ }, [fetchNextBranchPage, hasNextPage, isBranchMenuOpen, isFetchingNextPage]); const branchListRef = useRef(null); + // Tracks the highlighted picker value so Enter can activate it even when the + // virtualized row is not mounted (Base UI Enter clicks the mounted element). + const highlightedBranchValueRef = useRef(null); const updateBranchListScrollFades = useCallback(() => { const scrollElement = branchListRef.current?.getScrollableNode?.(); if (!(scrollElement instanceof HTMLElement)) { @@ -676,6 +680,20 @@ export function BranchToolbarBranchSelector({ const prUrl = currentLinkedPr?.url ?? displayedPr?.url; const openPrLink = useOpenPrLink(threadRef); + function selectPickerItem(itemValue: string) { + highlightedBranchValueRef.current = null; + if (itemValue === checkoutPullRequestItemValue && prReference && onCheckoutPullRequestRequest) { + handleOpenChange(false); + onComposerFocusRequest?.(); + onCheckoutPullRequestRequest(prReference); + } else if (itemValue === createBranchItemValue) { + createRef(trimmedBranchQuery); + } else { + const refName = branchByName.get(itemValue); + if (refName) selectBranch(refName); + } + } + function renderPickerItem(itemValue: string, index: number) { if (checkoutPullRequestItemValue && itemValue === checkoutPullRequestItemValue) { return ( @@ -685,15 +703,7 @@ export function BranchToolbarBranchSelector({ index={index} value={itemValue} className="pe-2" - onClick={() => { - if (!prReference || !onCheckoutPullRequestRequest) { - return; - } - setIsBranchMenuOpen(false); - setBranchQuery(""); - onComposerFocusRequest?.(); - onCheckoutPullRequestRequest(prReference); - }} + onClick={() => selectPickerItem(itemValue)} >
@@ -715,7 +725,7 @@ export function BranchToolbarBranchSelector({ index={index} value={itemValue} className="pe-1.5" - onClick={() => createRef(trimmedBranchQuery)} + onClick={() => selectPickerItem(itemValue)} > Create new ref "{newRefName}" @@ -743,7 +753,7 @@ export function BranchToolbarBranchSelector({ index={index} value={itemValue} className="pe-1.5" - onClick={() => selectBranch(refName)} + onClick={() => selectPickerItem(itemValue)} onContextMenu={(event) => handleBranchContextMenu(event, itemValue)} >
@@ -760,7 +770,8 @@ export function BranchToolbarBranchSelector({ filteredItems={filteredBranchPickerItems} autoHighlight virtualized - onItemHighlighted={(_value, eventDetails) => { + onItemHighlighted={(value, eventDetails) => { + highlightedBranchValueRef.current = typeof value === "string" ? value : null; if (!isBranchMenuOpen || eventDetails.index < 0 || eventDetails.reason !== "keyboard") { return; } @@ -828,6 +839,24 @@ export function BranchToolbarBranchSelector({ placeholder="Search refs..." value={branchQuery} onChange={(event) => setBranchQuery(event.target.value)} + onKeyDown={(event) => { + if (event.key !== "Enter" || event.nativeEvent.isComposing || event.keyCode === 229) { + return; + } + const highlightedValue = highlightedBranchValueRef.current; + if ( + highlightedValue === null || + !filteredBranchPickerItems.includes(highlightedValue) + ) { + return; + } + ( + event as typeof event & { preventBaseUIHandler?: () => void } + ).preventBaseUIHandler?.(); + event.preventDefault(); + event.stopPropagation(); + selectPickerItem(highlightedValue); + }} />
No refs found. diff --git a/apps/web/src/components/PullRequestThreadDialog.tsx b/apps/web/src/components/PullRequestThreadDialog.tsx index 4004b4930c27..ddbd2876d4d2 100644 --- a/apps/web/src/components/PullRequestThreadDialog.tsx +++ b/apps/web/src/components/PullRequestThreadDialog.tsx @@ -222,9 +222,12 @@ export function PullRequestThreadDialog({ if (event.key !== "Enter") { return; } + if (event.nativeEvent.isComposing || event.keyCode === 229) { + return; + } event.preventDefault(); if (!isResolving && !preparePullRequestThreadAction.isPending) { - void handleConfirm("local"); + void handleConfirm("worktree"); } }} /> From c789cd174ae50f50ecb0a34b96a81e00b0f5c823 Mon Sep 17 00:00:00 2001 From: maria Date: Sun, 20 Sep 2026 20:34:06 -0300 Subject: [PATCH 16/24] fix(web): skip image inline chip when composer is empty (#12528) --- apps/web/src/components/chat/ChatComposer.tsx | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/apps/web/src/components/chat/ChatComposer.tsx b/apps/web/src/components/chat/ChatComposer.tsx index 6ae4d6fe390e..3fe6e740fb9f 100644 --- a/apps/web/src/components/chat/ChatComposer.tsx +++ b/apps/web/src/components/chat/ChatComposer.tsx @@ -206,6 +206,7 @@ import { import { useOpenPrLink } from "~/lib/openPullRequestLink"; import { collectInlineContextIds, + stripInlineContextReferences, type ComposerContextReference, ensureInlineContextReferences, formatInlineContextReference, @@ -5241,6 +5242,7 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) options?: { readonly source?: ChatFileAttachment["source"]; readonly selection?: { start: number; end: number }; + readonly skipImageInlineChip?: boolean; }, ): Promise => { if (!activeThreadId || files.length === 0 || isRevertingCheckpointRef.current) return false; @@ -5260,6 +5262,20 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) // large image is being compressed, and the attachments and errors belong // to the thread the paste happened in. const threadId = activeThreadId; + // Images landing with no prose live on the shelf with no chip. Read before + // the awaits below: compression is async and the prompt may change while it + // runs. An explicit selection replace and states where the editor refuses + // input (connecting, approval, pending questions, project selection) still + // get chips so the image is never invisible, unless paste-as-text explicitly + // requests no inline image chip. + const imageAttachmentsGetChips = + !options?.skipImageInlineChip && + (options?.selection !== undefined || + isConnecting || + isComposerApprovalState || + pendingUserInputs.length > 0 || + projectSelectionRequired || + stripInlineContextReferences(promptRef.current).trim().length > 0); // Validation happens synchronously so concurrent pastes see each other: // accepted files reserve their attachment slots (via the pending counter) @@ -5416,7 +5432,7 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) : [], ); const storedImages = nextImages.filter((image) => storedImageIds.has(image.id)); - if (storedImages.length > 0) { + if (storedImages.length > 0 && imageAttachmentsGetChips) { insertedAny = insertAttachmentReferences(storedImages.map(imageContextReference)) || insertedAny; } @@ -5444,6 +5460,8 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) /** * Chips for freshly attached files land at the caret; when the editor cannot take * input (approval, pending questions) they are appended so the file is never invisible. + * Images skip this when they land with no prose and the editor takes input: + * the shelf thumbnail is enough. */ const insertAttachmentReferences = ( references: ReadonlyArray, @@ -5582,7 +5600,7 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) ) { event.preventDefault(); event.stopPropagation(); - void addComposerAttachments(files); + void addComposerAttachments(files, { skipImageInlineChip: bypassAutoAttachment }); return; } From f4d979a6c6139ab10aa1013335bd94a313a5b56e Mon Sep 17 00:00:00 2001 From: "t3-code[bot]" <269035359+t3-code[bot]@users.noreply.github.com> Date: Sun, 20 Sep 2026 20:35:02 -0300 Subject: [PATCH 17/24] fix(web): only show notice details when text is clipped (#12760) Co-authored-by: t3-code[bot] <269035359+t3-code[bot]@users.noreply.github.com> Co-authored-by: Exotic <118054752+extoci@users.noreply.github.com> --- .../chat/ComposerBannerStack.test.tsx | 103 +++++++++++++++ .../components/chat/ComposerBannerStack.tsx | 117 ++++++++++++------ 2 files changed, 182 insertions(+), 38 deletions(-) create mode 100644 apps/web/src/components/chat/ComposerBannerStack.test.tsx diff --git a/apps/web/src/components/chat/ComposerBannerStack.test.tsx b/apps/web/src/components/chat/ComposerBannerStack.test.tsx new file mode 100644 index 000000000000..d2e9a649df10 --- /dev/null +++ b/apps/web/src/components/chat/ComposerBannerStack.test.tsx @@ -0,0 +1,103 @@ +import { cloneElement, type ReactElement, type ReactNode } from "react"; +import { act, create, type ReactTestRenderer } from "react-test-renderer"; +import { afterEach, expect, it, vi } from "vite-plus/test"; + +import { ComposerBannerStack } from "./ComposerBannerStack"; + +vi.mock("../ui/popover", () => ({ + Popover: "popover", + PopoverTrigger: ({ render, children }: { render: ReactElement; children: ReactNode }) => + cloneElement(render, {}, children), + PopoverPopup: "popup", +})); +vi.mock("../ui/button", () => ({ Button: "button" })); +vi.mock("../ui/scroll-area", () => ({ ScrollArea: "div" })); + +let renderer: ReactTestRenderer; +afterEach(async () => { + if (renderer) await act(() => renderer.unmount()); + vi.unstubAllGlobals(); +}); + +it("only offers notice details when the description cannot fit", async () => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + let resize = () => {}; + let mutate = () => {}; + vi.stubGlobal( + "MutationObserver", + class { + constructor(callback: () => void) { + mutate = callback; + } + observe() {} + disconnect() {} + }, + ); + vi.stubGlobal( + "ResizeObserver", + class { + constructor(callback: () => void) { + resize = callback; + } + observe() {} + disconnect() {} + }, + ); + let position = "static"; + vi.stubGlobal("getComputedStyle", () => ({ position })); + let availableWidth = 200; + const nested = { clientWidth: 100, scrollWidth: 80 }; + const text = { + querySelectorAll: () => [nested], + get clientWidth() { + return ( + availableWidth - + (renderer?.root.findAllByProps({ "aria-label": "Show notice details" }).length ? 28 : 0) + ); + }, + scrollWidth: 80, + }; + await act(() => { + renderer = create( + , + { + createNodeMock: (element) => + element.type === "span" ? text : element.type === "button" ? { offsetWidth: 24 } : null, + }, + ); + }); + const details = () => renderer.root.findAllByProps({ "aria-label": "Show notice details" }); + expect(details()).toHaveLength(0); + text.scrollWidth = 300; + await act(() => resize()); + expect(details()).toHaveLength(1); + // It fits without the icon: the icon must not keep its own overflow alive. + availableWidth = 308; + await act(() => resize()); + expect(details()).toHaveLength(0); + text.scrollWidth = 80; + await act(() => resize()); + expect(details()).toHaveLength(0); + nested.scrollWidth = 500; + await act(() => mutate()); + expect(details()).toHaveLength(1); + nested.scrollWidth = 80; + await act(() => mutate()); + expect(details()).toHaveLength(0); + position = "absolute"; + await act(() => resize()); + expect(details()).toHaveLength(1); + position = "static"; + await act(() => resize()); + expect(details()).toHaveLength(0); +}); diff --git a/apps/web/src/components/chat/ComposerBannerStack.tsx b/apps/web/src/components/chat/ComposerBannerStack.tsx index b47790b96039..ec4b44259321 100644 --- a/apps/web/src/components/chat/ComposerBannerStack.tsx +++ b/apps/web/src/components/chat/ComposerBannerStack.tsx @@ -242,6 +242,82 @@ export function ComposerBannerStack({ className, items }: ComposerBannerStackPro ); } +/** Keep full descriptions reachable only when their inline copy is clipped. */ +function NoticeDescription({ children, compact }: { children: ReactNode; compact?: boolean }) { + const descriptionRef = useRef(null); + const detailsRef = useRef(null); + const [showDetails, setShowDetails] = useState(false); + + useLayoutEffect(() => { + const description = descriptionRef.current; + if (!description) return; + const measure = () => { + // Ignore the space taken by the details button itself so it cannot + // sustain its own overflow after the description would otherwise fit. + const recoveredWidth = detailsRef.current ? detailsRef.current.offsetWidth + 4 : 0; + const hidden = getComputedStyle(description).position === "absolute"; + setShowDetails( + hidden || + [description, ...description.querySelectorAll("*")].some( + (element) => element.scrollWidth > element.clientWidth + recoveredWidth, + ), + ); + }; + measure(); + const observer = new ResizeObserver(measure); + observer.observe(description); + // A child can reveal new text without resizing its clipped box. + const mutations = new MutationObserver(measure); + mutations.observe(description, { childList: true, subtree: true, characterData: true }); + return () => { + observer.disconnect(); + mutations.disconnect(); + }; + }, []); + + return ( + + + {children} + + {showDetails ? ( + + + } + > + + + + + {children} + + + + ) : null} + + ); +} + function ComposerBannerStackAlert({ item, attached, @@ -278,44 +354,9 @@ function ComposerBannerStackAlert({ {item.title} {item.description ? ( - - - {item.description} - - - - } - > - - - - - {item.description} - - - - + + {item.description} + ) : null} {item.actions || item.onDismiss ? ( From 9f73ca3677faa975be00f0ac55460279671df726 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sun, 20 Sep 2026 16:44:32 -0700 Subject: [PATCH 18/24] fix(devices): recover simulator streams after failures (#12639) --- .../devices/DevicePreviewRouteScreen.tsx | 4 +- .../features/devices/DeviceStreamWebView.tsx | 153 +++- .../devices/device-stream-document.test.ts | 46 +- .../devices/device-stream-document.ts | 18 +- .../devices/device-stream.browser.test.ts | 117 +++ .../features/devices/device-stream.browser.ts | 54 +- apps/server/src/device/DeviceService.test.ts | 91 +++ apps/server/src/device/DeviceService.ts | 10 +- .../components/device/DeviceLoadingView.tsx | 3 + .../device/DeviceStreamView.test.tsx | 83 +- .../components/device/DeviceStreamView.tsx | 28 +- .../components/device/deviceStream.test.ts | 345 -------- .../client-runtime/src/device/stream.test.ts | 735 ++++++++++++++++++ packages/client-runtime/src/device/stream.ts | 273 +++++-- 14 files changed, 1457 insertions(+), 503 deletions(-) create mode 100644 apps/mobile/src/features/devices/device-stream.browser.test.ts delete mode 100644 apps/web/src/components/device/deviceStream.test.ts create mode 100644 packages/client-runtime/src/device/stream.test.ts diff --git a/apps/mobile/src/features/devices/DevicePreviewRouteScreen.tsx b/apps/mobile/src/features/devices/DevicePreviewRouteScreen.tsx index 82b2b90fecad..824164dc2e2b 100644 --- a/apps/mobile/src/features/devices/DevicePreviewRouteScreen.tsx +++ b/apps/mobile/src/features/devices/DevicePreviewRouteScreen.tsx @@ -68,7 +68,7 @@ function DevicePreviewScreen({ const insets = useSafeAreaInsets(); const { themeVariables } = useAppearancePreferences(); const focused = useIsFocused(); - const [foreground, setForeground] = useState(AppState.currentState === "active"); + const [foreground, setForeground] = useState(AppState.currentState !== "background"); const [selectedKey, setSelectedKey] = useState(null); const [inputConnected, setInputConnected] = useState(false); const [streamAttempt, setStreamAttempt] = useState(0); @@ -87,7 +87,7 @@ function DevicePreviewScreen({ ); useEffect(() => { const subscription = AppState.addEventListener("change", (state) => - setForeground(state === "active"), + setForeground(state !== "background"), ); return () => subscription.remove(); }, []); diff --git a/apps/mobile/src/features/devices/DeviceStreamWebView.tsx b/apps/mobile/src/features/devices/DeviceStreamWebView.tsx index 9a5cef83ae3f..423fdb1c84a0 100644 --- a/apps/mobile/src/features/devices/DeviceStreamWebView.tsx +++ b/apps/mobile/src/features/devices/DeviceStreamWebView.tsx @@ -1,7 +1,19 @@ import deviceStreamScript from "@t3tools/mobile-device-stream"; -import { useImperativeHandle, useLayoutEffect, useMemo, useRef, useState, type Ref } from "react"; -import { Platform } from "react-native"; +import { + useEffect, + useEffectEvent, + useImperativeHandle, + useLayoutEffect, + useMemo, + useRef, + useState, + type Ref, +} from "react"; +import { ActivityIndicator, Platform, Pressable, View } from "react-native"; import { WebView } from "react-native-webview"; +import type { DeviceStreamStatus } from "@t3tools/client-runtime/device/stream"; + +import { AppText } from "../../components/AppText"; import { deviceStreamDocument, @@ -27,6 +39,7 @@ export function DeviceStreamWebView({ ...props }: DeviceStreamConfiguration & NativeStreamBridge) { const [attempt, setAttempt] = useState(0); + const processRetried = useRef(false); const configuration = JSON.stringify({ access: props.access, platform: props.platform, @@ -41,7 +54,21 @@ export function DeviceStreamWebView({ background={props.colors.background} onUnauthorized={props.onUnauthorized} onInputConnected={props.onInputConnected} - onRetry={() => setAttempt((attempt) => attempt + 1)} + onRetry={() => { + processRetried.current = false; + setAttempt((attempt) => attempt + 1); + void props.onUnauthorized(); + }} + onStreaming={() => { + processRetried.current = false; + }} + onRecoverProcess={() => { + if (processRetried.current) return false; + processRetried.current = true; + setAttempt((attempt) => attempt + 1); + void props.onUnauthorized(); + return true; + }} /> ); } @@ -53,12 +80,38 @@ function DeviceStreamDocumentView({ onUnauthorized, onInputConnected, onRetry, + onStreaming, + onRecoverProcess, }: NativeStreamBridge & { readonly configuration: string; readonly background: string; readonly onRetry: () => void; + readonly onStreaming: () => void; + readonly onRecoverProcess: () => boolean; }) { const webView = useRef(null); + const active = useRef(true); + const failed = useRef(false); + const [status, setStatus] = useState("connecting"); + const [error, setError] = useState(null); + const [started, setStarted] = useState(false); + const fail = (message: string) => { + if (!active.current || failed.current) return; + failed.current = true; + void onInputConnected(false); + webView.current?.injectJavaScript("window.T3DeviceStream?.stop(); true;"); + setError(message); + setStatus("error"); + }; + // The shared transport owns video timeouts once the document acknowledges startup. + const bootstrapTimedOut = useEffectEvent(() => + fail("Device viewer could not start. Reconnect to try again."), + ); + useEffect(() => { + if (started) return; + const timer = setTimeout(bootstrapTimedOut, 15_000); + return () => clearTimeout(timer); + }, [started]); const source = useMemo( () => ({ html: deviceStreamDocument(configuration, deviceStreamScript), @@ -78,32 +131,80 @@ function DeviceStreamDocumentView({ appSwitcher: () => command("appSwitcher"), rotate: () => command("rotate"), })); + const resetInput = useEffectEvent(() => void onInputConnected(false)); useLayoutEffect(() => { + active.current = true; + resetInput(); const view = webView.current; - return () => view?.injectJavaScript("window.T3DeviceStream?.stop(); true;"); + return () => { + active.current = false; + view?.injectJavaScript("window.T3DeviceStream?.stop(); true;"); + }; }, []); + const processTerminated = () => { + if (!active.current || failed.current) return; + void onInputConnected(false); + if (!onRecoverProcess()) fail("Device viewer stopped. Reconnect to try again."); + }; return ( - void onInputConnected(false)} - onShouldStartLoadWithRequest={(request) => - request.url === "about:blank" || request.url === source.baseUrl - } - onMessage={(event) => { - const message = deviceStreamMessage(event.nativeEvent.data); - if (message?.type === "unauthorized") void onUnauthorized(); - else if (message?.type === "input") void onInputConnected(message.connected); - else if (message?.type === "retry") onRetry(); - }} - /> + + fail("Device viewer could not load. Reconnect to try again.")} + onHttpError={() => fail("Device viewer could not load. Reconnect to try again.")} + onContentProcessDidTerminate={processTerminated} + onRenderProcessGone={processTerminated} + onShouldStartLoadWithRequest={(request) => + request.url === "about:blank" || request.url === source.baseUrl + } + onMessage={(event) => { + if (!active.current || failed.current) return; + const message = deviceStreamMessage(event.nativeEvent.data); + if (message?.type === "unauthorized") void onUnauthorized(); + else if (message?.type === "input") void onInputConnected(message.connected); + else if (message?.type === "retry") onRetry(); + else if (message?.type === "status") { + setStarted(true); + if (message.status === "error") fail(message.detail ?? "Device stream failed."); + else { + setStatus(message.status); + if (message.status === "streaming") onStreaming(); + } + } + }} + /> + {status !== "streaming" ? ( + + {status === "connecting" ? : null} + + {status === "error" ? error : "Connecting to device..."} + + {status === "error" ? ( + + Reconnect + + ) : null} + + ) : null} + ); } diff --git a/apps/mobile/src/features/devices/device-stream-document.test.ts b/apps/mobile/src/features/devices/device-stream-document.test.ts index ffc08667a147..95aa6a9dad23 100644 --- a/apps/mobile/src/features/devices/device-stream-document.test.ts +++ b/apps/mobile/src/features/devices/device-stream-document.test.ts @@ -1,5 +1,5 @@ import * as NodeVM from "node:vm"; -import { describe, expect, it } from "vite-plus/test"; +import { describe, expect, it, vi } from "vite-plus/test"; import { deviceStreamDocument, deviceStreamMessage } from "./device-stream-document"; @@ -13,7 +13,7 @@ describe("native device stream document", () => { ); const script = html.match(/`; + const failure = `window.ReactNativeWebView.postMessage(JSON.stringify({type:"status",status:"error",detail:"Device viewer stopped unexpectedly."}));`; + return ``; } export function deviceStreamMessage(data: string) { @@ -36,6 +37,21 @@ export function deviceStreamMessage(data: string) { ) { return { type: message.type, connected: message.connected } as const; } + if ( + message.type === "status" && + "status" in message && + (message.status === "connecting" || + message.status === "streaming" || + message.status === "error") && + (!("detail" in message) || typeof message.detail === "string") + ) { + return { + type: message.type, + status: message.status, + detail: + "detail" in message && typeof message.detail === "string" ? message.detail : undefined, + } as const; + } } catch { // Ignore messages that are not part of the stream bridge. } diff --git a/apps/mobile/src/features/devices/device-stream.browser.test.ts b/apps/mobile/src/features/devices/device-stream.browser.test.ts new file mode 100644 index 000000000000..5aeefdac0203 --- /dev/null +++ b/apps/mobile/src/features/devices/device-stream.browser.test.ts @@ -0,0 +1,117 @@ +import { afterEach, expect, it, vi } from "vite-plus/test"; +import { start, stop } from "./device-stream.browser"; + +class Element extends EventTarget { + readonly style = {}; + naturalWidth = 0; + naturalHeight = 0; + src = ""; + readonly tag: string; + constructor(tag: string) { + super(); + this.tag = tag; + } + setAttribute() {} + removeAttribute(name: string) { + if (name === "src") this.src = ""; + } + append() {} +} + +async function setup() { + vi.useFakeTimers(); + const elements: Element[] = []; + vi.stubGlobal("document", { + documentElement: { style: {} }, + body: { style: {}, replaceChildren() {} }, + createElement: (tag: string) => { + const element = new Element(tag); + elements.push(element); + return element; + }, + }); + const postMessage = vi.fn(); + vi.stubGlobal("window", { ReactNativeWebView: { postMessage }, addEventListener() {} }); + vi.stubGlobal("fetch", () => Promise.resolve(new Response("prime"))); + const sockets: Socket[] = []; + class Socket { + static OPEN = 1; + readyState = 1; + onopen: (() => void) | null = null; + close = vi.fn(); + send = vi.fn(); + constructor() { + sockets.push(this); + } + } + vi.stubGlobal("WebSocket", Socket); + const configuration = { + platform: "ios" as const, + deviceId: "fixture-device", + access: { + httpBase: "https://device.test", + wsBase: "wss://device.test", + credentials: false, + query: {}, + }, + colors: { + background: "white", + foreground: "black", + muted: "gray", + buttonBackground: "gray", + buttonForeground: "black", + buttonBorder: "gray", + }, + }; + start(configuration); + await vi.advanceTimersByTimeAsync(0); + return { + configuration, + elements, + sockets, + messages: () => + postMessage.mock.calls.map( + ([message]) => JSON.parse(message as string) as { type: string; status?: string }, + ), + }; +} + +afterEach(() => { + stop(); + vi.useRealTimers(); + vi.unstubAllGlobals(); +}); + +it("bridges shared first-frame readiness, image failure, and a successful fresh attempt to native", async () => { + const { elements, messages, sockets, configuration } = await setup(); + sockets[0]!.onopen?.(); + expect(messages()).not.toContainEqual({ type: "status", status: "streaming" }); + expect(messages()).toContainEqual({ type: "input", connected: true }); + const image = elements.find((element) => element.tag === "img")!; + image.naturalWidth = 400; + image.naturalHeight = 800; + await vi.advanceTimersByTimeAsync(250); + expect(messages()).toContainEqual({ type: "status", status: "streaming" }); + image.dispatchEvent(new Event("error")); + expect(messages()).toContainEqual({ + type: "status", + status: "error", + detail: "Could not receive the device stream. Reconnect to try again.", + }); + expect(messages()).toContainEqual({ type: "input", connected: false }); + expect(messages()).not.toContainEqual({ type: "unauthorized" }); + expect(image.src).toBe(""); + expect(sockets[0]!.close).toHaveBeenCalledOnce(); + start(configuration); + await vi.advanceTimersByTimeAsync(0); + const replacement = elements.findLast((element) => element.tag === "img")!; + replacement.naturalWidth = 400; + replacement.naturalHeight = 800; + replacement.dispatchEvent(new Event("load")); + expect(messages().at(-1)).toEqual({ type: "status", status: "streaming" }); + image.dispatchEvent(new Event("error")); + expect(sockets[1]!.close).not.toHaveBeenCalled(); + stop(); + expect(replacement.src).toBe(""); + expect(vi.getTimerCount()).toBe(0); +}); diff --git a/apps/mobile/src/features/devices/device-stream.browser.ts b/apps/mobile/src/features/devices/device-stream.browser.ts index efa81d9a043a..0b9e98a51b9f 100644 --- a/apps/mobile/src/features/devices/device-stream.browser.ts +++ b/apps/mobile/src/features/devices/device-stream.browser.ts @@ -12,13 +12,10 @@ declare global { } let activeClient: ReturnType | null = null; -let activeImage: HTMLImageElement | null = null; export function stop() { activeClient?.stop(); activeClient = null; - activeImage?.removeAttribute("src"); - activeImage = null; } export function command(button: "home" | "back" | "appSwitcher" | "rotate") { @@ -70,34 +67,6 @@ export function start(configuration: DeviceStreamConfiguration) { image.alt = ""; image.draggable = false; image.style.display = "none"; - const overlay = document.createElement("div"); - overlay.setAttribute("role", "status"); - Object.assign(overlay.style, { - position: "fixed", - inset: "0", - display: "flex", - flexDirection: "column", - alignItems: "center", - justifyContent: "center", - gap: "16px", - padding: "24px", - textAlign: "center", - background: colors.background, - }); - const detail = document.createElement("span"); - const retry = document.createElement("button"); - retry.textContent = "Retry"; - Object.assign(retry.style, { - padding: "12px 24px", - borderRadius: "20px", - border: `1px solid ${colors.buttonBorder}`, - background: colors.buttonBackground, - color: colors.buttonForeground, - font: "inherit", - display: "none", - }); - retry.addEventListener("click", () => post({ type: "retry" })); - overlay.append(detail, retry); const inputStatus = document.createElement("div"); inputStatus.setAttribute("role", "status"); inputStatus.textContent = "Reconnecting device controls..."; @@ -114,11 +83,17 @@ export function start(configuration: DeviceStreamConfiguration) { }); frame.append(canvas, image); container.append(frame); - document.body.replaceChildren(container, overlay, inputStatus); + document.body.replaceChildren(container, inputStatus); let pointerId: number | null = null; let inputConnected = false; let streaming = false; + const reportStatus = (status: "connecting" | "streaming" | "error", detail?: string) => { + if (activeClient !== client) return; + streaming = status === "streaming"; + inputStatus.style.display = streaming && !inputConnected ? "block" : "none"; + post({ type: "status", status, detail }); + }; const layout = (screen: DeviceScreenSize | null) => { const landscape = screen?.orientation === "landscape_left" || screen?.orientation === "landscape_right"; @@ -160,19 +135,11 @@ export function start(configuration: DeviceStreamConfiguration) { { ...configuration, preferMjpeg: platform === "ios" }, canvas, { - onStatus: (status, message) => { - streaming = status === "streaming"; - overlay.style.display = streaming ? "none" : "flex"; - inputStatus.style.display = streaming && !inputConnected ? "block" : "none"; - detail.textContent = - status === "error" ? (message ?? "Device stream failed.") : "Connecting to device..."; - retry.style.display = status === "error" ? "block" : "none"; - }, + onStatus: reportStatus, onScreen: layout, - onMjpegFallback: (url) => { + onMjpegFallback: () => { canvas.style.display = "none"; image.style.display = "block"; - image.src = url; }, onUnauthorized: unauthorized, onInputConnected: (connected) => { @@ -183,8 +150,7 @@ export function start(configuration: DeviceStreamConfiguration) { }, ); activeClient = client; - activeImage = image; - image.addEventListener("error", unauthorized); + client.setMjpegImage(image); const touch = (event: PointerEvent, phase: "begin" | "move" | "end") => { const rect = frame.getBoundingClientRect(); client.sendTouch( diff --git a/apps/server/src/device/DeviceService.test.ts b/apps/server/src/device/DeviceService.test.ts index f1ed9a7f0253..c70f30b1fe87 100644 --- a/apps/server/src/device/DeviceService.test.ts +++ b/apps/server/src/device/DeviceService.test.ts @@ -11,6 +11,7 @@ import * as Deferred from "effect/Deferred"; import * as Fiber from "effect/Fiber"; import * as PubSub from "effect/PubSub"; import * as Ref from "effect/Ref"; +import * as Schema from "effect/Schema"; import * as Stream from "effect/Stream"; import { HttpClient, HttpClientResponse } from "effect/unstable/http"; import { ServerSettingsService } from "../serverSettings.ts"; @@ -19,6 +20,8 @@ import { NodeRuntimeUnavailableError } from "@t3tools/shared/nodeRuntime"; import { type DeviceService, makeWithHosts, stateStream } from "./DeviceService.ts"; +const decodeJson = Schema.decodeUnknownSync(Schema.fromJsonString(Schema.Unknown)); + const baseState: DeviceServiceState = { hosts: [], hostStatus: "idle", @@ -378,3 +381,91 @@ it.effect("keeps shutdown successful when subsequent discovery fails", () => expect(state.devices.find((device) => device.id === session.deviceId)?.booted).toBe(false); }).pipe(Effect.scoped), ); + +it.effect.each(["shutdown", "close"] as const)( + "%s releases iOS capture so reopening uses a fresh session", + (operation) => + Effect.gen(function* () { + const deviceId = DeviceId.make("11111111-1111-1111-1111-111111111111"); + const threadId = ThreadId.make("capture-recovery"); + let booted = true; + let capture: number | null = null; + let generation = 0; + const ready: DeviceHost.DeviceHostReady = { + nodePath: process.execPath, + hub: { origin: "http://device.test" }, + helpers: { serveSimAxSettings: null, serveSimCli: null }, + run: () => Effect.succeed({ code: 0, stdout: "", stderr: "" }), + }; + const host: DeviceHost.DeviceHost["Service"] = { + id: LOCAL_DEVICE_HOST_ID, + summary: Effect.succeed({ + id: LOCAL_DEVICE_HOST_ID, + kind: "local", + label: "Simulator host", + platforms: [{ platform: "ios", available: true }], + hubInstalled: true, + agentDeviceInstalled: false, + }), + platformAvailability: (platform) => Effect.succeed({ platform, available: true }), + ensureReady: () => Effect.succeed(ready), + ensureAgentReady: () => Effect.die("Agent access is not used in this test"), + current: Effect.succeed(ready), + stopAgent: Effect.void, + stop: Effect.void, + }; + const http = HttpClient.make((request) => + Effect.sync(() => { + const path = new URL(request.url).pathname; + if (path === "/api/devices") { + return HttpClientResponse.fromWeb( + request, + Response.json({ + emulators: [], + simulators: [ + { + id: deviceId, + name: "iPhone", + platform: "ios", + version: "26", + physical: false, + booted, + }, + ], + }), + ); + } + if (path === "/vendor/serve-sim/grid/api/start") capture ??= ++generation; + else if (path === "/vendor/serve-sim/grid/api/shutdown") { + if (request.body._tag !== "Uint8Array") throw new Error("Missing shutdown body"); + expect(decodeJson(new TextDecoder().decode(request.body.body))).toEqual({ + udid: deviceId, + }); + capture = null; + booted = false; + } else if (path === "/api/devices/shutdown") { + // This route powers off without releasing serve-sim's cached capture. + booted = false; + } else if (path === "/api/devices/boot") booted = true; + else throw new Error(`Unexpected hub path: ${path}`); + return HttpClientResponse.fromWeb(request, Response.json({ ok: true, id: deviceId })); + }), + ); + const service = yield* makeWithHosts(new Map([[host.id, host]])).pipe( + Effect.provideService(HttpClient.HttpClient, http), + ); + const input = { threadId, deviceId, platform: "ios" as const }; + yield* service.open(input); + expect(capture).toBe(1); + if (operation === "shutdown") yield* service.shutdown(input); + else yield* service.close({ threadId, deviceId, shutdown: true }); + expect(capture).toBeNull(); + expect((yield* service.state).sessions).toEqual([]); + yield* service.open(input); + expect(capture).toBe(2); + expect((yield* service.state).sessions).toHaveLength(1); + }).pipe( + Effect.provide(ServerSettingsService.layerTest({ enableDeviceSupport: true })), + Effect.scoped, + ), +); diff --git a/apps/server/src/device/DeviceService.ts b/apps/server/src/device/DeviceService.ts index 30e1f18c0497..8b7db9b5e590 100644 --- a/apps/server/src/device/DeviceService.ts +++ b/apps/server/src/device/DeviceService.ts @@ -670,8 +670,14 @@ export const makeWithHosts = Effect.fn("DeviceService.makeWithHosts")(function* platform: DevicePlatform, ) { const ready = yield* readiness(hostId); - yield* HttpClientRequest.post(`${ready.hub.origin}/api/devices/shutdown`).pipe( - HttpClientRequest.bodyJson({ platform, id: deviceId }), + // serve-sim's shutdown closes its in-process capture session before powering off. + // The hub's generic shutdown can leave that session cached across a reboot. + const path = + platform === "ios" ? `${vendorPrefix("ios")}/grid/api/shutdown` : "/api/devices/shutdown"; + yield* HttpClientRequest.post(`${ready.hub.origin}${path}`).pipe( + HttpClientRequest.bodyJson( + platform === "ios" ? { udid: deviceId } : { platform, id: deviceId }, + ), Effect.mapError( (cause) => new DeviceOperationError({ operation: "shutdown", reason: "invalid_payload", cause }), diff --git a/apps/web/src/components/device/DeviceLoadingView.tsx b/apps/web/src/components/device/DeviceLoadingView.tsx index 70e848c3ce7e..332d3994cca1 100644 --- a/apps/web/src/components/device/DeviceLoadingView.tsx +++ b/apps/web/src/components/device/DeviceLoadingView.tsx @@ -1,3 +1,4 @@ +import type { ReactNode } from "react"; import { Smartphone } from "lucide-react"; import { Spinner } from "~/components/ui/spinner"; @@ -8,6 +9,7 @@ export function DeviceLoadingView(props: { readonly stage: "opening" | "stream"; readonly message: string; readonly error?: boolean; + readonly children?: ReactNode; }) { return (
: null} {props.message}
+ {props.children} {!props.error ? (
({ refreshDeviceHubAccess: vi.fn(), })); const access = { httpBase: "http://test", wsBase: "ws://test", query: {}, credentials: true }; -vi.mock("@t3tools/client-runtime/device/stream", () => ({ - createDeviceStreamClient: ( - _target: unknown, - _canvas: unknown, - events: { onMjpegFallback: (url: string) => void }, - ) => ({ - start: () => events.onMjpegFallback("http://test/stream.mjpeg"), - stop: vi.fn(), - }), -})); import { DeviceStreamView } from "./DeviceStreamView"; + +class Image extends EventTarget { + src = ""; + naturalWidth = 0; + naturalHeight = 0; + removeAttribute(name: string) { + if (name === "src") this.src = ""; + } +} let renderer: ReactTestRenderer | undefined; afterEach(async () => { await act(async () => renderer?.unmount()); + renderer = undefined; + vi.useRealTimers(); vi.unstubAllGlobals(); }); -it("removes MJPEG requests while hidden and reconnects when shown", async () => { +async function setup() { + vi.useFakeTimers(); vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + vi.stubGlobal("fetch", () => Promise.resolve(new Response("prime"))); + vi.stubGlobal( + "WebSocket", + class { + static OPEN = 1; + readyState = 1; + send() {} + close() {} + }, + ); vi.stubGlobal( "ResizeObserver", class { @@ -34,6 +46,7 @@ it("removes MJPEG requests while hidden and reconnects when shown", async () => disconnect() {} }, ); + const images: Image[] = []; const view = (visible: boolean) => (
); } diff --git a/apps/web/src/components/pullRequest/pullRequestChecks.test.tsx b/apps/web/src/components/pullRequest/pullRequestChecks.test.tsx index a2b8e92d86fe..48a371a7a9c0 100644 --- a/apps/web/src/components/pullRequest/pullRequestChecks.test.tsx +++ b/apps/web/src/components/pullRequest/pullRequestChecks.test.tsx @@ -52,13 +52,20 @@ describe("pullRequestChecksState", () => { }); }); -/** Every element of the tree the row returned, so a nested indicator can be looked for. */ +/** + * Every element of the tree the row returned, so a nested indicator can be looked for. The row + * hands its slots to the shared row lines as props rather than children, so every prop that + * holds an element is walked too. + */ function flatten(node: ReactNode): ReadonlyArray> { const found: unknown[] = []; for (const child of Children.toArray(node)) { if (!isValidElement(child)) continue; found.push(child); - found.push(...flatten((child.props as { readonly children?: ReactNode }).children)); + for (const value of Object.values(child.props as Record)) { + if (isValidElement(value)) found.push(...flatten(value)); + else if (Array.isArray(value)) found.push(...flatten(value.filter(isValidElement))); + } } return found as ReadonlyArray>; } diff --git a/apps/web/src/components/pullRequest/pullRequestPresentation.tsx b/apps/web/src/components/pullRequest/pullRequestPresentation.tsx index 65caae761ced..d46cad36622c 100644 --- a/apps/web/src/components/pullRequest/pullRequestPresentation.tsx +++ b/apps/web/src/components/pullRequest/pullRequestPresentation.tsx @@ -4,7 +4,9 @@ import type { PullRequestCheck, PullRequestCheckStatus, PullRequestChecksState, + PullRequestLabel, PullRequestMergeability, + PullRequestReviewDecision, PullRequestState, } from "@t3tools/contracts"; import { @@ -13,14 +15,17 @@ import { CircleDotIcon, CircleXIcon, UserCheckIcon, + UserRoundIcon, + UserRoundXIcon, } from "lucide-react"; -import { Children, isValidElement, type ReactNode, useState } from "react"; +import { Children, type CSSProperties, isValidElement, type ReactNode, useState } from "react"; import { cn } from "~/lib/utils"; import { Badge } from "../ui/badge"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import type { PullRequestReviewOutcome } from "./pullRequestDetail.logic"; +import { pullRequestLabelColor } from "./pullRequestList.logic"; import { PULL_REQUEST_STATE_PRESENTATION, PullRequestGlyph, @@ -28,17 +33,87 @@ import { type PullRequestGlyphIcon, } from "./pullRequestIcons"; -export function PullRequestApprovalGlyph() { +/** + * A host label as a flat tinted tag in the label's own color: a wash of it behind, the name + * in a mix of it and the theme foreground. The mix leans to the foreground because hosts hand + * out any color at all: at 30% of the label on light and 45% on dark, white, black and + * GitHub's pale yellows all clear 4.5:1 on their wash, selected row included, and + * saturated colors sit well above. + * A label with no usable color falls back to the muted tag. Children ride after the name, + * for an overflow count. The height is pinned so a labeled row is as tall as one without. + */ +export function PullRequestLabelChip({ + label, + size = "sm", + className, + children, +}: { + label: Pick; + size?: "sm" | "default"; + className?: string; + children?: ReactNode; +}) { + const color = pullRequestLabelColor(label.color); + return ( + + {label.name} + {children} + + ); +} + +/** + * The review verdict as one glyph beside the checks glyph, so a row answers both "does it + * build" and "did someone say yes" in the same spot. "Awaiting review" is only drawn when the + * host reports it, which on GitHub means the branch rules require a review nobody has given. + */ +function reviewDecisionPresentation(decision: PullRequestReviewDecision) { + switch (decision) { + case "approved": + return { + Icon: UserCheckIcon, + label: "Approved", + toneClassName: CHECK_STATUS_PRESENTATION.success.toneClassName, + }; + case "changes-requested": + return { + Icon: UserRoundXIcon, + label: "Changes requested", + toneClassName: "text-amber-600/90 dark:text-amber-400/80", + }; + case "review-required": + return { + Icon: UserRoundIcon, + label: "Awaiting review", + toneClassName: "text-muted-foreground/60", + }; + } +} + +export function PullRequestReviewDecisionGlyph({ + decision, +}: { + decision: PullRequestReviewDecision; +}) { + const presentation = reviewDecisionPresentation(decision); return ( }> - - Approved + + {presentation.label} - Approved + {presentation.label} ); } diff --git a/apps/web/src/routes/_chat.pull-requests.tsx b/apps/web/src/routes/_chat.pull-requests.tsx index f54415b1986c..485fa7718e3e 100644 --- a/apps/web/src/routes/_chat.pull-requests.tsx +++ b/apps/web/src/routes/_chat.pull-requests.tsx @@ -24,11 +24,13 @@ import { LayersIcon, ListChecksIcon, PenLineIcon, + UsersIcon, Plug2Icon, Maximize2Icon, Minimize2Icon, SearchIcon, UserLockIcon, + type LucideIcon, } from "lucide-react"; import { useCallback, @@ -146,6 +148,7 @@ import { } from "../state/pullRequests"; import { useAtomCommand } from "../state/use-atom-command"; import { cn } from "~/lib/utils"; +import { Separator } from "~/components/ui/separator"; import { primaryServerKeybindingsAtom } from "~/state/server"; import { getSourceControlPresentationForKind } from "~/sourceControlPresentation"; import { PullRequestGlyph } from "~/components/pullRequest/pullRequestIcons"; @@ -188,6 +191,32 @@ export interface PullRequestsSearch extends PullRequestListPreferences { readonly selectedEnvironmentId?: EnvironmentId; } +/** + * A group reads like the sidebar's shelves: its glyph, its name, how many, then a rule out + * to the edge. The glyph is the one the involvement filter uses for the same idea. + */ +const GROUP_ICONS: Record = { + authored: PenLineIcon, + reviewRequested: EyeIcon, + others: UsersIcon, +}; + +function PullRequestGroupHeader({ + group, +}: { + group: { key: string; label: string; entries: ReadonlyArray }; +}) { + const Icon = GROUP_ICONS[group.key] ?? LayersIcon; + return ( +
+ +

{group.label}

+ {group.entries.length} + +
+ ); +} + // The state filters wear the same glyphs the rows do, so the two read as one vocabulary. const INVOLVEMENT_TABS = [ { value: "all", label: "All", Icon: LayersIcon }, @@ -1659,11 +1688,7 @@ function PullRequestsRouteView() {
{displayGroups.map((group) => (
- {group.label ? ( -

- {group.label} -

- ) : null} + {group.label ? : null} {group.entries.map((entry) => { const entryKey = pullRequestEntryKey(entry); return ( From 2efb8178d83f4f7c4ccc3ae1165f2a2ded0b2742 Mon Sep 17 00:00:00 2001 From: Simone Date: Mon, 21 Sep 2026 03:29:55 +0200 Subject: [PATCH 24/24] fix(clients): keep backslashes in copied Codex citations (#12243) Co-authored-by: Simone <185146821+Lucenx9@users.noreply.github.com> --- .../client-runtime/src/codexFileCitations.ts | 1 + .../src/codexMarkdownDirectives.test.ts | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/packages/client-runtime/src/codexFileCitations.ts b/packages/client-runtime/src/codexFileCitations.ts index b5042f0806e6..d29996501ea6 100644 --- a/packages/client-runtime/src/codexFileCitations.ts +++ b/packages/client-runtime/src/codexFileCitations.ts @@ -44,6 +44,7 @@ function markdownLabel(value: string): string { function markdownDestination(value: string): string { return value + .replaceAll("\\", "%5C") .replaceAll("<", "%3C") .replaceAll(">", "%3E") .replaceAll("\r", "%0D") diff --git a/packages/client-runtime/src/codexMarkdownDirectives.test.ts b/packages/client-runtime/src/codexMarkdownDirectives.test.ts index b2a22901eac2..efd5262d073c 100644 --- a/packages/client-runtime/src/codexMarkdownDirectives.test.ts +++ b/packages/client-runtime/src/codexMarkdownDirectives.test.ts @@ -8,6 +8,7 @@ import { renderCodexFileCitationsAsMarkdown, splitCodexArtifactTemplateMarkdown, } from "./codexMarkdownDirectives.js"; +import { parseMarkdownFileLink } from "./markdownLinks.js"; interface TestNode { readonly type: string; @@ -89,6 +90,24 @@ describe("remarkCodexDirectives", () => { }); }); +describe.each([ + { name: "renderCodexDirectivesForCopy", render: renderCodexDirectivesForCopy }, + { name: "renderCodexFileCitationsAsMarkdown", render: renderCodexFileCitationsAsMarkdown }, +])("$name file citation round trips", ({ render }) => { + it.each([ + "C:\\Users\\test\\[draft]\\report.md", + "\\\\server\\share\\report.md", + "outputs/report.md", + "/tmp/report%5C.md", + ])("preserves the literal path and line: %s", (path) => { + const markdown = render(`:codex-file-citation{path="${path}" line_range_start="7"}`); + const link = parseOrdinaryMarkdown(markdown).children?.[0]?.children?.[0]; + + expect(link?.type).toBe("link"); + expect(parseMarkdownFileLink(link?.url ?? "")).toEqual({ path, line: 7 }); + }); +}); + describe("native Markdown adapters", () => { it("uses the same parser to render file citations as portable links", () => { expect(renderCodexFileCitationsAsMarkdown(`Created ${FILE_CITATION}.`)).toBe(