Conversation
|
Here's a visual recap of what changed: Open the full interactive recap |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Builder reviewed your changes and found 8 potential issues 🟡
Review Details
Code Review Summary
This incremental review covers the latest retry bookkeeping, receipt validation, optimistic suggestion reconciliation, and composer concurrency changes. The three previously open findings were verified fixed: reply routing is now included in the idempotency receipt, same-thread resolution uses a synchronous in-flight guard, and refetched review records are deduplicated before overlays are applied. Those comments were resolved before this review.
New Findings
- 🟡 MEDIUM — Retry records are shared per composer, so a late success from submission A can erase submission B's operation ID and make an ambiguous retry non-idempotent.
- 🟡 MEDIUM — Failed submissions are dropped when newer text exists in the composer, losing the original text after optimistic rollback.
- 🟡 MEDIUM — Same-composer submissions are not kept in flight-locked, allowing a user to type and submit a second comment/reply before the first settles.
- 🟡 MEDIUM — Receipt matching includes mutable access/display snapshots, causing valid retries to be rejected after profile or ACL-context changes.
- 🟡 MEDIUM — A stale suggestion amendment response can overwrite a newer refetched revision.
- 🟡 MEDIUM — A replay after notification failure suppresses notification permanently.
The remaining approach is directionally sound and the new fixes address several prior race conditions, but retry state needs operation-level ownership rather than composer-level ownership. Risk level: high because this affects shared mutation, notification, and idempotency semantics.
🧪 Browser testing: Will run after this review (PR touches UI code)
| ); | ||
| clientOperationId: operationId, | ||
| }); | ||
| createRetryRef.current = null; |
There was a problem hiding this comment.
🟡 Do not clear another submission's retry identity
createRetryRef is shared by all root submissions, but any successful request clears it here. If submission B becomes ambiguous first and stores its operation ID, then an older concurrent submission A succeeds, retrying B generates a new UUID and can duplicate a comment that was already committed. Associate retry state with the specific operation/payload and clear it only when that same operation settles; the reply retry map has the same race.
Additional Info
Reported independently by 1 of 4 agents and confirmed against the shared retry ref.
| if ( | ||
| draftGenerationRef.current !== generation || | ||
| draftRef.current !== "" | ||
| ) { | ||
| return; |
There was a problem hiding this comment.
🟡 Retain failed submissions when newer typing exists
When a request fails after the user has typed a newer draft, this branch returns without retaining the submitted snapshot. The optimistic entry is rolled back, so the original comment text is silently lost and cannot be retried. Store failures by operation while leaving the newer draft editable; the reply path has the same loss condition.
Additional Info
Reported independently by 1 of 4 agents; confirmed from rollback behavior and this early return.
| setSubmittingTarget(resolutionTarget); | ||
| createComment.mutate( | ||
| { | ||
| const submitDraft = async (resolutionTarget: ReviewResolutionTarget) => { |
There was a problem hiding this comment.
🟡 Keep same-composer submissions in flight-locked
Clearing the draft prevents an immediate duplicate click, but it does not prevent the user from typing again and submitting from the same root composer while the first request is pending. The second submission receives a new UUID and persists a second comment; the same applies to a reply composer. Track an in-flight root submission and per-thread reply submissions without blocking unrelated composers.
Additional Info
Reported by 1 of 4 agents; this is distinct from the resolved same-thread resolution guard.
| const immutableFields: (keyof ReviewComment)[] = [ | ||
| "id", | ||
| "resourceType", | ||
| "resourceId", | ||
| "threadId", | ||
| "parentCommentId", | ||
| "targetId", | ||
| "kind", | ||
| "anchor", | ||
| "body", | ||
| "authorEmail", | ||
| "authorName", | ||
| "createdBy", | ||
| "resolutionTarget", | ||
| "mentions", | ||
| "ownerEmail", | ||
| "orgId", | ||
| "visibility", | ||
| "metadata", |
There was a problem hiding this comment.
🟡 Exclude mutable context from retry receipt matching
The retry fingerprint includes authorName, ownerEmail, orgId, and visibility, although these are recomputed from current profile/access context on every retry. A valid retry after a display-name, ownership, organization, or visibility-context change is rejected as a conflicting operation even though the immutable submission is identical. Match immutable submission fields and re-authorize the retry separately.
Additional Info
Reported by 1 of 4 agents; confirmed against the action context fields used to construct the receipt.
| onSuccess: (result) => (data) => | ||
| updateSuggestions(data, (suggestions) => | ||
| suggestions.map((suggestion) => | ||
| suggestion.id === input.id | ||
| ? (result as ResourceSuggestion) |
There was a problem hiding this comment.
🟡 Do not overwrite newer suggestion revisions with stale responses
The successful amendment transform unconditionally replaces the cached suggestion with the mutation result. If a refetch delivers a newer revision while the original request is in flight, a late older response temporarily restores stale operations/revision data until invalidation completes. Preserve the newer record using revision or updated-at ordering.
Additional Info
Reported by 1 of 4 agents; confirmed against the adjacent revision-aware reconciliation logic.
| return { | ||
| ...result.comment, | ||
| replayed: result.replayed, | ||
| notified: result.replayed | ||
| ? null |
There was a problem hiding this comment.
🟡 Retry notification delivery for idempotent replays
Once an idempotent comment row exists, replay returns notified: null and skips notifyReviewComment. If the original request committed the row but notification failed or the response was lost after notification failure, a same-operation retry reports success without ever retrying delivery, permanently losing the notification. Persist notification delivery state or retry it through an idempotent notification path.
Additional Info
Reported by 1 of 4 agents; confirmed against the action's post-insert notification await and replay branch.
| ...(submittedMentions.length ? { mentions: submittedMentions } : {}), | ||
| clientOperationId: operationId, | ||
| }); | ||
| delete replyRetriesRef.current[comment.id]; |
There was a problem hiding this comment.
🟡 Do not clear another reply's retry identity
The retry slot is keyed only by comment and is cleared by any successful reply for that comment. If reply B becomes ambiguous first and reply A succeeds later, this deletes B's operation ID; retrying B then uses a new UUID and can duplicate the persisted reply. Track retry state by operation/payload and clear only the matching submission.
Additional Info
Reported by 1 of 4 agents; confirmed against concurrent same-thread reply behavior.
| } catch { | ||
| if ( | ||
| (replyGenerationsRef.current[comment.id] ?? 0) !== generation || | ||
| (replyDraftsRef.current[comment.id] ?? "") !== "" | ||
| ) { | ||
| return; |
There was a problem hiding this comment.
🟡 Retain failed replies when newer typing exists
If a reply fails after newer text has been entered in that thread, this branch returns without retaining the submitted reply snapshot. Optimistic rollback then leaves neither the failed reply nor recoverable retry text, silently losing the original submission. Keep operation-scoped failure state separate from the current draft.
Additional Info
Reported by 1 of 4 agents; confirmed from the reply rollback path.
This branch was successfully deployed
1 participant
Problem
When a Content collaborator submits a comment or reply, the new entry appears optimistically while the same text remains in a disabled composer until the server responds. That makes one submission look duplicated and slows down the next reply. Core review comments and suggested-edit decisions also wait for server results in several paths.
Approach
Transfer each submitted draft to one optimistic entry and clear its composer immediately. Keep the submitted snapshot separate from new typing so a late response cannot erase a newer draft. Use a stable operation ID to reconcile server echoes and make Core comment and reply retries safe.
What changed
Verification
git diff --check, and the changed-code error-handling guard passed.Vite environment "nitro" is unavailablepage, and the T3 preview could not reach the editor. The Windows Content build compiled the client and server bundles but failed in its final packaging step while invoking the extensionless localesbuildbinary. Neither result is counted as a UI pass.Review focus
The reported Command-V failure also occurs on another website in T3 Code. The Content comment textarea does not intercept paste, and Alice excluded that separate host issue from this PR.