refactor(web): extract useFeedbackItemActions from the two feedback cards (#tech-debt) - #1078
Merged
Merged
Conversation
…ards (#tech-debt) diff-feedback-annotation.tsx and reviews-feedback-item.tsx declared byte-identical reply-draft state and submitReply/cancelReply/ updateResolution handlers, differing only in where the feedback item id came from. Both now call a shared useFeedbackItemActions(agentId, itemId) hook. No markup, class names or testids changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was fixed
apps/web/src/components/app/diff-feedback-annotation.tsxandreviews-feedback-item.tsxeach declared the byte-identical reply/mutation block — the same twouseStates, the same two mutation hooks, and byte-identicalsubmitReply,cancelReplyandupdateResolutionbodies (same trim, same empty-string early return, same two toast strings) — differing only in whether the item id came fromfeedbackItem.idoritem.id. That is the largest web↔web duplication left after the jobs-form cluster (16 duplicated 7-line windows in the 2026-09-05 audit).New
apps/web/src/components/app/use-feedback-item-actions.tsexportsuseFeedbackItemActions(agentId, itemId), returning{ reply, setReply, replying, startReply, cancelReply, submitReply, isSendingReply, updateResolution, isUpdatingResolution, pendingResolution }. Both components destructure it. No markup, class names, testids or rendered strings changed — the only JSX edits are three attribute values per file (addMessage.isPending→isSendingReply,setResolution.isPending→isUpdatingResolution,setResolution.variables?.resolution→pendingResolution) plusonStartReply={() => setReplying(true)}→onStartReply={startReply}.Net: +140/−82 including the new test; the two components lose 29 lines each.
Why it's tech debt
Two copies of the same async-mutation-plus-toast machinery that must be kept in sync by hand: any change to the error copy, the trim rule, or the optimistic reset had to be applied twice, and a fix to one silently left the other behind. No behavior changes, no new features.
Deliberate exclusions (near misses — please don't ask me to fold these in)
expanded/setExpandedis left local in both files. It is duplicated too, but the annotation drives it from afocusedprop via auseEffectthat also callsscrollIntoViewandonFocusComplete; the sidebar row does not. Sharing it would pull a scroll effect into a hook that only one caller wants.state === "fixed" ? <CheckCircle2 …>) is duplicated-looking but is presentation, not logic: different icon classes (shrink-0only in the sidebar), different derivation (!isResolved+resolutionvsitem.status !== "resolved"), and different label strings ("Feedback"+lineLabelvs`Feedback · ${stateLabel}`).stateis still computed at each call site.FeedbackItemRowinagent-history-timeline.tsx:137shares only the name. It is a read-only display row overHistoryFeedbackItemwith no mutations, no reply form and no resolution footer. That name collision is a separate backlog entry; it is not touched here.AnimatePresence. Only the behavior is shared.Shape chosen (question for the reviewer below)
Per standing ruling #1036 (review 993), a duplication spanning two callers of a shared component is fixed by extracting a hook, leaving the components' rendering untouched. I took the flat return above rather than the
fieldProps-bag form that ruling used verbatim — see the review question.Validation
pnpm run check— clean (exit 0)cd apps/web && pnpm vitest run— 128 files, 1888 tests passedpnpm run finalize:web— clean build (exit 0)pnpm run test:e2e— 198 passed, 12 skipped (Playwright; includesreview-agent-ui.spec.ts"opens their submitted review", which renders the sidebar feedback path)New test is non-vacuous. Reverting the fix is degenerate (it deletes the module the test imports), so I probed six defects into the hook instead, each reverted after: dropping
.trim()(2 fail), dropping theif (!body) returnguard (1 fail), resetting the draft even when the mutation rejects (1 fail), swapping the two toast strings (1 fail), hardcodingpendingResolution: undefined(1 fail), and makingcancelReplyskipsetReply("")(1 fail). All six failed; baseline is 7/7 green. No probe came back unexpectedly green this run.Queued next
errorMessagehas regressed to three inlineerror instanceof Error ? … : String(error)sites (apps/server/src/agents/pin-write.ts:52,shared/plugin-status.ts:124, and one more) — an explicit 15-minute filler. After that, thechat-entries.tsxcomplexity hotspot (1180 lines, never audited).🤖 Generated with Claude Code