feat(web): inline transcript segment editing#9353
Conversation
Git-on-my-level
left a comment
There was a problem hiding this comment.
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:
-
Optimistic update is not rolled back on failure.
handleSegmentTextChangecallsupdateSegmentText(which throws on non-2xx) but the optimisticonConversationUpdateruns 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'stranscriptEdited/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. -
onBlur={handleSave}on a textarea with a save-in-flight guard — if the user blurs mid-save,handleSavereturns early viaisSaving, which is correct, but consider whether a blur during a failing save should keep the editor mounted. Right nowonExitEditis 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. -
reprocessConversationcan 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 onlyconsole.error) would improve resilience. ThetranscriptEditedflag 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. -
No automated tests. The PR notes
web/appcurrently has no test runner. TheSegmentTextEditorsave/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. -
Minor: the
WebhookTypereformatting (single-line → multi-line union) is an unrelated prettier churn inapi.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.
62115a2 to
5fd591c
Compare
|
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 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.
5fd591c to
e9ef96a
Compare
kodjima33
left a comment
There was a problem hiding this comment.
Web feature: inline transcript segment editing. Approve only.
|
Heads up: a lot of this was reviewed against the pre-refactor commit. I pushed a follow-up that swapped the editor internals (
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. |
|
Follow-up review after the concurrency revisions. The serialized save queue ( A few things worth a human look before merge:
Feature fit is good — inline transcript correction is a natural complement to Keeping |
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:
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):
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-1ratchet (check_brand_ui.py --base upstream/main): no purple increase.PATCH .../segments/textreturned200, 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/appcurrently has no test runner (it relied onnext 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.