diff --git a/apps/api/src/routes/internal/errors.http.ts b/apps/api/src/routes/internal/errors.http.ts index fabc5a2096..9c9b993f72 100644 --- a/apps/api/src/routes/internal/errors.http.ts +++ b/apps/api/src/routes/internal/errors.http.ts @@ -38,6 +38,7 @@ export const HttpErrorsLive = HttpApiBuilder.group(MapleInternalApi, "errors", ( { note: payload.note, snoozeUntil: payload.snoozeUntil, + expectedState: payload.expectedState, }, ) }).pipe(Effect.withSpan("HttpErrors.transitionIssue")), diff --git a/apps/api/src/routes/v2/error-issues.http.ts b/apps/api/src/routes/v2/error-issues.http.ts index 87683b73c1..24e642b353 100644 --- a/apps/api/src/routes/v2/error-issues.http.ts +++ b/apps/api/src/routes/v2/error-issues.http.ts @@ -243,6 +243,9 @@ export const HttpV2ErrorIssuesLive = HttpApiBuilder.group(MapleApiV2, "errorIssu ...(payload.snooze_until === undefined ? undefined : { snoozeUntil: payload.snooze_until }), + ...(payload.expected_state === undefined + ? undefined + : { expectedState: payload.expected_state }), }) return toV2Issue(issue) }).pipe( diff --git a/apps/api/src/routes/v2/phase1-resources.http.test.ts b/apps/api/src/routes/v2/phase1-resources.http.test.ts index 8ab0590407..4244393ac2 100644 --- a/apps/api/src/routes/v2/phase1-resources.http.test.ts +++ b/apps/api/src/routes/v2/phase1-resources.http.test.ts @@ -832,7 +832,11 @@ describe("v2 error_issues over HTTP", () => { const call = (method: string, action: string, body: unknown) => harness.request(method, `/v2/error_issues/${ISS_ID}/${action}`, { token: key.secret, body }) - const transitioned = await call("POST", "transitions", { to_state: "todo", note: "looking" }) + const transitioned = await call("POST", "transitions", { + to_state: "todo", + note: "looking", + expected_state: "triage", + }) expect(transitioned.status).toBe(200) expect(transitioned.body).toMatchObject({ id: ISS_ID, @@ -855,7 +859,11 @@ describe("v2 error_issues over HTTP", () => { // Every action ran as the caller's user actor, with the wire fields translated. expect(harness.issueActions()).toEqual([ - { action: "transition", actorId: actorFixture.id, opts: { note: "looking" } }, + { + action: "transition", + actorId: actorFixture.id, + opts: { note: "looking", expectedState: "triage" }, + }, { action: "claim", actorId: actorFixture.id, leaseDurationMs: 600_000 }, { action: "severity", actorId: actorFixture.id, opts: { source: "manual" } }, { action: "release", actorId: actorFixture.id, opts: { transitionTo: "in_review" } }, diff --git a/apps/ios/Packages/MapleAPI/Sources/MapleAPI/openapi.json b/apps/ios/Packages/MapleAPI/Sources/MapleAPI/openapi.json index 0c463c1474..a3579d9bd0 100644 --- a/apps/ios/Packages/MapleAPI/Sources/MapleAPI/openapi.json +++ b/apps/ios/Packages/MapleAPI/Sources/MapleAPI/openapi.json @@ -4618,6 +4618,9 @@ "ErrorIssueTransitionParams": { "additionalProperties": false, "properties": { + "expected_state": { + "$ref": "#/components/schemas/_maple_WorkflowState" + }, "note": { "type": "string" }, diff --git a/apps/web/src/page/errors/hub/command.ts b/apps/web/src/page/errors/hub/command.ts index b871fe9399..d8d373537b 100644 --- a/apps/web/src/page/errors/hub/command.ts +++ b/apps/web/src/page/errors/hub/command.ts @@ -1,4 +1,4 @@ -import { ErrorIssueId } from "@maple/domain/http" +import { ErrorIssueId, WorkflowState } from "@maple/domain/http" import { V2ErrorIssueListQuery } from "@maple/domain/http/v2" import { getErrorsByType, @@ -207,10 +207,18 @@ export const MountFilterRail = Mount.define("MountErrorsFilterRail", { /** A row's or the selection's workflow write. */ export const RunHubWrite = Command.define("RunErrorsHubWrite", { - args: { ids: Schema.Array(ErrorIssueId), write: HubWrite, isBulk: Schema.Boolean }, + args: { + ids: Schema.Array(ErrorIssueId), + write: HubWrite, + isBulk: Schema.Boolean, + /** The state each row showed when picked, so a move another tab beat is refused (409). */ + expectedStates: Schema.Record(Schema.String, WorkflowState), + }, messages: [Message.CompletedHubWrite], - execute: ({ ids, write, isBulk }) => - runHubWrite(ids, write, (outcome) => Message.CompletedHubWrite({ ids, write, isBulk, outcome })), + execute: ({ ids, write, isBulk, expectedStates }) => + runHubWrite(ids, write, expectedStates, (outcome) => + Message.CompletedHubWrite({ ids, write, isBulk, outcome }), + ), }) /** diff --git a/apps/web/src/page/errors/hub/selection.scene.test.ts b/apps/web/src/page/errors/hub/selection.scene.test.ts index 00669a1143..dafba77ea7 100644 --- a/apps/web/src/page/errors/hub/selection.scene.test.ts +++ b/apps/web/src/page/errors/hub/selection.scene.test.ts @@ -1,4 +1,5 @@ import { OrgId } from "@maple/domain" +import { ErrorIssueTransitionError } from "@maple/domain/http" import { V2ErrorIssue } from "@maple/domain/http/v2" import { Result, Schema, Array as Arr } from "effect" import { describe, expect, test } from "vitest" @@ -132,6 +133,50 @@ describe("errors hub selection", () => { expect(done.model.writingIds).toEqual([]) }) + test("a write carries each row's shown state; a 409 toasts the server's message and re-reads", () => { + const selected = pageUpdate( + pageUpdate(loaded(), Message.PressedListKey({ key: "j", isShift: false })).model, + Message.PressedListKey({ key: "x", isShift: false }), + ).model + const writing = pageUpdate(selected, Message.ClickedBulkClaim()) + expect(writing.commands?.[0]).toMatchObject({ + args: { expectedStates: { [selected.focusedId ?? ""]: "triage" } }, + }) + // A second write on a row whose first is still out expects nothing: its state may be moving. + const again = pageUpdate( + pageUpdate(writing.model, Message.PressedListKey({ key: "x", isShift: false })).model, + Message.ClickedBulkClaim(), + ) + expect(again.commands).toEqual([ + expect.objectContaining({ args: expect.objectContaining({ expectedStates: {} }) }), + ]) + const message = + "This issue is now 'done', not 'triage'. Another change landed first; refresh and retry." + // Decoded as the internal client decodes the 409 body. + const conflict = Schema.decodeSync(ErrorIssueTransitionError)({ + _tag: "@maple/http/errors/ErrorIssueTransitionError", + message, + issueId: "0199a004-0000-7000-8000-000000000001", + fromState: "done", + toState: "in_progress", + }) + const done = pageUpdate( + writing.model, + Message.CompletedHubWrite({ + ids: [ids[0] ?? ""], + write: { _tag: "Transition", toState: "in_progress" }, + isBulk: false, + outcome: { succeeded: 0, failed: 1, firstFailure: toLoadFailure(conflict) }, + }), + ) + expect(done.outMessage).toMatchObject({ + type: "error", + title: "State change failed", + description: message, + }) + expect(names(done.commands)).toContain("FetchErrorIssuesPage") + }) + test("a row still being written is written again: the newest pick is sent, not dropped", () => { const press = (model: Model, key: string) => pageUpdate(model, Message.PressedListKey({ key, isShift: false })).model diff --git a/apps/web/src/page/errors/hub/selection.ts b/apps/web/src/page/errors/hub/selection.ts index d792762954..3f1ed6b4cc 100644 --- a/apps/web/src/page/errors/hub/selection.ts +++ b/apps/web/src/page/errors/hub/selection.ts @@ -1,6 +1,6 @@ -import { type ErrorIssueId, WORKFLOW_STATE_ORDER } from "@maple/domain/http" +import { type ErrorIssueId, WORKFLOW_STATE_ORDER, type WorkflowState } from "@maple/domain/http" import { Command, type Update } from "foldkit" -import { Option, Array as Arr } from "effect" +import { Option, Record as Rec, Array as Arr } from "effect" import * as Keyed from "@maple/views/data/keyed" import { requestKey } from "@maple/views/data/request-key" import { AppRoute, hrefOf } from "../../../route" @@ -124,9 +124,25 @@ export const startHubWrite = ( ? { model } : { model: { ...model, writingIds: [...model.writingIds, ...ids] }, - commands: [RunHubWrite({ ids, write, isBulk })], + commands: [RunHubWrite({ ids, write, isBulk, expectedStates: shownStates(model, ids) })], } +/** + * The workflow state each written row shows now: what a transition expects to move from. A row + * with a write still out shows a state that write may change, so it sends no expectation. + */ +const shownStates = ( + model: Model, + ids: ReadonlyArray, +): Readonly> => + Rec.fromEntries( + Arr.flatMap(signalsOf(model), (signal): ReadonlyArray => + ids.includes(signal.id) && !model.writingIds.includes(signal.id) + ? [[signal.id, signal.issue.workflowState]] + : [], + ), + ) + /** Drops one in-flight entry per written id (a row can have two writes out). */ const settledWriting = (writing: ReadonlyArray, ids: ReadonlyArray) => Arr.reduce(ids, writing, (left, id) => { @@ -188,7 +204,9 @@ export const completedHubWrite = ( ): Return => { const settled = { ...model, writingIds: settledWriting(model.writingIds, ids) } const toast = outcomeToast(write, outcome, isBulk) - if (outcome.succeeded === 0) return { model: settled, outMessage: toast } + // A conflict means the rows are stale, so they are re-read even when nothing was written. + const isStale = outcome.firstFailure?.type === "conflict_error" + if (outcome.succeeded === 0 && !isStale) return { model: settled, outMessage: toast } // Only the bulk bar clears the selection, when it is clicked (`bulkWrite`); a row's write keeps it. const refreshed = refreshIssues(settled, context, pageQuery) return { ...refreshed, outMessage: toast } diff --git a/apps/web/src/page/errors/hub/writes.ts b/apps/web/src/page/errors/hub/writes.ts index 3b9d2ddc03..82cedbb53d 100644 --- a/apps/web/src/page/errors/hub/writes.ts +++ b/apps/web/src/page/errors/hub/writes.ts @@ -36,7 +36,11 @@ export const WriteOutcome = Schema.Struct({ }) export type WriteOutcome = typeof WriteOutcome.Type -const writeOne = (issueId: ErrorIssueId, write: Exclude) => +const writeOne = ( + issueId: ErrorIssueId, + write: Exclude, + expectedState: WorkflowState | undefined, +) => Effect.gen(function* () { const client = yield* MapleInternalApiClient const params = { issueId } @@ -44,7 +48,10 @@ const writeOne = (issueId: ErrorIssueId, write: Exclude) => export const runHubWrite = ( ids: ReadonlyArray, write: HubWrite, + expectedStates: Readonly>, toMessage: (outcome: WriteOutcome) => M, ) => (write._tag === "Merge" ? Effect.exit(mergeAll(write.targetId, ids)).pipe( Effect.map((exit) => Arr.replicate(exit, ids.length)), ) - : Effect.forEach(ids, (id) => Effect.exit(writeOne(id, write)), { concurrency: 4 }) + : Effect.forEach(ids, (id) => Effect.exit(writeOne(id, write, expectedStates[id])), { + concurrency: 4, + }) ).pipe( Effect.map((exits) => { const failures = Arr.filter(exits, Exit.isFailure) diff --git a/apps/web/src/page/errors/issue/command.ts b/apps/web/src/page/errors/issue/command.ts index b4f033dfa1..3effd2a305 100644 --- a/apps/web/src/page/errors/issue/command.ts +++ b/apps/web/src/page/errors/issue/command.ts @@ -216,7 +216,10 @@ const runWrite = (issueId: ErrorIssueId, write: IssueWrite) => case "Transition": return yield* client.errors.transitionIssue({ params, - payload: new ErrorIssueTransitionRequest({ toState: write.toState }), + payload: new ErrorIssueTransitionRequest({ + toState: write.toState, + expectedState: write.fromState, + }), }) case "Claim": return yield* client.errors.claimIssue({ params, payload: new ErrorIssueClaimRequest({}) }) diff --git a/apps/web/src/page/errors/issue/message.ts b/apps/web/src/page/errors/issue/message.ts index d73ef467e4..2ff4108693 100644 --- a/apps/web/src/page/errors/issue/message.ts +++ b/apps/web/src/page/errors/issue/message.ts @@ -35,7 +35,8 @@ const result = (schema: A) => ({ /** The issue workflow writes the rail sends (`useMutationAction` in the route). */ export const IssueWrite = Schema.Union([ - Schema.TaggedStruct("Transition", { toState: WorkflowState }), + /** `fromState` is the state the rail showed; the server refuses the move (409) once it moved on. */ + Schema.TaggedStruct("Transition", { fromState: WorkflowState, toState: WorkflowState }), Schema.TaggedStruct("Claim", {}), Schema.TaggedStruct("Heartbeat", {}), Schema.TaggedStruct("Release", {}), diff --git a/apps/web/src/page/errors/issue/writes.scene.test.ts b/apps/web/src/page/errors/issue/writes.scene.test.ts index 3368fd8326..b2a8481582 100644 --- a/apps/web/src/page/errors/issue/writes.scene.test.ts +++ b/apps/web/src/page/errors/issue/writes.scene.test.ts @@ -1,5 +1,5 @@ import { OrgId } from "@maple/domain" -import { ErrorIssueEventsResponse, ErrorIssueId } from "@maple/domain/http" +import { ErrorIssueEventsResponse, ErrorIssueId, ErrorIssueTransitionError } from "@maple/domain/http" import { Result, Schema, Array as Arr, Option } from "effect" import { describe, expect, test } from "vitest" import { toLoadFailure } from "@maple/views/data/load-failure" @@ -119,6 +119,32 @@ describe("error issue writes", () => { expect(done.commands ?? []).toEqual([]) }) + test("a 409 from a stale tab toasts the server's message and re-reads the issue", () => { + const message = + "This issue is now 'in_review', not 'triage'. Another change landed first; refresh and retry." + // Decoded as the internal client decodes the 409 body. + const conflict = Schema.decodeSync(ErrorIssueTransitionError)({ + _tag: "@maple/http/errors/ErrorIssueTransitionError", + message, + issueId, + fromState: "in_review", + toState: "in_progress", + }) + const done = pageUpdate( + windowed(), + Message.CompletedIssueWrite({ + write: { _tag: "Transition", fromState: "triage", toState: "in_progress" }, + result: Result.fail(toLoadFailure(conflict)), + }), + ) + expect(done.outMessage).toMatchObject({ + type: "error", + title: "Couldn't change state", + description: message, + }) + expect(names(done.commands)).toEqual(expect.arrayContaining(["FetchErrorIssueDetail"])) + }) + test("a routed severity raise waits on the confirmation, then writes", () => { const routed = pageUpdate( windowed(), diff --git a/apps/web/src/page/errors/issue/writes.ts b/apps/web/src/page/errors/issue/writes.ts index eeaaec7926..3d4bc20f49 100644 --- a/apps/web/src/page/errors/issue/writes.ts +++ b/apps/web/src/page/errors/issue/writes.ts @@ -87,7 +87,7 @@ export const pickedState = (model: Model, value: string): Return => { const next = Arr.findFirst(allowed, (state) => state === value) return Option.isNone(next) || next.value === issue.workflow_state ? { model } - : startWrite(model, { _tag: "Transition", toState: next.value }) + : startWrite(model, { _tag: "Transition", fromState: issue.workflow_state, toState: next.value }) } /** `changeSeverity`: clearing writes at once, a level asks the escalation policy first. */ @@ -163,10 +163,14 @@ export const completedWrite = ( ): Return => { const settled = { ...model, busy: settledBusy(model.busy, BUSY[write._tag]) } return Result.match(result, { - onFailure: (failure) => ({ - model: settled, - outMessage: toast("error", ERROR_TITLE[write._tag], failure.message), - }), + // A conflict means this tab is stale: re-read so it shows the state that won. + onFailure: (failure) => + failure.type === "conflict_error" + ? { + ...refresh(settled), + outMessage: toast("error", ERROR_TITLE[write._tag], failure.message), + } + : { model: settled, outMessage: toast("error", ERROR_TITLE[write._tag], failure.message) }, onSuccess: ({ severity }) => { const refreshed = refresh(settled) return { diff --git a/packages/backend/src/services/errors/ErrorsService.test.ts b/packages/backend/src/services/errors/ErrorsService.test.ts index b18c5d06f4..19b7f093b6 100644 --- a/packages/backend/src/services/errors/ErrorsService.test.ts +++ b/packages/backend/src/services/errors/ErrorsService.test.ts @@ -4626,3 +4626,74 @@ describe("ErrorsService.runTick org config priming", () => { }), ) }) + +describe("ErrorsService.transitionIssue expectedState", () => { + // A stale tab saw `triage`; another tab already moved the issue to `in_review`. + // `in_progress` is reachable from both, so only the expected state can catch it. + it.effect("refuses a move from a state the issue has left, and writes nothing", () => + Effect.gen(function* () { + const errors = yield* ErrorsService + const actorService = yield* ErrorActorsService + const database = yield* Database + const issueId = asIssueId(randomUUID()) + const actor = yield* actorService.ensureUserActor(ORG, USER) + yield* seedIssue(issueId, { workflowState: "in_review" }) + + const failure = yield* Effect.flip( + errors.transitionIssue(ORG, actor.id, issueId, "in_progress", { expectedState: "triage" }), + ) + + assert.strictEqual(failure._tag, "@maple/http/errors/ErrorIssueTransitionError") + expect(failure.message).toContain("in_review") + const [row] = yield* database.execute((db) => + db.run( + PG.from(ErrorIssues) + .select("workflowState", "leaseHolderActorId") + .where(($) => [$.id.eq(issueId)]), + ), + ) + assert.strictEqual(row?.workflowState, "in_review") + assert.isNull(row?.leaseHolderActorId) + const events = yield* database.execute((db) => + db.run( + PG.from(ErrorIssueEvents) + .select("type") + .where(($) => [$.issueId.eq(issueId)]), + ), + ) + assert.lengthOf(events, 0) + }).pipe(Effect.provide(makeErrorsLayer())), + ) + + it.effect("applies the move when the issue is still in the expected state", () => + Effect.gen(function* () { + const errors = yield* ErrorsService + const actorService = yield* ErrorActorsService + const issueId = asIssueId(randomUUID()) + const actor = yield* actorService.ensureUserActor(ORG, USER) + yield* seedIssue(issueId, { workflowState: "in_review" }) + + const moved = yield* errors.transitionIssue(ORG, actor.id, issueId, "in_progress", { + expectedState: "in_review", + }) + + assert.strictEqual(moved.workflowState, "in_progress") + }).pipe(Effect.provide(makeErrorsLayer())), + ) + + it.effect("a stale move to the state the issue already has stays a no-op", () => + Effect.gen(function* () { + const errors = yield* ErrorsService + const actorService = yield* ErrorActorsService + const issueId = asIssueId(randomUUID()) + const actor = yield* actorService.ensureUserActor(ORG, USER) + yield* seedIssue(issueId, { workflowState: "done" }) + + const same = yield* errors.transitionIssue(ORG, actor.id, issueId, "done", { + expectedState: "triage", + }) + + assert.strictEqual(same.workflowState, "done") + }).pipe(Effect.provide(makeErrorsLayer())), + ) +}) diff --git a/packages/backend/src/services/errors/ErrorsService.ts b/packages/backend/src/services/errors/ErrorsService.ts index c4abe531aa..4c548f0678 100644 --- a/packages/backend/src/services/errors/ErrorsService.ts +++ b/packages/backend/src/services/errors/ErrorsService.ts @@ -182,7 +182,12 @@ export interface ErrorsServiceApi { actorId: ActorId, issueId: ErrorIssueId, toState: WorkflowState, - opts?: { readonly note?: string; readonly snoozeUntil?: string | null }, + opts?: { + readonly note?: string + readonly snoozeUntil?: string | null + /** The state the caller last saw; a mismatch is a 409 so a stale client cannot overwrite. */ + readonly expectedState?: WorkflowState + }, ) => Effect.Effect< ErrorIssueDocument, ErrorPersistenceError | ErrorIssueNotFoundError | ErrorIssueTransitionError | ErrorValidationError @@ -439,6 +444,20 @@ const make: Effect.Effect< }) const timestamp = yield* Clock.currentTimeMillis const current = yield* requireIssue(orgId, issueId) + // Checked before the lease is taken, so a stale move writes nothing. A move to the + // state the issue already has stays a no-op, as `applyTransition` treats it. + if ( + opts?.expectedState !== undefined && + opts.expectedState !== current.workflowState && + current.workflowState !== toState + ) { + return yield* new ErrorIssueTransitionError({ + message: `This issue is now '${current.workflowState}', not '${opts.expectedState}'. Another change landed first; refresh and retry.`, + issueId, + fromState: current.workflowState, + toState, + }) + } let snoozeUntilMs: number | null | undefined if (opts?.snoozeUntil !== undefined) { @@ -464,7 +483,9 @@ const make: Effect.Effect< // state change a human or agent is entitled to make, and `applyTransition` // already renews the lease of a holder who is still working. if (toState === "in_progress" && actorId !== null) { - yield* acquireLease(orgId, actorId, issueId, DEFAULT_LEASE_DURATION_MS, timestamp).pipe( + yield* acquireLease(orgId, actorId, issueId, DEFAULT_LEASE_DURATION_MS, timestamp, { + whileState: current.workflowState, + }).pipe( Effect.flatMap(({ leaseExpiresAt }) => current.leaseHolderActorId === actorId ? Effect.void @@ -522,6 +543,8 @@ const make: Effect.Effect< issueId: ErrorIssueId, leaseMs: number, timestamp: number, + // Only while the issue is still in this state, so a move that loses its race takes no lease. + opts?: { readonly whileState?: WorkflowState }, ) { const leaseExpiresAt = timestamp + leaseMs const claimed = yield* dbExecute((db) => @@ -541,6 +564,7 @@ const make: Effect.Effect< $.leaseHolderActorId.eq(actorId), $.leaseExpiresAt.lt(timestamp), ), + opts?.whileState === undefined ? undefined : $.workflowState.eq(opts.whileState), ]) .returning(), ), diff --git a/packages/domain/src/http/errors.ts b/packages/domain/src/http/errors.ts index 43a78e45bf..95572d4a86 100644 --- a/packages/domain/src/http/errors.ts +++ b/packages/domain/src/http/errors.ts @@ -462,6 +462,8 @@ export class ErrorIssueTransitionRequest extends Schema.Class("ErrorIssueClaimRequest")({ @@ -871,7 +873,8 @@ export class ErrorIssueNotFoundError extends HttpTaggedError()( +// Self-describing so clients read its message: the dashboard toasts it on a refused or stale move. +export class ErrorIssueTransitionError extends HttpTaggedError()( "@maple/http/errors/ErrorIssueTransitionError", { message: Schema.String, @@ -879,7 +882,14 @@ export class ErrorIssueTransitionError extends Schema.TaggedError()( diff --git a/packages/domain/src/http/v2/error-issues.ts b/packages/domain/src/http/v2/error-issues.ts index fc72c37622..9c49580353 100644 --- a/packages/domain/src/http/v2/error-issues.ts +++ b/packages/domain/src/http/v2/error-issues.ts @@ -220,6 +220,8 @@ export const V2ErrorIssueTransitionParams = Schema.Struct({ note: Schema.optionalKey(Schema.String), // Only meaningful with `to_state: "snoozed"`; `null` clears it. snooze_until: Schema.optionalKey(Schema.NullOr(Timestamp)), + // The state the caller last read; when the issue has since moved, the request is a 409. + expected_state: Schema.optionalKey(WorkflowState), }).annotate({ identifier: "ErrorIssueTransitionParams", title: "Error issue transition parameters" }) export const V2ErrorIssueClaimParams = Schema.Struct({