Fix login, voting and item bugs, and move the store to DynamoDB - #83
Open
VictorShan wants to merge 10 commits into
Open
Fix login, voting and item bugs, and move the store to DynamoDB#83VictorShan wants to merge 10 commits into
VictorShan wants to merge 10 commits into
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Redg3cR5EpGPH6Dp46Y9Af
A broadcast is a notification about a write that already committed, but every mutation awaited PusherServer.trigger bare, so an unreachable Soketi turned a successful add/vote/select into a 500 while the row stayed in the database. Route them through broadcastToLobby, which logs and swallows the failure. selectItem wrote its lobby log after that broadcast, so the log never landed when the broadcast threw - the Log panel was permanently empty. The log is part of the selection, so it is written with it now. isAuthed threw a bare Error, which tRPC reports as INTERNAL_SERVER_ERROR (and masks as "Internal server error" in production), so signed-out visitors got a 500 with no hint that they needed an account. It throws UNAUTHORIZED now. getAuth/currentUser throw when Clerk can't read a request, and that failed the whole batched request including the public queries that render a wheel for visitors who never sign in. Resolving the user is best effort now. Votes were a read-modify-write, so two people voting at once lost a vote; they are applied in SQL and clamped to +/-50. Item URLs are validated and blank input stores NULL instead of "", which used to make the item uneditable (updateItem rejected the stored ""). Timestamps are written as ISO strings. Fixes #68 Fixes #69 Fixes #74 Fixes #82 Refs #70, #77 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Redg3cR5EpGPH6Dp46Y9Af
…bby writes getLobbyInfo updated wheel_lobby with no where clause, so every read of any wheel restamped last_read_at on every lobby in the database - several times a minute per viewer, and it left last_read_at useless for expiry since no lobby ever looked stale. It is scoped to the lobby being read, and skipped entirely when the lobby does not exist. Items came back sorted ascending by upvotes, putting the least popular item at the top of the table; they are sorted best-first with a name tiebreak so the table does not reshuffle between refetches. createLobby returned undefined when all five generated cuids collided, and the browser followed that to /undefined, which renders as a plausible empty wheel. It throws CONFLICT instead. Reclaiming an expired cuid now also deletes that lobby's logs, which otherwise showed the previous lobby's history to whoever got the cuid next. updatePostRequest parsed the webhook body with a bare JSON.parse, answering bad input with a 500; parsing is part of the input schema now. The webhook body is normalised to an object on read, since the column default comes back as a raw string. getLogs had no order with its limit of 10, so it returned the ten oldest entries and the Log panel froze after ten selections. Fixes #72 Fixes #73 Fixes #78 Refs #79, #80 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Redg3cR5EpGPH6Dp46Y9Af
Weights were measured against the lobby's lowest score, so a downvote on one item re-weighted every other item: with scores a=0 b=0 c=1 the slices were 1/1/2, and downvoting b three times turned them into 4/1/5 - c's upvote went from doubling its odds against a to a 25% edge, though nobody voted on a or c. An item's weight now depends only on its own votes. Two ways the wheel went blank are gone too. Math.ceil of a negative age gave a weight of exactly zero when lastSelectedAt sat slightly in the future (server and viewer clocks disagree by seconds), and an unparseable timestamp gave NaN, which poisoned the total so every slice was drawn with NaN angles and the canvas showed nothing but the hub. Timestamps exist in three formats in this database, and the SQL CURRENT_TIMESTAMP form is Invalid Date in Safari. Weights are floored above zero, unparseable dates count as "not selected recently", and a total of zero falls back to equal slices. Fixes #75 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Redg3cR5EpGPH6Dp46Y9Af
…d trip Nothing invalidated the React Query cache, so the lobby page only refetched from the Pusher handler: the person who added an item or voted saw their change only if the broadcast came back to their own browser. AddItem called router.refresh(), which refreshes server components and has no effect on a client-side useQuery, and the vote/select/update/remove mutations had no onSuccess at all - so votes appeared not to register. Every mutation now invalidates getLobbyInfo (and getLogs where it applies); the broadcast stays as the mechanism for everyone else's browser. The item edit form used placeholders instead of values, so it looked empty and "saving" an untouched item wiped fields. It is a controlled form seeded from the item now, and submits "" for a cleared URL, which the router stores as NULL - previously a blank URL was stored as "" and updateItem rejected it as an invalid URL, making the item uneditable. Blank names and invalid URLs are caught inline before the request. Lobby settings seeded react-hook-form with defaultValues taken from a query that was still loading, so a saved webhook never appeared in the dialog and saving overwrote it; it uses `values` now. The Save button reported success from its own onClick even when validation failed - feedback comes from the mutation. Also: the log list toasted from the render path on error, timestamps are parsed before formatting instead of calling toLocaleString on a string, a failed lobby load is no longer rendered as a convincing empty wheel, and usePusher keeps its handler current instead of pinning the closure from first render. Fixes #76 Fixes #77 Fixes #79 Fixes #80 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Redg3cR5EpGPH6Dp46Y9Af
…needed SignInButton had no mode, so signing in navigated away to Clerk's hosted pages and, with no afterSignInUrl, came back to the home page. A wheel's random URL is the only handle anyone has on it, so signing in from a lobby meant going back to the group chat to find the link again. Sign-in and sign-up are modals that return to the page they were started from. Signed-out visitors were also shown Create Lobby, Add Item, vote and select controls that could only fail, since those procedures require a Clerk user. They now see what needs an account, next to a button that gets them one - viewing a wheel, its items and its log stays public. Fixes #70 Fixes #71 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Redg3cR5EpGPH6Dp46Y9Af
env.js still required DATABASE_URL to be a valid MySQL-ish URL, left over from PlanetScale. Nothing reads it - the app moved to Turso - but next.config.js imports env.js, so a deploy or a fresh clone without a dummy value fails to build on a database it never talks to. Removed from the schema, the runtime map and the Playwright workflow, along with the mysql2 and @planetscale dependencies. Added the .env.example that .gitignore already refers to, so there is one place listing what a contributor has to set. Fixes #81 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Redg3cR5EpGPH6Dp46Y9Af
Reported as slow load times. The lobby page needed a relational read plus a table-wide UPDATE that ran before it and blocked the response; a wheel is now one query. Single-table layout, everything about a wheel in one partition: pk = LOBBY#<cuid> sk = LOBBY | ITEM#<itemId> | LOG#<timestamp>#<rand> Sort keys are ordered so `sk < "LOG#"` returns the wheel and every item on it and stops before the log, which is read separately and capped. src/server/db/ lobbyStore.ts is the only module that builds a key; routers call it. What the store buys beyond the round trip: - Votes apply with ADD plus a ConditionExpression cap, so simultaneous voters no longer overwrite each other - the old read-modify-write dropped one of two votes cast at the same moment, and no SQLite equivalent was atomic. - createLobby claims a cuid with attribute_not_exists(pk) rather than reading first, so two people generating the same cuid can't both win it. - lastReadAt is fire-and-forget and only rewritten once stale, so a viewer never waits on a write whose result they never see. Item ids are cuid2 strings now: DynamoDB has no sequence, and a per-lobby counter would add a round trip to every add. The three client call sites that typed them as numbers move to strings. scripts/migrate-turso-to-ddb.ts copies existing data across, keeping old integer ids as strings so items keep their identity, and normalising the three timestamp formats SQLite accumulated into ISO (unparseable ones become "never selected"). Re-running it overwrites what it wrote, so an interrupted run can just be repeated. scripts/create-ddb-table.ts provisions the table and is safe to re-run. tests/lobbyStore.spec.ts covers every access pattern against a local DynamoDB the tests start themselves, so CI needs no AWS account. Verified end to end in a browser against a local DynamoDB: add, vote, select, spin and the log, plus a migration of representative legacy rows read back through the API. Drizzle, drizzle-kit and the SQLite schema are gone; @libsql/client stays as a dev dependency for the migration script alone. Fixes #54 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Redg3cR5EpGPH6Dp46Y9Af
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…r a missing wheel Three findings from a review of this branch. getLobbyInfo returned undefined for a wheel that doesn't exist. React Query treats an undefined result as a failed query (query-core throws "data is undefined"), so the page landed in its error state showing an internal message, and the "No wheel lives at this address" branch added in this branch was unreachable. Returning null makes it a real result. Verified in a browser: the message now renders. deleteLobby ignored UnprocessedItems from BatchWrite. Everything about a wheel shares one partition key, which is exactly the shape DynamoDB throttles, and declined writes are reported rather than thrown - so a large wheel could keep items and log entries that the next owner of that cuid would inherit. Declined writes are resent with backoff. getLobbyWithItems and listItemIds stopped at the first page, so a partition over the 1 MB limit silently yielded a truncated item list and the wheel would spin over a subset with no sign anything was missing. Both follow the cursor now, through the same helper deleteLobby already needed. Covered by a test that fills a wheel past the 25-row BatchWrite limit and reads and deletes it whole. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Redg3cR5EpGPH6Dp46Y9Af
From a security review of this branch.
This branch made item urls http(s)-only on the way in, but the old schema never
checked them - addItem took a bare string - so the SQLite data can hold whatever
a signed-in user typed, including javascript:. The migration copied url columns
verbatim, and the item dialog renders one into an anchor. React 18 only warns
about a javascript: href ("A future version of React will block javascript:
URLs"); it still renders a live link. Clicking Open Link on such an item would
run script in the app's origin under the viewer's Clerk session - enough to
repoint the lobby's webhook at an attacker's endpoint.
The migration drops any url that isn't http(s), reporting what it dropped, and
the dialog resolves urls through safeUrl so a value from any other source falls
back to the disabled button rather than a live link.
Verified: migrating an item carrying javascript:alert(document.cookie) stores
no url and logs the drop, while an https url beside it survives.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Redg3cR5EpGPH6Dp46Y9Af
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
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.
Fixes the reported bugs (sign-in, voting, adding items) and moves the data layer from Turso/SQLite to DynamoDB.
This PR does not deploy cleanly on its own — see Before merging / deploying. A DynamoDB table has to exist and the data has to be copied across.
Before merging / deploying
Every step below is required, in this order.
1. Create the table
The table is
pk(S) /sk(S), on-demand billing. No secondary indexes.2. Set the environment variables
New, required — the app will not boot without them (
src/env.jsvalidates at import time):AWS_REGIONus-east-1DDB_TABLE_NAMEwheelNew, optional:
DDB_ENDPOINTCredentials: in production, prefer the deploy environment's IAM role. Otherwise
AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEYare read by the SDK as usual.Removed — safe to delete from the deploy environment and from GitHub secrets:
DATABASE_URL(dead PlanetScale leftover; blocked startup for a database nothing talked to)TURSO_DATABASE_URL,TURSO_AUTH_TOKEN— keep these until step 3 is done, then remove. They are optional now and read only by the migration script.Unchanged: the Clerk and Soketi variables.
3. Copy the existing data across
Run this from a machine that can reach both Turso and DynamoDB, with
TURSO_DATABASE_URLandTURSO_AUTH_TOKENstill set:npm run ddb:migrate-from-turso -- --dry-run # reports counts, writes nothing npm run ddb:migrate-from-tursoWhat it does:
Invalid Date.4. IAM
The app's role needs, on that table only:
dynamodb:GetItem,PutItem,UpdateItem,DeleteItem,Query,BatchWriteItemRunning it locally
With
DDB_ENDPOINT="http://127.0.0.1:4567"in.env. Any values work for the AWS credentials against a local DynamoDB.What changed
DynamoDB migration (#54)
Single table, everything about one wheel in one partition:
The sort keys are ordered so
sk < "LOG#"returns the wheel and every item on it in one query, stopping before the log (read separately, capped).src/server/db/lobbyStore.tsis the only module that builds a key.On the load times in #54: the lobby page needed a relational read plus a table-wide
UPDATEthat ran before it and blocked the response. It is now one query, with thelastReadAtwrite moved off the response path — fire-and-forget, and only when the stored value is already stale. That is a structural improvement; I have no production before/after numbers, since the only measurements available here were a local SQLite file against a local DynamoDB. Worth timing after deploy.Things the old store could not do:
ADDplus aConditionExpressioncap. The old read-modify-write dropped one of two votes cast at the same moment, and SQLite had no atomic equivalent. Tested with 10 concurrent votes — all 10 land.createLobbyclaims a cuid withattribute_not_exists(pk)instead of check-then-write, so two people generating the same cuid cannot both win it.Item ids are cuid2 strings now, not autoincrement integers: DynamoDB has no sequence, and a per-lobby counter would add a round trip to every add. Three client call sites moved from
numbertostring.Drizzle, drizzle-kit and the SQLite schema are gone.
@libsql/clientstays as a dev dependency for the migration script alone.Not included: TTL on rows. It would auto-expire old wheels, but per-row TTL deletes items out from under an active wheel unless every read refreshes the whole partition. The 7-day reclaim-on-collision behaviour carries over unchanged instead.
Bug fixes
Found by running the app against a local database and driving it in a browser. Each was reproduced before being fixed.
await PusherServer.triggerunguarded after the writeselectItemwrote its log after the broadcast, so a broadcast failure skipped itisAuthedthrew a bareError→ 500, not 401SignInButton, noafterSignInUrllast_read_aton every lobby in the database.where()NaN/0 weights poisoned every slicerouter.refresh()does not touch a clientuseQuery"", which then made the item uneditableaddItemdid not validate URLs,updateItemrejected""createLobbycould navigate to/undefined; recycled cuid inherited old logsundefinedlimitwith noorderBydefaultValuesseeded from a still-loading queryDATABASE_URLnothing usedgetAuththrowing failed the whole batched requestSign-in and sign-up are modals that return to the page they started from, and signed-out visitors are told what needs an account instead of meeting buttons that 401. Viewing a wheel, its items and its log stays public.
Testing
tests/lobbyStore.spec.ts— 12 tests over every DynamoDB access pattern, against a local DynamoDB the tests start themselves. Its own Playwright project (npx playwright test --project=store), so it runs once rather than per browser, and CI needs no AWS account.npm run lint,tsc --noEmitandnpm run buildare clean.Not run here: the existing browser tests against real Clerk credentials. Signed-in UI paths were verified through a temporary local Clerk stub, which is not part of this branch; signed-out paths were verified for real.
Follow-up worth considering
One vote per user, per item. It is the other honest reading of "the voting is bad" — right now one person can click
+up to the cap. It needs its own item type in the table (VOTE#<userId>), so I left it out of a PR that is already moving the database.Generated by Claude Code