Dropper v2 - #13399
Conversation
POST /works/OL…W/ratings.json and POST /people/…/lists.json both answered 405 on the web.py port. Ratings had no entry in DEPRECATED_PATHS at all; the lists entries were there but never took effect, because infogami keys its `pages` registry by the literal regex text, so an equivalent pattern spelled differently registers alongside the old GET-only handler instead of replacing it. Collapsing the five per-seed-type patterns into the one string lists.py actually registers covers all of them — works, books, authors and subjects were shadowed the same way people was. The editions read in the request context also has to declare _method="GET". Without it web.py parses multipart POST bodies there, draining wsgi.input before the handler sees it, which surfaces as an empty form — "Invalid bookshelf" on a shelf post that carried one. This is dev-only routing, but it takes the sitewide star-rating form and the list dropper's "create list" with it, not just the components that follow. Claude-Session: https://claude.ai/code/session_01CCv12GjjKDZZGeTGQb6GHg
The book components render whatever shelf and rating they are handed, so a server-rendered page sets them directly. Anything that renders books on the client — a carousel, a search result list — has to ask, and asking once for the whole batch is what keeps the public payload cacheable and user-agnostic. Ratings had no batch query, only per-work reads; Bookshelves had one but its signature disagreed with the numeric work ids every caller actually passes. Claude-Session: https://claude.ai/code/session_01CCv12GjjKDZZGeTGQb6GHg
Two components that let a reader shelve, rate and list a book from wherever the book appears. ol-shelf-button is the control — a split button for a row, a bookmark for a cover — and ol-book-actions is the popover both shapes open: the four reading-log shelves, a star rating, and an "Add to list" pane with a filter and inline list creation. State is optimistic. The UI updates first and an error toast rolls it back, and neither component writes its own shelf: they emit ol-book-state-change and the surface that owns the book applies it, which then flows back down. That is what keeps two cards for the same work in step, and it makes an optimistic update and its rollback the same code path. Signed out, the trigger stands alone and its click goes to login with the intent remembered — without user-key no popover is built at all. ol-icon gains a `filled` attribute along the way, for the on/off states these need: a saved bookmark, a rated star. Claude-Session: https://claude.ai/code/session_01CCv12GjjKDZZGeTGQb6GHg
The domain layer gets its own group: everything in it knows what a book is, while the generic pieces it composes — Button, Popover — stay where they are. Both demos write to the signed-in reader's real reading log and lists, so the gallery hands them the reader's key; empty sends the demo to log in instead. Claude-Session: https://claude.ai/code/session_01CCv12GjjKDZZGeTGQb6GHg
The surface the icon-shaped shelf button was built to sit on: a cover at a fixed 2:3 ratio with an `overlay` slot that owns its top-right corner, so a consumer never positions the control itself and clicking it does not follow the cover's link. With no artwork it draws a title/author panel rather than leaving a hole, so a shelf of covers keeps its rhythm. With `tooltip` a pointer gets the title, year and author over the cover, for layouts with no room to print them underneath. Claude-Session: https://claude.ai/code/session_01CCv12GjjKDZZGeTGQb6GHg
700 on a generated cover was heavier than any real jacket; 500 reads as a title without shouting. The padding drops a step at both sizes — a step further at small, where a 72px cover was giving up a quarter of its width to inset — and the small title drops to 12px, below where the title scale bottoms out. `size` reflects now, so the small-size styles apply when it is set as a property and not only when it is authored as an attribute. Claude-Session: https://claude.ai/code/session_01CCv12GjjKDZZGeTGQb6GHg
Both components were overriding ol-popover's own bottom-center default with bottom-end. Right-aligning a 300px panel to a narrow trigger throws almost all of it to one side, so it reads as unattached — centering keeps the panel under the thing that opened it. Nothing is at risk near a viewport edge: the positioner clamps horizontally either way, and transform-origin already tracks the trigger's centre, so the open still radiates from the button. `placement` is still there for a caller that needs an edge alignment. Claude-Session: https://claude.ai/code/session_01CCv12GjjKDZZGeTGQb6GHg
A popover panel is nearly always wider than the control that opens it, so centering makes it straddle the trigger and edge-aligning to the far side throws it clear across. Leading edges together is the alignment that reads as attached, and it was already what every menu on the site asked for by hand: ol-menu-popover, ol-select-popover and ol-options-popover each spelled out bottom-start, and those overrides can go. The book components stop naming a placement at all — the pass-through only sets the attribute when a caller supplies one, so an unset `placement` is ol-popover's choice rather than a copy of it. The one surface that genuinely wants centering, the masthead's Browse menu, already says so explicitly, so nothing changes for it. Claude-Session: https://claude.ai/code/session_01CCv12GjjKDZZGeTGQb6GHg
The book-cover overlay example never passed user-key, so its shelf button rendered permanently signed-out and every click went to the login page instead of opening the actions popover. It joins the shelf-button demos on the same work, so the shared listener there keeps all three in step. The example gets an explicit code snippet so the copyable markup shows /people/you rather than the reader's own key.
"On a shelf" hardcoded shelf 1, 2 and 3 on three placeholder works to illustrate the three states, but the buttons were live: clicking one wrote to the reader's real account while the shelf it displayed stayed whatever the attribute said. A demo that shows one thing and does another is worse than no demo, and the note admitting it was longer than the example earned. The two remaining demos open on the reader's real state for The Secret Garden, so the shelved appearance is already on show — and honestly. Claude-Session: https://claude.ai/code/session_01XJ9QNXNg3UfbtH3dmDxUfV
Found while putting these on a real surface; they belong with the components rather than with the first page to use them. bookshelves.json answers a rejected write with 200 and an `error` key, so checking the response status alone let a failed write look like a save — the optimistic update stayed on screen and the reader believed it stuck. Signed out, the button sent the reader to the book's own page after logging in. On a book page that is where they already were, but from a list of results it strands them somewhere they never asked to go. It now resumes on the page they were on, as the reading-log dropper does. Matomo's click trigger matches `data-ol-link-track` attributes, which it cannot see inside a shadow root — events retarget to the host on the way out. So shelf, rating and list actions now push to `_paq` directly, under the same category/action names the dropper's forms use, and the existing dashboards stay continuous across a migration. Adds `hide-rating`, which drops the popover's stars for surfaces that already show a rating control for the same book. The reading log is the case that needs it: its `macros.StarRatings` is a real input, not the aggregate byline that search results carries. Claude-Session: https://claude.ai/code/session_01XJ9QNXNg3UfbtH3dmDxUfV
Marking a book Already Read now slides in a pane asking for a finish date, the way "Add to list" already slides in. Answering was previously a separate prompt rendered outside the popover, which opened a modal — two overlay paradigms for one question. The check-ins schema stores partial dates: a year alone, or a year and month, are both valid. That is what makes "In 2026" offerable as one tap rather than a date picker. "Other date" swaps in year/month/day selects, each enabling the next, in the same pane rather than a fourth one — the lists pane swaps in its create form the same way. Month names come from Intl in the page's language, so they cost no translated strings. Only an explicit shelf choice opens it. Rating a book moves it to Already Read server-side too, and interrupting that would turn one tap into two. Nor does it open when the reader is taking a book *off* the shelf. The pane track was hardcoded to two panes — 200% wide, halves, a class that translated by -50%. Width and slide now both derive from the PANES list, so a third pane is an entry there plus a `_render*`. Escape in any sub-pane goes back one step instead of only in the lists pane. Not yet handled: showing or editing a date that already exists. The finish date is write-once from here until we decide how the row surfaces it — relabelling the main button "Read in 2026" is one idea. Claude-Session: https://claude.ai/code/session_01XJ9QNXNg3UfbtH3dmDxUfV
The tooltip opt-in on ol-book-cover is gone — every cover renders the hover card, with ol-tooltip's pointer media query still deciding when it arms. Also drops the implementation-detail notes from the shelf-button and book-actions gallery pages. Claude-Session: https://claude.ai/code/session_01RtTEd3rAHJYz3zKi3zUWwD
…ve#13400 Remove from shelf row, read-date display and amending (readDate/eventId through to setCheckIn's event_id), quickYears, the Other-date disclosure, the ol-list-created bridge and ol-book-check-in event, press feedback, fixed row heights, 16px list checkboxes, per-option select seeding, and the accent-color token fix — taken from the search-results branch so this PR carries the component work. Claude-Session: https://claude.ai/code/session_01RtTEd3rAHJYz3zKi3zUWwD
testing.openlibrary.org only routes already-launched paths to its FastAPI container, so the new /reading-state.json (and the bookshelves route its nginx is missing) 500 with the web.py deprecation error there. The /_fast/ prefix exposes the whole FastAPI app on testing, so prefix every request with it on that host until the olsystem nginx routes land. Production has no /_fast and is unaffected. Claude-Session: https://claude.ai/code/session_01D5yRkR7WnUVDXxjc9Gv6Tz
Only the design-system demos called the endpoint: search results pass the state as server-rendered attributes, and the client-rendered surfaces that genuinely need a batch endpoint (carousels) are not in this PR — it can land with them instead, alongside its olsystem routing. The demos now get shelf/rating the same way real surfaces do, from design.py at render time. Also reverts the /_fast testing shim; the testing nginx routing is being fixed properly instead. Claude-Session: https://claude.ai/code/session_01D5yRkR7WnUVDXxjc9Gv6Tz
The Lit bundle can import the webpack modules directly, so the copied analytics and pending-action-cookie helpers go away, the lists partial URL regains its lang handling via buildPartialsUrl, and the shelf label/event vocabulary now lives once in books-api. The popover's three panes render on first click instead of at page load, which was building thousands of nodes on book-heavy pages for popovers never opened. Also drops the fictional legacy-dropper list-created protocol, a fake fmt compatibility re-export, and shares the lists route regex between lists.py and deprecated_handler.py instead of a keep-in-sync comment. Claude-Session: https://claude.ai/code/session_01RtTEd3rAHJYz3zKi3zUWwD
lokesh
added
Needs: Review
lokesh
changed the title
RayBB
left a comment
RayBB
left a comment
There was a problem hiding this comment.
Looks beautiful but have some first draft code changes here.
Here is some feedback I think we should really consider. It's just the first round based on high level things.
1. The date helpers do not belong in OlBookActions.js
The file has date functions inside it (MONTHS(), formatReadDate(), quickYears(), partialDate()).
We checked the client code. There is no shared date utility yet — these are the first. But the component also does not own this idea. Dates are not part of a book-actions popover; other components will need them next.
Suggestion: Give the date helpers a shared home beside the other shared modules (lit/utils/books-api.js, lit/utils/labels.js). Then the component keeps only its panes, and the date logic is there for whatever comes next.
2. Four copies of the same write-then-undo pattern
Four places repeat the same flow: change the UI, send the request, on success record the event, on failure undo the change.
OlShelfButton._onMainClickOlBookActions._postShelfOlBookActions._onRateOlBookActions._onListToggle
Problem: Four copies of one idea will drift apart over time.
Suggestion: We think there is a better shape here than four copies. Please look at these four flows together before merge and see if they can share something.
3. The lists cache state looks tangled
The list data is held in more than one place: a module-level promise, a copy on each component instance, and manual sync code after creates.
Problem: When one piece of data lives in several places, they can stop agreeing, and readers must trace all the sync code to trust it.
Suggestion: This looks like it should be improved before more surfaces build on it. We do not have one answer in mind — please consider how to keep fewer copies of this state.
The row rendered the date but nothing ever supplied one: the popover did not keep its own copy after a save, and the design demo never server-rendered it. Now the component updates readDate/eventId optimistically like shelf and rating — which also makes a re-save amend the event instead of recording a second finish — and the demo passes the latest finish event down. Claude-Session: https://claude.ai/code/session_01RtTEd3rAHJYz3zKi3zUWwD
Review feedback (internetarchive#13399): the date helpers belonged beside the other shared utils, and the lists cache lived in three places (a module-level promise, per-instance copies, and event-based syncing) that could drift. lit/utils/dates.js takes MONTHS, formatReadDate, quickYears and partialDate unchanged. lit/utils/lists-store.js holds the one canonical lists object; components read getLists() and re-render on subscribe, and the store owns each optimistic update and its rollback. That retires the per-instance copies, the document-level ol-list-created listener, and the promise-swap after a create. The event still fires outward as documented API. Claude-Session: https://claude.ai/code/session_01RtTEd3rAHJYz3zKi3zUWwD
|
@RayBB Thanks for the review. 1 and 3 are addressed in 1a1dd17; here's where each landed. 1. Date helpers — Moved to 3. Lists state — There's now one canonical copy in 2. The write-then-undo copies — Looked at all four together and decided to leave them for now, for a couple of reasons. The store refactor already absorbed one: the list-toggle undo now lives in Ready for another review. |
RayBB
left a comment
RayBB
left a comment
There was a problem hiding this comment.
Definitely a lot to review but I'm working through and reading it. Trying to give you feedback as much as I can now now instead of waiting until I fully review.
There are two things here that I think are more up to your judgement but are overall seem like a good idea to me.
Split out into focused components
This component has grown into a small application rather than a UI component. Split into focused components:
- OlBookActions (coordinator, ~300 lines) — owns pane switching and event delegation
- OlShelfPicker (shelves UI, ~200 lines)
- OlRatingPicker (stars UI, ~150 lines)
- OlListsPane (lists browser, ~300 lines)
- OlCheckInPane (date picker, ~250 lines)
Each pane should be a self-contained component with clear props/events.
I understand these things may not be likely to be used anywhere else but I think if we can do this it would make it a lot easier to review/work on each individual part rather than having it all in one huge file. However, if this add more complexity/boilerplate than you deem worth it then I wouldn't say it's a blocker.
Extract Optimistic Update Pattern into Reusable Mixin
The optimistic update pattern is repeated inconsistently across OlBookActions.js and OlShelfButton.js, with each mutation handler implementing its own rollback logic. This is fragile:
_onRate()rolls back 2 properties, easy to miss one- Manual rollback order matters and is error-prone
- Error handling duplicated but inconsistent
- No prevention of concurrent mutation race conditions
Extract this into a reusable Lit mixin following the existing pattern used by FormAssociatedMixin and FocusableHostMixin. The mixin should provide a _performOptimisticMutation() method that:
- Captures state snapshot for guaranteed consistent rollback
- Orchestrates: optimistic apply → server action → rollback on error
- Prevents concurrent mutations
- Centralizes error handling (401 → login, others → toast)
Refactor _postShelf(), _onRate(), _saveCheckIn(), _onCreateSubmit(), and OlShelfButton._onMainClick() to use it.
This eliminates a class of bugs where developers forget to rollback properties or get the order wrong. Future mutations will use a tested pattern instead of hand-rolling fragile error handling.
Again, I think this would be very nice to have, especially because it's a pattern we should be using quite often, but I'll leave it up to you what's best for it.
|
Also seems we could: Use repeat() — OlBookActions.js:735, 861, 898, 984. Without repeat(), Lit reconciles by index. Filtering Add to list (OlBookActions.js:982 filter) or toggling shelf recreates DOM nodes, loses input:focus/checked state and wastes work. repeat(items, keyFn, tplFn) keys by stable id. |
A rating moves two properties, so restoring them by hand is a place to forget one. _mutate() captures whatever it is about to change and puts all of it back on failure, and refuses to start while another write is in flight — the disabled rows only take effect on the next render, so two fast clicks could otherwise both go out. The shelf button owns no state to snapshot, but it had the same race: guard its main click too. Claude-Session: https://claude.ai/code/session_011AqgrMKn29JVncut49JNtf
|
@RayBB Both good calls. Optimistic updates: adopted the pattern, skipped the mixin. Rollback now works off a state snapshot instead of hand-restoring properties one at a time, so a handler that moves two things can't put only one of them back, and a write can't start while another is in flight. That second part was a real race, not just tidiness. Kept it local to the component rather than a shared mixin, because only 2 of the 5 call sites are actually this pattern. The others apply state after the response, get rollback from the lists store, or hold no state at all: Splitting the component: I'd rather not. The line count is mostly styles, and the panes aren't independent parts. They're one track: a shared layout that derives its width and offset from the pane list, measures the active pane to animate the panel height, and moves focus between panes inside a single shadow root. Splitting trades one file for five shadow roots, duplicated styles, prop plumbing, and focus management across shadow boundaries, which has been the fiddliest area in this component set. Tests came with both fixes, each checked to fail without them. |
…-book-actions # Conflicts: # openlibrary/plugins/openlibrary/deprecated_handler.py
2 participants
new-dropper.mp4
ol-shelf-buttonshelves a book in a click and opensol-book-actionsfor the rest: the four shelves, a star rating, and an "Add to list" pane with a filter and inline list creation. Two shapes — a split button for a row, a bookmark for the corner of cover art — so the same component drops into a book row, a search result or a carousel card instead of being rebuilt for each.ol-book-coverprovides that corner. Signed out, the trigger goes to login and the patron lands back where they were.Gallery-only for now, in a new Books group at
/developers/design?section=components. Carousels and search results come next.Technical
The components call only endpoints FastAPI already serves:
bookshelves.json,ratings.json,check-ins, theMyBooksDropperListspartial, and the list seed/create writes. No new endpoints in this PR.The reading-log writes now reach FastAPI from the web.py port in dev — ratings had no
DEPRECATED_PATHSentry, and the lists entries never took effect because infogami keyspagesby the literal regex text. Related: the editions read inrequest_context.pyneeded_method="GET", without which web.py drains a multipart POST body before the handler sees it. Both fix the sitewide rating form and the list dropper too.Why this is low risk
Ratings.get_users_ratings_of_works, twin of the existing bookshelves one), a type-hint widening, the design-page demos, and the routing-table lines below.DEPRECATED_PATHSchanges only touch paths nginx already routes to FastAPI. Verified against live traffic:POST /works/…/ratings.json,POST …/bookshelves.jsonandGET …/lists.jsonon openlibrary.org all answerx-served-by: FastAPItoday, so web.py never sees these requests in production. The entries are the safety net — a misrouted environment fails loudly instead of silently using the legacy handler, which is the mechanism's purpose.request_context.pyis one line, scoped to the solr-editions query read. It stops web.py from parsing multipart POST bodies during that read; nothing passeseditionsin a body./developers/designrenders differently. The new components ship in the site-wide bundle but nothing instantiates them elsewhere yet. The touched shared components are behavior-preserving: thebottom-startpopover default matches what every existing consumer already set explicitly (each one checked), and theol-iconfilledprop is additive.Testing
Sign in and open
/developers/design?section=components→ Books. The demos write to your real reading log: shelve the book from either shape, rate it, add it to a list, reload. Both demos are the same work, so one updates the other. Signed out, a trigger goes to login.Screenshot
See above
Stakeholders
@cdrini @mekarpeles