Skip to content

fix(library): modal close flash, rename of an empty library, toolbar spacing - #247

Merged
MaryWylde merged 3 commits into
devfrom
fix/library-modal-close-and-rename
Sep 28, 2026
Merged

MaryWylde merged 3 commits into
devfrom
fix/library-modal-close-and-rename

Conversation

@MaryWylde

Copy link
Copy Markdown
Contributor

What

  • Modal close flash. Closing a book or video overview on a shelf faded it out, drew it again for a moment, then closed it. The shared Modal switched back to open right after onClose, but the shelf closes the overview through a URL change that removes it only once the route settles. onClose may now return a promise, and the Modal stays faded out until it settles. Synchronous closes and guarded no-ops (delete in flight, a discard prompt) behave as before.
  • Rename of an empty library. A new, empty library has no row yet, so the URL slug matching the username is the only proof of ownership. A username-only save left the page on the old slug and it showed "No such library". The page now moves to /library/<new-username>, and the account takes the new name in the same render as the new address, so nothing flashes. Libraries that already have a row also move to the new address now, instead of staying on a slug that fails on reload.
  • Toolbar spacing. 10px between the search summary and the meter on desktop.

Checks

  • ESLint, Prettier and tsc are clean on the changed files.
  • Modal close: confirmed on DEV.
  • Rename: not tested in the browser (needs a fresh signed-in account). To check: fresh account, empty library, Edit library, change only the username, save. The URL should follow and the empty-library screen should stay.

🤖 Generated with Claude Code

MaryWylde and others added 3 commits September 28, 2026 11:14
The Modal reverted to open right after onClose, but the shelf closes the
overview by a route change that unmounts it only later, so the dialog was
drawn again for a beat. onClose may now return its promise and the Modal
stays faded out until it settles.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e new address

A username-only save left the page on the old slug; with no library row
yet the slug was the only proof of ownership, so it read as No such
library. The page now moves to the new address and the account takes the
new name in the same render.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…esktop

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Claude finished @MaryWylde's task in 4m 10s —— View job


Review complete

  • Gather context (PR diff, CLAUDE.md/AGENTS.md conventions)
  • Review Modal.tsx / Modal.types.ts (async onClose flash fix)
  • Review EditLibraryModal.tsx (rename redirect fix)
  • Review Shelf.tsx (integration with async onClose)
  • Review ObjectOverviewModal.tsx changes
  • Review LibraryToolbar.module.scss spacing change
  • Post inline comments for issues found
  • Post final summary

Summary

Solid, well-scoped fix — three independent bugs, each with a minimal targeted change. No AGENTS.md violations (import order, aliases, component/barrel structure, no App Router/Tailwind/CSS-in-JS creep).

Modal close flash (Modal.tsx/Modal.types.ts): letting onClose return a promise and gating the revert-to-open on its settlement is a clean fix. The cancelled flag correctly guards against setState after the effect's cleanup fires (whether from unmount or a changed onClose/isClosing dep), and the reduced-motion / sync paths are untouched. ObjectOverviewModal's guardedOnClose correctly returns undefined on both guard branches (edit-pending, delete-in-flight) so those still revert synchronously as before — only the real navigating close now returns the promise. Shelf.tsx propagating router.push's promise out of closeObject (and void-ing it at the two call sites that don't need to await) is exactly the right shape.

Rename of an empty library (EditLibraryModal.tsx): using beforeHistoryChange to swap accountData in lockstep with the URL update is a clever way to avoid a frame where the name and address disagree. One real issue flagged inline: if router.replace rejects, the catch swallows the error but the trailing swap() still runs unconditionally, so accountData picks up the new username even though the URL never moved — recreating the exact "No such library" mismatch this PR fixes, on the failure path. See the inline comment on EditLibraryModal.tsx for a suggested fix.

Also worth flagging (not a code defect, a process one): the PR description says the rename path is not tested in a browser. This is exactly the kind of timing-sensitive router-event mechanism that's easy to get subtly wrong in practice, and per this repo's Library release-gate rules a missing verification should stay flagged rather than implicitly passing. Recommend running the described manual check (fresh account, empty library, username-only edit) before this merges.

Toolbar spacing: the 769px/10px values match the existing breakpoint already used elsewhere in this same file and in Shelf.module.scss, so this isn't inventing a new value — no concern.

Comment on lines +210 to +222
if (onLibraryPage && freshUser.username) {
router.events.on('beforeHistoryChange', swap);
try {
await router.replace(libraryPath(freshUser.username), undefined, {
scroll: false,
});
} catch (error) {
console.error('EditLibraryModal could not follow the rename:', error);
} finally {
router.events.off('beforeHistoryChange', swap);
}
}
swap();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If router.replace rejects (e.g. a concurrent navigation cancels this one — the realistic case, since a failed getServerSideProps typically renders an error page rather than rejecting), the catch only logs and the unconditional swap() on line 222 still fires, so accountData.username moves to the new name while the URL is left on the old slug. That's the exact "name and address disagree" state this PR is fixing, just reachable again on the failure path.

Consider only swapping in the catch when you can confirm the address actually moved (e.g. check router.asPath against libraryPath(freshUser.username)), or surface an error instead of silently applying the rename to accountData.

Fix this →

@MaryWylde
MaryWylde merged commit 76ea39c into dev Sep 28, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant