Add the pencil case: physical review tools for contributions - #11
Conversation
A capsule tray floats at the bottom of a contribution page with three tools, modelled on the way highlighte.rs treats its pens as objects: they stand in the tray with their tips showing, rise on hover, lift out when picked, and a soft outline travels between them on a spring. Ink crossfades. One quint curve everywhere; a linear() spring only for things that arrive. - Highlighter: drag over prose to lay a real highlighter mark (@highlighters/core, MIT) and get a "Quote in discussion" chip; quoted comments render with the same mark - Highlighter/Note on the diff: Pierre line selection tinted with the ink, a sticky note under the last line, pinned as a comment anchored to the checkpoint and file with the L2-L4 reference - Stamp: LGTM / Needs work / Question / Nice land with a spring and a rubber-stamp ink texture; the verdict posts as a comment the discussion renders as a stamp again - Ruled paper under the discussion; opt-in synthesised squeak and thunk - Toolbar/radio semantics, Escape puts the tool down, reduced motion honoured - No protocol changes: every action is an ordinary comment - docs/design/review-tools.md writes the interaction language down as rules
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c533fe6779
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onPost={(text) => { | ||
| review?.onNote(text, file.name, reference); | ||
| setPending(null); | ||
| }} |
There was a problem hiding this comment.
When posting is rejected or times out, review.onNote starts the asynchronous command but this immediately clears pending, unmounting the note and discarding everything the reviewer typed. This can also happen silently when another command is already in flight and useCommand.send returns false; make onNote return the send result and only clear the note after a successful submission.
Useful? React with 👍 / 👎.
| side: "additions", | ||
| lineNumber: pending.end, | ||
| metadata: pending, |
There was a problem hiding this comment.
When a reviewer selects removed lines, the selection state discards the side and the annotation is always attached to additions. In a deletion-only file there is no matching addition line, so the sticky note does not render; in mixed hunks it can render beside an unrelated new-side line with the same number. Preserve the range side and use it for both the controlled selection and annotation.
Useful? React with 👍 / 👎.
| if ( | ||
| (event.target as HTMLElement).closest( | ||
| "button, a, input, textarea, select", | ||
| ) |
There was a problem hiding this comment.
With the stamp selected, clicking an interactive element not in this selector posts a permanent verdict comment. For example, the tool-call disclosures in Transcript.tsx use <summary>, so expanding one also triggers dropStamp; interactions inside the file-tree shadow DOM can likewise be retargeted to its host and bypass this check. Detect interactive elements through the composed event path or explicitly stop stamping from these controls.
Useful? React with 👍 / 👎.
| setComment( | ||
| `${comment.trim() ? `${comment.trim()}\n\n` : ""}${quoted}\n\n— quoting ${quote.author}`, | ||
| ); |
There was a problem hiding this comment.
Selecting more than roughly 8,000 characters from an agent response sets an oversized controlled value directly into the discussion composer, and the attribution can push an otherwise valid selection over the limit as well. The textarea's maxLength only limits user editing and does not truncate this programmatic value, while the server rejects comment bodies over 8,000 characters, so the generated quote cannot be posted without manual trimming.
Useful? React with 👍 / 👎.
| const match = /^\*\*([^*]+)\*\*/.exec(body.trim()); | ||
| if (!match) return null; | ||
| return STAMPS.find((s) => s.label === match[1]) ?? null; |
There was a problem hiding this comment.
Any existing or manually posted comment beginning with **LGTM**, **Question**, **Nice**, or **Needs work** is now classified as a tool-generated stamp. Common prose such as **Question**: see [the docs](...) consequently loses its normal Markdown rendering, and the remainder is emitted as plain text. Require an unambiguous complete stamp format or stored marker instead of recognizing every matching bold prefix.
Useful? React with 👍 / 👎.
| {writable && !offline && exchange.status !== "closed" && ( | ||
| <PencilCase review={review} /> | ||
| )} |
There was a problem hiding this comment.
If a contribution becomes closed while a stamp or highlighter is selected, this condition unmounts the toolbar without clearing review.tool. The panel handlers remain active, and disabled does not include the closed status, so subsequent clicks can keep posting stamps while no selected tool is visible; comments on closed exchanges are accepted by the host. Clear the active tool when the tray becomes unavailable or guard the handlers with the same visibility conditions.
Useful? React with 👍 / 👎.
| ? "Changes" | ||
| : "Discussion" | ||
| } | ||
| onPointerUp={(event) => paintSelection(event.currentTarget)} |
There was a problem hiding this comment.
The highlighter is invoked only from pointerup, so selecting prose with Shift+Arrow and completing the selection from the keyboard never calls paintSelection; no mark or quote action appears despite the toolbar itself being keyboard operable. Add an appropriate keyboard or selection-change path so the feature is usable without a pointing device.
Useful? React with 👍 / 👎.
| @media (max-width: 780px) { | ||
| :root { | ||
| --sidebar-w: 68px; | ||
| } | ||
| .pencil-case { | ||
| gap: 10px; | ||
| padding: 0 12px 0 14px; |
There was a problem hiding this comment.
At this mobile breakpoint the dock only receives smaller gaps and padding, while stamp mode still renders all four labeled chips alongside the three tools. The resulting fixed toolbar is roughly 440 pixels wide, so on common 320–390 pixel viewports it extends beyond the screen and the rightmost stamp choices cannot be reached because the fixed element provides no horizontal scrolling or wrapping.
Useful? React with 👍 / 👎.
| <StampLayer | ||
| stamps={landed.filter((stamp) => stamp.where === tab)} | ||
| /> |
There was a problem hiding this comment.
Landed stamps are filtered only by tab, so if the agent publishes a new checkpoint while the reviewer remains on the contribution, stamps placed on the old diff are immediately drawn at the same coordinates over the new diff. The corresponding durable comment remains anchored to the old commit, but the visible overlay can now look like an LGTM or Needs work verdict on unrelated new content. Include the checkpoint in Landed or clear changes-tab stamps when the checkpoint changes.
Useful? React with 👍 / 👎.
| to { | ||
| transform: none; | ||
| opacity: 1; |
There was a problem hiding this comment.
Because pc-pop uses animation-fill-mode: both, this final transform: none continues overriding each component's base transform after the animation finishes. In particular, the quote chip loses its inline translateX(-50%), leaving its left edge—not its center—at the selection midpoint and potentially clipping most of the control at the panel's right edge; sticky notes also lose their intended rotation. Animate a nested element or retain the component's base transform in the keyframes.
Useful? React with 👍 / 👎.
1 participant
What
The pencil case: physical review tools, modelled on how highlighte.rs treats its pens as objects. A capsule tray floats at the bottom of a contribution page; tools stand in it with their tips showing, rise on hover, lift out when picked, and a soft outline travels between them on a spring. Ink crossfades. One quint curve for state changes; a
linear()spring only for things that arrive.@highlighters/core, MIT) and get a Quote in discussion chip; quoted comments render with the same markL2–L4reference_axp/commentLook at
docs/design/review-tools.md— the interaction language written down as seven rulesui/src/review/npm run demo:ui→ open a contribution; the tray appears for anyone who can postVerification
npm run checkandnpm run test:uipass.Landing
Adjacent-line conflicts with siblings in
THIRD_PARTY_NOTICES.md/scripts/ui-notices.mjsonly.