Skip to content

refactor(web): extract useFeedbackItemActions from the two feedback cards (#tech-debt) - #1078

Merged
selfcontained merged 1 commit into
mainfrom
tech-debt/feedback-item-actions-hook
Sep 8, 2026
Merged

refactor(web): extract useFeedbackItemActions from the two feedback cards (#tech-debt)#1078
selfcontained merged 1 commit into
mainfrom
tech-debt/feedback-item-actions-hook

Conversation

@selfcontained

Copy link
Copy Markdown
Owner

What was fixed

apps/web/src/components/app/diff-feedback-annotation.tsx and reviews-feedback-item.tsx each declared the byte-identical reply/mutation block — the same two useStates, the same two mutation hooks, and byte-identical submitReply, cancelReply and updateResolution bodies (same trim, same empty-string early return, same two toast strings) — differing only in whether the item id came from feedbackItem.id or item.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.ts exports useFeedbackItemActions(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.isPendingisSendingReply, setResolution.isPendingisUpdatingResolution, setResolution.variables?.resolutionpendingResolution) plus onStartReply={() => 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 / setExpanded is left local in both files. It is duplicated too, but the annotation drives it from a focused prop via a useEffect that also calls scrollIntoView and onFocusComplete; the sidebar row does not. Sharing it would pull a scroll effect into a hook that only one caller wants.
  • The status icon + label block (state === "fixed" ? <CheckCircle2 …>) is duplicated-looking but is presentation, not logic: different icon classes (shrink-0 only in the sidebar), different derivation (!isResolved + resolution vs item.status !== "resolved"), and different label strings ("Feedback" + lineLabel vs `Feedback · ${stateLabel}`). state is still computed at each call site.
  • FeedbackItemRow in agent-history-timeline.tsx:137 shares only the name. It is a read-only display row over HistoryFeedbackItem with no mutations, no reply form and no resolution footer. That name collision is a separate backlog entry; it is not touched here.
  • The two components' JSX is not shared. The annotation is a sticky in-diff card with a line label and a status pill; the sidebar row is a bordered button row with a file-path tooltip, a diff snapshot and 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 passed
  • pnpm run finalize:web — clean build (exit 0)
  • pnpm run test:e2e — 198 passed, 12 skipped (Playwright; includes review-agent-ui.spec.ts "opens their submitted review", which renders the sidebar feedback path)
  • No backend files changed, so the server Vitest suite was not run.

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 the if (!body) return guard (1 fail), resetting the draft even when the mutation rejects (1 fail), swapping the two toast strings (1 fail), hardcoding pendingResolution: undefined (1 fail), and making cancelReply skip setReply("") (1 fail). All six failed; baseline is 7/7 green. No probe came back unexpectedly green this run.

Queued next

errorMessage has regressed to three inline error 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, the chat-entries.tsx complexity hotspot (1180 lines, never audited).

🤖 Generated with Claude Code

…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>
@selfcontained
selfcontained merged commit e250a80 into main Sep 8, 2026
1 check passed
@selfcontained
selfcontained deleted the tech-debt/feedback-item-actions-hook branch September 8, 2026 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant