Repository navigation
Guest voting, one language per visitor, and a deploy that can fail safely - #75
Merged
Merged
Conversation
The tagline advertised 'running a bar' while the in-app terms state the service is designed for personal, private use and grant no licence for public performance or commercial use. Marketing the excluded use case on the public README is a contradiction we do not want on record while the YouTube API compliance review is open. Replaces it with private-gathering examples and states the private-use scope explicitly, linking the terms.
… a success
The 87 server tests ran in no workflow, and update_server.sh restarted
whatever it pulled: it reported 'Update Completed Successfully' whether or
not the process came back, and 'git reset --hard origin/main' had already
destroyed the tree it could have rolled back to.
CI: add a server test job, and cover cuevote-mcp at all — its path was
missing from the filters, so OAuth and token-minting code merged untested.
Deploy: record the deployed commit before the reset, run the tests before
touching the running process, poll /health after the reload, and roll back
to the recorded commit when it does not come up. Worktree mode has no
rollback target and now says so instead of claiming success.
Verified /health answers 200 {"status":"ok"} against a locally started
server, and the port parser against the real .env plus the missing-file
fallback.
…that actually runs src/db.ts and src/types.ts were a parallel port of db.js that nothing ever required — one commit, untouched since, while db.js kept growing. tsc compiled them (and transpiled the CommonJS into a dist/ no process runs) with checkJs disabled, so the 'server — build' CI job type-checked exactly nothing. Measured before deciding: enabling checkJs across the server yields 202 errors, most of them the CommonJS/@types/ws interop making every socket look like the module namespace. That is its own migration, not a step here, so the pretense goes rather than being half-kept. In its place the server gets the static analysis it never had: eslint was already a devDependency with no config and no script. The first run found 13 real issues, all fixed — a Set built and never read in checkVideoAvailability, an unused loop binding in the shutdown path, unused parameters, unnecessary escapes inside regex character classes, and two stale requires in dev scripts. Two rules are tuned with the reason in the config: rest-sibling destructuring is how PII is stripped, and defensive initialisers are deliberate. Tests stay at 87/87.
…uire accounts Voting and suggesting both refused anyone without a Google account, and ws.user was only ever set by LOGIN or RESUME_SESSION — there was no guest identity anywhere in the code. A guest who scanned the QR code at a party had to accept cookies, open an OAuth popup and pick a Google account before their first thumbs-up. That is the largest drop-off on the path the product exists for. A guest identity is now a random id the server signs (HMAC) and the client keeps in localStorage. It is deliberately stateless: no row in users, no row in sessions, nothing personal stored, nothing to erase on request. The id is namespaced 'guest:' so it can never collide with a Google subject id or be mistaken for a room owner. The secret behind it comes from GUEST_TOKEN_SECRET, or is generated once and kept in server_config so a default install needs no configuration — either way it survives a restart, so a deploy mid-party does not sign everyone out. Owners who want every participant tied to an account get a per-room 'Login required' switch, off by default. Room.canParticipate is the single place that decides, so voting and suggesting cannot drift apart, and the client mirrors it only to explain the rule instead of collecting a rejection. Guests have no account name, so the server sends a flag and a four-character tag and the client renders the translated word — otherwise every guest in a German room would read 'Guest'. All 35 languages carry the three new keys. Schema: this is also the first change to follow the migration convention. require_login and server_config land as real migrations (008, 009) rather than another try/catch ALTER in db.js, migrations/README.md writes the rule down, and migrator.js now execs each file as one script instead of splitting it on semicolons. Verified against a running server over a real socket: a fresh guest is issued a signed token, votes (score 3 -> 4), reconnects to the same identity with its vote remembered, toggles it back off, and a token with one character changed is rejected and replaced — while the voters map still never leaves the server. 104 unit tests pass, 17 of them new.
translations.js held every language in one module: 408 KB raw, 114 KB gzipped — larger than React, the router and the OAuth client combined, and on the critical path, because the render gate waits for it. A guest joining a party over venue wifi downloaded 34 languages they cannot read to see the one they can. Each language is now its own module under translations/locales/, loaded through import.meta.glob so the bundler emits one chunk per language. A visitor fetches their own plus English, which stays loaded as the fallback for any key a translation is missing. Detection no longer needs the strings — only the list of codes — so it settles synchronously and the right chunk starts downloading in the same tick the app boots. Switching language pulls that chunk once and keeps it. For a German visitor: 114.5 KB gzip becomes 8.4 KB (en 4.0 + de 4.6), 92% off the critical path. An English visitor loads 4.0 KB. These remain dynamic imports, so the TDZ crash fixed in 1b57d50 stays fixed. check-i18n.mjs reassembles the locale files and enforces the same five checks as before: 235 keys × 35 languages still pass. While verifying this in the browser, the header greeted a guest as 'Guest 9255' in a German UI — the server's English fallback name leaking into the one place it is read by a person. Added displayName(), so the greeting, the profile heading and the avatar initial all translate the word: 'Willkommen, Gast 9255', 'Bienvenue, Invité 9255'. Verified in the browser: only en+nl load on first paint, switching to German fetches de.js alone, and switching to French at runtime fetches fr.js alone — never the other 32.
… thinking-out-loud comments Three call sites each re-implemented 'slice off the head, sort the rest, put the head back' — in handleSuggestSong, handleVote and the playback error path. Index 0 is the track that is playing (the client mirrors it as currentTrack), so getting that wrong skips a song mid-play with nothing in the logs to explain it. That is a footgun worth removing, not repeating three times. sortUpcoming() in queue-order.js is now the only place that knows the rule: owner priority first, then score, ties keeping arrival order because the sort is stable. It never mutates its input and never moves the head. Ten tests pin that behaviour, including the head-stays-put invariant that the duplication kept putting at risk. Also rewrote the comments that recorded how someone arrived at the code instead of what it does — 'Let's stick to strict Title check as requested', 'I suspect index.js logic forwards it blindly', 'maybe wait?'. One of them was a live question, so it got answered rather than reworded: DELETE_ACCOUNT is handled in the index.js switch, which returns before any room routing, so the case in Room.js is unreachable and now says so. 114 tests pass.
…n RoomBody RoomBody carried 39 useEffects. Six of them were the same three lines: mirror a piece of state into a ref so the YouTube player's event handlers — bound once at construction, closing over whatever the state was then — read current values instead of stale ones. Identical, easy to get subtly wrong (a mismatched dependency silently freezes the ref), and crowding out the effects that actually do something. useLatestRef() replaces all six. 39 effects down to 33, with no change to what any of them do. Removing the boilerplate surfaced dead state: tRef was written on every change of the translation function and never read anywhere. The linter could only see it once the mirror effect stopped referencing it. Removed. The linter proves useRef() is stable but cannot prove that of a custom hook's return, so the four dependency arrays that read these refs now list them. A stable value never re-triggers an effect, so this is bookkeeping, not behaviour. This is the safe part of decomposing this component. The effect graph itself — playback recovery, socket sync, overlay state — should not be pulled apart without a client test harness to catch what moves, and there is none yet. Verified in a fresh browser tab: the app loads with no console errors.
Review of the guest-voting change found one root cause behind six defects:
ws.user now carries two kinds of identity, but only Room.canParticipate
told them apart. Every other `!ws.user` gate silently changed meaning from
"is signed in" to "has any identity", which every guest does.
guest.hasAccount(ws) is now the question those gates ask:
CREATE_ROOM rooms.owner_id is a foreign key into users, so a guest
owner threw SQLITE_CONSTRAINT_FOREIGNKEY — after
bcrypt.hashSync had already blocked the thread ~100ms
for an unauthenticated caller.
DELETE_ACCOUNT ran a full room_state scan plus a WAL checkpoint and a
broadcast to every client in every loaded room, per
message, for an id that was never in users.
MCP_AUTHORIZE bound an AI-DJ OAuth grant to a browser-local random
number and burned the single-use handle.
FETCH_SUGGESTIONS the most expensive YouTube call there is (~100 of
10.000 daily units on a cache miss). Its own comment
said every quota-spending action requires a login;
suggesting and voting were opened to guests on
purpose, this was not.
The identity itself was also mintable without limit. GUEST_SESSION only
refused to overwrite a signed-in user, so a socket that was already a
guest could ask for a fresh identity and vote again with each one —
defeating the single property the module exists to provide. Proved before
the fix: one socket, one IP, never signed in, 21 identities, one track
moved by 20. A socket now gets exactly one identity and re-sends are
answered with it; minting a new one costs a new connection, and
MAX_GUEST_MINTS_PER_IP (60 per 10 min, generous for a party behind one
NAT) bounds churning them. Retested after the fix: 22 requests, 1
identity, no inflation.
Signing in mid-party no longer counts one person twice. Room.mergeIdentity
re-keys the guest's votes and attributions onto the account, and takes the
guest vote back out of the score when the account had already voted from
another device.
canParticipate stopped conflating its two refusals: a socket with no
identity yet was being told the channel requires a signed-in account, in
rooms where that setting is off. NO_IDENTITY and LOGIN_REQUIRED now carry
their own accurate messages.
Tests: 120 pass, 6 new. The suite also stopped hanging after its last
assertion — the async write worker is a live handle and nothing closed it,
which surfaced once several runs overlapped.
Every sign-in affordance was written as `user ? profile : <GoogleAuthButton>` back when `user` could only mean an account. Handing every visitor a guest identity made that condition true within a round-trip of page load, so the Google button stopped rendering anywhere: Header, both Lobby variants and the AI-connect page. Nobody who was not already signed in could sign in, signing out re-guested immediately so no account switch was possible, and a room with the new Require-Login toggle told guests to sign in with no control anywhere that could. isSignedIn(user) is the client's counterpart to guest.hasAccount on the server, and now gates all four, plus the create-channel tile, which offered guests an enabled button whose insert always failed on the owner_id foreign key. Server refusals also stopped being English-only. The server sends a code with each one; the client maps it to a translated string and falls back to the server text when it does not recognise the code. The two local gates no longer reuse the suggest copy for votes, and they distinguish "this channel wants an account" from "your session is not ready yet" — the second used to claim the first, in rooms where that setting is off. Four new keys in all 35 languages (239 × 35 pass check:i18n). Verified in the browser: with a guest token present and no session token, the lobby renders "Se connecter avec Google" and the create tile takes the not-signed-in path.
Five defects from the review, all in the machinery the split introduced. A rejected load was cached forever. loadLanguage stored the promise before it settled and never evicted it, so one dropped request pinned that language to an error for the whole session — selecting it again returned the same rejection without another attempt. Failures are now evicted, so picking the language again is a real retry, and a module without a usable default export is rejected rather than stored as undefined (which the consumer's truthiness guard would have turned into a render loop). English had no failure path at all. It was the one leg of the Promise.all without a .catch, and the .then that consumed it had no rejection handler, so a single 404 — a visitor on a cached index.html after a deploy — left the app on a text-free spinner forever with an unhandled rejection and nothing logged. English is retried once, the result never rejects, and if it still fails the app offers a reload control instead of spinning. The render gate did not do what it said. Guarding on "translations && (translations[language] || translations.en)" is the same as guarding on "translations", because English is always present, so switching language repainted the entire UI in English for the length of the fetch. It now waits for the chosen language, and stops waiting when that language is known to have failed. localStorage moved back out of harm's way. Reading it at module scope meant a browser with site data blocked — or a storage-partitioned frame — threw during evaluation of a chunk main.jsx imports before createRoot, so the page went blank with no ErrorBoundary. Reads and writes are guarded. The guardrail stopped covering the loader. check-i18n.mjs validated the locale directory but no longer touched the module the app imports: changing the glob by one character printed "i18n OK" while the build emitted zero locale chunks. It now checks the glob and the prefix codeFromPath strips — verified by reproducing both breaks and watching the check fail. Also drops the manualChunks rule for the deleted translations.js, which matched nothing, and updates docs/TDZ-FIXES.md, which still described both it and the old dynamic import as active.
…ver uses Two defects from the review, both in the safety net added earlier on this branch, and both of which made a healthy or recoverable deploy worse. set -e killed the script before the rollback. "pm2 reload || pm2 restart" is a list, not a condition, so when BOTH fail — a wedged or out-of-memory pm2, exactly what the rollback exists for — set -e exits right there. The health check and the whole rollback path never ran, and the working tree had already been reset onto the new commit. Verified: the old shape exits 1 without reaching the next line; the new one handles it and continues. The health check polled the wrong port. server_port kept everything after the first "=", so PORT=8080 with a trailing comment became 8080#httpport, which curl reads as a fragment and turns into a request for / — answered with 404, failing all 15 attempts and rolling back a deploy that was fine. It also ignored an exported PORT, which the server itself honours because dotenv does not override an existing variable and pm2 --update-env passes the environment through. Resolution now matches the server's, and inline comments, quotes, export and spaces around = all parse; anything that is not a plain number falls back to 8080 instead of being polled. While fixing that: the helpers resolved paths relative to the working directory while do_update walks between package directories, so the rollback introduced on this branch would have run npm ci in the wrong place. Everything is anchored to the script's own directory now, and git operations name the repo explicitly. Exit codes also stopped conflating the two bad outcomes: 1 means the deploy failed but the previous version is serving again, 2 means the site needs a human. A rollback restores the server and not the branch, so the script now says so — the next run would otherwise redeploy the same bad commit, tests included, since they passed on it. DEPLOYMENT.md documents the codes and MAX_GUEST_MINTS_PER_IP.
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.
Works through the seven points from the review of what I would do differently. One commit per point, each independently reviewable.
What changed
1. Guests can vote and suggest (
70c18cd) — the headline change. Voting and suggesting both refused anyone without a Google account, and there was no guest identity anywhere in the code. A guest who scanned the QR code had to accept cookies, open an OAuth popup and pick an account before their first thumbs-up.A guest identity is now a random id the server signs (HMAC) and the client keeps in localStorage. Stateless by design: no row in
users, no row insessions, nothing personal stored, nothing to erase on request. Ids are namespacedguest:so they can never collide with a Google subject id.Owners get a per-room "Login required" switch, off by default — so existing rooms open to guests on deploy. Turn it on and the room behaves exactly as it does today.
Room.canParticipateis the single place that decides, so voting and suggesting cannot drift apart.2. One language per visitor (
ae18976) —translations.jsheld all 35 languages in one module: 114 KB gzipped on the critical path, more than React, the router and the OAuth client combined. Now one chunk per language. A German visitor loads 8.4 KB instead of 114.5 KB — 92% off the critical path.3. Tests gate deploys (
3951d4e) — the 87 server tests ran in no workflow, andupdate_server.shrestarted whatever it pulled, reporting success whether or not the process came back. Now: a test job in CI (pluscuevote-mcp, which had no CI at all), tests run before the restart,/healthis polled after it, and a failure rolls back to the commit recorded beforegit reset --hard.4. Real migrations (part of
70c18cd) — migrations 001–007 are allSELECT 1; the schema actually lived indb.jsasCREATE TABLE IF NOT EXISTSplustry { ALTER TABLE } catch {}. 008 and 009 are the first real ones,migrations/README.mdwrites the rule down, andmigrator.jsnow execs each file as one script instead of splitting on semicolons.5. Dead TypeScript removed (
5895ac8) —src/db.tswas a parallel port ofdb.jsthat nothing required, compiled by a CI job that type-checked nothing (checkJs: false). Measured before deciding: enablingcheckJsyields 202 errors, mostly CommonJS/@types/wsinterop — its own migration, not a step here. So the pretense goes, and the server gets the static analysis it never had: eslint was already a devDependency with no config and no script. First run found 13 real issues, all fixed.6. Structure and comments (
1bae25b,5cfdd9e) — the queue ordering rule was re-implemented at three call sites, each re-deriving "never move index 0" (the invariant behind the silent-skip footgun). Now one tested function. Six identical ref-mirror effects inRoomBodycollapse intouseLatestRef, 39 effects down to 33 — which surfacedtRef, dead state written on every render and never read. Comments that recorded how someone arrived at the code were rewritten; one was a live question, so it got answered rather than reworded.7. README (
c1b4a33) — the tagline advertised "running a bar" while the in-app terms grant no licence for public performance, with the YouTube compliance review open.Verification
en+ one locale load on first paint; switching language at runtime fetches that one chunk and never the other 32. A fresh tab loads with no console errors.Decisions worth a second opinion
require_logindefaults to 0, so every existing room opens to guests when this deploys. That is the point of the change, but it is a live behaviour change for rooms already out there — say the word and I will flip the default and make it opt-out instead.GUEST_TOKEN_SECRETis optional. Unset, the server generates one and stores it inserver_config, which is right for a single instance. Documented in DEPLOYMENT.md; set it explicitly before running a second process.RoomBody's effect graph is not in here. Playback recovery, socket sync and overlay state should not be pulled apart without a client test harness, and there is none. Worth its own piece of work.Not verified
The in-room UI for a guest was checked at protocol level and through the lobby (which showed "Willkommen, Gast 9255", correctly translated), but not clicked through in the room itself — that sits behind the cookie consent dialog, and accepting terms on your behalf is not something I do unasked.
Review round (commits
70df9f1…e8d286c)A high-effort review of this branch found 15 confirmed defects, all fixed here. One root cause produced six of them:
ws.userstarted carrying two kinds of identity, but onlyRoom.canParticipatetold them apart — so every other!ws.usergate quietly changed meaning from "is signed in" to "has any identity", which every guest does.Two were ship-blockers:
GUEST_SESSIONonly refused to overwrite a signed-in user, so an existing guest could mint a fresh identity on demand and vote again with each one — defeating the single property the guest module exists to provide. Proved before the fix: one socket, one IP, never signed in, 21 identities, one track moved by 20. After: 22 requests, 1 identity, no inflation.user ? profile : <GoogleAuthButton>, and every visitor became a guest within a round-trip of page load — so the Google button stopped rendering anywhere, and a Require-Login room told guests to sign in with no control that could.The rest: the YouTube Search-quota gate had silently opened to guests (its own comment explains why that matters);
CREATE_ROOM,DELETE_ACCOUNTand the AI-DJ OAuth finalize all admitted guests; signing in mid-party let one person vote twice;set -ekilled the deploy script before the rollback this branch added could ever run; the health check polled a port an inline.envcomment had mangled; and five defects in the language split — a rejected chunk cached forever, English with no failure path, a render gate that never waited,localStoragemoved where no ErrorBoundary could catch it, and a guardrail that stopped covering the module the app actually imports.Each fix is verified rather than asserted: the exploit was re-run against the fixed server, the gates were exercised over a real socket, the sign-in button and the language switch were checked in the browser, both deploy defects were reproduced and re-tested in isolation, and the i18n guardrail was proved to catch the one-character break that used to ship green. 120 server tests (14 new), all six CI jobs pass.