Skip to content

feat(web): inline transcript segment editing#9353

Open
the-scott-davis wants to merge 1 commit into
BasedHardware:mainfrom
the-scott-davis:feat/web-editable-transcript
Open

feat(web): inline transcript segment editing#9353
the-scott-davis wants to merge 1 commit into
BasedHardware:mainfrom
the-scott-davis:feat/web-editable-transcript

Conversation

@the-scott-davis

@the-scott-davis the-scott-davis commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What changed and why

Adds inline transcript editing to the web app (app.omi.me) so users can correct mistranscribed names/entities. Each transcript block gets a pencil affordance that opens per-segment inline text editors, saving via the existing PATCH /v1/conversations/{id}/segments/text. After an edit, a nudge offers to reprocess the conversation so the summary/search reflect the change.

Part of #4095. That issue is coupled (edit transcript and conversation title). The conversation-title half already appears implemented (EditableTitle), so this PR completes the transcript-segment-editing half, for the web surface. Mobile/desktop remain out of scope here. Related: #4044.

Discussed with the team in Discord: https://discord.com/channels/1192313062041067520/1231903583717425153/1522531112709263441

Interaction:

  • Hover a transcript block → pencil → per-segment editable fields.
  • Enter saves & closes, Esc cancels & closes, Done exits.
  • Merged same-speaker blocks split into per-segment fields so each edit maps 1:1 to the one-segment-at-a-time API.

Concurrency hardening (from PR review)

A reviewer flagged three real races. Addressed as follows (single-client fully covered; the cross-client/backend root causes are tracked in #9392):

  1. Concurrent segment saves overwriting each other — saves now run through a serialized queue (one PATCH in flight at a time), so the backend's read-modify-write of the whole segments array can't lose-update between a client's own saves. Shown as a "Saving X of N" banner + per-segment spinner; a failed save reverts to the persisted text. Cross-tab/cross-client still needs the backend transaction — see Transcript edits can lose-update: non-transactional segment write + reprocess stale-snapshot overwrite #9392.
  2. Edit erased during Reprocess — editing and reprocess are now mutually exclusive (pencils disabled while reprocessing; Reprocess disabled while a save is pending). Reprocess's stale-snapshot write root cause is in Transcript edits can lose-update: non-transactional segment write + reprocess stale-snapshot overwrite #9392.
  3. Switching conversations mid-request corrupting UIfully fixed: every async completion is guarded by the conversation id captured at request time, and optimistic updates read the latest state via refs, so a stale response can't overwrite the newly displayed conversation.

Product invariants affected

None binding. INV-CHAT-1's path globs are desktop-only; this writes to the single backend-owned transcript, honoring its single-source-of-truth spirit. INV-UI-1: no new purple — verified with .github/scripts/check_brand_ui.py (neutral/white tokens only).

How it was verified

  • tsc --noEmit: clean. Prettier: clean.
  • INV-UI-1 ratchet (check_brand_ui.py --base upstream/main): no purple increase.
  • Exercised the real user path against the production backend: opened conversations, edited segments — PATCH .../segments/text returned 200, edits persisted across reload. Verified serialized "Saving X of N", per-segment spinner, edit↔reprocess mutual exclusion, and the conversation-switch guard in the running app.

Tests

web/app currently has no test runner (it relied on next lint, removed in Next 16), so there is no hermetic suite to extend. Verified via typecheck + driving the real UI as above. Happy to add a runner (e.g. Vitest) in a follow-up if maintainers want one.

Follow-up

Backend concurrency root causes (non-transactional segment write + reprocess stale-snapshot overwrite) are tracked in #9392 — the durable, cross-client fix. Happy to take that PR too if wanted.

@Git-on-my-level Git-on-my-level added needs-maintainer-review Needs human maintainer review before merge needs-tests PR introduces logic that should be covered by tests labels Jul 9, 2026

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this — the implementation is clean, well-commented, and the interaction model (per-segment editors mapped 1:1 to the one-segment-at-a-time API, Enter/blur/Escape semantics, optimistic state patching, the Enter+blur dedupe fix) is thoughtful. The reprocess nudge for stale summary/search is a good product instinct, and the 402 plan-gating is handled explicitly.

I verified against main that the backend PATCH /v1/conversations/{id}/segments/text, reprocessConversation, MixpanelManager.track, Sparkles, and RefreshCw all already exist — no phantom references. The {status} response shape matches. Leaving this for a human maintainer since it's a user-facing feature that persists user edits to transcript data; a couple of things worth a look before merge:

  1. Optimistic update is not rolled back on failure. handleSegmentTextChange calls updateSegmentText (which throws on non-2xx) but the optimistic onConversationUpdate runs after the awaited API call, so the local state is only patched on success — good. However, on failure the editor surfaces the error but the parent's transcriptEdited/segment state is left as-is, which is fine; just flagging that there's no rollback path because the optimistic write never happened. This is actually correct, just worth confirming it's intentional.

  2. onBlur={handleSave} on a textarea with a save-in-flight guard — if the user blurs mid-save, handleSave returns early via isSaving, which is correct, but consider whether a blur during a failing save should keep the editor mounted. Right now onExitEdit is only called on Enter-success / Escape, so blur never closes the editor. That's a slightly unusual pattern (blur usually dismisses inline editors) but defensible given the "keep the error visible" goal. Worth a conscious maintainer call on the UX.

  3. reprocessConversation can be slow/heavy. There's a spinner but no timeout or cancellation. If reprocessing hangs, the button stays in "Reprocessing…" indefinitely. A timeout or at least surfacing the failure (currently only console.error) would improve resilience. The transcriptEdited flag also never clears on reprocess failure, so the banner persists — which is arguably correct (the summary is still stale) but the user gets no feedback that reprocessing failed.

  4. No automated tests. The PR notes web/app currently has no test runner. The SegmentTextEditor save/dedupe/error logic is the kind of finicky state machine that benefits most from tests. Not blocking on its own given the manual verification, but it's a good candidate for the first Vitest suite if/when one lands. The double-fire dedupe (lastSavedRef) in particular is subtle enough that a regression test would pay for itself.

  5. Minor: the WebhookType reformatting (single-line → multi-line union) is an unrelated prettier churn in api.ts. Harmless, but it's not part of this feature.

Feature fit is good — correcting mistranscribed names/entities inline is a natural complement to the existing EditableTitle, and the single-source-of-truth invariant (INV-CHAT-1) is respected since it writes to the one backend-owned transcript. The scope is appropriately narrow (web-only; mobile/desktop explicitly deferred).

Tagging needs-maintainer-review + needs-tests. Not approving formally since this is a user-data-persisting product feature that warrants human product/UX judgment.

@the-scott-davis

Copy link
Copy Markdown
Contributor Author

Thanks for the really sharp review — all three were legit races. Here's how I resolved them in the latest push:

1. Concurrent segment saves overwriting each other. The UI now serializes saves through a queue — only one PATCH /segments/text is in flight at a time, so each save reads the state the previous one left behind. There's a "Saving X of N" progress banner + a per-segment spinner, and a failed save reverts to the persisted text. This fully removes the lost-update race for a single client. The remaining cross-tab/cross-client case can't be fixed from the client (it's the backend's non-transactional read-modify-write of the whole segments array), so I filed #9392 to wrap that in a Firestore transaction (it also affects the speaker-assign endpoints).

2. Edit erased if made during Reprocess. Editing and reprocess are now mutually exclusive in the UI — pencils are disabled while a reprocess runs, and Reprocess is disabled while any save is pending, so the client can't issue an edit that reprocess's stale-snapshot write would clobber. The root cause (reprocess persisting a whole-conversation snapshot that can include a stale transcript) is also in #9392 — the durable fix is to have reprocess re-read / only write derived fields.

3. Switching conversations mid-request replacing the other conversation's UI. Fully fixed client-side: every async completion (segment save + reprocess) captures the conversation id at request time and ignores its result if you've since switched conversations, and optimistic updates now read the latest state via refs instead of a stale closure. So A's delayed response can no longer replace B's UI.

Net: #3 is fully resolved; #1 and #2 are resolved for the single-client path, with the backend transaction + reprocess-narrowing tracked in #9392 for cross-client durability. Happy to take that backend PR too if useful.

Let users correct mistranscribed names/entities directly on the
conversation transcript in the web app. Each transcript block gets a
pencil affordance that opens per-segment inline text editors; edits save
to the existing `PATCH /v1/conversations/{id}/segments/text` endpoint
(keyed by segment id). After edits, a nudge offers to reprocess the
conversation so the summary/search reflect the change.

- api: `updateSegmentText()` wrapper + `SegmentEditPlanRequiredError` (402)
- TranscriptView: pencil toggle + `SegmentTextEditor`. Enter saves & closes,
  Esc cancels & closes, blur commits & closes, Done exits; merged
  same-speaker blocks split into per-segment fields (1:1 with the API).
- ConversationDetailPanel: optimistic updates + reprocess nudge + analytics
- Neutral styling only — no new purple (INV-UI-1 ratchet holds)

Concurrency & resilience (from PR review):
- Serialized save queue — one PATCH in flight at a time, so the backend's
  read-modify-write of the whole segments array can't lose-update between
  concurrent client saves. "Saving X of N" banner + per-segment spinner;
  failed saves revert to the persisted text.
- Editing and reprocess are mutually exclusive (no edit clobbered by a
  reprocess snapshot write, and vice versa).
- Async completions (save + reprocess) are guarded by the conversation id
  captured at request time, and optimistic updates read the latest state via
  refs — so switching conversations mid-request can't let a stale response
  overwrite the newly displayed conversation.
- Reprocess is raced against a 90s timeout and failures are surfaced with a
  retry banner (no more indefinite "Reprocessing…" / silent console error).

Cross-client/cross-tab races and the reprocess stale-snapshot write are
backend concerns tracked in BasedHardware#9392; this PR fixes the single-client path.

Verified: tsc clean; Prettier clean; INV-UI-1 guard passes; exercised the
real user path against the production backend.
@the-scott-davis the-scott-davis force-pushed the feat/web-editable-transcript branch from 5fd591c to e9ef96a Compare July 10, 2026 14:19

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web feature: inline transcript segment editing. Approve only.

@the-scott-davis

Copy link
Copy Markdown
Contributor Author

Heads up: a lot of this was reviewed against the pre-refactor commit. I pushed a follow-up that swapped the editor internals (handleSave/isSaving/lastSavedRef) for a serialized save queue in response to separate feedback, so a few of these are already handled:

  1. Optimistic rollback: edits apply optimistically now and revert to the saved text if the save fails, so there's a rollback path.
  2. Blur not closing the editor: changed that. Blur now commits and closes, same as Enter. Esc cancels and closes.
  3. Reprocess resilience: added a 90s timeout and a retry banner on failure instead of the indefinite spinner + silent console.error. Good call.
  4. Tests: agreed, not blocking. Good candidate for a first Vitest suite when one lands. Fwiw the lastSavedRef dedupe you flagged is gone now; the serialized queue + a commit guard replaced it.
  5. WebhookType churn: reverted. api.ts is just the updateSegmentText addition now.

Also opened #9392 for the backend root causes (non-transactional segment write + the reprocess stale-snapshot overwrite), since the frontend can't cover the cross-client case.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Follow-up review after the concurrency revisions.

The serialized save queue (processSaveQueue with saveQueueRef/processingRef), the cross-conversation staleness guard (convIdRef), the optimistic-update rollback on save failure, and the reprocess timeout (90s Promise.race) + retry/error surfaces are all solid improvements over the earlier iteration. They directly address the races and the "stuck on Reprocessing…" concern from the prior pass. The SegmentTextEditor committedRef dedupe is the right fix for the Enter→blur double-fire.

A few things worth a human look before merge:

  1. 402 detection via string matchingupdateSegmentText checks error.message.includes('402') to throw SegmentEditPlanRequiredError. This works against the current fetchWithAuth error format (API error: 402 ...) but is fragile if that format ever changes. Consider having fetchWithAuth expose the status code or a typed error so plan-gating detection doesn't depend on substring matching.

  2. Cross-client concurrency — the serialized queue protects within a single tab, but two tabs editing simultaneously can still lose-update against the backend's read-modify-write of the full segments array. Glad this is tracked in Transcript edits can lose-update: non-transactional segment write + reprocess stale-snapshot overwrite #9392; flagging it here so a human maintainer confirms the backend transaction is tracked before this ships.

  3. No tests — the serialized queue drain logic, optimistic revert, and the committedRef state machine are the kind of finicky logic that regresses silently. Good candidate for the first Vitest suite when one lands. Keeping the needs-tests label for that reason.

Feature fit is good — inline transcript correction is a natural complement to EditableTitle, and the scope is appropriately narrow (web-only; mobile/desktop deferred). Backend auth/validation already gates this endpoint and the mobile app already calls it.

Keeping needs-maintainer-review + needs-tests. Not approving formally — this is a user-data-persisting product feature that warrants human product/UX sign-off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-maintainer-review Needs human maintainer review before merge needs-tests PR introduces logic that should be covered by tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants