Inline unstaged diff comment composer cannot be dismissed: Cancel renders as invisible text, Escape does nothing #566
Labels
No labels
actions
bug
cli
duplicate
enhancement
fork
forum
git storage
help wanted
insights
invalid
issues
moderation
oidc
ownership transfer
packages
pr/merge protection rules
projects
pull requests
question
releases
sponsorships
tags
webhooks
wiki
wontfix
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
crueber/walhub#566
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Unstaged inline diff comment composer cannot be dismissed — the Cancel affordance renders as invisible text, and Escape does nothing
What's requested
A working, visible Cancel for the unstaged inline diff comment composers on the PR conversation page (
DiffFilekeyed drafts) and the PR Files tab (PullDiffFilestaged selection): a Cancel that is visibly an interactive control, dismisses via button click AND Escape, removes only that composer instance, and never stages anything.Evidence (static analysis of the current tree; no live repro)
The dismiss control exists and its handler is correctly wired — the failure is that it is invisible:
web/src/pages/Pull.jsx:499(conversation page, inline draft composer):<button type="button" class="link ml-2" onClick={() => closeDraft(draftKey(hi(), ri()))}>cancel</button>, rendered inside the muted caption<p class="mb-1 text-xs text-zinc-500 dark:text-zinc-400">commenting on <span class="font-mono">…</span>cancel</p>.web/src/pages/PullFiles.jsx:84(Files tab staged-selection composer): same pattern —<button type="button" class="link ml-2" onClick={() => setStaged(null)}>cancel</button>inside the same muted caption idiom.The whole bug: the
.linkclass has zero CSS rules in the shipped stylesheet.web/src/ui.cssis the only bundled sheet (index.jsx imports only./ui.css; the repo's own header comment at ui.css:39 notesweb/css/*is dead/unbundled). ui.css defines.btn,.pill,.card,.err-line, etc. in its@layer components, but grepping the entireweb/tree for a.link {rule returns nothing. Under Tailwind v4 preflight, a classless button is reset to plain text: no background, no border, inherited color,cursor: default. So the Cancel renders as the lowercase word "cancel" in the same size and muted zinc-500 color as the caption it sits in — visually part of the sentence "commenting on a/path @12 cancel". Users cannot tell it is clickable, so the composer reads as impossible to dismiss. (The click handler itself is sound; this is an affordance defect, not a logic defect.)Adjacent: there is no Escape dismissal anywhere on these composers — no
onKeyDownon the composer panel, no document-level listener. The repo's established popover/dismissal family (SplitCloseMenu inCommentComposer.jsx:38-43, RefPicker/ReactionMenu patterns) closes on Escape and refocuses the trigger; the inline composers are the outliers.Architecture notes
${hunkIdx}:${rowIdx},getDrafts/closeDraftat Pull.jsx:355-367); dismissal must delete only that key — sibling open composers stay untouched with their text preserved. That behavior already exists; the fix is affordance + keyboard, not state rework.getStaged/setStaged(null), PullFiles.jsx:36, 84); dismissal clears it without posting and without touchinggetCreated.props.onClose(issue Close/Reopen) inCommentComposer.jsx— do not conflate. The composer component itself needs no change for the conversation page (the caption lives in the consumer); if the control is centralized into CommentComposer, keep the dismissal callsite-provided.<button class="link">/<A class="link">is used across ~10 pages (Pull.jsx:181, 280, 585, 589, 732; Checks.jsx:151, 205; Team.jsx:57; Settings.jsx:1021, 1310-1314; PullFiles.jsx:84, 105).<A class="link">accidentally looks acceptable because basea { color: var(--accent) }styling applies, but every button-shaped.linkrenders as bare text. Suggest a follow-up ticket for a shared.linkcomponent rule + page sweep once this lands.Fix prescription
.linkcomponent rule to ui.css@layer components(accent color,cursor-pointer,hover:underline) — which fixes the systemic class incidentally — or (b) give the composer Cancel a small secondary.btntreatment consistent with the row's caption. Pick one; (b) is stronger affordance, (a) is the smaller diff. Either way the control must be distinguishable from its caption in both themes.onKeyDownEscape on the composer panel (and/or textarea) dismisses:closeDraft(key)on the conversation page,setStaged(null)on Files tab. Follow the SplitCloseMenu convention (CommentComposer.jsx:38-43): Escape closes and refocuses the trigger (the gutter "+" that staged the draft), listener cleanup inonCleanupif a document listener is used. Mind the toggle-fight trap: dismissal must not re-trigger staging.onStage, no thread create), and leaves other open drafts and the staged selection's diff row intact.Acceptance criteria
.linkclass sweep across other pages.Fixed by #569 (merged): Cancel takes the secondary .btn treatment on both composers (visible both themes, no new CSS); Escape dismisses + refocuses the staging trigger on both surfaces. Dismissal drops only that draft, stages nothing. Follow-up .link sweep tracked separately as #568. Verified: 1352 unit tests green (smoke excluded, pre-existing), vite/esbuild green, independent review APPROVE.