Library: tag hints, a description that is read, and fifty objects a shelf - #215
Library: tag hints, a description that is read, and fifty objects a shelf#215manager wants to merge 7 commits into
Conversation
A tag on no book stood in the panel with a text caret over it, offering a selection its label never answers and no reason for the click it refuses. It now carries the pointer of a label and says Tag not used on hover, through the shared Tooltip every other disabled control explains itself with. A clipped name keeps its word in that hint. In the object form's menu the pill follows the row it stands in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @manager's task in 2m 18s —— View job Claude finished reviewing this PR
SummarySmall, focused change: an unused tag ( One issue posted inline ( Everything else checks out:
I was not able to run |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ? (label ?? '') | ||
| : ''; | ||
|
|
||
| return tooltipContent ? ( |
There was a problem hiding this comment.
Accessibility: the "Tag not used" hint is mouse-only.
When hint is set without onClick (the unused-tag case in Sidebar.tsx:527), role/tabIndex/aria-pressed on the pill (lines 35-39, unchanged by this diff) all fall through to undefined, so the pill is never a tab stop. react-tooltip triggers on hover and focus by default, but with no tabIndex the element can never receive keyboard focus in the first place — keyboard users and screen-reader users get no way to learn why an unused tag doesn't respond. Only mouse users see the tooltip.
This also contradicts the hint doc comment in Tag.types.ts:12-13 ("the same way every other disabled control in the Library explains itself") — the actual convention elsewhere (Dropdown.tsx:233-236, TagMultiSelect.tsx:128-131) keeps the element focusable and adds aria-disabled/role even when the action is unavailable, rather than dropping focusability entirely.
Suggest keeping the pill in the tab order and marking it disabled when a hint exists without a click, e.g.:
const isDisabledHint = !!hint && !onClick;
...
role={onClick || isDisabledHint ? 'button' : undefined}
tabIndex={onClick || isDisabledHint ? 0 : undefined}
aria-disabled={isDisabledHint || undefined}so the tooltip (and the reason for the dead click) is reachable via keyboard, not just :hover.
The hint was hover only: with no click to give, the pill left the tab order, so a reader on the keyboard met a tag that ignored them and no sentence saying why. It keeps its tab stop as a control that is off, marked aria-disabled, and answers focus with the same ring and the same words the pointer gets. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @manager's task in 1m 16s —— View job Claude finished reviewing this PR
SummaryThis push ( The fix is correct and matches the suggested approach:
Also good:
No new issues found — this is a clean, scoped fix. Nothing further to flag. I was not able to run |
The tag form promised it in its own placeholder and nothing ever showed it: a description was saved and then only ever seen again in the form that wrote it. The pill now says it on hover, under the tag's own name when the pill is too narrow to hold the word, and above the note that no book carries it. The sentence is capped at 180 characters, counted under the field as it is typed. The form used to allow 500 while the CMS refused past 150, so a longer description was written, sent and lost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
manager
changed the title
|
Claude finished @manager's task in 2m 48s —— View job Reviewing PR #215
SummaryThis push adds the tag-description hover hint on top of the already-reviewed "Tag not used" a11y fix. The keyboard-accessibility issue flagged in the earlier review round ( One issue posted inline ( Everything else checks out:
Minor (non-blocking) nit: I was not able to run |
|
|
||
| // Mirrors the `description` cap in the CMS tag schema. Shared so the counter | ||
| // in the form and the validator behind it cannot drift apart. | ||
| export const MAX_TAG_DESCRIPTION_LENGTH = 180; |
There was a problem hiding this comment.
Possible mismatch with the actual CMS limit.
This PR's own CLAUDE.md update says the reason for this cap is that "the form used to allow 500 where the CMS refused past 150" — i.e. the backend (Strapi) rejects tag descriptions longer than 150 characters. But MAX_TAG_DESCRIPTION_LENGTH here is set to 180, and that same constant drives the client-side maxLength, the CharCount display, and the createTagSchema Zod validator (src/utils/library/schema/createTagSchema.ts:20-26).
If the CMS-side validation is still 150 (no CMS schema change is included in this PR's changed files, and no CMS commit/deployment is referenced the way other CMS-contract changes in CLAUDE.md are — e.g. the AI Shelf preference section cites a specific keepsimple-cms-new commit), then descriptions of 151-180 characters will pass client validation and still be rejected by the backend with a raw Strapi error — reintroducing the exact mismatch this change is meant to fix, just with a narrower gap (30 chars instead of 350).
Worth confirming: has the CMS-side cap actually been raised to 180? If yes, please cite that (commit/deployment) the way other CMS-touching entries in CLAUDE.md do. If no, this constant should be 150, not 180.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The 180 is real on the CMS side, not assumed: keepsimple-cms-new #409 (merge ad1cb40) raises the schema cap from 150 to 180, and it is on the staging CMS since the image built 2026-09-09T10:30Z. Proved against the live staging API as the library owner, not by CI: a 180-character description saved and read back at 180, a 181-character one refused with |
|
Claude finished @manager's task in 50s —— View job Reviewing PR #215 (new commits pushed)
SummaryThe only change since the last review round ( This directly resolves the one open item from the previous review:
That's the same evidence-citation pattern the AI Shelf preference section already uses elsewhere in Everything from earlier rounds still holds (unchanged since the last full review):
No new issues found. This PR is clean from a code-review standpoint; the one open question (CMS-side cap parity) is now closed with cited deployment evidence per this repo's release-evidence gates. I was not able to run |
Thirty was the figure the shelves were built against and the owners filling them ran out of room, so it is fifty. One number on each side: the CMS refuses the fifty-first, the Add control goes quiet at fifty, and the full-shelf copy still names the same rule. The backend message is matched on its shape, not on its number, so the two never disagree while a rollout is halfway through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
manager
changed the title
|
Claude finished @manager's task in 1m 27s —— View job Reviewing PR #215 (new commits pushed)
SummaryThe tag-hint/description work ( New since then: The change itself is correct and consistent:
One anomaly worth surfacing, not a PR-diff issue: the local checkout has an uncommitted working-tree change to No blocking issues found in the new commit. I was not able to run |
The palette could be created, renamed and filtered by, but the only place that put a tag on a book was step 2 of the edit form. The object overview now carries the picker in its own action row: one icon button opening the library's tags as a multi-select, each click saved on its own, the Tags row under the notes answering it and the panel's counts re-read after it. An edit now states the whole set on save, the empty set included, so taking the last tag off a book takes it off instead of reading as no change. Both pickers read one cap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @manager's task in 3m 8s —— View job Reviewing PR #215 (new commits pushed)
SummaryNew since the last review round ( Two issues posted inline, both in
One minor nit ( Everything else checks out:
I was not able to run |
| {tagEntries.map(({ item: tag, leaving }) => ( | ||
| <span | ||
| key={tag.id} | ||
| data-flip-id={String(tag.id)} | ||
| className={classNames(styles.tagSlot, { | ||
| [styles.tagLeaving]: leaving, | ||
| })} | ||
| > | ||
| <Tag label={tag.name} color={tag.color} /> | ||
| </span> | ||
| ))} |
There was a problem hiding this comment.
Removed tag pills never actually leave — missing data-flip-leaving.
Every other useAnimatedList consumer in this codebase (Sidebar.tsx:515, Shelf.tsx:164, LibraryToolbar.tsx:232, RecommendedShelf.tsx:375, Home.tsx:230, Library.tsx:1467, ShareSelectionPanel.tsx:416) sets data-flip-leaving={leaving ? 'true' : undefined} on the slot. This one doesn't — the <span> here only uses leaving to toggle the styles.tagLeaving CSS class, never to set the data-flip-leaving attribute.
useAnimatedList.ts:157 reads the state purely from the DOM attribute: const leaving = slot.dataset.flipLeaving === 'true';. Without that attribute, the if (leaving) { ... } branch (useAnimatedList.ts:159-184) never runs for a departing tag, which means:
- the fade/collapse-width exit animation never plays, and
finish()(which callssetDeparting(list => list.filter(d => d.key !== key))) is never called.
So once a tag is removed from an object, its pill stays in departing state forever — a ghost pill (rendered with pointer-events: none via .tagLeaving) that never gets cleaned up and accumulates with every further removal.
Fix: add data-flip-leaving={leaving ? 'true' : undefined} to the <span>, matching every other consumer of this hook.
| const flushTagSave = async () => { | ||
| if (tagSaveInFlight.current) return; | ||
| const next = pendingTags.current; | ||
| if (!next) return; | ||
| pendingTags.current = null; | ||
| tagSaveInFlight.current = true; | ||
| try { | ||
| const res = await updateObject(id, { tags: next.map(t => t.id) }); | ||
| savedTags.current = next; | ||
| const withRelations = preserveRelations(res.data); | ||
| onUpdated?.({ | ||
| ...withRelations, | ||
| attributes: { | ||
| ...withRelations.attributes, | ||
| // A PUT answers without the relation it just wrote, and a cleared | ||
| // set has nothing to carry forward, so the saved list is written in | ||
| // here for the card and the hover dossier to read. | ||
| tags: { | ||
| data: next.map(t => ({ | ||
| id: t.id, | ||
| attributes: { name: t.name, color: t.color }, | ||
| })), | ||
| }, | ||
| }, | ||
| }); | ||
| // The panel counts the books behind each tag and the gathered row is | ||
| // drawn from the same list, so both are re-read once the tag lands. | ||
| await refreshLibraryTags(); | ||
| } catch (e) { | ||
| console.error('[ObjectOverviewModal] tag save failed', e); | ||
| pendingTags.current = null; | ||
| setTags(savedTags.current); | ||
| setTagsError('Could not save these tags. Please try again.'); | ||
| } finally { | ||
| tagSaveInFlight.current = false; | ||
| if (pendingTags.current) void flushTagSave(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
A queued tag change is silently dropped if the in-flight save it's queued behind fails.
The comment at line 319-321 promises "a save in flight is never raced: the newest choice is queued behind it, so a run of quick clicks ends with the server holding exactly what is on screen." That holds on the success path, but not on failure:
- User adds tag A →
handleTagsChangesetstags=[A],pendingTags.current=[A], callsflushTagSave. flushTagSavecapturesnext=[A], clearspendingTags.current=null, setstagSaveInFlight=true, and awaitsupdateObject.- While that request is in flight, the user adds tag B →
handleTagsChangesetstags=[A,B]andpendingTags.current=[A,B](queued, sinceflushTagSavereturns immediately due totagSaveInFlight). - The
[A]request fails. Thecatchblock runspendingTags.current = null;— this discards the queued[A,B], thensetTags(savedTags.current)reverts the UI to whatever was saved before step 1.
The user's second click (B) is lost entirely: never sent to the server, and silently erased from the local UI on the very next render after the revert (since pendingTags.current is now null, finally's if (pendingTags.current) void flushTagSave(); does nothing). The user sees the picker snap back to the pre-A state with only the generic "Could not save these tags" error, with no indication their second click ever happened.
Consider preserving a still-current queued change across a failed save, e.g. only reset pendingTags.current in the catch block if it still matches next (nothing newer queued), otherwise let the newer pending value flow through to the next flushTagSave call in finally.
| // cap on one surface and refused on the other. | ||
| export const MAX_TAGS_PER_OBJECT = 10; | ||
|
|
||
| export const TAG_PER_OBJECT_LIMIT_MESSAGE = `Up to ${MAX_TAGS_PER_OBJECT} tags per item. Remove one to add another.`; |
There was a problem hiding this comment.
Minor: TAG_PER_OBJECT_LIMIT_MESSAGE is defined but never imported/used anywhere. TagMultiSelect.tsx:169 renders the identical text (`Up to {maxItems} tags per item. Remove one to add another.`) as an inline template literal instead of this constant, so the two can drift apart later even though this commit's own comment says it exists so "a book cannot be filled past the cap on one surface and refused on the other." Worth wiring TagMultiSelect.tsx to use this constant (or dropping it if the generic maxItems-based copy in TagMultiSelect is meant to stay independent of any one caller's wording).
1 participant
Everything queued for the next staging build, in one place.
Tags. An unused tag no longer stands there with a text caret: it carries a label pointer and says
Tag not used, on hover and on keyboard focus, keeping its tab stop as a control that is off. A tag description was written and never seen again, though the form promises it appears on hover; the pill now says it, under the tag name when the name is clipped. That sentence is capped at 180 characters at both ends, counted under the field. The form used to allow 500 where the CMS refused past 150, so anything in between was silently lost.Shelves. The per-shelf cap goes from 30 to 50, all types counted together. The full-shelf error is matched on its shape rather than its number, so the two sides cannot disagree mid-rollout.
CMS sides, both already live on the staging CMS and proved there against the live API: keepsimple-cms-new #409 (180 characters) and #410 (fifty objects, the 51st refused).
🤖 Generated with Claude Code