Clean up, polish and modernise the retro board - #36
Merged
Conversation
Correctness and security: - Note updates and deletes went unauthorised: any authenticated user could edit or delete anyone's note, including notes belonging to a different retro. Content changes and deletes are now author-only while moving notes between columns and groups stays open, since that is the point of the group stage. - The websocket accepted any Origin. Sessions are cookie based, so this let any page a logged-in user visited drive their retros. - Client send channels were unbuffered and the hub wrote to them synchronously, so a single stalled client froze broadcasts for the whole retro. They are now buffered, with stalled clients dropped. - The 512 byte frame limit silently closed connections: a retro_update with a full title and ten tags already exceeded it. - note_created always obfuscated its content regardless of stage, so notes written after brainstorm read as noise until a refresh. - Optimistic updates all shared one "placeholder" id, so any failure wiped every in-flight create and failed edits were never rolled back. Mutations now carry a ref that the server echoes on both confirmation and failure. Identity: logging in inserted a fresh user row every time, so people lost ownership of their own notes on re-login. Names are the identity in a name-only auth model, so logins now resolve to a single row. The migration collapses existing duplicates onto the earliest row, repoints notes and votes, and adds a unique index. Consistency: column count limits now agree between API and UI, max_votes: 0 reports as a range error rather than "required", names may contain spaces and accents, retro tags load in one query instead of one per retro, Obfuscate can emit the last character of each class, and GIFs survive the markdown export. Tests: first coverage in the repo - the status machine, note authorisation, the websocket payload binder, the obfuscator, the markdown exporter, the user migration and the notes reducer. CI now runs build, vet, race tests, lint, typecheck and the UI build on every push and pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Google shut the Tenor API down on 2026-06-30, so GIF search had been failing on every request. Giphy no longer has a free tier either, so the gif package is now a small registry rather than one hard-wired vendor. - Klipy is the new default: free for life, and the closest thing to a drop-in Tenor replacement. Giphy ships alongside it for anyone holding a key. - THOUGHTS_GIF_PROVIDER / THOUGHTS_GIF_API_KEY replace THOUGHTS_TENOR_API_KEY, which now only logs a warning explaining the shutdown. - Providers gained trending and pagination, so the picker has something to show before anyone types and can keep loading as you scroll. - Outbound calls now have an 8s timeout; http.DefaultClient had none, so a hung provider could pin a request goroutine indefinitely. - GET /api/gifs replaces POST, since it is a read. Search is no longer required for the feature to work. The picker has a second tab where you paste any https image link and see a live preview before attaching it, so images work with zero configuration. Because those URLs are now genuinely user-supplied, img_url must be https and under 2048 characters. The picker itself was a bare form with nine results, no loading state and no error handling. It now debounces as you type, loads trending on open, reserves each tile's space so the grid does not reflow as images stream in, scrolls infinitely, and has real empty, error and loading states. Also fixes THOUGHTS_DATA in the Dockerfile: the config key is data_path, so that variable was never read and the database landed in the container's working directory rather than the mounted volume. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A typo in a column title used to mean recreating the whole retro: columns had no update path at all, since dal.RetroUpdate never touched the field. Columns are a JSON blob on the retro, not a table, and the websocket payload binder cannot decode nested structs, so each change is its own flat event: column_create, column_update and column_delete. They broadcast the existing retro_updated, so every connected client re-renders for free. Guards, all covered by tests: a column must exist, only empty columns can be deleted, a retro keeps at least two columns and gains at most five, and a note can no longer be written into a column that has just been deleted - without that check it would land somewhere nothing renders or exports while still counting towards the retro's total. Simultaneous edits are a read-modify-write race on one blob, so the broker serialises them with a mutex, released before broadcasting. A transaction would not help: sqlx issues a deferred BEGIN, so under WAL a concurrent writer fails rather than serialising. The controls sit on the column header, revealed on hover like the note actions, and are available in every stage - deleting an empty column strands nothing, because no notes means no groups and no votes. When a column cannot be deleted the button is disabled with a tooltip saying why. Two fixes fell out of this: - The retro_updated listener lived in Settings, which renders inside the board's collapsible header. Radix unmounts collapsed content, so with the header collapsed no settings or column change from anyone else arrived. It now lives on the route. - The column grid picked its width from an array indexed by child count, which was undefined outside 2..6 and collapsed the board into a single column. Discuss appends a Tasks column, so a six-column retro already hit this. The index is now clamped and Discuss declares its real count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The UI worked but had no shared visual or motion language: instant DOM swaps, five near-identical grey "chart" tokens doing nothing, and a board that laid out five fixed columns at every screen size. Motion, via motion/react behind LazyMotion and a global MotionConfig reducedMotion="user", so the OS setting is honoured everywhere without per-component guards. All timings come from one small vocabulary in lib/motion.ts. The showpiece is a shared layoutId on each note: advancing a stage no longer blinks the board away and back, the same cards glide from their brainstorm positions into their groups and then into vote-ranked order. Around that, notes spring in and out, groups reflow, the dragged card lifts and tilts, drop targets outline in their column's colour, and the vote count pops when it changes. Colour: the dead --chart-1..5 tokens become a five-hue column accent palette, tuned separately for light and dark. Accents are derived from column position, so no migration and no change to the creation form. Once notes are grouped and reordered by vote, colour is the only remaining signal of where a thought came from. Surface tokens let cards layer instead of sitting on flat white. Component work: - A stage rail replaces two anonymous chevrons, showing where the retro is, with the active pill sliding between steps and direction-aware confirm copy. - Note cards move to elevation and a ring, with author chips after brainstorm, and GIFs in a fixed aspect box with a placeholder - they used to pop in at their natural height and shove the column down the page. - The home page leads with the retros. Creating one is behind a "New retro" dialog rather than a permanently open form taking half the page, the list is a card grid with a stage-coloured spine, and the hero stats count up. - Presence shows avatars that animate in and out, with a pulse on the live dot. - Real empty states and loading skeletons: columns used to render blank both while fetching and when genuinely empty, which look identical. Two fixes: - The board is a horizontal snap-scroller below `lg`, so a five-column retro is usable on a phone instead of being crushed to unreadable slivers. The header stacks too - the title and the rail cannot share 375px. - The note dialog said "press enter to submit" and had no button, relying on implicit form submission. It has a Save button now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It pointed at ./build/thoughts, which only exists after a bundled build, so it would fail for anyone who ran it without building first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Logging in returned 200 and then /api/retros and /api/stats came back 401. The cause: the route guard treated localStorage as proof of authentication. localStorage only caches the name so the nav can render immediately - the session cookie is the credential, and it can expire or stop resolving to a user without the browser saying so. Land on the app with a stale cache and _auth/beforeLoad waved you through, the loaders fired, and every one of them 401'd. The auth provider now asks the server before anything decides what to render, and nothing mounts the router until that answer arrives. A stale cache with no cookie is now a single /api/auth/self 401 and the login screen, rather than four failing requests and a bounce. Two things made it stick rather than recover: - The axios interceptor threw a router redirect on 401. That is only caught inside a loader; from the provider's own effect it was a swallowed rejection that had already cleared the stored user on the way past, so a slow /api/auth/self could wipe a login that had just succeeded. The interceptor now reports expiry to the auth layer and the router decides where to send people, so a session dying mid-use is one 401 and a clean trip to the login screen. - Nav redirected to /login whenever it had no user, racing the route guard it duplicated. Removed; the guard owns this. Server: the auth middleware wrote 500 on a lookup failure and then carried on into the handler with a nil user, where UserFromRequest panicked. Covered by a test, along with the sessions that legitimately fail to resolve - no cookie, a cookie signed with a rotated key, and one pointing at a user that no longer exists. Login also tells you when it fails now, instead of the rejection vanishing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The actual reason logins would not stick. gorilla/sessions v1.4.0 builds NewCookieStore with Secure: true and SameSite=None, so the session cookie was only ever storable over https. Chrome treats http://localhost as a trustworthy origin and keeps it anyway, which is why this went unnoticed; Safari does not, and neither does anything else served over plain http. The login returned 200 with a Set-Cookie the browser then threw away, so every request after it came back 401. The options are now set explicitly rather than inherited: - Secure only when this server terminates TLS, or when the request arrived over https, so a deployment behind a TLS-terminating proxy still gets it. - SameSite=Lax rather than None. Everything here is same-site, and None cannot be used without Secure. - HttpOnly, which gorilla did not set. Nothing reads this cookie from JavaScript. CORS is now credential-aware whether or not the UI is bundled. A wildcard origin cannot carry credentials at all, so pointing a browser at the Vite dev server while a bundled binary served the API silently dropped the cookie in the same way. Rejections now say why, under THOUGHTS_VERBOSE=true. The useful part is whether a cookie arrived at all: none means the browser never stored it, one that names nobody means it failed to decode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Descriptions were single-line inputs, so anything longer than the box scrolled sideways out of sight - the one thing you most want to read back before creating a board. They are textareas that grow with their content now, with a character count that only speaks up near the limit. Sized in JS rather than with CSS field-sizing, which only Chrome supports. It also needs its own element and a merged ref: callers spread a react-hook-form field in, and that carries a ref of its own which was landing after ours and winning, leaving nothing to measure. The AI feature was an outline button in a row of three, hiding a bare popover. It is now a panel of its own with a gradient edge that spins while it thinks, a wand that waves, and one-click theme suggestions so nobody has to invent a prompt to find out what it does. Generating shows shimmering placeholder columns when the board is empty, and leaves columns you already have in place, dimmed - they are only replaced if the generation succeeds. Templates were a dropdown of bare names, so picking one was a guess. Each now shows its columns as chips in the colours they will be on the board. The rest of the screen: a wider dialog, columns as cards carrying their board accent, a count against the limit, a real empty state, hover-revealed remove, and the visibility toggle demoted from a bordered block to a line. Three fixes found while verifying this: - The AI prompt sat inside the create-retro form. HTML has no nested forms, so the browser dropped the inner one and its submit button belonged to the outer form: pressing Enter on a theme, or clicking Generate, submitted the retro instead of generating anything. - The generate call had no timeout, so an unreachable provider left someone watching a spinner. A bad key took two minutes to come back. - A generated template could exceed what the create endpoint accepts. The response is clamped to five columns and the field lengths, and max tokens raised from 250, which could not fit five columns and truncated the JSON into a parse failure. Also stops router.invalidate() firing on first mount, where beforeLoad ran before the router had its context and threw on every load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GIF search returned nothing because the response shape was wrong. Klipy
nests size first and then format, under "file":
file.md.gif not files.gif.md
Every item therefore mapped to an empty URL and was filtered out as an ad.
The canned payload in the test encoded the same wrong guess, so it passed
while the feature returned zero results. It is now shaped from a real
response, and there is a contract test that talks to the live API, skipped
unless THOUGHTS_GIF_API_KEY is set:
THOUGHTS_GIF_API_KEY=... go test ./cmd/thoughts/gif/ -run Live
Creating a retro:
- Column title and description are visible form fields with labels, rather
than borderless text that gave no hint it could be typed in.
- The accent colour is gone from the column cards.
- Any column can be removed, including the last: the minimum belongs to
validation on submit, not to a disabled button with no explanation. Added
a Clear button to empty them in one go.
- Remove is always visible rather than appearing on hover.
Tags: Tab now commits what you have typed instead of moving focus away and
discarding it, the way it works anywhere else you type a list of labels.
With nothing typed, Tab moves on as normal.
Stages: moving on used to mean knowing you could click the next step in what
looked like a progress indicator. There is a button for it now, labelled
with what happens next ("Start grouping"), and going back is a quiet icon
beside it. The rail is read-only.
"Brainstorm" is "Reflect" in the UI. It says what people are doing in that
stage rather than naming a meeting format, and it does not collide with
"Group" the way "Gather" would. Wire values are unchanged; the labels now
live in one place so the board, the rail and the home page cannot drift.
Em dashes are gone from the interface copy.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tab used to commit whatever raw text you had typed, and the suggestion list could only be reached with the mouse: our own input owned the keydown handler, so cmdk's list never saw an arrow key. Arrow keys now move through the list and Tab or Enter takes the highlighted entry, so typing "mob" and tabbing gives you the existing "mobile" rather than a near-duplicate. Existing tags rank above the "Add ..." fallback for the same reason. Owning the list outright left cmdk with no users, so it and the shadcn command primitive go with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AnimatePresence's popLayout mode clones every child with a ref of its own so it can measure it. In DraggableNote and DroppableNoteGroup that clone's ref landed in the trailing props spread and overwrote dnd-kit's setNodeRef, so neither the dragged note nor the groups it can be dropped on ever registered a DOM node. Nothing moved under the cursor because dnd-kit had nothing to move. Compose those refs and destructure them out of the spread, and carry the dragged card in a DragOverlay: a note has a layoutId, so motion owns its transform and dnd-kit's could never win even once the ref lands. What stays behind is the dimmed gap it left. Registering the group droppables meant a group, which fills its column, beat that column on overlap every time, leaving no way to drop a note back into open space. Collision detection now asks where the pointer is instead, and taking a note out of a group gets a button on the note as the reliable route. Also drop the overlay's drop animation, which flew the card back to the slot it started in, and stop the note lists using popLayout: it tears a note out of the flow to animate it away while its layoutId is gliding it into the new column. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Earlier on this branch I read the repeated user rows as a bug and made the name the identity, reusing a row on login and adding a unique index to keep it that way. That is not the model: a name here is a label, not an account. Two people called Alex are two people, and one person entering their name twice is two sessions. Worse, the migration that came with it merged existing users by name and deleted the duplicates, which is destructive and cannot be undone. It is removed, and a new migration drops the index for anyone who already ran it. The test that locked in the wrong behaviour is replaced with one that locks in the right one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Too many of them narrated the code rather than saying anything it could not. Kept the ones recording a quirk, a rejected alternative, or a value that has to stay in step with another file, and cut or shortened the rest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Carries over the architecture and conventions, and adds the two things this branch got wrong: that a name is a label rather than an account, and that comments should be sparing and earn their place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The destructive migration it was undoing is gone from this branch, so
merged history never creates the index and there is nothing to drop.
A dev database that ran the removed migration still has the index, and
still has its goose version recorded. goose up copes with the orphan
version, but a second user of the same name will fail on the constraint
until it is cleared by hand:
drop index if exists unique_user_name;
delete from goose_db_version where version_id = 20260810120000;
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fast refresh only works for a module that exports components and nothing else, so the variant definitions, the form-field hook and the shared textarea class move to modules of their own, and main.tsx keeps only the render call. Also gives vote.tsx the setter it reads in an effect. It comes from useState so its identity is stable, and the dependency array was the only thing claiming otherwise. Lint is now clean rather than merely passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Down from around 510 added lines to 130. What is left names a library or browser quirk, says why a reasonable-looking alternative is wrong here, or flags a value that has to track another file. Everything restating the code, narrating a layout or justifying an unremarkable choice is gone. CLAUDE.md now says the default is no comment, and that anything staying has to carry information the code cannot. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seventeen commits covering four areas: real defects in the backend, a replacement for the dead GIF provider, editable columns, and a visual and motion pass over the whole UI.
The commits are ordered and self-contained, so it reads best commit by commit.
Correctness and security
handleNoteUpdateandhandleNoteDeleteignored the user entirely, so a crafted socket frame could edit or delete anyone's note, including notes in a different retro. Retro scoping is now always enforced; ownership is enforced for content-bearing fields and deletes, while structural moves stay open so grouping still works.Secureon a plain http server, which broke login outside Chrome. gorilla/sessions v1.4 defaults toSecure+SameSite=None, so the options are now set explicitly and derived from the scheme.Identity is unchanged, after a wrong turn
Earlier on this branch I read the repeated user rows as a bug and made the name the identity — reusing a row on login, with a unique index and a data migration to enforce it. That was wrong and has been fully reverted.
A name here is a label, not an account: two people called Alex are two people, and one person entering their name twice is two sessions. The migration that came with it merged users by name and deleted the duplicates; both it and the follow-up that dropped the index are gone, so
migrations/is identical tomain. There is a test asserting two logins with the same name get separate identities, andCLAUDE.mdrecords the rule.If you ran the removed migration locally, your dev database still has the index.
goose upcopes with the orphaned version row, but a second Alex will fail on the constraint until you clear it:GIFs
Google shut the Tenor API down, and GIPHY's free tier is gone. Replaced with a small provider registry (
Provider,Resolve,SearchPage) shipping Klipy, which is free, plus a keyless paste-a-link path so the feature works with no configuration at all.POST /api/gifsbecame aGETwith trending on an empty query.Worth flagging for review: the first cut of the Klipy mapping was wrong — the API nests size before format (
file.md.gif, notfiles.gif.md) — and the unit test encoded the same wrong guess, so it passed while the feature returned nothing. There is now a contract test that hits the live API, skipped unlessTHOUGHTS_GIF_API_KEYis set:Columns
Columns can be renamed, added and deleted from the board header in any stage. Deletion requires the column to be empty, which is what makes it safe without stage gating. Columns live in a JSON blob rather than a table, and the socket payload binder only handles flat scalars, so the events are one column at a time and the read-modify-write is guarded by a mutex on the broker.
This also fixed a latent bug: the only
retro_updatedlistener lived inside a collapsed Radix panel, which unmounts, so with the board header collapsed a client never saw other people's changes.UI and motion
A visual pass plus a shared motion vocabulary (
ui/src/lib/motion.ts). Notes carry alayoutIdso they glide between stages rather than blinking; the board is a snap-scroller on small screens instead of crushing five columns onto a phone; stage progression is a labelled button rather than something you have to guess is clickable; "Brainstorm" reads as "Reflect" in the interface, with stage labels centralised inui/src/lib/stages.ts.Tests, CI and housekeeping
There was no test harness at all. Added one for both sides — a SQLite-backed helper for Go tests, Vitest for the frontend — and a CI workflow running build, vet and tests for Go, and lint, tsc, tests and build for the UI.
pnpm lintnow reports zero warnings, not just zero errors. The react-refresh warnings came from modules exporting both components and other values, so the variant definitions, the form-field hook and the shared textarea class moved into modules of their own andmain.tsxkeeps only the render call..github/copilot-instructions.mdis replaced by aCLAUDE.mdcovering the same architecture and conventions, plus the rules this branch got wrong: a name is a label rather than an account, migrations are append-only, and the default for a comment is not to write one.Reviewer notes
AnimatePresence mode="popLayout"clones each child with a ref of its own, which was silently overwriting dnd-kit'ssetNodeReffrom inside a props spread — for both the draggable notes and the group droppables. Same shape as a react-hook-formfield.refcollision fixed earlier in the branch, so worth a look anywhere a ref is passed through a spread.pointermoveevents per drag, which is not enough to start a dnd-kit drag, so the overlay and drop behaviour were reasoned from the mechanism rather than observed. Dropping a note into open column space to ungroup it was still not landing in manual testing, which is why ungrouping has an explicit button on the note.🤖 Generated with Claude Code