fix(document-viewer): harden manual-tag editor UX (validation, delete confirm, 401)#1035
Merged
Conversation
… confirm, 401) Address the three pre-existing manual-tag-editor findings deferred from #1025 — they predate the decomposition, so folding them into that pure-move PR would have broken its no-behaviour-change contract. Client-side only; the server already enforces the label contract and auth. - Match the server's 2-64 char label rule (manualLabelSchema in src/app/api/documents/[id]/labels/route.ts) in the field: maxLength={64} on the add and edit inputs, and disable submit/save until the trimmed value reaches the 2-char minimum, so a too-short/too-long tag no longer round-trips to a generic 400. - Require an explicit second click to confirm a manual-tag delete (inline two-click confirm with a Cancel, no native window.confirm) so a single misclick can't remove a curated label. - Return after onUnauthorized() on a 401 instead of falling through to the generic throw, so the unauthorized handoff no longer also raises a stale inline error banner. Adds tests/manual-tag-editor.dom.test.tsx covering all three behaviours. Verified: typecheck, lint, and the full unit suite (3,065 passing incl. the 3 new tests; only the pre-existing container-only pdf-extraction-budget flake fails). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Jc1ZYHFjXjn6mE6U6riVU
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe manual tag editor enforces label length limits, handles HTTP 401 responses through ChangesManual tag editor behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
BigSimmo
marked this pull request as ready for review
July 21, 2026 02:31
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/document-viewer/manual-tag-editor.tsx`:
- Around line 242-260: The confirmingDeleteId branch in ManualTagEditor must not
replace the original Remove target with a destructive action button, since rapid
repeated clicks can invoke deleteManualTag. Render a non-action confirmation
prompt there and provide a visibly labeled Confirm remove control in a separate
location; update tests/manual-tag-editor.dom.test.tsx at lines 61-72 to rapidly
click the initial Remove control and verify no DELETE request is issued.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0434f048-76f3-4bc2-93c2-69c58e8d9f07
📒 Files selected for processing (2)
src/components/document-viewer/manual-tag-editor.tsxtests/manual-tag-editor.dom.test.tsx
…e (CodeRabbit) The first version rendered the "Confirm remove" button at the same fragment index as the "Remove" button, so React reused the Remove button's DOM node for the destructive confirm — a rapid double-click on Remove could land its second click on the in-place delete control. Fix it two ways: - Give the Rename/Remove and confirm/cancel controls distinct `key`s so React never reuses the Remove node for the destructive confirm — the control the user pressed is never turned into "delete" in place. - Render a non-action "Remove this tag?" prompt in the position the Remove button was clicked from, shifting the confirm control off that hit target, and label the confirm button "Confirm remove". Adds a regression test: after one Remove click the original Remove control is gone (replaced by the prompt) and re-clicking it issues no DELETE. Verified: typecheck, lint, the manual-tag-editor dom suite (4 passing), and the full unit suite (3,066 passing; only the pre-existing pdf-extraction-budget flake fails). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Jc1ZYHFjXjn6mE6U6riVU
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
2 participants
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.
Summary
Follow-up to #1025 — addresses the three pre-existing
manual-tag-editor.tsxfindings thatCodeRabbit surfaced there. They predate the decomposition (the editor was moved verbatim), so
folding them into that pure-move PR would have broken its no-behaviour-change contract; they
belong in a dedicated PR. Client-side only — the server already enforces the label
contract and auth; this closes the gap between the field and the API.
labelto be 2–64 chars(
manualLabelSchemainsrc/app/api/documents/[id]/labels/route.ts). AddmaxLength={64}to the add and edit inputs and disable submit/save until the trimmed value reaches the 2-char
minimum, so a too-short/too-long tag no longer round-trips to a generic 400.
two-click confirm (
Remove→Confirm remove/Cancel), no nativewindow.confirm(whichthe repo avoids) — so a single misclick can't remove a curated label.
submitManualTagnow returns afteronUnauthorized()instead offalling through to the generic
throw, so the unauthorized handoff no longer also raises astale inline error banner underneath the parent's unauthorized UI.
Off the RAG-ranking lane; no server, schema, permission, or data-access behaviour changed.
Verification
npm run typecheck— 0 errorsnpm run lint— 0 warningsnpm run test— 3,065 passing, incl. the newtests/manual-tag-editor.dom.test.tsx(React Testing Library) covering all three behaviours:Add stays disabled below the 2-char minimum + input caps at 64; delete requires the explicit
confirm (and Cancel backs out) before any request; a 401 calls
onUnauthorizedwithoutrendering the error banner. The only failure is the pre-existing container-only
pdf-extraction-budgetflake (Python OCR stack absent in this VM).UI note: the manual-tag editor is admin-gated (
canManage) and not reachable in the demoPlaywright flow, so the RTL component test — which renders the component and drives the exact
new interactions in jsdom — is the behavioural proof here rather than a Chromium spec.
Risk and rollout
schema, or permission change.
Clinical Governance Preflight
Not applicable — client-side field validation and confirmation UX only. No ingestion, answer
generation, retrieval/ranking, source governance, document-access rules, privacy, schema, or
production behaviour changed; the server-side label contract, auth, and permissions are
untouched.
Notes
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests