tiennm99/rplace
1
0 Fork 0
mirror of https://github.com/tiennm99/rplace.git synced 2026-10-11 03:13:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

plans: drop reports directory

Browse Source 
All reports referenced plans that have shipped (Upstash→DO migration +
post-migration P1 fixes). Git history retains content if ever needed.
This commit is contained in:
tiennm99 committed 2026-05-11 16:43:47 +07:00
1 parent 7ef64ecd4f
commit bc26fb21d7
16 files changed
-3725
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

plans/reports/brainstorm-260509-2309-canvas-on-do-storage.md
-177
View File
@@ -1,177 +0,0 @@
# Brainstorm Report: Canvas Storage on Cloudflare DO (Free-Tier, Scalable)
**Date:** 2026-05-09 23:09 (Asia/Saigon)
**Status:** Approved by user. Proceeding to `/ck:plan`.
---
## Problem Statement
Current rplace stack uses Upstash Redis for canvas (BITFIELD) + cooldown (SET NX EX). Goal: eliminate Upstash, move all state into the existing `CanvasRoom` Durable Object, while:
1. Staying inside Cloudflare Free Tier forever ($0/month).
2. Making future canvas size expansion a config change + redeploy (no migration code).
3. Migrating existing Upstash canvas data one-shot.
---
## Constraints (user-confirmed)
| Constraint | Value |
|---|---|
| Canvas target (1–2 yr) | 4096×4096 (no expansion planned) |
| Peak traffic | Hobby: 1–50 concurrent, <1 placement/sec |
| Resize behavior | Config change + redeploy; no live migration |
| Budget | $0 forever — hard constraint |
| Migration of existing data | One-shot import from Upstash → DO |
---
## Approaches Considered
### A. Single DO, all-in (chosen)
- DO owns canvas (chunked SQLite BLOB) + cooldown + WS broadcast
- Worker = thin validation/proxy
- ✅ Simplest, $0, no external deps
- ⚠️ Single-DO bottleneck — irrelevant at 50 users
### B. DO for WS only + Cloudflare KV for canvas
- KV stores 16 MB canvas (under 25 MB cap)
- ❌ Read-modify-write races, eventual consistency, KV write quotas
- Rejected: more complex than A, no benefit at this scale
### C. Worker + R2 + DO (hybrid)
- R2 for snapshots, DO for deltas
- ❌ Massive over-engineering for hobby canvas
- Rejected: YAGNI
**Decision: Approach A.**
---
## Final Design
### Architecture
```
Browser ──HTTP/WS──▶ Worker (Hono, thin proxy)
│
└─▶ CanvasRoom DO (idFromName('main'))
├── canvas_chunks (SQLite BLOB rows)
├── cooldowns (SQLite TTL rows, lazy GC)
└── WebSocket hibernation hub
```
### SQLite Schema (inside DO)
```sql
CREATE TABLE canvas_chunks (
chunk_id INTEGER PRIMARY KEY,
bytes BLOB NOT NULL -- exactly CHUNK_BYTES bytes
);
CREATE TABLE cooldowns (
user_id TEXT PRIMARY KEY,
expires_at INTEGER NOT NULL -- ms epoch
);
CREATE INDEX idx_cooldowns_expires ON cooldowns(expires_at);
```
### Constants (drives "resize = redeploy")
```js
export const CANVAS_WIDTH = 4096;
export const CANVAS_HEIGHT = 4096;
export const CHUNK_BYTES = 65536; // 64 KB
export const TOTAL_PIXELS = CANVAS_WIDTH * CANVAS_HEIGHT;
export const CHUNK_COUNT = Math.ceil(TOTAL_PIXELS / CHUNK_BYTES); // 256
```
Resize: bump width/height → redeploy → DO lazy-inits missing chunks (zero-fill on first read).
### Worker → DO Routing
| Path | Worker action | DO method |
|---|---|---|
| `GET /api/canvas` | Forward to DO | `getFullCanvas()` returns concat of all chunks |
| `POST /api/place` | Validate body (size, pixel ranges, batch cap) → forward to DO with `userId` | `placePixels(userId, pixels)` does cooldown check + writes + broadcast in one transaction |
| `GET /api/ws` | Upgrade → forward to DO | `accept(ws)` |
### Why Chunked BLOB (not single 16 MB row)
- Batch of 2048 pixels touches typically 1–2 chunks → small UPDATE round-trips.
- Disjoint chunk reads are concurrent.
- Future spatial sharding (per-region DO) is a routing change, not a storage rewrite.
### Free Tier Math (peak hobby)
| Resource | Peak/day | Free quota | Headroom |
|---|---|---|---|
| Workers requests | ~87K (86K places + 500 fetch+ws) | 100K | 13% — tight |
| DO storage | ~16 MB | 1 GB | 60× |
| DO subrequests | ~86K | uncapped on free | ✓ |
| WS connections | ~50 | 32K/DO | ✓ |
| Bandwidth | trivial | unlimited | ✓ |
**Mitigation for tight Workers quota:**
- Edge cache on `/api/canvas` (`s-maxage=10`) absorbs repeat fetches.
- Pixel placements already batched up to 2048/request.
- WS messages don't count as Workers requests.
### One-Shot Migration (Upstash → DO)
Admin-only endpoint `POST /admin/import-canvas` (token-gated):
1. Worker: read full canvas from Upstash (existing `getFullCanvas` from old `canvas-storage.js`).
2. Forward bytes to DO via `room.fetch('/import', {body: bytes})`.
3. DO: split into N chunks, INSERT OR REPLACE all rows in one transaction.
4. Run once. Delete endpoint after migration completes.
---
## Scalability Levers (deferred, not built now)
1. **Bigger canvas** → bump constants. Up to ~32K×32K (1 GB DO cap).
2. **More writes/sec** → spatial sharding: one DO per region, worker routes by `chunk_id`. Chunk-ID abstraction already in storage layer makes this surgical.
YAGNI today; baked-in tomorrow.
---
## Risks
- **Workers req/day at 87% of cap on peak day.** Real but mitigated by edge cache.
- **Single DO = single region.** Same as today's Upstash setup; no regression.
- **Test suite rewrite.** testcontainers Redis tests → DO test helpers (Wrangler `unstable_dev` or Vitest CF pool). ~half day.
- **DO storage billing live since Jan 7, 2026.** Free tier still 1 GB/DO; monitoring needed if canvas grows beyond.
---
## Migration Plan (handed off to /ck:plan)
Phases will be derived by the planner. Rough breakdown for sizing:
1. Add DO storage layer (`canvas-storage` + `cooldown` modules inside DO)
2. Add DO methods (`getFullCanvas`, `placePixels`, `accept`)
3. Refactor Worker to thin proxy
4. One-shot import endpoint + run
5. Delete Upstash code paths + dependency
6. Rewrite tests for DO storage
7. Deploy + smoke test
---
## Success Criteria
- [ ] Zero Upstash references in `src/`
- [ ] `package.json` no `@upstash/redis`
- [ ] All existing tests pass against DO storage
- [ ] Canvas reads/writes verified in production
- [ ] WS broadcast still functional
- [ ] Resize procedure documented (bump constants → redeploy → verify)
- [ ] $0 monthly bill confirmed
---
## Unresolved Questions
None — all clarified during brainstorm.
plans/reports/code-review-260417-0919-backend.md
-308
View File
@@ -1,308 +0,0 @@
# Backend Code Review — rplace
Date: 2026-04-17
Scope: backend only (worker.js, durable-objects/, lib/, wrangler.json, package.json)
Reviewer: code-reviewer
## Summary
Backend is small, readable, and the core data path (Hono → Redis BITFIELD → DO broadcast) works. Found **2 Critical**, **5 High**, **6 Medium**, **4 Low**, **3 Nit** issues. Biggest risks: rate-limit retryAfter unit drift, IP-hash collisions sharing rate buckets, missing hibernation API on the DO (cost + memory), and the unprotected GET /api/canvas (2.5MB) becoming an Upstash quota / egress black hole under load.
---
## Critical
### C1. `retryAfter` is in credits, not seconds — silently correct only when `CREDIT_REGEN_RATE == 1`
File: `src/lib/rate-limiter.js:25`
```lua
return {0, accrued, count - accrued}
```
Returned value is the **credit deficit**, but the API contract (worker.js:60, README L133) labels it `retryAfter` in seconds. Today `CREDIT_REGEN_RATE = 1` so deficit-credits ≈ seconds, masking the bug. Anyone tuning regen rate (e.g. `CREDIT_REGEN_RATE = 0.5` for slower regen, or `2` for faster) breaks the contract immediately. Client could happily retry too early and hammer the API.
Fix:
```lua
local deficit = count - accrued
local retryAfter = math.ceil(deficit / tonumber(ARGV[4]))
return {0, accrued, retryAfter}
```
Add a unit test that flips REGEN_RATE to 0.5 and asserts retryAfter doubles.
### C2. `ARGV[4]` (regen rate) is read but `math.floor(elapsed * regen)` truncates fractional regen — fractional rates are silently broken
File: `src/lib/rate-limiter.js:21`
```lua
local accrued = math.min(tonumber(ARGV[3]), credits + math.floor(elapsed * tonumber(ARGV[4])))
```
With `CREDIT_REGEN_RATE = 0.5`, `elapsed * 0.5` is fractional; `math.floor` discards everything < 2 sec elapsed → user never regens credits unless they wait whole multiples. Also: accrued is computed as float-then-floor, but credit values are stored as floats in Redis HASH (HSET stores strings) — string round-tripping a float vs integer causes drift over time.
Fix: explicitly require integer regen rates, OR multiply both sides up to milliseconds and floor at the end. Document constraint in `constants.js`.
---
## High
### H1. IP hash uses a 32-bit space with a weak Java-style hash → collisions at ~65k unique IPs share rate-limit buckets
File: `src/lib/get-user-id.js:11-16`
```js
let hash = 0;
for (let i = 0; i < ip.length; i++) {
hash = ((hash << 5) - hash + ip.charCodeAt(i)) | 0;
}
return `anon:${(hash >>> 0).toString(36)}`;
```
- Birthday paradox: collisions become likely at ≈√(2^32) ≈ 65,536 unique IPs. For a viral r/place clone this is plausible in hours.
- Two colliding users **share** the same `credits:{userId}` HASH → one user's spend depletes the other's credits → griefing vector.
- The hash provides **zero privacy** (no salt, deterministic, function is in the public client repo). If the goal is privacy, use HMAC with a secret env var. If the goal is just opacity in logs, say so.
Fix: Use SHA-256 of IP + a server-side secret salt (`env.IDENTITY_SALT`), truncate to 16 hex chars (64 bits → collisions at ~4B). Use `crypto.subtle.digest('SHA-256', ...)`.
### H2. `getUserId` falls back to `'127.0.0.1'` in dev → all dev users share one credit bucket; in prod the fallback masks misconfiguration
File: `src/lib/get-user-id.js:9`
```js
const ip = request.headers.get('cf-connecting-ip') || '127.0.0.1';
```
- In `wrangler dev` there is no `cf-connecting-ip` header → every request is `anon:<hash of 127.0.0.1>` → testing rate-limit with multiple browser sessions appears broken.
- In production, if a request somehow arrives without the header (e.g. internal Worker→Worker, or test traffic), the silent fallback hides the bug AND grants a fresh user the shared global bucket.
Fix: in dev, fall back to a per-session token (cookie or `request.headers.get('x-forwarded-for')` first hop). In prod, return 400 if header missing — that case should never legitimately happen.
Also: no IPv6 normalization. `2001:db8::1` and `2001:0db8:0000:0000:0000:0000:0000:0001` hash differently. Cloudflare normalizes, but test it.
### H3. Durable Object uses `server.accept()` (non-hibernation) → DO stays pinned in memory while ANY client connected; you pay duration charges 24/7 for an idle room
File: `src/durable-objects/canvas-room.js:33`
```js
server.accept();
this.sessions.add(server);
```
Per Cloudflare docs, `acceptWebSocket()` (hibernation API) lets the runtime evict the DO from memory between messages while keeping connections open. With `accept()`, the DO is pinned for the entire connection lifetime. For a broadcast-only room that receives messages O(once per pixel placement), this is wasted compute spend.
Fix: Use the [WebSocket Hibernation API](https://developers.cloudflare.com/durable-objects/best-practices/websockets/):
```js
constructor(state, env) {
this.state = state;
this.sessions = new Set(state.getWebSockets()); // restore on wake
}
async fetch(request) {
// ...
this.state.acceptWebSocket(server);
this.sessions.add(server);
// ...
}
async webSocketClose(ws) { this.sessions.delete(ws); }
async webSocketError(ws) { this.sessions.delete(ws); }
```
Also: client→server messages (currently none, but if added later) need `webSocketMessage` handler.
### H4. GET /api/canvas: no compression, no real CDN cache, calls Upstash on every request → free-tier Redis quota dies in seconds; egress costs balloon
File: `src/worker.js:12-20`
```js
'Cache-Control': 'public, max-age=1, s-maxage=1, stale-while-revalidate=5',
```
- 2.5 MB raw binary × 10K concurrent loads = 25 GB egress. No `Content-Encoding: gzip` (Worker doesn't compress octet-stream automatically).
- `s-maxage=1` means Cloudflare CDN caches for **1 second**. For a canvas that updates every few seconds this is reasonable, but every miss re-runs `redis.getrange(...)` → 1 Upstash command per load. Upstash free tier = 10K commands/day → 10K page loads kills the day.
- BITFIELD-encoded 5-bit data is not compressible by general-purpose gzip well, but you could (a) cache the response in a Worker KV or DO memory for 1-2 sec, (b) increase `s-maxage` to 5-10s and let WS deltas bring stale clients in sync, (c) serve the canvas from the DO directly (it already has all the writes) instead of re-reading Redis.
Fix priority:
1. Add `Content-Encoding: gzip` (compress on the Worker — gzip a 5-bit packed buffer still saves 30-40% because of zero regions).
2. Bump `s-maxage` to at least `5` (matches `stale-while-revalidate`).
3. Long-term: keep canvas in DO memory and serve from DO; Redis is the durable source-of-truth, DO is the hot read cache.
### H5. /api/place awaits the broadcast call → DO latency is on every write's critical path, AND failures are swallowed silently
File: `src/worker.js:67-76`
```js
try {
const roomId = c.env.CANVAS_ROOM.idFromName('main');
const room = c.env.CANVAS_ROOM.get(roomId);
await room.fetch(new Request('http://internal/broadcast', {
method: 'POST',
body: JSON.stringify(pixels),
}));
} catch (err) {
console.error('Broadcast failed:', err);
}
```
- `await room.fetch(...)` blocks the response to the placing user. DO cold-start (~50-200ms) + network adds latency for the most cost-sensitive path.
- The `try/catch` only catches thrown errors. If the DO returns a 5xx, the response is logged as success and other clients silently miss the update.
- Comment claims "non-blocking" — code is blocking.
Fix:
```js
const broadcast = c.env.CANVAS_ROOM.get(c.env.CANVAS_ROOM.idFromName('main'))
.fetch(new Request('http://internal/broadcast', {
method: 'POST',
body: JSON.stringify(pixels),
}))
.then(r => { if (!r.ok) console.error('Broadcast non-OK:', r.status); })
.catch(err => console.error('Broadcast failed:', err));
c.executionCtx.waitUntil(broadcast);
```
This makes broadcast truly non-blocking and ensures Cloudflare keeps the request alive until it finishes.
---
## Medium
### M1. Pixels persisted to Redis but broadcast can fail → cross-client divergence
File: `src/worker.js:64-76`
`setPixels()` succeeds → broadcast fails (DO error, network) → other clients miss the pixel until they reload `/api/canvas`. README says "real-time updates" but there's no retry, no version vector, no out-of-order detection.
Mitigation: clients periodically re-fetch (not implemented), or add a sequence number to `/api/place` responses and let clients detect gaps from the WS stream.
For now, with H5 fixed (waitUntil + non-OK detection), at least the failure is logged. Document this as a known limitation.
### M2. No duplicate-pixel-in-batch detection → user can spam same coordinate 32× to defeat per-pixel write limits
File: `src/worker.js:39-52`
Validation loop checks bounds but doesn't dedupe. A batch of 32 writes to (0,0) with the same color is wasted work and still drains 32 credits. Worse: a batch with conflicting writes to the same pixel — last-write-wins inside BITFIELD chain — silently discards the others. User pays for all 32.
Fix: dedupe by `${x},${y}` key keeping last entry; OR reject batch if duplicates exist (clearer contract). Update README.
### M3. BITFIELD chain has no overflow handling — silently wraps modulo 32 if `color >= 32` slips through
File: `src/lib/canvas-storage.js:60-62`
Validation in worker.js:46 already enforces `color < MAX_COLORS`, so this is defense-in-depth. But: BITFIELD u5 SET with value > 31 wraps to value mod 32. If anyone bypasses worker validation (direct DO RPC in future, internal admin tools), corruption is silent.
Fix: add explicit guard in `setPixels`:
```js
if (color < 0 || color >= 32) throw new RangeError(`color out of range: ${color}`);
```
### M4. Lua script HSET stores `cr` as the result of `accrued - count` which can be a float string — drift over many requests
File: `src/lib/rate-limiter.js:28-29`
`accrued` comes from `math.min(MAX, credits + math.floor(elapsed*regen))`. Currently regen=1 (integer) and credits start integer, so accrued is integer. But subtraction of `tonumber(ARGV[1])` (also integer) keeps it integer. With non-integer regen (see C2), this becomes "1.0" or "0.99999..." string in Redis. tonumber re-parses but precision can drift over thousands of requests.
Fix: `math.floor(remaining)` before HSET, or always use integer math (multiply rate by 1000, divide later).
### M5. No CORS headers; no security headers (CSP, X-Frame-Options, X-Content-Type-Options)
Files: `src/worker.js` (all routes)
If you ever serve the API from a different domain than the frontend, CORS preflight will fail. If the frontend is embedded somewhere else, no clickjack protection. Octet-stream response without `X-Content-Type-Options: nosniff` lets some browsers MIME-sniff.
Fix: add a small middleware to set:
```js
app.use('*', async (c, next) => {
await next();
c.header('X-Content-Type-Options', 'nosniff');
c.header('Referrer-Policy', 'no-referrer');
});
```
For CORS, only add when needed; default same-origin is safer.
### M6. Errors leak internal stack traces via Hono default error handler
Files: `src/worker.js` (no `app.onError` registered)
If `setPixels` throws (Upstash 5xx, network error), Hono returns the error message + stack to the client by default in development; in production it's a generic 500 but the inner `console.error` still logs internals. Better: explicit error envelope.
Fix:
```js
app.onError((err, c) => {
console.error('Unhandled:', err);
return c.json({ error: 'internal' }, 500);
});
```
---
## Low
### L1. `getFullCanvas`: `atob(data)` may succeed on non-base64 strings that happen to be valid base64 alphabet, returning garbage
File: `src/lib/canvas-storage.js:27-31`
```js
try { raw = atob(data); } catch { raw = data; }
```
A raw binary string accidentally consisting of base64 chars (a..z, A..Z, 0..9, +, /, =) will decode to wrong bytes silently. Better: detect Upstash's encoding mode explicitly. If you set `Upstash-Encoding: base64` header in client config, you know it's base64. Otherwise assume raw.
The current Upstash JS SDK abstracts this — verify which mode it uses for `getrange` (likely already base64 → the try/catch is paranoid but sometimes returns raw on first call). Either way, document the assumption.
### L2. `getFullCanvas` returns zero-filled buffer when key missing, no logging
File: `src/lib/canvas-storage.js:20-22`
Silent. If the canvas key is wiped (manual ops, Redis eviction), every client gets a blank canvas with no indication. Add `console.warn('Canvas key empty or missing')`.
### M3 already covers BITFIELD overflow.
### L3. Sessions Set in DO is unbounded — no max connections per room
File: `src/durable-objects/canvas-room.js:9`
A single DO has a soft limit of ~32k WebSocket connections per Cloudflare's limits, but nothing in this code prevents pathological client behavior (10k connections from one IP). Combined with non-hibernation (H3), this is a vector to keep the DO pinned with high memory.
Fix: add a max sessions cap (`if (this.sessions.size >= 5000) return new Response('busy', { status: 503 });`) and consider per-IP connection limits using rate-limiter.js.
### L4. JSON body size not capped on /api/place
File: `src/worker.js:23-29`
`c.req.json()` will happily parse a 100MB body before validation runs. With MAX_BATCH_SIZE=32, max legitimate body is ~1KB. Send a 100MB payload → Worker burns CPU parsing → 4xx.
Fix: check `c.req.header('content-length')` and reject > 4096 before parsing.
---
## Nit
### N1. `getRedis` creates a new client per request
File: `src/lib/redis-client.js:8-13`
Cheap (no connection pool — REST), but allocates an object every call. A module-scoped singleton keyed on env URL would save GC. Skip if measured cost is negligible.
### N2. Magic `'main'` room id — no constant
File: `src/worker.js:68, 88`
Two callsites with the literal `'main'`. If you ever support multiple rooms, this needs to be parameterized. Extract to `constants.js`.
### N3. `/broadcast` route in DO has no auth — any code that gets a DO stub can broadcast
File: `src/durable-objects/canvas-room.js:16`
DO stubs are scoped to your Worker, so this is theoretical. But if multiple Workers share the namespace, anyone can send arbitrary `pixels` payloads to clients. A shared secret in Worker→DO requests, or RPC over the new DO RPC API, would harden this.
---
## Looked at and OK
- **BITFIELD bit ordering / endianness**: Verified. Redis docs confirm "bit 0 is the most significant bit of the first byte" (big-endian). Decoder in `canvas-decoder.js:18` reads `(hi<<8 | lo) >> (11 - bitOffset) & 0x1f` which extracts bits in the correct order. Cross-checked at offsets 0, 1, 7 (byte boundary), and 8.
- **Last-pixel offset arithmetic**: Pixel 4194303 → bit 20971515 → byte 2621439 (within CANVAS_BYTES=2621440). All 5 bits live in byte 2621439; decoder reads byte 2621440 as `|| 0` which is harmless.
- **Worker Hono routing / method matching**: Routes are simple, no path traversal vectors, no query param handling.
- **WebSocketPair upgrade response shape**: Status 101 + `webSocket: client` is the documented Cloudflare pattern.
- **Set iteration with delete during forEach**: JS Set delete during for..of is safe — iterator advances correctly.
- **Pixel coordinate bounds checks**: x, y, color all validated for type, range, and integer-ness in worker.js:40-52.
- **Rate-limit Lua atomicity**: EVAL is atomic in Redis; HGETALL→math→HSET runs without interleaving.
- **Credit clamp at MAX_CREDITS**: Lua `math.min(MAX, credits + accrued)` correctly caps regen.
- **TTL on credits hash**: 86400s = 24h. Reasonable; long-idle users get fresh full credits.
- **wrangler.json migration**: v1 with new_classes is correct for first-time DO deploy.
- **Hono dependency version**: 4.7.6 is current.
- **Frontend optimistic update on place**: client decrements credits and renders before server ACK; server response corrects credit count. Acceptable UX pattern.
---
## Recommended fix priority order
1. **C1, C2** — fix retryAfter unit + fractional regen now. One commit.
2. **H4** — bump cache, add gzip, before any traffic spike kills your Upstash quota.
3. **H3** — switch to hibernation API (cost reduction, simple change).
4. **H5** — wrap broadcast in `waitUntil`, check response.ok.
5. **H1, H2** — replace IP hash with HMAC + env salt; fix dev fallback.
6. M1-M6, L1-L4, N1-N3 as time permits.
---
## Unresolved questions
1. Is the canvas ever expected to be cleared/reset? If so, who issues the operation (admin endpoint? cron?) and how is it broadcast?
2. What's the deployment plan — same Cloudflare zone for Worker + frontend (no CORS), or split? This determines whether M5 is needed.
3. Upstash plan: free tier (10K cmd/day) or paid? The cost analysis in H4 changes accordingly.
4. Multi-room future plans — N2 only matters if yes.
5. Is there any analytics / observability integration (logpush, Sentry)? Several findings suggest logging that has nowhere to go right now.
---
**Status:** DONE_WITH_CONCERNS
**Summary:** Reviewed 9 backend files. 2 Critical (rate-limit unit bug, fractional regen), 5 High (IP hash collisions, dev fallback, no DO hibernation, canvas endpoint cost, blocking broadcast), 6 Medium, 4 Low, 3 Nit. Core BITFIELD encoding/decoding is correct. Concerns: C1 and C2 ship a latent rate-limit bug that activates the moment anyone tunes regen rate; H4 is a real production-readiness blocker for any traffic above hobbyist scale.
**Concerns/Blockers:** None blocking review; recommend addressing C1/C2/H4 before any public launch.
---
Sources consulted:
- [Redis BITFIELD command](https://redis.io/docs/latest/commands/bitfield/) — bit ordering semantics
- [Upstash REST API](https://upstash.com/docs/redis/features/restapi) — base64 / binary response modes
- [Cloudflare Durable Objects: Use WebSockets](https://developers.cloudflare.com/durable-objects/best-practices/websockets/) — hibernation vs accept
- [Build a WebSocket server with Hibernation](https://developers.cloudflare.com/durable-objects/examples/websocket-hibernation-server/)
plans/reports/code-review-260417-0919-frontend.md
-355
View File
@@ -1,355 +0,0 @@
# Frontend Code Review — rplace
**Date:** 2026-04-17
**Scope:** Frontend only — Svelte 5 + Canvas + WebSocket client
**Files reviewed:** 12 (client/, lib/canvas-decoder.js, lib/constants.js, index.html, vite/svelte config)
**LOC:** ~600 frontend
**Reviewer mode:** adversarial — assume bugs exist
---
## Summary
Solid Svelte 5 runes usage and clean component split. Several concrete bugs around reactivity (`pan` is plain object, won't trigger re-render), zoom math in handleWheel (uses stale `zoom` after onZoomChange), DPR not handled (blurry on retina/HiDPI), missing $effect cleanup for resize, decoder boundary read past buffer end, no UI feedback for 429 / network errors, and credit timer drift on tab inactive. Touch handlers prevent default unconditionally (blocks system gestures off-canvas). No keyboard a11y for color picker. WS reconnect lacks resync — client drifts after reconnect.
---
## Critical
### C1 — `pan` mutation does not trigger reactivity
**File:** `src/client/components/CanvasRenderer.svelte:10, 109-110, 168-169, 180-181, 184-185`
**What:** `let pan = { x: 0, y: 0 };` is plain (no `$state`). Code mutates `pan.x += dx` then calls `render()` manually.
**Why it matters:** Works only because `render()` is called explicitly after every mutation. Fragile — any future code path that mutates `pan` without calling `render()` will silently fail to redraw. Also bypasses the runes-based reactivity model that the rest of the app uses, violating the principle of least surprise.
**Fix:** Either (a) declare `let pan = $state({ x: 0, y: 0 })` and let `$effect(() => { pan.x; pan.y; render() })` fire (simpler, idiomatic), or (b) document explicitly that `pan` is intentionally non-reactive and all mutations must be paired with `render()`.
### C2 — `handleWheel` uses stale `zoom` after `onZoomChange`
**File:** `src/client/components/CanvasRenderer.svelte:124-133`
**What:** Computes `newZoom`, mutates `pan` using the *old* `zoom` prop, then calls `onZoomChange(newZoom)`. The pan math is correct (uses `newZoom / zoom` ratio), but the render pipeline relies on the `$effect(() => { zoom; render(); })` firing after zoom prop updates — which means render uses the new `zoom` *but* with pan computed against the old. Plus, `pan` mutation does not trigger render (see C1), so the wheel-zoom render relies entirely on the zoom prop changing to fire the effect.
**Why it matters:** If user wheel-zooms but newZoom equals current zoom (clamped at 0.25 or 64), pan mutates but render is never triggered → pan visually frozen at zoom limits. Repro: zoom out to min (0.25), keep scrolling down → pan adjustments accumulate invisibly until you scroll up.
**Fix:** Call `render()` explicitly at end of `handleWheel`, or fix C1 (make pan reactive).
### C3 — Decoder reads past buffer end on last pixel
**File:** `src/lib/canvas-decoder.js:18`
**What:** `(bytes[byteIndex] << 8 | (bytes[byteIndex + 1] || 0))` — guards `byteIndex+1` with `|| 0`, but does not guard `bytes[byteIndex]` itself. For a 2048×2048 5-bit canvas: `totalBits = 4194304 * 5 = 20971520 bits = 2621440 bytes`. Last pixel `bitPos = 20971515`, `byteIndex = 2621439`, `bitOffset = 3`. `bytes[2621439]` is valid (last byte index), `bytes[2621440]` is undefined → `0`. OK for exact-size buffer.
**However** — if server returns a truncated buffer (network error, partial response, future smaller canvas), `bytes[byteIndex]` could be undefined → `(undefined << 8 | 0) = 0`. JavaScript silently coerces; you get black pixels instead of an error. No length check on incoming buffer.
**Why it matters:** Silent data corruption on truncated response. Also, `0 || 0 === 0` masks the case where a byte is genuinely 0 vs. missing.
**Fix:** Validate `buffer.byteLength === Math.ceil(totalPixels * 5 / 8)` at top of `decodeCanvas`, throw if mismatch. Caller in `CanvasRenderer.svelte:217-228` should handle the throw and surface to UI.
### C4 — WebSocket reconnect does not resync canvas
**File:** `src/client/App.svelte:42-46`
**What:** On `ws.onclose`, schedules reconnect with backoff. But after reconnect succeeds, client does not refetch `/api/canvas` — any pixels placed by other users during the disconnect window are lost forever (until next full reload).
**Why it matters:** Core correctness violation of the "real-time collaborative" promise. A 30s disconnect (worst-case backoff) on a busy canvas can lose hundreds of updates. User sees stale canvas with no indication.
**Fix:** On `ws.onopen` after a *reconnect* (not initial open), call `canvasRenderer.refetchCanvas()` which re-runs the GET /api/canvas + decode + render flow. Track `isReconnect` flag (true after first close).
---
## High
### H1 — DevicePixelRatio ignored — blurry on Retina/HiDPI
**File:** `src/client/components/CanvasRenderer.svelte:209-213`
**What:** `canvasEl.width = window.innerWidth; canvasEl.height = window.innerHeight;` — sets backing store to CSS pixels. On `devicePixelRatio = 2` displays (Mac, modern phones), every pixel is upscaled by browser → blurry rendering, especially zoomed-in pixel art (the entire point).
**Fix:**
```js
const dpr = window.devicePixelRatio || 1;
canvasEl.width = window.innerWidth * dpr;
canvasEl.height = window.innerHeight * dpr;
canvasEl.style.width = window.innerWidth + 'px';
canvasEl.style.height = window.innerHeight + 'px';
ctx.scale(dpr, dpr); // in render() before pan/zoom
```
Then `screenToCanvas` math stays in CSS pixel space (don't multiply clientX by dpr). Also wheel/touch focal points stay correct.
### H2 — Resize listener leak in onMount
**File:** `src/client/components/CanvasRenderer.svelte:208-231`
**What:** `onMount` returns a cleanup that removes resize listener. But `onMount`'s returned function is the standard Svelte teardown — works for unmount, but the `return` inside an `async` callback of `onMount` returns a Promise, not the cleanup function. Svelte will receive `Promise<() => void>`, not the cleanup fn.
**Why it matters:** Resize listener leaks on every component remount (HMR, route changes if added later, parent re-creates child). Each remount adds another listener → resize fires N times.
**Fix:** Move resize setup outside `async` — use a synchronous `onMount(() => { ... return cleanup; })` for the listener, and a separate `$effect` or top-level `await` for the canvas fetch. Or use `$effect`:
```js
$effect(() => {
function resize() { ... }
resize();
window.addEventListener('resize', resize);
return () => window.removeEventListener('resize', resize);
});
```
### H3 — Credit timer drifts on tab inactive
**File:** `src/client/App.svelte:17-24`
**What:** `setInterval(..., 1000)` regenerates 1 credit/sec client-side. When tab is backgrounded, browsers throttle setInterval to ≥1s but don't pause; on long inactivity (laptop sleep, tab discard), timer doesn't fire at all. After 5 min away, user expects ~256 credits (capped) but client may show only the value from 5 min ago. Server-side credits regen correctly via timestamp math, but client UI lies.
**Why it matters:** UX confusion — user sees "5 credits" but server says they have 256. Optimistic deduction (line 60) deducts from the wrong baseline. First placement after wake will get a server response with the correct credit count, but until then the bar is wrong, and rapid-clicking before that response arrives will be repeatedly rejected for no visible reason.
**Fix:** Track `lastTickTimestamp`. On each tick, compute `elapsed = now - lastTick` in seconds and add `elapsed * CREDIT_REGEN_RATE` (capped). Also re-fetch credits from server on `visibilitychange` → visible, or have server include `credits` in WS hello message.
### H4 — No UI feedback on 429 / network error
**File:** `src/client/components/CanvasRenderer.svelte:64-79`
**What:** On rate-limit (429) or network failure, code logs to console.warn/error. Optimistic pixel stays drawn (line 74 comment), credits remain deducted (line 60). User has no idea their action failed.
**Why it matters:** Silent failure mode. User clicks 10 times, sees 10 pixels appear locally, sees credits drop, then WS broadcasts arrive showing only the first one was accepted — pixels appear to flicker. Especially bad with `retryAfter` info (server provides it but client ignores).
**Fix:** Surface error state via prop callback or store. Show toast/banner: "Rate limited — try again in {retryAfter}s" or "Network error — retrying...". Roll back optimistic update on failure (revert pixel + restore credit).
### H5 — Optimistic credit deduction races with server response
**File:** `src/client/components/CanvasRenderer.svelte:60, 72`
**What:** Client deducts 1 credit optimistically (line 60), then awaits server which returns authoritative count (line 72). Between the two, user can click again — second click sees `credits - 1`, deducts again. But meanwhile credit timer may have ticked, adding back 1. The final `onCreditsChange(data.credits)` clobbers all of that with server truth, but timing of clobber matters.
**Why it matters:** Specifically — rapid clicks while `/api/place` is in flight all use a stale `credits` snapshot from props. Possible to over-spend client-side: 5 rapid clicks with `credits=3` → all 5 see `credits >= 1` at click time (each only checks before its own deduction), all 5 fire requests. Server rejects last 2, but client UI shows 5 placed.
Actually re-reading: `credits` is a $state in App, prop to CanvasRenderer — when `onCreditsChange(credits - 1)` fires, Svelte 5 prop update is sync. Next click in same tick reads new value. So 5 clicks in 5 ticks → 3 succeed, 2 see `credits <= 0` and bail. OK *if* clicks happen sequentially.
Still problematic: between optimistic deduct and server response, if server says `credits = 250` (regen happened server-side), client jumps from `credits - 1` back up to 250, throwing away any other in-flight optimistic deductions.
**Fix:** Either (a) drop optimistic deduction (rely on server response only — slower UX but correct), or (b) track in-flight count and reconcile: `onCreditsChange(data.credits - inFlight)`.
### H6 — Color picker has no keyboard navigation / no roving tabindex
**File:** `src/client/components/ColorPicker.svelte:8-17`
**What:** 32 `<button>` swatches with `aria-label` only. Keyboard users can Tab through all 32 (tedious), no arrow-key grid navigation, no `aria-pressed` to indicate selected. `title` attribute used for tooltip but not for screen readers.
**Why it matters:** WCAG 2.1 keyboard accessibility. Power users want arrow keys.
**Fix:** Use `role="radiogroup"`, swatches as `role="radio" aria-checked={i === selectedColor}`, single `tabindex=0` on selected (others `tabindex=-1`), arrow-key handler to move selection. Add visible focus ring (currently no `:focus` style — invisible focus).
### H7 — Touch `preventDefault` on canvas blocks legitimate browser gestures
**File:** `src/client/components/CanvasRenderer.svelte:151, 163`
**What:** `e.preventDefault()` unconditionally on every touchstart/touchmove. Combined with `touch-action: none` CSS (line 247).
**Why it matters:** Both together is correct *for the canvas*. But:
- `preventDefault` in handler requires the listener to be non-passive. Svelte's `ontouchmove={handleTouchMove}` registers as non-passive automatically when handler calls preventDefault — but Chrome warns about non-passive touchstart listeners on scroll-blocking elements.
- `e.preventDefault()` on touchstart also blocks browser-native double-tap zoom — your app overrides that with pinch-zoom, which is fine, but means iOS users can't double-tap to zoom (they expect this).
- `touch-action: none` on canvas is correct, but the canvas fills the viewport — there is no scrollable area for users to escape. If they want to refresh by pull-down or use system back-swipe-from-edge, behavior is OS-dependent.
**Fix:** Acceptable trade-off given full-screen canvas is the app, but document. Consider not preventing default on touchstart when `touches.length === 1` (only on move, when actually panning) to allow OS gestures to start cleanly. Test mobile Safari edge swipe.
### H8 — Long-press detection has no visual feedback and is timing-sensitive
**File:** `src/client/components/CanvasRenderer.svelte:193-203`
**What:** Long-press = touchend after >300ms with no movement. No visual indicator during the wait — user doesn't know if they're holding correctly. `touchMoved` only flips on >4px move (line 167), so trembling fingers (especially over many seconds) may register movement and silently cancel placement.
**Why it matters:** UX — users will tap-and-hold, see nothing happen, lift, and wonder why nothing was placed. Also: a 200ms tap places nothing (correct, prevents accidental placement during pan), but no feedback distinguishes "tap" from "long-press canceled by movement".
**Fix:** Show a circular progress indicator at touch point during long-press wait. Cancel with haptic or visual on movement. Consider lowering threshold to 250ms and increasing movement tolerance to 8px (trembling).
---
## Medium
### M1 — `applyUpdates` does not validate WebSocket message shape
**File:** `src/client/components/CanvasRenderer.svelte:82-87`
**What:** Iterates `pixels` array and reads `x, y, color`. No bounds check on `x, y` (line 48 calculates `offset = (y * CANVAS_WIDTH + x) * 4` — out-of-bounds write to ImageData would silently corrupt other pixels). Server is trusted, but WS messages parsed in `App.svelte:36` from JSON → if server bug or malicious WS proxy injects bad data, client writes anywhere in the ImageData buffer.
**Why it matters:** Trust boundary — WS messages cross it. Defense in depth.
**Fix:** In `updatePixel` (line 45), early return if `x < 0 || x >= CANVAS_WIDTH || y < 0 || y >= CANVAS_HEIGHT || colorIndex < 0 || colorIndex >= MAX_COLORS`.
### M2 — `render()` not batched via requestAnimationFrame
**File:** `src/client/components/CanvasRenderer.svelte:24-36`
**What:** `render()` called synchronously on every mousemove during pan, every touchmove, every applyUpdates. On a busy WS broadcast (many pixel updates per second), can fire render >60Hz, wasting CPU.
**Why it matters:** Perf — drawImage of 2048×2048 offscreen at 60Hz on a slow device is the bottleneck. Browser will skip frames anyway, but the redundant work runs.
**Fix:** Wrap `render()` in rAF coalescer:
```js
let rafScheduled = false;
function scheduleRender() {
if (rafScheduled) return;
rafScheduled = true;
requestAnimationFrame(() => { rafScheduled = false; render(); });
}
```
Replace all `render()` call sites with `scheduleRender()`. Single-frame coalescing eliminates redundant draws.
### M3 — `putImageData` to offscreen on every render
**File:** `src/client/components/CanvasRenderer.svelte:30`
**What:** `offCtx.putImageData(imageData, 0, 0)` runs on every render — even if ImageData hasn't changed (e.g., user is just panning).
**Why it matters:** Perf — putImageData of 2048×2048 (16MB) every pan frame is expensive.
**Fix:** Track `imageDataDirty` flag. Set true in `updatePixel` and `applyUpdates`, set false after putImageData. Only call putImageData when dirty.
### M4 — `loading` state never propagates to UI placement gating
**File:** `src/client/components/CanvasRenderer.svelte:13, 55-80`
**What:** `loading = $state(true)` shows "Loading canvas..." overlay, but `placePixel` doesn't check it. If user clicks before initial fetch completes, `imageData` is null → `updatePixel` early-returns (line 46), but `onCreditsChange(credits - 1)` already fired (line 60). Credits drop without any pixel placed.
**Why it matters:** UX bug + credit waste.
**Fix:** Early return at top of `placePixel`: `if (loading || !imageData) return;`.
### M5 — Coordinate display lags during fast cursor movement
**File:** `src/client/components/CanvasRenderer.svelte:99-103`
**What:** `onCursorMove` fires on every mousemove, updates `cursorPos` in App which re-renders `CanvasControls`. Throttle would help.
**Why it matters:** Minor perf, possible visible lag on slow devices.
**Fix:** Throttle to ~30Hz with rAF coalescing in App, or use `$effect.pre` on cursorPos.
### M6 — Touch handlers don't update `cursorPos` for `CanvasControls`
**File:** `src/client/components/CanvasRenderer.svelte:150-203`
**What:** Mouse handlers call `onCursorMove` (line 100) but touch handlers don't. On mobile, the coordinates display in CanvasControls stays at `(0, 0)` forever. Possibly intentional (no hover on touch), but inconsistent.
**Fix:** Either hide coords display on touch-only devices (`@media (hover: none)`), or update on touchmove.
### M7 — Pan not clamped — user can pan canvas off-screen entirely
**File:** `src/client/components/CanvasRenderer.svelte:109-110, 168-169`
**What:** `pan.x += dx` accumulates without limits. User can drag the 2048×2048 canvas completely off-screen and lose orientation.
**Why it matters:** UX — easy to "lose" the canvas. Reset zoom button restores zoom but not pan.
**Fix:** Clamp pan so at least `MIN_VISIBLE_PX` (e.g., 50px) of canvas remains visible:
```js
const minX = -CANVAS_WIDTH * zoom + 50;
const maxX = window.innerWidth - 50;
pan.x = Math.max(minX, Math.min(maxX, pan.x));
```
Same for y. Also fix `onResetZoom` in App.svelte:72 to also reset pan.
### M8 — Reset zoom doesn't reset pan
**File:** `src/client/App.svelte:72`
**What:** `onResetZoom={() => zoom = 1}` only resets zoom. Pan stays at whatever the user dragged to.
**Why it matters:** UX — "reset" implies "back to start". User clicks reset expecting to see whole canvas, sees same off-center view at 1x.
**Fix:** Add `onResetPan` callback that sets pan to center-the-canvas (or 0,0). Or have a single "reset view" button that does both. Centering math:
```js
pan = { x: (window.innerWidth - CANVAS_WIDTH) / 2, y: (window.innerHeight - CANVAS_HEIGHT) / 2 };
```
### M9 — `wsRetryDelay` is module-scope (not per-instance)
**File:** `src/client/App.svelte:27`
**What:** Declared outside any function/component scope (well, inside `<script>` but at module top). Works fine for an SPA with single root. But if App is mounted multiple times (testing, dev HMR), they share state.
**Fix:** Move inside `connectWebSocket` as a closure-scoped variable, or use `$state` inside the component.
### M10 — `<title>` only includes app name, no dynamic state
**File:** `src/index.html:6`
**What:** Static title. Could indicate disconnected state, credit availability, etc.
**Fix:** Optional. Could use `<svelte:head>` to show "(disconnected)" prefix when WS down.
---
## Low
### L1 — `zoomLabel` formatting breaks for non-power-of-2 zoom
**File:** `src/client/components/CanvasControls.svelte:4-6`
**What:** `1 / zoom` for fractional zoom — fine for 0.5, 0.25 (gives "1/2x", "1/4x"), but for 0.333 gives "1/3.0030030030030033x". Currently zoom is always doubled/halved so always power of 2 — but pinch-zoom (line 177) produces continuous values. After pinching to e.g. 0.7x, label shows "1/1.4285714285714286x".
**Fix:** `zoom >= 1 ? \`${zoom.toFixed(1)}x\` : \`${(zoom * 100).toFixed(0)}%\``. Or `Math.round(zoom * 100) / 100`.
### L2 — Hardcoded zoom limits duplicated
**File:** `App.svelte:70-71`, `CanvasRenderer.svelte:127, 177`
**What:** `0.25` and `64` hardcoded in 4+ places.
**Fix:** Add `MIN_ZOOM = 0.25, MAX_ZOOM = 64` to `constants.js`.
### L3 — `selectedColor = $state(27)` magic number
**File:** `src/client/App.svelte:8`
**What:** `27` is index of black in palette. Comment says so but not self-documenting.
**Fix:** Add `BLACK_INDEX = 27` or `DEFAULT_COLOR_INDEX = 27` to constants.
### L4 — `applyUpdates` does not deduplicate concurrent updates from optimistic + WS echo
**File:** `src/client/components/CanvasRenderer.svelte:55-87`
**What:** Client places pixel, optimistically updates locally (line 61), server broadcasts to all WS including the placer, client receives its own pixel back via `applyUpdates`, redraws same pixel.
**Why it matters:** Wastes a render cycle. If multiple pixels in batch, multiple re-draws.
**Fix:** Server could include placer ID and skip echo, OR client just lives with the redundant render (cheap if M3 fix applies). Low priority.
### L5 — Inline event handlers create new closures each render
**File:** `src/client/App.svelte:63, 65, 66, 70-72, 75`
**What:** `onCreditsChange={(c) => credits = c}` etc. — fresh function each parent render → child sees new prop → may trigger child re-render.
**Why it matters:** Svelte 5 fine-grained reactivity may handle this, but explicit functions are cheaper and more debuggable.
**Fix:** Optional. Hoist to `function setCredits(c) { credits = c; }` etc.
### L6 — Hex parsing in constants happens at module load, no error handling
**File:** `src/lib/constants.js:32-35`
**What:** `parseInt(hex.slice(1), 16)` — if any palette entry is malformed, NaN propagates silently, RGBA values become NaN, ImageData rejects.
**Why it matters:** Currently safe (palette is hardcoded, validated by humans). Defense-in-depth.
**Fix:** Optional. Validate at module load with `if (n !== n) throw` or similar.
### L7 — `Cache-Control: max-age=1` on /api/canvas
**File:** `src/worker.js:17` (server-side, but affects client behavior)
**What:** Browser cache for 1s. If user reloads twice within 1s, second load gets stale canvas. Fine for fresh page loads, but interacts with C4 fix (refetch on reconnect): the refetch may hit stale cache.
**Fix:** When refetching after reconnect, add `?_=Date.now()` cache-buster to bypass cache.
### L8 — `app.css` uses `overflow: hidden` on body without `html`
**File:** `src/client/app.css:11`
**What:** `body { overflow: hidden; }` but `html` not set. Some browsers (older Safari) need both.
**Fix:** `html, body { overflow: hidden; }`.
---
## Nit
### N1 — `cursor: grabbing` only when `dragging` true; otherwise crosshair
**File:** `src/client/components/CanvasRenderer.svelte:247`
**What:** Should show `cursor: grab` when hovering (not dragging) to hint draggability. `crosshair` suggests pixel-place mode, which is also true. Mixed UX signal.
**Fix:** Style: `crosshair` is appropriate for placing; `grabbing` during pan. Alternative: use `crosshair` always, since you can both place and drag from any state.
### N2 — `loading` state used only as guard but always finally set false
**File:** `src/client/components/CanvasRenderer.svelte:226-228`
**What:** Even on fetch failure, `loading = false` and overlay disappears. User sees blank canvas with no error indicator.
**Fix:** Add error state and overlay: "Failed to load — refresh".
### N3 — `console.warn`/`console.error` not gated by env
**File:** `App.svelte:39 (commented)`, `CanvasRenderer.svelte:75, 78, 225`
**What:** Production builds will log. Vite default keeps console.* unless explicitly stripped. Minor.
**Fix:** Use `import.meta.env.DEV` gate, or just leave (cheap diagnostic in prod).
### N4 — Variable name `lastMouse` reused for touch center
**File:** `src/client/components/CanvasRenderer.svelte:155, 158, 188`
**What:** `lastMouse` set from touch coords. Misleading name.
**Fix:** Rename to `lastPointer` to cover both.
### N5 — `aria-label="Select color #6d001a"` reads as "select color hash six d zero zero one a"
**File:** `src/client/components/ColorPicker.svelte:15`
**What:** Screen readers will spell out the hex character by character. Useless to a blind user.
**Fix:** Use color names ("dark red", "white", etc.) — adds 32 strings to constants but actually accessible. r/place's palette has standard names available.
### N6 — Build/dev: no source maps configured for production debugging
**File:** `vite.config.js`
**What:** No `build.sourcemap` set.
**Fix:** Add `sourcemap: true` for prod debugging (or `'hidden'` to not expose to users).
### N7 — `rgba` array creation in `indicesToRgba` could use bitwise pack
**File:** `src/lib/canvas-decoder.js:31-42`
**What:** Could use Uint32Array view for 1-write-per-pixel instead of 4. Marginal perf gain (~2-3x) on 4M pixel array.
**Fix:** Optional perf optimization for slower mobile devices.
---
## Looked at and OK
- **constants.js palette / RGBA precompute** — clean, immutable, correct hex→RGBA math.
- **canvas-decoder.js bit math** — correct 5-bit unpack formula `(bytes[i] << 8 | bytes[i+1]) >> (11 - bitOffset) & 0x1f`. Boundary handling for last byte OK *if* buffer length validated (see C3).
- **`indicesToRgba` fallback to `COLORS_RGBA[0]`** — graceful for OOB color indices. Good defense-in-depth.
- **OffscreenCanvas usage (CanvasRenderer:21)** — correct pattern for high-zoom rendering, avoids re-uploading ImageData per pan frame *if* M3 fix applies.
- **`imageSmoothingEnabled = false` (CanvasRenderer:27)** — correct for pixel art.
- **WebSocket protocol switching (App.svelte:30)** — correctly handles HTTPS→WSS upgrade.
- **WS exponential backoff** — capped at 30s, sensible.
- **Vite proxy config** — `/api` → :8787, ws: true, correct for dev mode.
- **`bind:this={canvasRenderer}`** — correct Svelte 5 ref pattern; `applyUpdates` exported correctly.
- **`$derived` usage in UserInfo, CanvasControls** — clean, no side effects, correct reactivity.
- **`touch-action: none`** — correct CSS for canvas drag/pan.
- **CSS `transform: translate(-50%, -50%)` for centering** — standard and correct.
- **Server-side validation (worker.js:40-52)** — full pixel validation independent of client; no trust boundary leak. Good.
- **`Number.isInteger` checks** — catches NaN/float attacks.
- **No XSS surface** — no innerHTML, no `{@html}` usage. All Svelte interpolation auto-escaped.
- **No localStorage / sensitive data on client** — nothing to leak.
- **No hardcoded URLs** — uses `location.host`, environment-portable.
---
## Recommended Actions (priority order)
1. **Fix C1 + C2** (pan reactivity + handleWheel render) — silent visual bugs.
2. **Fix C3** (decoder buffer length validation) — silent data corruption.
3. **Fix C4** (WS reconnect canvas refetch) — core correctness.
4. **Fix H1** (devicePixelRatio) — visible quality issue, easy fix.
5. **Fix H2** (resize listener leak in async onMount) — memory leak per remount.
6. **Fix H3** (credit timer drift) — UX confusion after tab inactive.
7. **Fix H4** (UI feedback for 429/error) — silent failure mode.
8. **Address M1** (applyUpdates validation) — defense in depth for WS trust boundary.
9. **Address M3 + M2** (render coalescing + ImageData dirty flag) — perf for busy canvas.
10. **Address M4** (placePixel before load) — credit waste.
11. **Address M7 + M8** (pan clamp + reset includes pan) — UX polish.
12. Lows + Nits as time permits.
---
## Metrics
- Files reviewed: 12 (all frontend + shared lib)
- LOC: ~600
- Critical: 4
- High: 8
- Medium: 10
- Low: 8
- Nit: 7
- Positive observations: 18
- Type coverage: N/A (vanilla JS, JSDoc partial)
- Test coverage: 0 (no test files in frontend)
- Linting: not checked (no config visible in scope)
---
## Unresolved Questions
1. Is the optimistic-update strategy intentional? If yes, what's the rollback policy on server reject? README implies "WS will correct" — does server actually broadcast the *correct* pixel back when rejecting an attempt? (Saw worker.js — server does *not* broadcast on rejection, so client's optimistic pixel stays drawn forever until user pans away or another update arrives at that location.)
2. Why no test coverage for the canvas-decoder? Bit-packing math is exactly the kind of code that benefits from unit tests (round-trip encode/decode against known fixtures).
3. Is mobile a P0 platform? Long-press UX (H8) and DPR (H1) matter most on mobile.
4. Should the canvas be panned-clamped, or is "lose the canvas off-screen" considered acceptable (some r/place clones intentionally allow it)?
5. WebSocket — what happens if Durable Object hibernates and the client's WS goes idle? Is there a heartbeat from server, or does TCP keepalive handle it? Browsers may close idle WS after 60s of no traffic.
6. Are emoji-rich labels (e.g., from N5 color names) acceptable in JSON / does the project have an i18n strategy planned?
---
**Status:** DONE_WITH_CONCERNS
**Summary:** Solid Svelte 5 architecture but 4 critical correctness bugs (pan reactivity, wheel render gating, decoder boundary, WS resync), 8 high-severity (DPR, listener leak, credit drift, no error UX), and several mid-tier UX gaps. No security vulnerabilities found in frontend scope (server validates independently). Recommend addressing C1-C4 before next deploy.
**Concerns:** Frontend has silent failure modes (C2, H4, C4) that won't surface in CI but will frustrate real users on mobile/lossy networks. No tests cover the decoder or canvas math.
plans/reports/code-review-260417-0919-security-scale.md
-280
View File
@@ -1,280 +0,0 @@
# rplace — Adversarial Security & Scalability Review
**Date:** 2026-04-17
**Scope:** ~934 LOC. CF Worker + Hono + Upstash Redis + Svelte 5
**Reviewer angle:** abuse / cost amplification / scale limits
---
## Executive Summary
The project ships a small, clean codebase, but rate-limit identity is essentially trivial to bypass and there is **no edge cache** on the 2.5MB canvas endpoint. A single laptop can drain Upstash free tier in minutes. **Going public without fixes 1–3 below = guaranteed outage and/or unbounded cost.**
Critical: 2 · High: 5 · Medium: 7 · Low: 4
---
## CRITICAL
### C1 — Rate limiter trivially bypassed via IP-only identity (collision + spoofing)
**File:** `src/lib/get-user-id.js:7-17`, `src/worker.js:55-58`
**Vector:**
- Identity = 32-bit non-cryptographic hash of `cf-connecting-ip`. Hash space = `2^32`. Birthday collisions at ~65k unique IPs cause innocent users to share buckets. Targeted preimage trivial (4-byte hash, attacker can find IPs that collide with victim).
- IPv6: each user gets a /128, but ISPs hand out /48–/64 prefixes. Attacker on Hurricane Electric tunnel / cloud provider has billions of /128s = billions of free buckets → effectively unlimited pixel rate.
- IPv4 mobile NAT / corporate NAT: many real users share one IP → all share one 256-credit bucket. Already broken for legitimate users, before any attack.
- Tor / open proxies / residential proxy services (BrightData, etc.): attacker rotates IPs at $0.50/GB → 1k req/sec each at full credit refill.
- Cloudflare Workers themselves cost $0 outbound; an adversary can deploy a free worker that proxies through ~100 colos → 100 distinct `CF-Connecting-IP` values immediately.
**Impact:** Rate limit is theater. Attacker can repaint the entire 4M-pixel canvas in minutes. Free tier will burn before the canvas finishes redrawing once.
**Mitigation:**
- Aggregate IPv6 to /64 (or /48) before hashing.
- Drop the JS hash; use the raw IP (stored only in Redis with TTL — already private, never echoed back). The hash adds zero security and creates collisions.
- Layer a global rate limit (Cloudflare Rate Limiting Rules in front of Worker, free up to 10k req/10s).
- Optional: require a signed cookie / hCaptcha turnstile token to gate first placement; cookie identifies bucket instead of IP.
### C2 — `/api/canvas` is uncached → every request hits Upstash for 2.5MB
**File:** `src/worker.js:12-20`, `src/lib/canvas-storage.js:14-46`
**Vector:**
- `Cache-Control: public, max-age=1, s-maxage=1, stale-while-revalidate=5`. `s-maxage=1` means Cloudflare edge revalidates every 1 second, and **CF only caches `Cache-API`-stored or static asset responses by default for Workers fetch responses unless explicitly stored**. This dynamic Hono response is **not in CF cache** without `caches.default.put()`. Even if it were, every 1s window = full re-fetch.
- Each request triggers `redis.getrange("canvas", 0, 2_621_439)` over Upstash REST/HTTP. Upstash free tier: 10k commands/day, 256MB egress/day.
- 2.5MB × 100 requests = 250MB → **single user at F5 spam burns the entire daily egress in <30 seconds**.
- Worker CPU: parsing base64 of 2.5MB + `Uint8Array` loop bytewise = easily exceeds the 10ms free-tier CPU budget per request (50ms paid). `atob` of ~3.5MB base64 string is ~30–80ms cold. Will hit "exceeded CPU" 1015 errors under load.
- `data` from `getrange` is loaded as a JS string then iterated character-by-character — on a 2.6MB string this is ~10ms+ of pure JS even without atob.
**Impact:** First viral moment = $$$ overage and/or service outage. One user with curl loop = denial of service.
**Mitigation:**
- Use Cloudflare Cache API explicitly: `caches.default.match(req)` → on miss `fetch from Redis`, then `caches.default.put(req, response.clone())` with `max-age=2`. This caches at edge, so Upstash sees ≤1 req/sec/colo regardless of load.
- Better: snapshot canvas to R2 every N seconds (cron trigger), serve from R2 (free egress to CF). `/api/canvas` becomes a redirect or R2 binding fetch.
- Even better: store canvas IN the Durable Object (in-memory + persisted to DO storage), broadcast deltas, never round-trip to Upstash on read.
- Use `Uint8Array` from `Buffer.from(data, 'base64')` instead of charCodeAt loop.
---
## HIGH
### H1 — `/api/place` does **not** cache-bust `/api/canvas`
**File:** `src/worker.js:23-79`
After a write, the cached canvas (if any) becomes stale until `max-age` expires. New tabs will load a 1-second-stale canvas, see the WS pixel update, and apply it on top of stale state. With unset pixels (no read-modify-write), this is OK by accident — but as soon as you cache properly (C2), you must purge or version the canvas key.
**Mitigation:** Append `?v={epoch_seconds}` query in client; bump on WS reconnect. Or use `If-None-Match` ETag from a Redis-stored canvas-version counter.
### H2 — WebSocket connections are not rate-limited and use **standard** (non-hibernating) WS API
**File:** `src/durable-objects/canvas-room.js:30-44`
**Vector:**
- `server.accept()` uses the standard WebSocket API. Standard WS keeps the DO instance billable in active memory continuously. The hibernation API (`state.acceptWebSocket()`) lets the DO sleep between events.
- A single DO has soft cap ~32k WebSockets, hard memory cap 128MB. Each connection carries event-listener closures + JS state.
- All clients hit `idFromName('main')` → **single DO instance** for the whole world. No sharding, no cap, no auth. An attacker opens 50k WS connections from cloud → DO OOMs or hits CPU limit, **all real users disconnected**.
- No per-IP cap on WS. No origin check. CSWSH (Cross-Site WebSocket Hijacking) trivially possible — any malicious page can `new WebSocket("wss://your.app/api/ws")` and read pixel placements (though this stream is not sensitive, the DDoS vector remains).
- Broadcast uses `await room.fetch(...)` per `/api/place` → blocks request return on DO round-trip. Under load, /api/place latency = DO fan-out latency.
**Impact:** Single attacker disconnects everyone. Cost: standard WS instances stay billed 24/7 even when idle; hibernation reduces this 100x.
**Mitigation:**
- Switch to hibernation API: `this.state.acceptWebSocket(server)` + `webSocketMessage()` / `webSocketClose()` handlers on the class.
- Enforce per-IP WS connection cap (track in DO memory): reject if same IP has >5 connections.
- Validate `Origin` header on upgrade — reject non-allowlisted origins.
- Make broadcast fire-and-forget: `c.executionCtx.waitUntil(room.fetch(...))` instead of `await`. Don't block client response.
- Shard by region or hash if you ever go big (`idFromName(\`room:${region}\`)`).
### H3 — `setPixels` does N writes per batch via builder chain → not actually atomic per-batch
**File:** `src/lib/canvas-storage.js:54-64`
**Vector:**
- Comment claims "single atomic BITFIELD command" but Upstash REST `bitfield` builder pattern executes a **single Redis command** with N subcommands — that part *is* atomic. **Good.**
- However: `BITFIELD canvas SET u5 #X v ... SET u5 #Y v` is **one HTTP request to Upstash REST** carrying 32 subcommands. At 2 KB request body size, fine.
- BUT the request between `/api/place` and `/api/canvas` is **not** atomic: a client doing GET then POST sees racy state if pixels arrive between them. WS layer compensates if connected first. Initial-load race window is real but cosmetic.
- Real risk: **rate limiter Lua** runs in a *separate* Upstash REST call from the BITFIELD write. Sequence:
1. POST handler validates pixels (CPU-bound)
2. Lua `eval` (network trip 1)
3. BITFIELD `exec` (network trip 2)
4. DO `fetch` for broadcast (network trip 3)
- A client can fire 32 parallel `/api/place` requests with `count=1` each; each reads credits = 256, deducts 1, writes 255 — all atomic individually but 32 succeed where only 256 should ever exist. Wait — the Lua re-reads on each call so it's serialized through Redis single-thread. **Lua is atomic per script call**, so 32 parallel /place requests serialize and only the first 256 succeed total. **OK.**
- Real exposure: between Lua approving and BITFIELD writing, if BITFIELD fails the credit was already deducted — user loses credits with no pixel placed. No compensation logic.
**Impact:** Lost-credit grief on Upstash hiccup; potential reverse race where user sees credit deducted but pixel never appears.
**Mitigation:** Reverse the order — write pixels first, then deduct credits (refund pixel buffer if credits insufficient is harder). Or wrap both calls in pipeline + handle failure by refunding via second Lua call. At minimum, log the inconsistency and let WS reconcile.
### H4 — Body size is unbounded — DoS via giant JSON
**File:** `src/worker.js:23-37`
**Vector:** `await c.req.json()` parses the entire request body before checking `pixels.length > MAX_BATCH_SIZE`. Attacker sends 100MB JSON `{"pixels":[...10M items...]}`. Worker loads all of it into memory, then the length check fires after.
- CF Workers do enforce a 100MB request body cap on free tier, but a 100MB POST per request × 1k req/sec = 100GB ingress amplification.
- More subtle: `{ "pixels": [...32 items...], "garbage": "<10MB string>" }` — passes length check, wastes CPU on JSON parse.
**Impact:** CPU and memory amplification. Easy to push request over 50ms CPU limit and trip 1102 errors.
**Mitigation:** Check `content-length` header before reading body: reject if > 4 KB (max valid batch is ~2 KB). Use `c.req.raw.body` and abort if exceeds limit.
### H5 — No CSP, X-Frame-Options, X-Content-Type-Options, Referrer-Policy
**File:** `src/worker.js` (no header middleware), `src/index.html` (no meta CSP)
**Vector:**
- Page can be iframed by any origin → clickjacking. Attacker overlays UI tricks user into placing pixels they didn't mean to. Long-press touch = especially exploitable on mobile (transparent overlay over color picker → user "places" attacker's pixel pattern).
- No CSP → if any future feature ever renders user-supplied text, immediate XSS. Currently no UGC text, but pixel coordinates and color from network are not the only risk; a future feature drift will break this.
- API responses lack `X-Content-Type-Options: nosniff`. The 2.5MB binary canvas could be sniffed as HTML by old browsers if served from same origin.
**Mitigation:** Add Hono `secureHeaders()` middleware or set:
```
Content-Security-Policy: default-src 'self'; script-src 'self'; connect-src 'self' wss:; img-src 'self' data:; style-src 'self' 'unsafe-inline'; frame-ancestors 'none'
X-Frame-Options: DENY
X-Content-Type-Options: nosniff
Referrer-Policy: no-referrer
```
---
## MEDIUM
### M1 — DO room state lost on eviction; no canvas sync on WS connect
**File:** `src/durable-objects/canvas-room.js`, `src/client/App.svelte:26-55`
- DO never persists `sessions`. On eviction (CF migrates DO between machines), in-flight broadcasts during migration window dropped. Acceptable but worth noting.
- Client `onclose` reconnects but **never refetches the canvas**. After WS gap (network blip, sleep, throttle), client's `imageData` is now stale; pixels placed during disconnect window are missed forever — until full page reload.
**Mitigation:** On WS `onopen` after a previous `onclose`, refetch `/api/canvas` and replace `imageData`. Or: server pushes a sequence number per pixel batch; client requests gap fill via REST.
### M2 — `applyUpdates` does not validate WS payload
**File:** `src/client/components/CanvasRenderer.svelte:82-87`
- WS payload `data.pixels` iterated and written directly to ImageData. No bounds check, no integer check. A malicious DO message (or Man-in-the-Middle on plain `ws://` if HTTPS-stripped) could write `x=1e9` → `offset` is huge → silent JS array OOB write that is no-op but wastes cycles. Or `x=1.5` → `(y * W + x) * 4` = fractional offset = writes to wrong pixel.
- Server controls the WS messages via `setPixels` which already validates. Defense-in-depth: validate at the boundary anyway.
**Mitigation:** Reuse the same validator from worker.js in client `applyUpdates`. Drop invalid pixels silently.
### M3 — Optimistic UI never rolls back on rejection
**File:** `src/client/components/CanvasRenderer.svelte:55-80`
- `placePixel` deducts credits + paints local pixel before fetch. On 429 rate-limited, client logs warning and **leaves the wrong-color pixel on screen** until WS broadcast or refresh. Worse: client has now decremented credits the server never deducted (server returned `remaining` but client only updates on `data.ok`). User sees impossibly low credit count.
- Comment says "WS will correct" — only true if the *real* color was placed by someone else; if no one paints over, ghost pixel persists locally forever.
**Mitigation:** On non-ok, restore credits (`onCreditsChange(credits + 1)`) and undo pixel by repainting from cached server state (need to keep "last server state per pixel" or just re-fetch).
### M4 — `console.error('Broadcast failed', err)` and other error paths leak nothing externally — but error responses leak Redis behavior
**File:** `src/worker.js:74-76`, `src/lib/canvas-storage.js`
- `console.error` is fine — CF logs only. Not an external leak.
- However: an Upstash REST 5xx (network blip) bubbles unhandled out of `getFullCanvas` → Hono returns 500 with whatever Hono's default body is. Could include `Error: fetch failed` stack in dev mode. Production CF strips stacks, so impact is mostly UX.
- `getrange` on missing key returns empty in code path: covered. But `redis.bitfield(...).exec()` failure throws → /api/place returns 500 with credits already deducted (see H3).
**Mitigation:** Wrap each Redis call in try/catch; return generic `{error: "upstream_unavailable"}` 503. Add `app.onError()` handler that always returns sanitized JSON.
### M5 — DO `/broadcast` endpoint accepts any inbound request that reaches it
**File:** `src/durable-objects/canvas-room.js:16-27`
- The DO is only addressable via DO bindings — cannot be hit from public internet directly. Risk is zero from outside.
- BUT: any worker bound to `CANVAS_ROOM` (including future workers in same account) can post arbitrary JSON pixels, which then get broadcast to all clients with `type: 'pixels'` shape unchecked.
- The DO does not validate the broadcast payload (`x`, `y`, `color` ranges). Trusts caller.
**Mitigation:** Validate pixel shape in `/broadcast` handler too. Defense-in-depth — the DO is the source of truth for what gets fan-out.
### M6 — Long-lived WebSocket DO holds entire `Set<WebSocket>` in memory
**File:** `src/durable-objects/canvas-room.js:9, 19-25`
- `for (const ws of this.sessions) { try { ws.send(message); } catch { this.sessions.delete(ws); } }` — modifying Set during iteration. JavaScript spec allows this (Set iterator handles deletes mid-iteration), but the `catch` triggers if `send` throws on a closed socket and the `close`/`error` listener may not have run yet. Resulting state OK, just inefficient.
- At 10k WS, broadcasting a 32-pixel update = 32 × 10k = 320k JSON serialization events? No — `JSON.stringify` once, then 10k `send` calls. Each `send` enqueues a frame; no backpressure. Slow client backs up the queue → DO memory grows.
**Mitigation:** Use hibernation API (H2) which the runtime handles efficiently. Drop slow clients (track outstanding bytes via some heuristic; not directly exposed).
### M7 — `EXPIRE 86400` on credits hash means inactive users keep MAX_CREDITS for 24h
**File:** `src/lib/rate-limiter.js:30`
- Inactive user comes back after 25h → key gone → `credits = MAX_CREDITS` (initialized from ARGV[3]). Good.
- Same user coming back at 23h59m → still has whatever credits (probably full from regen). Also good.
- BUT key resets on every successful place, so "active spammer" key never expires; bot can hold key alive forever for free in Redis.
**Mitigation:** Negligible at user scale. If Redis storage matters, drop EXPIRE down to 1h since regen recovers full credits in 256s anyway.
---
## LOW
### L1 — `Math.floor(Date.now() / 1000)` precision: drift in credit accrual
- 1-second precision on rate limiter means if user posts at t=1.99s and again at t=2.01s, lastUpdate goes 1→2 but elapsed=1s gives 1 credit. Effectively floor() rounding cuts up to 1 credit per place call. Cosmetically fine.
### L2 — Color picker / UserInfo z-index conflicts at small viewports
- All overlays z-index 10, color-picker bottom-center, user-info top-left. Touch users on small phones may have controls overlap canvas tap zones near edges.
### L3 — `parseInt(hex.slice(1), 16)` in `constants.js` runs on module init both client and worker
- Trivial cost (32 colors × 4 ops). Mentioning only because the import surface could be split: client doesn't need worker-only constants and vice versa.
### L4 — `redis-client.js` constructs new Redis object per request
- `new Redis({...})` each call is cheap (no connection pool — REST is stateless HTTP). Fine. Worth noting in case it's mistakenly converted to TCP client later.
---
## Scalability — concrete numbers
### Per-action cost
| Action | Upstash commands | Worker CPU | Egress |
|---|---|---|---|
| `GET /api/canvas` (uncached) | 1 (GETRANGE 2.6MB) | ~50ms (atob+loop) | 2.6MB |
| `GET /api/canvas` (cached, ideal) | ~0 (every 2s/colo) | ~5ms | 2.6MB from edge |
| `POST /api/place` (32 pixels) | 2 (EVAL + BITFIELD-32-subcmds) | ~10ms | <1KB |
| WS broadcast (per place) | 0 | ~5ms × N clients | 1KB × N clients |
### 1k concurrent users, 1 pixel/sec each
- /place: 1000 req/s × 2 Upstash cmd = **2000 cmd/s** → 172M cmd/day.
- Upstash free tier: 10k cmd/day. **Burns in 5 seconds.**
- Pay-as-you-go: $0.20/100k cmd → **$345/day**.
- WS: 1000 connections in single DO. Soft cap fine, but no hibernation = always-on DO memory ~50MB (~50KB/conn).
- Broadcast fan-out: 1000 messages/sec to DO, each broadcast to 1000 sockets = **1M sends/sec**. DO CPU **maxed**. Real cap: a single non-hibernating DO can sustain ~10k–30k sends/sec sustained.
- Worker invocations: 1000/s × 86400 = 86M/day. Free tier = 100k/day. **Burns in 100 sec.**
### 10k WS connections
- Single DO `idFromName('main')` = single instance. CF DO doc: ~32k WS soft cap, 128MB memory.
- 10k × ~50KB JS state = 500MB → **OOM**. Hibernation API drops to ~1KB/socket idle = 10MB. **Must use hibernation.**
### Cold-start /api/canvas
- Cold worker init: ~10–30ms.
- Upstash REST call from CF colo to closest Upstash region: typically 30–80ms RTT.
- 2.5MB body transfer: ~50ms over 1Gbps from Upstash to CF.
- atob(3.5MB base64 string) in V8: 30–80ms.
- charCodeAt loop on 2.6MB string: 10–30ms.
- **Total cold p99: ~200–300ms; CPU time: 60–100ms** → exceeds free tier 10ms CPU per request. Will trip "exceeded CPU limit" errors immediately on free tier.
### Bottleneck order before things break
1. **Worker free-tier CPU limit (10ms)** — broken by /api/canvas on first request.
2. **Upstash free-tier daily commands (10k/day)** — broken by ~100 page loads or ~5k pixel placements.
3. **Worker free-tier daily requests (100k)** — broken by ~10 active users.
4. **DO memory (128MB)** — broken at ~2.5k concurrent WS without hibernation.
5. **Single DO CPU** — broken at ~10k pixels/sec broadcast rate.
6. Upstash bandwidth — broken at ~100 canvas fetches.
### Cost to run at "1k DAU, 100 pixels/user/day"
- Place: 100k × 2 = 200k Upstash cmd/day → $0.40/day Upstash pay-as-you-go.
- Canvas reads (1k users × ~5 loads/day uncached): 5k × 2.6MB = 13GB egress. Upstash bandwidth $0.03/GB → $0.39/day. **Cached: <$0.01/day.**
- Workers: 100k place + 5k canvas = 105k requests = free tier breakeven. Paid: $0.30/M = $0.03/day.
- DOs: 1 instance × 24h with WS = $0.15/GB-hour memory + $0.20/M requests. Hibernating: ~$0.50/day. Standard: ~$5/day.
- **Realistic minimum monthly cost at 1k DAU done right: $30–60/mo. Done wrong (uncached canvas, no hibernation): $500+/mo.**
---
## Top 3 things to fix BEFORE going public
1. **Cache `/api/canvas` at the edge** (C2). Use CF Cache API explicitly. Without this, *one user* can take you down. **Single most important fix.**
2. **Switch DO to WebSocket Hibernation API + cap connections per IP + validate Origin** (H2). Otherwise one botnet shuts down realtime for everyone.
3. **Add CF Rate Limiting Rules in front of the Worker, plus IPv6 /64 aggregation in `getUserId`** (C1). Identity-layer rate limit alone is insufficient; need a network-layer floor that isn't easy to spoof. Add `secureHeaders()` middleware (H5) in the same change.
Bonus near-mandatory: bound request body size (H4), make broadcast non-blocking via `waitUntil` (H2), and refetch canvas on WS reconnect (M1).
---
## Positive Observations
- BITFIELD with 5-bit packing is the right primitive — atomic per call, dense on disk.
- Lua script for credit deduction is genuinely atomic (Redis single-threaded eval).
- Pixel input validation in `/api/place` covers integer, range, NaN, type — quite thorough.
- Stackable credits with regen is the right UX model (matches Reddit r/place).
- Codebase is small and readable; good module separation (constants, redis-client, rate-limiter, canvas-storage are correctly factored).
- WS reconnect with exponential backoff in client is well-implemented (App.svelte:42-46).
- No use of `eval` / `Function`, no obvious prototype-pollution sinks. Body parsed via Hono safely.
---
## Unresolved Questions
1. Is the project intended for production (real users) or demo? Several mitigations (hCaptcha, CF Rate Limiting Rules) add friction; only worth it if real adversaries expected.
2. What's the expected concurrent user ceiling? 100? 10k? Decides whether single-DO-room is sufficient or sharding is needed.
3. Is account on Cloudflare paid plan or free tier? Free tier 10ms CPU is hard cap that breaks /api/canvas immediately.
4. Is canvas reset (full wipe) ever needed? No `DEL canvas` admin endpoint exists. Adversarial fill = irreversible.
5. Should pixel placement carry attribution (who placed which pixel)? No audit log currently — abuse takedown impossible.
6. Mobile NAT collision: acceptable for users to share buckets, or need cookie-based bucketing?
---
**Status:** DONE_WITH_CONCERNS
**Summary:** Codebase is clean and small; security is largely fine for a demo, but the IP-only rate limit and uncached 2.5MB canvas endpoint will break under public load — going live without C1, C2, H2 fixed will result in immediate cost overrun and/or outage.
**Concerns:** (a) project may already be intended public — fixes 1–3 are blocking; (b) free-tier limits will trip on first viral burst regardless of attacker presence.
plans/reports/code-review-260417-0926-backend.md
-246
View File
@@ -1,246 +0,0 @@
# Backend Code Re-Review — rplace
Date: 2026-04-17
Scope: backend re-review after 10 commits since prior review (`code-review-260417-0919-backend.md`)
Reviewer: code-reviewer
Commits reviewed (newest → oldest): `8e1f8c4 fcddb1f 33cfd3d b35769c e3eb34c e0cf802 eef6879 50f4365 c357a3f f97ca4d`
## Summary
Substantial progress: H3 (Hibernation API), the binary canvas read (b35769c), and the Redis key prefix (f97ca4d) are now implemented. **C1, C2, H1, H2, H4, H5, M1–M6 are all UNCHANGED** — the new commits focus on infra correctness (BITFIELD, base64, hibernation, observability) and the new client batch-drawing flow, but did not touch rate-limiter, get-user-id, or the canvas/place HTTP layer. Two **new Critical** issues introduced: (1) silent migration switch from `new_classes` → `new_sqlite_classes` on the same `v1` tag will brick re-deploys against an existing DO namespace; (2) `MAX_BATCH_SIZE` was raised 32 → 512 without a corresponding rate-limit ceiling check, so a single request can request 2× the credit cap and burn server work before the rate-limiter rejects it. One **new High**: `redisRaw` ignores the response body, so an Upstash 200-with-error payload silently passes for every BITFIELD write. Hibernation handler implementation is *almost* right but `webSocketClose` calling `ws.close()` is unnecessary and the `wasClean` parameter is ignored — minor.
---
## Prior Findings Status
| ID | Status | Evidence |
|----|--------|----------|
| C1 (`retryAfter` in credits not seconds) | **Unchanged** | `src/lib/rate-limiter.js:25` still `return {0, accrued, count - accrued}` |
| C2 (`math.floor(elapsed*regen)` truncates fractional) | **Unchanged** | `src/lib/rate-limiter.js:21` identical |
| H1 (32-bit IP hash collisions) | **Unchanged** | `src/lib/get-user-id.js:11-16` identical |
| H2 (silent `127.0.0.1` fallback) | **Unchanged** | `src/lib/get-user-id.js:9` identical |
| H3 (DO uses `accept()` not hibernation) | **Fixed (with minor nits)** | `src/durable-objects/canvas-room.js:31` `state.acceptWebSocket(server)`; broadcast iterates `state.getWebSockets()` (line 17). See N4–N5 below for residual nits |
| H4 (GET /api/canvas: no compression, `s-maxage=1`) | **Unchanged** | `src/worker.js:17` still `s-maxage=1`, no `Content-Encoding` set; per-request hits Upstash via `redisRawBinary` |
| H5 (broadcast awaited, 5xx swallowed) | **Unchanged** | `src/worker.js:74` still `await room.fetch(...)` inside `try/catch`; no `waitUntil`, no `r.ok` check |
| M1 (broadcast/persist divergence) | **Unchanged** | Same code path as H5 |
| M2 (no batch dedup) | **Unchanged** (server) — *fixed in client* | `src/worker.js:39-52` no dedup. Client `pixel-buffer.js:40-48` does dedup, but server cannot trust client |
| M3 (BITFIELD u5 overflow guard) | **Unchanged** | `src/lib/canvas-storage.js:42-51` no explicit `color < 32` guard inside `setPixels` |
| M4 (HSET float drift) | **Unchanged** | `src/lib/rate-limiter.js:28-29` identical |
| M5 (CORS / security headers) | **Unchanged** | No middleware in `src/worker.js` |
| M6 (no `app.onError`) | **Partially mitigated** | `src/worker.js:64-69` adds `try/catch` around `setPixels` returning a JSON envelope; still no global `app.onError`; `c.req.json()` failure handled (line 27); other route exceptions still default-handled |
| L1 (atob garbage-decode risk) | **Fixed by design** | `src/lib/canvas-storage.js:22` now decodes from a *known-base64* response (explicit `Upstash-Encoding: base64` header) — no ambiguity left |
| L2 (silent zero-fill on missing key) | **Unchanged** | `src/lib/canvas-storage.js:18-20` returns zeros, no warn |
| L3 (unbounded sessions Set) | **N/A — Set removed** | Hibernation API: no app-managed Set; CF runtime handles. Per-room hard cap not enforced (see new finding M11) |
| L4 (no body size cap) | **Unchanged** | `src/worker.js:26` `c.req.json()` reads unbounded body |
| N1 (per-request Redis client) | **Unchanged** | `src/lib/redis-client.js:8-13` identical |
| N2 (magic `'main'` room id) | **Unchanged** | `src/worker.js:72, 92` |
| N3 (`/broadcast` route in DO has no auth) | **Unchanged** | `src/durable-objects/canvas-room.js:14` |
---
## New Critical
### NC1. `wrangler.json` migration changed `new_classes` → `new_sqlite_classes` on the SAME `v1` tag (commit c357a3f) — re-deploy against an existing DO namespace will fail or wipe state
File: `D:/tiennm99/rplace/wrangler.json:16-21`
```json
"migrations": [ { "tag": "v1", "new_sqlite_classes": ["CanvasRoom"] } ]
```
- Migration tags are immutable per environment. If `v1` was previously deployed with `new_classes` (which was the case in commit `fc49de1` — verified via `git show fc49de1:wrangler.json`), Cloudflare's deploy system will either (a) reject the re-applied `v1` tag with a different shape, or (b) on first publish under a new account succeed, but in any prod environment that already saw the old `v1`, the deploy will mis-identify the class storage mode.
- If the Worker has *never* been deployed yet, this is fine. If it was deployed once, the correct fix is to add a **`v2` migration** like `{ "tag": "v2", "delete_sqlite_classes": [], "new_sqlite_classes": ["CanvasRoom"] }` and coordinate state migration; usually you cannot convert non-SQLite → SQLite DOs in place.
- The `gitignore` of `.wrangler/` in the same commit suggests local state was wiped to make this work locally, masking the issue.
Action: confirm whether the DO has ever been deployed under a real Cloudflare account. If yes, do not push; consult Cloudflare DO migration docs. If no, change the tag to `v2` (or leave `v1` only if you are confident no deploy ever happened).
### NC2. `MAX_BATCH_SIZE` raised 32 → 512 (`constants.js:11`) but `MAX_CREDITS = 256` — server accepts 512-pixel POST then rejects via rate-limit, after fully validating + reading body
File: `D:/tiennm99/rplace/src/lib/constants.js:11-13`
```js
export const MAX_BATCH_SIZE = 512;
export const CREDIT_REGEN_RATE = 1;
export const MAX_CREDITS = 256;
```
Flow: `worker.js:35` accepts body up to 512 pixels → loop validates all 512 (lines 40-52) → `checkAndDeductCredits(env, userId, 512)` → Lua sees `count=512 > accrued≤256` → returns rate_limit. So **a legitimate user can never spend a 512-pixel batch in one shot**, but a malicious client can flood the server with 512-element JSON arrays that always fail validation/rate-limit. Server burns CPU on validation + a Redis EVAL roundtrip per request. Combined with L4 (no body cap), each rogue request can carry MBs of `{x,y,color}` objects.
Action options (pick one):
1. Cap `MAX_BATCH_SIZE = MAX_CREDITS` (i.e. 256) so the system contract is internally consistent.
2. Validate `pixels.length <= MAX_CREDITS` at the worker before the per-pixel loop and reject earlier with a clear `error: 'exceeds_max_credits'`.
3. Change credit math to allow oversized batches (probably wrong product-wise).
Add a `content-length` cap (L4) regardless.
---
## New High
### NH1. `redisRaw` returns `res.json()` (the whole envelope) — Upstash REST returns `{result, error}`; an `error` field is silently ignored, so a BITFIELD write can fail with HTTP 200 and the worker reports success
File: `D:/tiennm99/rplace/src/lib/redis-client.js:21-35`
```js
if (!res.ok) { ... throw ... }
return res.json();
```
- Upstash REST: a malformed command returns HTTP 200 with `{"error":"ERR ..."}`. The current code only throws on `!res.ok`. `setPixels` doesn't inspect the return value (`canvas-storage.js:50` is `await redisRaw(env, command)`), so a BITFIELD that fails server-side (bad offset syntax, eviction, OOM) returns 200, the worker logs no error, then **broadcasts pixels that were never persisted**. Clients see them flicker, then they vanish on reload.
- Inconsistent with `redisRawBinary` which destructures `result` from the body (line 56), but it also doesn't check `error`.
Fix:
```js
const body = await res.json();
if (body && Object.prototype.hasOwnProperty.call(body, 'error')) {
throw new Error(`Redis error: ${body.error}`);
}
return body.result;
```
Apply to both `redisRaw` and `redisRawBinary`. Update `setPixels` if it ever needs the result.
### NH2. `getFullCanvas` zero-pads when Upstash returns short data, masking partial reads / corruption
File: `D:/tiennm99/rplace/src/lib/canvas-storage.js:28-33`
```js
if (bytes.length < CANVAS_BYTES) {
const padded = new Uint8Array(CANVAS_BYTES);
padded.set(bytes);
return padded;
}
```
- For a fresh canvas, GETRANGE on a non-existent key returns empty → handled at line 18 (zero-fill). Good.
- For a *truncated* response (Upstash request truncation, mid-deploy state, partial write), this silently zero-pads the tail → users see the canvas blank below some row → no log, no metric. Could mask a real corruption incident for hours.
- Redis GETRANGE on a 2.5MB key has no documented Upstash response size cap on REST, but a network-layer truncation IS possible.
Fix: log a warning when `bytes.length < CANVAS_BYTES` *and* the canvas key is known to exist (separate `EXISTS` check, or remember the key was non-empty). Or just `console.warn` with the byte counts and serve zero-padded — the warn alone makes future incidents debuggable.
### NH3. Per-request `fetch()` to Upstash on `GET /api/canvas` with `s-maxage=1` — same as prior H4, but now I can confirm the fetch is `redisRawBinary` issuing a path-style GET that's NOT cacheable by Cloudflare's reverse cache (auth header forces miss-on-vary)
File: `D:/tiennm99/rplace/src/lib/canvas-storage.js:14-16` + `src/lib/redis-client.js:44-58`
- The internal Upstash `fetch` carries an `Authorization: Bearer ...` header. Cloudflare's outbound cache will not store responses with `Authorization` unless `Cache-Control: public` is set on the upstream (Upstash does not). So every Worker invocation re-pays the Upstash request, even within the 1-second `s-maxage` window — the CDN cache only protects against re-invocations of the *Worker's own response* by re-using the worker's outgoing `Response`.
- This is the same H4 risk, restated with the new code path. Fix priority hasn't changed: gzip the response, raise `s-maxage`, or cache in DO memory.
### NH4. `webSocketClose` calls `ws.close(code, reason)` — for Hibernation, this is unnecessary and may emit a duplicate close frame; also `wasClean` param dropped silently
File: `D:/tiennm99/rplace/src/durable-objects/canvas-room.js:42-44`
```js
webSocketClose(ws, code, reason, wasClean) {
ws.close(code, reason);
}
```
- Per CF docs: "Calling close() is safe but no longer required" with the auto-close compatibility (compat date `2026-04-07`). Project's `compatibility_date = "2025-04-01"` — **older than that** — so auto-close is OFF. Without auto-close, calling `ws.close()` here is the documented pattern, but calling it on **every** close (including `wasClean=true` client-initiated) is redundant: the client already sent the close frame. The runtime won't crash, but you're sending an extra close frame back that the disconnected peer ignores.
- More importantly: `wasClean` is dropped — if false (abnormal disconnect), there's no metric/log; this would have helped diagnose the hibernation bug that motivated `eef6879`.
- `webSocketError` similarly closes with code 1011 unconditionally (no log of what `error` was) — debugging future broadcast failures will be hard.
Fix: at minimum, log `code/reason/wasClean` and `error.message` (with rate-limiting if noisy); consider bumping `compatibility_date` to `>= 2026-04-07` and removing the `ws.close()` calls.
---
## New Medium
### M7. `getRedis()` is called inside `checkAndDeductCredits` but the SDK is unused for canvas writes — two clients, two error envelopes
Files: `src/lib/rate-limiter.js:42` (uses `@upstash/redis` SDK `eval()`), `src/lib/canvas-storage.js` (uses raw fetch).
- Rate-limit failure errors are SDK-shaped; canvas write errors are raw-fetch-shaped. Logging downstream has to handle both. Not urgent, but worth a note.
### M8. `redisRawBinary` URI-encodes args via `encodeURIComponent` but the BITFIELD/GETRANGE *path* mode does NOT support binary args anyway — fine for keys/numbers, undefined for arbitrary bytes
File: `src/lib/redis-client.js:44-58`
- Current usage is GETRANGE with key + integer offsets, all ASCII. Safe today.
- If anyone later calls `redisRawBinary(env, ['SET', key, binaryValue])`, `String(binaryValue)` will produce mojibake. Document or assert ASCII args only.
### M9. `pixel-buffer.js:44` uses `y*65536+x` as a Map key — fine for 2048×2048, but a hard-coded magic number with no constant link
File: `src/lib/pixel-buffer.js:44, 64, 74`
- Three callsites all do `y*65536+x`. Should be a single helper or use `${x},${y}` string. Magic 65536 will silently collide if `CANVAS_WIDTH` ever exceeds 65535 (won't soon, but the constant is in `constants.js` and ought to be referenced).
### M10. `pixel-buffer.js` `pixelCount` getter rebuilds a `Set` on every read — O(strokes × pixels-per-stroke) per access
File: `src/lib/pixel-buffer.js:71-77`
- Likely called from a Svelte reactive expression on every render (toolbar `pixelCount` display). For a user with 10 strokes × 50 pixels each, every keystroke / interaction iterates 500 entries to compute one number. Cache it (`addStroke/undo/redo/clear` mutate, recompute lazily).
### M11. No per-room or per-IP WebSocket connection cap
File: `src/durable-objects/canvas-room.js`
- Hibernation removed the explicit `Set`, but Cloudflare's per-DO 32k WS limit is still the only ceiling. One client can open hundreds of WS connections (cheap on the client) → 100% of room broadcast bandwidth goes to one peer.
- Combine with H1 (IP hash collisions) and abuse becomes hard to identify per-IP.
- Fix: cap connections in `fetch` upgrade path, e.g. `if (state.getWebSockets().length >= 5000) return new Response('busy', { status: 503 })`.
### M12. Broadcast loop swallows `ws.send` errors then forces close — but `getWebSockets()` includes hibernated sockets, and closing one is an active-write that may wake the entire DO
File: `src/durable-objects/canvas-room.js:17-23`
- For 5000 hibernated sockets, sending in a tight `for` loop wakes the DO, allocates 5000 messages, and any failed send forces a `ws.close()` which itself triggers `webSocketClose` → recursive accounting. Mostly fine, but at scale you want either (a) `await Promise.allSettled(...)` to parallelize and not block on one slow socket, or (b) detect mass-failure and back off.
---
## New Low
### L5. `canvas-room.js` constructor signature dropped `env` parameter
File: `src/durable-objects/canvas-room.js:6`
```js
constructor(state) {
```
- Cloudflare DO constructor is `constructor(state, env)`. Dropping `env` means future code that needs env (e.g. for logging, secrets, BROADCAST_AUTH) has to refactor. Costless to keep `constructor(state, env) { this.state = state; this.env = env; }`.
### L6. `pixel-buffer.js` getters duplicate logic between `getAllPixels` and `getAffectedKeys`
File: `src/lib/pixel-buffer.js:40-67`
- Same nested loop, only the value differs. Extract a private `forEachPixel(cb)` helper.
### L7. Tests directory now exists (`test/**`) but no CI configuration was inspected — verify GitHub Actions / pre-push runs them
Out-of-scope file: not in commit list, but worth a check.
---
## New Nit
### N4. Hibernation API reference: `compatibility_date` is `2025-04-01` — predates `2026-04-07` auto-close compat flag; consider bumping
File: `wrangler.json:4`. Once you bump, the `ws.close()` calls in NH4 become outright redundant.
### N5. `webSocketError` log loses `error` content; `webSocketMessage` no-op should at least drop the connection (defensive)
File: `src/durable-objects/canvas-room.js:37-49`. Clients aren't supposed to send messages — if one does (misbehaving client / abuse), silently ignoring it is acceptable, but a `ws.close(1003, 'unexpected message')` would be safer (1003 = "received data of a type that cannot be accepted").
### N6. `console.error` everywhere, no structured logging
With `observability.logs.invocation_logs = true` enabled (commit `50f4365`), structured `console.log({event:'...', ...})` would be queryable in CF Logpush — currently logs are free-form strings.
---
## Looked at and OK (new code)
- **`pixel-buffer.js`** — pure client-side abstraction, *not imported by worker or DO*; `Grep` confirms only `src/client/components/CanvasRenderer.svelte` consumes it. No conflict with `canvas-storage.js`. Undo/redo logic is correct (push to `undone` on `undo`, clear `undone` on new `addStroke`). Stroke-level granularity is reasonable for a paint-mode UX.
- **`canvas-room.js` Hibernation switch (eef6879)** — `state.acceptWebSocket(server)` is called correctly; `state.getWebSockets()` is queried inside `/broadcast` (not stale at wake-up); `webSocketMessage`/`webSocketClose`/`webSocketError` all defined (required by hibernation runtime). Sessions Set removed, no stale state. Broadcast still works after hibernate-and-wake because `getWebSockets()` returns connections persisted by the runtime.
- **`canvas-storage.js` base64 decode (b35769c)** — explicit `Upstash-Encoding: base64` header guarantees the response IS base64; `atob` is now safe (was previously paranoid). The `Uint8Array(raw.length)` + per-char `charCodeAt` is the standard binary-string-to-bytes pattern, correct for octets 0–255.
- **`redis-client.js` raw command shape** — `POST` to base URL with a JSON-array body is the documented Upstash REST raw-command shape (verified via Upstash docs). `Bearer` auth header correct.
- **`redisRawBinary` path encoding** — `encodeURIComponent` per arg + `/` join + base64-encoding header is documented Upstash pattern for binary-safe responses.
- **Redis key prefix (f97ca4d)** — `REDIS_KEY_PREFIX = 'rplace:'` applied at both call sites: `REDIS_CANVAS_KEY` (`constants.js:17`) and `${REDIS_KEY_PREFIX}credits:${userId}` (`rate-limiter.js:44`). Consistent. Lua script in rate-limiter uses `KEYS[1]` so prefix is in the key passed in — clean.
- **`wrangler.json` observability** — `invocation_logs:true` + `traces.enabled:true` is the documented shape for the new observability config (no schema concerns).
- **Tests added (`test/**`)** — coverage for `canvas-storage`, `pixel-buffer`, `get-user-id`, `redis-client`, integration roundtrip, DO. Good signal even if not reviewed in detail this pass.
---
## Recommended fix priority order
1. **NC1** — investigate whether the `v1` migration was ever deployed; if yes, add `v2` before next deploy. (May be a deploy-blocker.)
2. **NH1** — check `error` field in `redisRaw` / `redisRawBinary`. Silent BITFIELD failure is the worst kind of bug.
3. **C1, C2** — still latent rate-limit bugs from prior review, must fix before any tuning of regen rate.
4. **NC2** — reconcile `MAX_BATCH_SIZE` vs `MAX_CREDITS`; add early reject + content-length cap.
5. **H4 (= NH3)** — canvas read cost; bump `s-maxage`, gzip, or move to DO-cached.
6. **H5** — `waitUntil` + `r.ok` check on broadcast.
7. **H1, H2** — IP hash + dev fallback.
8. **NH2** — log truncation case in `getFullCanvas`.
9. **NH4 / N4 / N5** — hibernation polish: bump compat date, log close codes, defensive close on unexpected client message.
10. **M7–M12, L5–L7, N6** as time permits.
---
## Unresolved questions
1. **NC1 critical:** has this Worker (with `CanvasRoom` DO) ever been deployed to a real Cloudflare account, or is `wrangler dev` the only user so far? Determines whether the `v1`→`new_sqlite_classes` switch is a brick-on-deploy or a no-op.
2. The constants change `MAX_BATCH_SIZE = 512` (commit `e0cf802`, batch drawing feature) — was the rate-limit ceiling intentionally left at 256 for cost control, or is `MAX_CREDITS` supposed to be raised to match? Product decision needed.
3. Is the canvas ever expected to be cleared/reset? Still unanswered from prior review.
4. Are tests run in CI? `test/` directory exists; haven't verified GH Actions config.
5. Is there a planned bump of `compatibility_date` (currently `2025-04-01`) to enable WS auto-close (≥ `2026-04-07`)? Affects NH4 cleanup.
6. Long-term: any plan to move canvas reads from Upstash to DO memory (NH3 mitigation, also unlocks per-DO multi-room sharding)?
---
**Status:** DONE_WITH_CONCERNS
**Summary:** Re-reviewed 6 files (2 new, 4 modified) + verified 21 prior findings. **3 prior items fixed (H3, L1, L3), 14 unchanged, 1 partial (M6).** **2 new Critical (NC1 wrangler migration, NC2 batch/credit mismatch), 4 new High (NH1 silent Redis errors, NH2 silent canvas truncation, NH3 = restated H4, NH4 hibernation polish), 6 new Medium, 3 new Low, 3 new Nit.**
**Concerns/Blockers:** NC1 may block next production deploy. NH1 (silent BITFIELD failures) is the highest-impact production bug — failed writes are reported as success, broadcast pixels that don't exist on reload. C1/C2/H4/H5 from prior review remain. Strongly recommend a follow-up commit addressing NC1, NH1, plus C1+C2 before any traffic.
plans/reports/code-review-260417-0926-frontend.md
-256
View File
@@ -1,256 +0,0 @@
# Frontend Code Re-Review — rplace (post commit e0cf802)
**Date:** 2026-04-17
**Scope:** Frontend re-review after batch-drawing rewrite + 9 other commits
**Files reviewed:** App.svelte, CanvasRenderer.svelte, DrawToolbar.svelte (NEW), pixel-buffer.js (NEW), ColorPicker.svelte, CanvasControls.svelte, UserInfo.svelte, canvas-decoder.js, constants.js
**Reviewer mode:** adversarial — verify prior findings + scrutinize new batch-drawing flow
**Prior report:** code-review-260417-0919-frontend.md
---
## Prior Findings Status
| ID | Severity | Status | Evidence |
|---|---|---|---|
| C1 | Critical | **Unchanged** | `pan = { x: 0, y: 0 }` (CanvasRenderer:13) still plain object. All mutation sites (172-173, 180-181, 206-207, 252-254, 263-266) still rely on explicit `render()`. New `handleWheel` (206-207) mutates `pan` but `render()` only fires through `$effect(() => { zoom; render(); })` (285) — same problem. |
| C2 | Critical | **Unchanged** | `handleWheel` (202-209) still mutates pan then calls `onZoomChange(newZoom)`. If newZoom equals current zoom (clamped at 0.25 / 64), `$effect` does not re-run → pan changes are not rendered. No explicit `render()` at end of handler. |
| C3 | Critical | **Unchanged** | `canvas-decoder.js:18` identical: `(bytes[byteIndex] << 8 | (bytes[byteIndex + 1] || 0))`. No `buffer.byteLength === Math.ceil(totalPixels * 5 / 8)` check at top of `decodeCanvas`. Caller (CanvasRenderer:296-308) catches errors but does not surface to UI. |
| C4 | Critical | **Unchanged** | `App.svelte:46-50` only resets `wsRetryDelay` on `onopen`. No `isReconnect` flag. No call to `canvasRenderer.refetchCanvas()` (method doesn't exist). After WS reconnect, drift persists until full page reload. |
| H1 | High | **Unchanged** | `CanvasRenderer.svelte:289-290` still `canvasEl.width = window.innerWidth; canvasEl.height = window.innerHeight;`. No `devicePixelRatio` handling, no `ctx.scale(dpr, dpr)`, no separate CSS sizing. Blurry on Retina. |
| H2 | High | **Unchanged** | `onMount(async () => { ... return () => window.removeEventListener('resize', resize); })` (287-311). Returned cleanup is inside async — Svelte receives a `Promise<fn>`, not the fn. Resize listener leaks on remount/HMR. |
| H3 | High | **Unchanged** | `App.svelte:21-28` still naïve `setInterval(..., 1000)` without `lastTickTimestamp` math. No `visibilitychange` handler. Tab-inactive drift persists. Comment at line 20 ("server corrects on submit") acknowledges drift but does not fix the UI lie. |
| H4 | High | **Regressed (in different direction)** | New `handleSubmit` (App:73-101) catches errors but only `console.error` / `console.warn`. No toast/banner. New failure mode: on rejected submit, `commitPending()` is NOT called — pending pixels stay in buffer, `submitting` resets to false, but user sees no error message. Worse than before because the user has potentially placed *many* pixels via batch and they all silently fail to commit. |
| H5 | High | **Fixed (architecture change)** | Optimistic credit deduction removed entirely. `placePixel` no longer exists; pixels accumulate in `pixelBuffer` locally. Credits only decremented on successful submit (App:91 `credits = data.credits` from server). Race no longer possible. New trade-off: client never gates "is this batch affordable?" client-side — server may reject entire batch. See N1 below. |
| H6 | High | **Unchanged** | `ColorPicker.svelte:8-17` identical. Still no `role="radiogroup"`, no roving tabindex, no arrow-key handler, no `aria-pressed`/`aria-checked`, no `:focus` style. |
| H7 | High | **Unchanged** | `handleTouchStart`/`handleTouchMove` (CanvasRenderer:223, 240) still call `e.preventDefault()` unconditionally. Same trade-off discussion applies. |
| H8 | High | **Partially Fixed** | Long-press-to-place still in `handleTouchEnd` (276-281) without visual indicator. Movement threshold still 4px (245). Touch start still records time (226), but no progress UI shown during the 300ms wait. *However*, draw mode (drag-to-draw, 247-250) now provides a clear alternative for mobile — users wanting to place multiple pixels can switch to draw mode and just drag. Reduces severity of H8 in practice. |
---
## New Critical Findings (Batch Drawing Flow)
### NC1 — Submit failure leaves buffer + UI in inconsistent state with no recovery
**File:** `src/client/App.svelte:73-101`
**What:** On HTTP error (network, 500), or `data.ok === false` (rate_limited, batch_too_large, invalid_pixel, storage_failed), code logs to console and resets `submitting = false`. Pending pixels stay in buffer. Credits not updated. User sees their pixels still drawn locally with the Submit button re-enabled but no indication anything went wrong.
**Specifically broken paths:**
- `429 rate_limited`: server returns `{ retryAfter, remaining }` — both ignored. User can immediately re-click Submit and get rejected again in a loop.
- `400 batch_too_large`: returned when `pixels.length > 512`. Buffer can grow unbounded (see NC2). User clicks Submit, gets silent reject, pixels still pending forever.
- `400 invalid_pixel`: should be impossible if client validates, but server returns `pixel: p` so client could surface bad-pixel info.
- Network/timeout: `catch (err)` — buffer state preserved but no retry UX.
**Why it matters:** With per-pixel placement (old flow) the cost of failure was 1 pixel. With batch drawing, a single failed Submit can lose hundreds of pixels of work that the user thinks they've saved. In a multi-user scenario, those pending-but-not-submitted pixels also block WS pixel updates from other users at those coords (line 98 of CanvasRenderer: `if (buffer.getColorAt(x, y) < 0 && !currentStrokeKeys.has(...))`) — so the user is staring at a stale local view.
**Fix:** Add error toast/banner UI (single text element + state). On 429 honor `retryAfter` (disable Submit button, countdown). On 400 surface error code. On 500/network preserve buffer and show "Submit failed — try again". Consider `commitPending()` only the successfully accepted pixel range if server returns partial-accept (server currently doesn't, but should — see UQ4).
### NC2 — Pixel buffer is unbounded — memory & client-side batch-size violation
**File:** `src/lib/pixel-buffer.js`, `src/client/components/CanvasRenderer.svelte:74-90`
**What:** No client-side check on buffer size. `addToStroke` keeps appending to `currentStroke` until `mouseup`/`touchend`. `addStroke` keeps pushing to `strokes` array. User in draw mode can drag for 30 seconds covering 100k+ pixels (a full-canvas swipe at 1x zoom hits 2048×2048 = 4M coords — `addToStroke` dedupes via `currentStrokeKeys` so realistic cap is `CANVAS_WIDTH * CANVAS_HEIGHT = 4M` unique pixels, eating ~64MB just for the Set keys).
**Concrete failure:** User drags across canvas to draw a long line. `currentStrokeKeys` is a `Set` of integers. After 100k pixels it's ~3-5MB. `currentStroke` is array of `{x, y, color}` objects — ~4MB. Then `buffer.addStroke` deep-copies via `[...pixels]` (pixel-buffer.js:13) → another 4MB. `getAllPixels()` builds another Map → another 4MB. Then `JSON.stringify` for fetch body → ~5MB string. Server has `MAX_BATCH_SIZE=512` so this is rejected with `batch_too_large` after burning all that memory and bandwidth.
**Worse:** `pixel-buffer.js:71-77 pixelCount` getter rebuilds a Set on EVERY access. The toolbar shows `pixelCount` reactively, so every reactivity tick (and there are many during a draw stroke) rebuilds the Set. O(N×M) where N=strokes, M=pixels per stroke.
**Why it matters:** Memory exhaustion on long draws. Failed submits with no recovery (NC1 compounds). Janky UI from O(N×M) `pixelCount`.
**Fix:**
1. Cap `currentStrokeKeys.size` at `MAX_BATCH_SIZE` (512). When reached, `addToStroke` early-returns or auto-finishes the stroke.
2. Cap total `buffer.pixelCount` at `MAX_BATCH_SIZE`. Disable drawing when full. Show warning in toolbar.
3. Cache `pixelCount` in pixel-buffer, invalidate on `addStroke`/`undo`/`redo`/`clear`.
4. Match client batch limit to server's `MAX_BATCH_SIZE` from constants (currently 512). Import and use it explicitly: `if (buffer.pixelCount >= MAX_BATCH_SIZE) return;`.
### NC3 — `committedColors` accessed before fetch completes — null deref
**File:** `src/client/components/CanvasRenderer.svelte:71, 96, 127, 137, 300`
**What:** `committedColors = null` initially, only assigned at line 300 inside `onMount`'s async fetch block. If WS message arrives before initial fetch completes (race condition: WS opens fast, canvas fetch takes >fetch-time), `applyUpdates` (94-103) writes to `committedColors[...]` — null deref → TypeError → unhandled. The `catch` in `connectWebSocket`'s `ws.onmessage` (App:43) silently swallows JSON parse errors but NOT this TypeError because it's outside the `try { JSON.parse ... }` block.
Wait — re-read App:37-44: the `try { ... } catch { /* ignore */ }` wraps the entire onmessage including `canvasRenderer.applyUpdates(...)`. So the error IS swallowed. But that means the WS update is silently dropped, and `committedColors` is never updated for that coord — drift on those pixels until next fetch.
Same null-deref in `restorePixel` (71), `clearPending` (127), `commitPending` (137) — all assume `committedColors` exists. None can be triggered before mount completes (they're only called via user interaction, which presumably comes after load), but there's no defensive check.
**Why it matters:** WS pixel updates that arrive during initial canvas load are silently dropped. Drift after page load if other users are actively placing.
**Fix:** Initialize `committedColors = new Uint8Array(CANVAS_WIDTH * CANVAS_HEIGHT)` at top, OR queue WS updates until fetch completes, OR guard `applyUpdates` early-return: `if (!committedColors) { pendingUpdates.push(pixels); return; }` and replay in onMount.
---
## New High Findings
### NH1 — `pixelCount` getter is O(N×M) on every access; called reactively from App.svelte
**File:** `src/lib/pixel-buffer.js:71-77`, `src/client/App.svelte:15, 132`, `src/client/components/CanvasRenderer.svelte:62-66`
**What:** `notifyBuffer` (CanvasRenderer:62) constructs `{ pixelCount: buffer.pixelCount }` on every stroke addition/undo/redo/clear/commit. The getter rebuilds a `Set` from all strokes' all pixels. `addToStroke` calls `notifyBuffer` indirectly (no — actually only `finishStroke` calls it). OK then — only fires on stroke completion, not per-pixel. But `getColorAt` (51-58) IS called per applyUpdate per pixel (CanvasRenderer:98), and IS O(N×M).
**Why it matters:** WS broadcast of 100 pixels with N=20 strokes of M=50 pixels each → 100 × 20 × 50 = 100k iterations per WS message.
**Fix:** Maintain a `Map<key, color>` inside pixel-buffer cached on stroke add/undo/redo. `getColorAt` becomes O(1). Same data also serves `getAllPixels` and `pixelCount`.
### NH2 — Undo/redo loses strokes silently when WS update arrives at affected coords
**File:** `src/client/components/CanvasRenderer.svelte:94-103, 105-119`
**What:** Scenario: user draws stroke at (100,100) red. WS broadcast arrives saying another user placed (100,100) blue. Code path:
1. `applyUpdates` line 96: `committedColors[idx] = blue` ✓
2. Line 98: `buffer.getColorAt(100, 100)` returns red (pending) → branch NOT taken → display stays red ✓ (correct — don't overwrite user's pending).
3. User clicks Undo. `undo()` line 105: pops stroke, calls `restorePixel(100, 100)`.
4. `restorePixel` line 69-72: `pending = buffer.getColorAt(100, 100)` returns -1 (we just popped the stroke). Falls through to `committedColors[idx]` = blue. Display becomes blue. ✓ Correct! Blue is the truth.
Actually OK — the design works. But: what about Redo? User Redo's the stroke. `redo()` line 113-119: re-applies stroke as red. Display becomes red (pending). On submit, server gets red, accepts, broadcasts red, `commitPending` sets `committedColors[idx] = red`. Now red is committed. The blue pixel from the other user is *lost* — we wrote red over it. Server's authoritative state will accept this (server doesn't know about the conflict).
**Why it matters:** Last-write-wins is the server's policy, so the conflict outcome is "correct by spec". But there's no UI signal of the conflict. User undid → saw blue pixel appear → redid → red comes back without warning that they're overwriting someone else's pixel.
**Fix:** Optional. Could mark stroke pixels as "stale" if WS update arrived during pending state. Show indicator. Likely Won't Fix — last-write-wins is canonical r/place behavior.
### NH3 — Mode switch mid-stroke leaves dangling state
**File:** `src/client/App.svelte:13, 125`, `src/client/components/CanvasRenderer.svelte:74-90, 145-198, 240-282`
**What:** User in draw mode is mid-drag (mouse held down, currentStroke populated). Clicks DrawToolbar `Paint` button. `mode` prop changes. Next mousemove (e.buttons & 1) goes through `handleMouseMove` line 165 → `mode === 'paint'` so falls into pan branch → `pan.x += dx`. The `currentStroke` array is now orphaned — never finished, never cleared. On mouseup, `handleMouseUp` line 188-198: `mode === 'paint'` (now), `!dragging` (depends), so falls into the place-single-pixel branch which calls `addToStroke` then `finishStroke` — but `finishStroke` line 84-90 commits whatever's in `currentStroke` (which contains the abandoned draw-mode pixels) PLUS the new paint pixel as a single combined stroke.
**Why it matters:** User intent violated. Pixels they thought they were panning over (after switching to paint) get committed as if drawn. Possibly hundreds of pixels.
**Fix:** In a `$effect(() => { mode; ... })` watcher in CanvasRenderer, finish any in-progress stroke when mode changes. Or expose `cancelStroke()` and call from App on mode change.
### NH4 — Touch handlers omit `cursorPos` updates AND don't update `committedColors` properly across mode switches
**File:** `src/client/components/CanvasRenderer.svelte:223-282`
**What:** Touch handlers never call `onCursorMove` (consistent with M6 prior unfixed). Coordinates display stays at (0,0) on mobile. Also: `handleTouchEnd` (273-282) calls `finishStroke` whenever mode is draw — even if no pixels were added (e.g., touchstart on an OOB coord, or user just panned with one finger after starting in draw mode). Empty stroke is no-op (line 85 early return) so OK. But: in pinch (touches=2, line 233-237) it calls `finishStroke` if `currentStroke.length` — defensive, good.
However: in two-finger pinch (line 257-270), no `finishStroke` after — if user started with one finger drawing (added stroke), then put down second finger to pinch-zoom, then released both, `handleTouchEnd` fires with `e.changedTouches.length` = 2 typically. The check `e.changedTouches.length === 1` (line 276) means no finishStroke for the long-press case. And `mode === 'draw'` calls finishStroke unconditionally on line 274 — which IS called regardless of changedTouches count. OK.
Actually wait — line 273: `handleTouchEnd(e)` — `mode === 'draw'` → `finishStroke()` always. Includes the pinch case where `currentStroke` was already finished at line 234. Empty `currentStroke` so finishStroke is a no-op. OK.
The real issue: `handleTouchEnd` does not preventDefault (only handlers 223, 240 do). If end fires and mode is draw and finishStroke was empty, no harm. But missing cursorPos update is real.
**Fix:** Add `onCursorMove` calls to `handleTouchMove` (single-finger branch).
### NH5 — Decoder error in onMount swallowed; loading set false → blank canvas with no error
**File:** `src/client/components/CanvasRenderer.svelte:296-308`
**What:** `try { fetch + decode } catch { console.error } finally { loading = false }`. On any failure (network, decoder throw, JSON parse), `imageData` stays null, `committedColors` stays null. Loading overlay disappears. User sees blank dark canvas. Mouse handlers no-op (setPixelRgba early returns when !imageData). User has no idea what's wrong. Equivalent to prior N2.
**Why it matters:** Same as before but compounds with NC3 (committedColors null) — drawing in draw mode silently does nothing.
**Fix:** Add error state, show retry button, disable interactivity when in error state.
---
## New Medium Findings
### NM1 — `getAffectedKeys` and `pixelCount` both rebuild Sets — duplicate work in `clearPending`
**File:** `src/client/components/CanvasRenderer.svelte:121-131`, `src/lib/pixel-buffer.js:60-67, 71-77`
**What:** `clearPending` calls `buffer.getAffectedKeys()` (rebuilds Set) then `buffer.clear()` then `notifyBuffer()` which reads `buffer.pixelCount` (rebuilds Set). Two full traversals of the strokes array.
**Fix:** Pre-compute `affectedKeys` from the cached map (NH1 fix).
### NM2 — `currentStrokeKeys` size cap missing → memory pressure on long strokes
**File:** `src/client/components/CanvasRenderer.svelte:74-82`
**What:** Same root cause as NC2 but specific to mid-stroke. `currentStrokeKeys` Set grows unbounded. Add cap.
### NM3 — Coordinate data type confusion (`buffer.getColorAt` returns -1 sentinel)
**File:** `src/lib/pixel-buffer.js:51-58`, `src/client/components/CanvasRenderer.svelte:70-71, 98`
**What:** Returns -1 sentinel for "not pending". Caller uses `pending >= 0` check — OK, but loses the distinction between "pending color 0" (which is valid: `0` is dark red `#6d001a`). Wait — color indices are 0-31, all valid. -1 is fine as sentinel since outside range. But code at line 71: `pending >= 0 ? pending : committedColors[...]` — if `pending` is exactly 0 (color 0), `0 >= 0` true, uses 0. ✓ Correct. False alarm.
But: `getColorAt` is O(strokes × pixels-per-stroke). Already noted in NH1.
### NM4 — `handleSubmit` does not handle non-2xx HTTP statuses correctly
**File:** `src/client/App.svelte:79-95`
**What:** `await fetch(...)` resolves with `Response` regardless of status. Code reads `text` then tries `JSON.parse`. If server returns plain-text 502 from a proxy, `JSON.parse` throws, catch logs and `return`. But `submitting` is in the outer `finally` (98-100) so resets correctly. Buffer untouched. Same NC1 issue: silent. No `res.ok` check.
**Fix:** `if (!res.ok) { showError(`HTTP ${res.status}`); return; }`. Then parse JSON.
### NM5 — Submit button can be clicked again immediately after submitting=false; no debounce
**File:** `src/client/App.svelte:73-101`, `DrawToolbar.svelte:25`
**What:** `submitting` flag prevents double-click during in-flight fetch. After resolve/reject, `submitting=false` immediately. If user double-clicks fast and request is fast (50ms), second click could fire after `submitting=false` set but before user notices first response. With successful first submit, `commitPending()` clears buffer, second click hits `if (!pixels?.length) return` at line 75 — safe. With failed first submit (NC1), buffer NOT cleared — second click resends same batch. Probably OK semantics (re-submit), but could double-charge credits if server accepts on second try after rejecting on first (race in server-side credit math). Server-side rate-limiter is atomic Lua so safe.
**Fix:** Add cooldown (200ms) after submit complete before re-enabling Submit button. Or only disable while submitting + during HTTP-429 retry-after window.
### NM6 — Keyboard shortcut Ctrl+Z works during text input
**File:** `src/client/App.svelte:62-70`
**What:** `<svelte:window onkeydown={handleKeyDown} />` listens globally. Currently no text inputs exist in the UI, so safe. But if a future feature adds an `<input>` (username, search), Ctrl+Z would intercept and break native undo in that input.
**Fix:** Defensive: `if (e.target.matches('input, textarea')) return;` early.
### NM7 — `mode` prop change doesn't update cursor style reactively in a clean way
**File:** `src/client/components/CanvasRenderer.svelte:328`
**What:** `style="cursor: {mode === 'draw' ? 'crosshair' : dragging ? 'grabbing' : 'crosshair'}; touch-action: none"` — paint mode and draw mode both show crosshair. No visual distinction. Toolbar shows mode (good) but the cursor doesn't change.
**Fix:** Different cursor for draw mode (e.g., `pen` or custom SVG). Currently both same.
### NM8 — Buffer survives no persistence across page reload
**File:** `src/lib/pixel-buffer.js`
**What:** User draws 200 pixels, accidentally reloads page (or browser crashes). All work lost. No localStorage backup.
**Why it matters:** Core promise of "buffer until submit" is that work is safe. It's not.
**Fix:** Optional but valuable. Persist `strokes` to localStorage on each `addStroke`/`undo`/`redo`/`clear`/`commitPending` (debounced). Restore on mount. Bound by quota.
---
## New Low Findings
### NL1 — `currentStroke` and `currentStrokeKeys` are not `$state` but read in template indirectly
**File:** `src/client/components/CanvasRenderer.svelte:19-20`
**What:** Plain `let`. Mutated directly, no reactivity. Not displayed, so OK. Comment-worthy as intentional non-reactive state.
### NL2 — `pixel-buffer.js` not unit-tested
Same as prior unresolved-question #2. Bit-buffer math, undo/redo invariants, dedup behavior are exactly the kind of code that benefits from unit tests. Now even more important since pixel-buffer is the core of the new feature.
### NL3 — `DrawToolbar` lacks `aria-label` on icon-less text buttons
**File:** `DrawToolbar.svelte:8-26`
**What:** Buttons have visible text ("Paint", "Draw", "Undo", "Redo", "Clear", "Submit"). Screen readers will read the text. `title` attributes provide tooltips. OK for a11y. But "Submit (5)" with `disabled` state could announce "Submit 5 pixels, button, dimmed" — currently announces "Submit, dimmed" which isn't clear about why disabled.
**Fix:** Optional. Use `aria-label="Submit 5 pixels"` when count > 0.
### NL4 — Touch double-tap to zoom not handled in draw mode
**File:** `src/client/components/CanvasRenderer.svelte:223-282`
**What:** No double-tap detection. iOS users may expect double-tap to zoom-in. Touch handlers preventDefault so OS double-tap is killed. No app-level replacement.
**Fix:** Optional.
### NL5 — `handleSubmit` doesn't disable other interactions during submit
**File:** `src/client/App.svelte:73-101`
**What:** While Submit is in-flight, user can keep drawing (extends buffer). Those new pixels are NOT included in the submitted batch. After submit succeeds, `commitPending()` (CanvasRenderer:135-141) commits all pending — including the new ones the user drew during submit, which were NOT submitted to server. Server doesn't know about them but client thinks they're committed. Drift!
**Why it matters:** Users will keep drawing during the latency of submit (network is slow on mobile). Their post-submit pixels appear committed locally but are absent server-side.
**Fix:** Snapshot buffer at submit time, send snapshot, on success only commit the snapshotted pixels (not whole buffer). Or disable drawing during submit.
This is borderline a high — promote if user testing confirms.
### NL6 — Constants from previous review still not addressed (L2, L3)
- `0.25` and `64` zoom limits still hardcoded at App:118-119, CanvasRenderer:205, 262.
- `selectedColor = $state(27)` still magic at App:9.
---
## Looked at and OK
- **`commitPending` correctly updates `committedColors` for accepted pixels** (CanvasRenderer:135-141).
- **`undo`/`redo` invariants in pixel-buffer.js** — `addStroke` clears redo stack (correct), undo pops to redo, redo pops to strokes. Sound.
- **Mode toggle in DrawToolbar** uses `class:active` correctly with mode comparison.
- **`getAllPixels` dedup with last-stroke-wins via Map** — correct semantics.
- **Right-click pan in draw mode** (CanvasRenderer:179-184) — works as documented in DrawToolbar tooltip.
- **`buffer.getColorAt` correctly handles color 0 (dark red)** — sentinel is -1, all colors are >=0.
- **WebSocket message `try/catch` around JSON.parse and applyUpdates** (App:37-44) — prevents bad messages from killing the WS handler.
- **Backend `MAX_BATCH_SIZE=512` matches the constant client uses** (constants.js:11) — single source of truth via shared lib import.
- **DrawToolbar buttons sized 44×44 min-height** — good touch target (Apple HIG).
- **Submit button disable when `pixelCount === 0`** — prevents empty submit.
- **Keyboard Ctrl+Z / Ctrl+Y / Ctrl+Shift+Z** — covers all common undo/redo shortcuts.
- **No XSS surface introduced** — DrawToolbar uses interpolation only.
- **`bind:this={canvasRenderer}` + exported functions** — clean API surface.
- **Server-side validation independent of client** — no trust boundary leak (worker.js:40-52).
---
## Unresolved Questions
1. **Partial-batch acceptance:** Does the server need to support partial-accept (place first N pixels of batch, reject rest if rate-limited mid-batch)? Currently it's all-or-nothing, which is simpler but means a batch of 200 pixels with only 199 credits is fully rejected. UX-wise users will be frustrated.
2. **Buffer persistence:** Is losing all pending work on accidental reload acceptable? r/place historically did NOT have local buffering — you placed atomically. The new model creates a new failure mode (lost work) that didn't exist before.
3. **Conflict signaling:** When a remote pixel arrives at a coord with pending local pixel, should the user be told? Currently they're not — they may overwrite a popular team's work without realizing.
4. **Mode-switch UX intent:** Should mode switch mid-stroke commit the in-progress stroke, discard it, or block the switch until mouseup? Current behavior (NH3) is "merge with subsequent paint pixel" which is buggy.
5. **Why was the optimistic deduction removed?** H5 fix is good but loses snappy "credits go down immediately" UX. Is the trade-off intentional (correctness over feedback)?
6. **What's the reconnect story?** C4 still unfixed — does the team consider WS disconnect during active editing a real concern, or is the assumption that users won't be disconnected for long?
7. **Why was MAX_BATCH_SIZE bumped from 32 to 512?** Significant change in server load profile per request. Any rate-limit re-tuning planned to compensate?
---
## Metrics
- Files reviewed: 8 (including 2 new: DrawToolbar.svelte, pixel-buffer.js)
- LOC delta vs prior: +66 App.svelte, +74 CanvasRenderer.svelte, +117 DrawToolbar (new), +79 pixel-buffer (new) = +336 LOC
- Critical: 4 prior unfixed (C1, C2, C3, C4) + 3 new (NC1, NC2, NC3) = **7**
- High: 6 prior unfixed (H1, H2, H3, H4, H6, H7) + 1 partial (H8) + 1 fixed (H5) + 5 new (NH1-NH5) = **12 unresolved**
- Medium: most prior unfixed + 8 new (NM1-NM8)
- Low: prior + 6 new (NL1-NL6)
- Type coverage: N/A (vanilla JS, partial JSDoc)
- Test coverage: 0 frontend (some backend tests added in newer commits)
---
## Recommended Actions (Priority Order)
1. **NC2** — Cap `currentStrokeKeys` and `buffer.pixelCount` at `MAX_BATCH_SIZE` (512). Quick fix, prevents OOM on long draws.
2. **NC1** — Add error UI for failed Submit. Critical UX gap with batch flow.
3. **NC3** — Initialize `committedColors` upfront or queue WS updates during initial fetch.
4. **NH3** — Cancel/finish stroke on mode change to prevent dangling-state bugs.
5. **NH1** — Cache `getColorAt` / `pixelCount` via Map in pixel-buffer for O(1) access.
6. **C1 + C2** — Still open from prior. Fix pan reactivity OR call `render()` in `handleWheel`.
7. **C3** — Decoder buffer length validation.
8. **C4** — WS reconnect canvas refetch.
9. **H1** — devicePixelRatio.
10. **H2** — Resize listener leak in async onMount.
11. **NL5** — Snapshot buffer at submit time (drift bug).
12. **NM4** — Check `res.ok` before parsing JSON.
13. **NM8** — localStorage persistence of buffer (optional but high-value).
14. Remaining mediums + lows as time permits.
---
**Status:** DONE_WITH_CONCERNS
**Summary:** Batch drawing rewrite (e0cf802) introduces meaningful new UX (paint/draw modes, undo/redo, batch submit) but compounds existing failure modes. Of 12 prior findings (4 critical + 8 high), only H5 fully fixed (architecturally), H8 partially mitigated. Remaining 10 carry forward unchanged. New flow adds 3 critical bugs (silent submit failure, unbounded buffer, null deref race) and 5 high (perf, mode-switch dangling state, error swallow, missing cursor updates on touch). The new batch-drawing UX is a net feature win but the *failure modes* of batch operations are 100-1000x more painful than per-pixel failures.
**Concerns:** Buffer-loss-on-failed-submit (NC1) and unbounded-buffer (NC2) are user-visible disasters waiting to happen. Strongly recommend addressing NC1+NC2+NC3 before shipping further. Server-side `MAX_BATCH_SIZE=512` is large enough to cause significant memory + bandwidth issues if client allows reaching it without backpressure.
plans/reports/code-reviewer-260510-0211-rplace-do-migration.md
-321
View File
@@ -1,321 +0,0 @@
# Code Review — rplace DO migration (Phases 1–4)
**Date:** 2026-05-10
**Scope:** Last 4 commits (c3f7c02 → a977adc) — Upstash → DO SQLite migration + cleanup
**Reviewer:** code-reviewer
## Summary
Migration is functionally correct on the happy path. SQLite-backed canvas + cooldown design is sound and well-commented. Primary issues are (1) **stale documentation referencing deleted code** (real risk: misleads contributors / ops on rollback), (2) a **client-side race** where WebSocket pixel broadcasts received during the initial canvas fetch are silently dropped, (3) a **resize-grow correctness bug** in `writePixels` for canvases whose previous last-chunk was short, and (4) several **minor security / DOS gaps** at the edge.
LOC reviewed: ~700 (server) + ~1200 (client). Tests: 8 files, all green per commit notes (94/94).
---
## Critical
### C1. Stale documentation references deleted code paths
**Files:**
- `README.md:84-97` — describes `src/admin/migrate-from-upstash.js`, `src/lib/canvas-storage.js`, `src/lib/redis-client.js`, `src/lib/rate-limiter.js` as if they exist (all deleted in a977adc)
- `README.md:146-151` — documents `POST /admin/migrate-from-upstash` endpoint as "transitional" but the worker no longer mounts it (returns 404 in production per commit message)
- `docs/system-architecture.md:117-124` — same migration endpoint documented as live
- `docs/deployment-guide.md:34-65,107-111` — full "Optional One-Shot Migration from Upstash" section + troubleshooting entries reference the deleted endpoint and secrets
- `docs/code-standards.md:29-32` — "Functions receive env parameter for Cloudflare bindings (Redis credentials...)", "Use `@upstash/redis/cloudflare`", "Bitfield operations use builder pattern"
**Impact:** A dev reading these docs will try to call a 404 endpoint, set secrets that don't exist, or write code against `@upstash/redis` which is no longer in `package.json`. Ops doing rollback per `docs/deployment-guide.md` will be confused.
**Severity:** Critical (docs claim functionality that's been removed; this is exactly what a "migration cleanup" PR should not leave behind).
**Fix:** Strike the entire migration / Upstash sections. Replace `code-standards.md:27-32` with the DO-binding pattern actually in use. Verify `docs/references.md` Redis links remain only as historical references.
### C2. WebSocket updates dropped during initial canvas fetch
**File:** `src/client/components/CanvasRenderer.svelte:465-484`
The renderer pre-allocates a zero `committedColors` so WS messages arriving before `loadCanvas` resolves don't null-deref (line 16, comment at 14-16). But once the fetch completes, line 473 unconditionally **replaces** the array:
```js
committedColors = new Uint8Array(indices); // replace pre-allocated zero array
```
Any pixel writes that landed in the pre-allocated array between WS connect and fetch resolve are silently overwritten. Same hazard for `imageData` (line 475).
**Repro:** User A opens app. WS connects fast, fetch is slow (16 MB binary). User B places a pixel. User A's WS receives it → writes to `committedColors[idx]`. Fetch resolves → `committedColors = new Uint8Array(indices)` (which was sampled at server BEFORE B's pixel hit, since the GET hits a 10s edge cache). User B's pixel is invisible to A until the next WS update or refresh.
**Severity:** Critical — this is the exact data-loss scenario the architecture comment at `App.svelte:122-126` ("Refetch canvas after a reconnect") tries to prevent, but only triggers on `isReconnect`, not initial connect.
**Fix options:**
1. Buffer WS messages until fetch resolves, replay on completion.
2. After replacing `committedColors`, re-apply the pre-fetch WS edits (track them in a side map).
3. Open WS only after fetch completes (loses real-time updates during load — likely worst option).
### C3. `writePixels` silently drops writes on resize-grow path
**File:** `src/durable-objects/lib/chunk-storage.js:88-101`
When the canvas is grown (CANVAS_WIDTH or CANVAS_HEIGHT bumped), the old last-chunk row's BLOB is stored at the old `chunkSize(lastChunkId)` size, which is shorter than the new `CHUNK_BYTES`. The grow-path read returns that short blob; `new Uint8Array(buf)` (line 92) preserves the short length; `next[byteOffset] = color` (line 94) is a **no-op when byteOffset ≥ next.length** (typed-array OOB writes are silently dropped per spec). The short blob is then INSERT-OR-REPLACE'd unchanged.
**Effect:** New pixels written into the formerly-last chunk after a resize-grow disappear.
This isn't theoretical — `docs/canvas-resize-procedure.md` explicitly recommends growing the canvas as a config-only change. With a 4096×4096 canvas all chunks are full 64KB so the bug is dormant *today*. Bump width to 4097 and the bug activates immediately.
**Severity:** Critical (data loss after a documented operation).
**Fix:** In `writePixels`, allocate `next` to `chunkSize(chunkId)` and copy `buf` into it:
```js
const expected = chunkSize(chunkId); // current expected size for this chunk
const next = new Uint8Array(expected);
next.set(buf.subarray(0, Math.min(buf.length, expected)));
```
Or always allocate `CHUNK_BYTES` for non-last chunks and `chunkSize(lastId)` for the last; never trust the persisted blob's length.
---
## High
### H1. Cooldown is consumed even on storage failure
**File:** `src/durable-objects/canvas-room.js:73-86`
`tryAcquire` runs first (line 73). If `writePixels` then throws (lines 78-83), the user gets a 500 but their cooldown row is already updated. They're locked out for 1s without their pixel placement having succeeded.
**Severity:** High — bad UX on transient errors; effectively turns a storage flake into a 1s soft-DOS of the user.
**Fix:** Rollback the cooldown UPDATE in the catch:
```js
sql.exec('DELETE FROM cooldowns WHERE user_id = ?', userId);
```
Or run write first, then cooldown — but that opens a different race (concurrent placements within DO would all succeed before any cooldown row exists). Safer is the rollback.
### H2. Server response leaks raw error to client
**File:** `src/durable-objects/canvas-room.js:82`
```js
return Response.json({ error: 'storage_failed', message: String(err) }, { status: 500 });
```
`String(err)` may include SQLite error messages, paths, query fragments. Low risk because the DO's SQL is internal but it still violates the "don't leak internals" rule and sets a precedent.
**Severity:** High (security best-practice: never echo `err.toString()` over the wire from a 5xx).
**Fix:** Drop `message` from the response; keep only `error: 'storage_failed'`. Log the full error (already done at line 81).
### H3. Chunk-storage `readAllChunks` crashes on orphaned shrink rows
**File:** `src/durable-objects/lib/chunk-storage.js:46-56`
`SELECT chunk_id, bytes FROM canvas_chunks` returns ALL rows including any with `chunk_id ≥ CHUNK_COUNT` (orphans left after a resize-shrink — explicitly documented as possible in `docs/canvas-resize-procedure.md:32-37`). Then `out.set(view, chunkId * CHUNK_BYTES)` will throw RangeError if `chunkId * CHUNK_BYTES + view.length > out.length`. Since `out` is `TOTAL_PIXELS` long and an orphan has a higher chunk_id, this will throw.
**Severity:** High — `GET /api/canvas` would 500 forever after a shrink, until manual cleanup.
**Fix:** `WHERE chunk_id < ?` bound, or `if (chunkId >= CHUNK_COUNT) continue;` defensive skip. The resize doc's "to reclaim, run DELETE" advice should not be a precondition for the read path.
### H4. WebSocket connections are uncapped per-IP
**File:** `src/durable-objects/canvas-room.js:89-94`
`#handleWsUpgrade` accepts every incoming WS upgrade unconditionally. A single client can open thousands of WSs — CF DO has a soft tens-of-thousands limit per object, but no per-source cap. Each connected WS receives every broadcast (`#broadcastPixels` iterates `getWebSockets()`), so 10K connections × N pixels-per-broadcast = 10K × N message sends.
**Severity:** High at hobby scale (one bad actor can saturate CPU on the singleton DO).
**Fix:** Track WS-per-userId via `state.acceptWebSocket(server, [userId])` tags, and deny upgrade past N existing tagged sockets (`getWebSockets(userId)`). Even a 10-per-IP cap eliminates the trivial DOS.
### H5. `content-length` body cap is bypassable
**File:** `src/worker.js:24-27`
`parseInt(c.req.header('content-length') || '0', 10)` defaults to 0 when missing. A malicious client can send chunked-transfer-encoded body without `content-length` and bypass the 128KB pre-parse cap. The CF runtime caps overall request size to 100 MB; until then, the JSON parser allocates as it reads. Practical exposure is "force the worker to allocate up to 100 MB before failing the per-pixel `batch_too_large` check."
**Severity:** High (DOS amplification, easy to fix).
**Fix:** Read raw body via `c.req.arrayBuffer()` with a hard byte cap, then `JSON.parse(decoder.decode(buf))`. Or stream-validate while reading. Or short-circuit if `content-length === 0` (no header → reject).
---
## Medium
### M1. `retryAfter` is always 1s, never the actual remaining window
**File:** `src/durable-objects/lib/cooldown-store.js:55`
On rate-limit denial, the function returns `retryAfter: REQUEST_COOLDOWN_SEC` (always 1s), but the user may have, e.g., 200ms left. Client (`App.svelte:220-222` and `image-uploader.js:104`) waits a full second when ~200ms would suffice.
**Severity:** Medium (UX, not correctness).
**Fix:** When the INSERT throws, run `SELECT expires_at FROM cooldowns WHERE user_id = ?` and return `Math.ceil((expires - now)/1000)`. Optional micro-opt: structure as `INSERT … ON CONFLICT … RETURNING` to avoid the round-trip.
### M2. Comment about WebSocket compat date is misleading / inverted
**File:** `src/durable-objects/canvas-room.js:119`
```js
// Required pre-2026-04-07 compat date; harmless after.
ws.close(code, reason);
```
The wrangler `compatibility_date` is `2025-04-01` (almost a year before the comment's "pre-2026-04-07" cutoff), so the explicit close IS needed. The comment reads as "you can delete this any day now" but actually says the opposite. Worth fixing before someone deletes the line.
**Severity:** Medium (foot-gun for future maintenance).
**Fix:** "Required because compatibility_date (2025-04-01) is before the 2026-04-07 default-close cutoff. Remove if/when wrangler.json bumps past that date."
### M3. WebSocket protocol is broadcast-only but doesn't ping/keepalive
**File:** `src/durable-objects/canvas-room.js:110-113`
`webSocketMessage` immediately closes any inbound message. That means clients can't ping the server to detect zombies. Browser will only know the connection is dead when the OS / proxy times it out (could be minutes). On the wire there's no heartbeat — `App.svelte:131-135` reconnects on `onclose`, but that won't fire if the network silently drops.
**Severity:** Medium (real-time UX during flaky networks).
**Fix:** Either accept `'ping'`/`'pong'` text messages (add small whitelist), or rely on Hibernation API auto-ping (verify CF behavior; docs are sparse). Lowest-cost option: client sends `WebSocket` protocol-level pings via a heartbeat timer; server must permit them.
### M4. Dev-bucket userId = `anon:dev` collapses all dev traffic into one rate-limit bucket
**File:** `src/lib/get-user-id.js:10-14`
In dev (no `cf-connecting-ip`), every request is bucketed as `anon:dev`. If this fallback ever triggers in prod (proxy misconfig, custom domain misrouted), all users share a single 1 req/s budget — soft-DOS for everyone.
**Severity:** Medium (production blast radius is total but trigger is unlikely).
**Fix:** In production (e.g., `env.ENVIRONMENT === 'production'`), throw or 500 instead of falling back. Or use `request.cf?.colo` as a salt, or fall back to `x-real-ip` / `x-forwarded-for`. Document the assumption clearly.
### M5. `Math.random()`-based GC sample assumes per-call randomness in CF Workers
**File:** `src/durable-objects/lib/cooldown-store.js:36,50`
`Math.random()` in CF Workers historically had unusual semantics around isolate reuse. If it returns the same value across all `tryAcquire` calls in an isolate, GC either fires every time or never. Modern CF runtime is supposed to handle this, but worth verifying with a quick log.
**Severity:** Medium (correctness depends on platform behavior, not visible from the code).
**Fix:** If unsure, swap to `crypto.getRandomValues(new Uint8Array(1))[0] < 256 * GC_SAMPLE_RATE` for guaranteed entropy. Or trigger GC every Nth call via a counter on the DO instance.
---
## Low
### L1. `new Uint8Array(buf)` copy in `writePixels` is slower than `.slice()`
**File:** `src/durable-objects/lib/chunk-storage.js:92`
The iterable-constructor copy walks element-by-element. `buf.slice()` uses memcpy. For 64KB and 1 req/sec, irrelevant — but the comment at 90-91 implies a copy is required for safety, and `.slice()` reads cleaner.
**Severity:** Low.
### L2. Cooldown `retryAfter` not surfaced in seconds with sub-second precision
**File:** `src/durable-objects/lib/cooldown-store.js:55`
If we ever want sub-second cooldowns, the `retryAfter` integer second contract caps us. Worth typing `retryAfterMs` for forward compat.
### L3. `writePixels` per-pixel branch could be vectorized for big batches
**File:** `src/durable-objects/lib/chunk-storage.js:88-101`
For a 2048-pixel batch all in one chunk, the inner write loop runs in JS one byte at a time. CF DO SQLite charges per-row, so cost is dominated by the BLOB write — but if batches grow (e.g., admin imports), the pure-JS loop becomes the bottleneck. Not relevant today.
### L4. `setOverlay`'s Texture.from is called twice on race
**File:** `src/client/components/CanvasRenderer.svelte:233-240,521-528`
If `setOverlay` is called before `initPixi` finishes, `overlayState` holds the data. After init, lines 521-528 materialize the sprite. But if a second `setOverlay` arrives mid-init, the first `overlayState` is overwritten silently (no second materialize-from-overlayState pass), so only the latest survives — actually correct behavior, just non-obvious. Worth a one-line comment.
### L5. Server-side `MAX_BATCH_SIZE` import in `canvas-room.js` is redundant
**File:** `src/durable-objects/canvas-room.js:58-60`
The DO re-validates bounds even though the worker did. Defense-in-depth is the stated rationale, fine. But the worker validates `pixels.length > MAX_BATCH_SIZE` *and* the DO does. If they ever diverge (different `MAX_BATCH_SIZE`), edge would reject what DO accepts. Single source of truth via shared constant — already true here; just a note.
### L6. WebSocket message JSON is rebuilt per broadcast; payload not reused
**File:** `src/durable-objects/canvas-room.js:97`
`JSON.stringify` once per broadcast (line 97) — already correct. Disregard. (Including this as a non-finding to confirm I checked.)
---
## Nit
### N1. `idx_cooldowns_expires` only used by the GC sweep
**File:** `src/durable-objects/lib/schema.js:30-32`
The index is consulted by `gc()` only — `tryAcquire` queries by primary key. Comment at 22-23 says "the index keeps the GC sweep cheap" — accurate but redundant given the index name. Fine as-is.
### N2. Worker comment about MAX_BODY_BYTES math
**File:** `src/worker.js:9-10`
`MAX_BATCH_SIZE * 64` overestimates by ~10× (real `{"x":2047,"y":2047,"color":31}` is 27 bytes plus 2 for `,` and brackets ≈ 30B). The 64B headroom is fine but noting that the actual cap on parsed JSON could be tighter if it ever matters.
### N3. `loadCanvas` overwrites `loadError` to null on retry — but doesn't clear the `loading` text in the error state
**File:** `src/client/components/CanvasRenderer.svelte:466-467,573-577`
When the user clicks Retry, both `loading` and the error banner are visible until the fetch completes. Visual nit.
### N4. `handleWsUpgrade` doesn't validate auth or origin
**File:** `src/durable-objects/canvas-room.js:89-94`
For a public collaborative canvas, no auth is needed. But the DO accepts upgrades from any origin. CF's WAF would handle malicious traffic before it gets here. Fine for the scope; document if any future feature gates per-user state.
---
## Edge Cases Found by Scout
| Path | Edge case | Found in |
|---|---|---|
| Initial render | WS message arrives between fetch start and replace | C2 |
| Resize-grow | Last chunk shorter than CHUNK_BYTES, new write past short length silently dropped | C3 |
| Resize-shrink | Orphan rows past CHUNK_COUNT crash readAllChunks with RangeError | H3 |
| Storage flake | Cooldown consumed but write failed → user soft-DOS'd 1s | H1 |
| 500 error path | Raw error string echoed to client | H2 |
| WebSocket flood | No per-IP / per-userId cap on concurrent sockets | H4 |
| Chunked POST | content-length = 0 bypasses pre-parse cap | H5 |
| Dev-misroute to prod | All anon:dev share 1/s globally | M4 |
| Network silent-drop | No client/server WS heartbeat | M3 |
| Math.random in workers | GC may fire every call or never depending on runtime | M5 |
| 429 retryAfter | Always 1s, never sub-second remaining | M1 |
---
## Positive Observations
- Atomic-by-virtue-of-DO model is the right call for a hobby-scale rplace clone. The "no await between cooldown + write + broadcast" comment at `chunk-storage.js:85-87` is excellent — exactly the kind of invariant that breaks when someone drops in `await` later.
- `tryAcquire`'s UPDATE-then-INSERT race-safe rate-limit is genuinely clever and well-explained at lines 14-19 of `cooldown-store.js`.
- Lazy chunk allocation (zero-fill on read) makes resize-grow trivially correct **on the read side**. Only the write side has the bug (C3).
- Schema is `IF NOT EXISTS`, idempotent — survives DO eviction cleanly.
- Edge validation at the worker is thorough and re-validated at the DO; a `Number.isInteger` check correctly rejects strings (test `worker-validation.test.js:115-119`).
- The WebSocket hibernation pattern is correct (`state.acceptWebSocket`, `webSocketMessage/Close/Error` handlers all present).
- `package-lock.json` cleanly removed 184 packages with the Upstash / testcontainers cleanup — no orphan deps observed in `package.json`.
---
## Recommended Actions
1. **Critical (must fix before next deploy):**
- C1 — purge migration / Redis references from README, system-architecture, deployment-guide, code-standards.
- C2 — buffer WS pixel messages until initial canvas fetch resolves; merge instead of replace.
- C3 — `writePixels` must size `next` against `chunkSize(chunkId)`, not against the persisted blob's length.
2. **High (should fix this week):**
- H1 — rollback cooldown row on `writePixels` failure.
- H2 — strip raw error from 500 response.
- H3 — bound `readAllChunks` by `chunk_id < CHUNK_COUNT` (or skip orphans).
- H4 — per-userId WS connection cap.
- H5 — replace content-length cap with bounded body read.
3. **Medium (next-sprint backlog):**
- M1 — return precise `retryAfterMs` from cooldown denial.
- M2 — fix the misleading WS-close comment.
- M3 — add WS heartbeat (server permits ping or auto-pings).
- M4 — production fail-closed when `cf-connecting-ip` missing.
- M5 — verify `Math.random()` semantics in CF Workers; switch to `crypto` if unclear.
4. **Low / Nit:** L1–L6, N1–N4 at code-review-cycle pace, no urgency.
---
## Metrics
- Files reviewed: 12 server + 4 client (skim) + 4 docs
- LOC reviewed: ~1900
- Issues found: 3 Critical, 5 High, 5 Medium, 6 Low, 4 Nit (total 23)
- Type coverage: N/A (JS, JSDoc-typed)
- Test coverage: 94/94 unit tests pass per commit message; not independently re-run
- Linting issues: not run (no lint script in package.json)
---
## Unresolved Questions
1. **Math.random() in CF Workers** — does it return per-call entropy or per-isolate-fixed values? (Affects M5 GC behavior.) Worth a one-line `wrangler tail` log to confirm.
2. **Hibernation API auto-ping** — does `state.acceptWebSocket` arrange a TCP-level keepalive, or do clients need to explicitly heartbeat? CF docs are unclear; would unblock M3.
3. **Production smoke-test for resize-grow** — is there an environment where we can verify C3 with a non-aligned `CANVAS_WIDTH` (e.g., 4097)? Otherwise the fix is correct-by-construction but unproven.
4. **CF `cf-cache-status: HIT` ratio in production** — README and deployment-guide claim 10s edge-cache will absorb most `/api/canvas` traffic, but no telemetry is wired up. Worth a single-line log + dashboard panel before traffic ramps.
5. **Single-DO singleton failure mode** — when `idFromName('main')` colocates a single DO, what is the user-visible behavior during CF colo failover? (Probably brief 5xx then recover.) Worth documenting in `docs/system-architecture.md`'s "Operational Notes" section.
---
**Status:** DONE_WITH_CONCERNS
**Summary:** Migration is structurally sound; 3 Critical and 5 High issues identified, primarily around stale docs (C1), client-side WS race (C2), resize-grow data-loss bug (C3), error-path hygiene (H1/H2/H3), and edge DOS surface (H4/H5). All have clear, scoped fixes.
**Concerns/Blockers:** C1 (stale docs) is the most embarrassing — fix before any new contributor onboards. C2 and C3 are real correctness bugs that the existing test suite will not catch.
plans/reports/debugger-260510-0211-rplace-edge-cases.md
-223
View File
@@ -1,223 +0,0 @@
# rplace edge-case audit (post-DO-migration)
Static adversarial review. Scope: cooldown + chunk storage + WS hub + edge cache + image-importer DoS + SQLite limits.
Constants used (from `src/lib/constants.js`):
- `CANVAS_WIDTH = CANVAS_HEIGHT = 4096`, `TOTAL_PIXELS = 16_777_216`
- `MAX_COLORS = 256`, `MAX_BATCH_SIZE = 2048`, `REQUEST_COOLDOWN_SEC = 1`
- `CHUNK_BYTES = 65536`, `CHUNK_COUNT = 256`
---
## Punch list (severity-ordered)
### CRITICAL
#### C1. Cooldown consumed even on storage failure → user locked out 1s with no write
- **Scenario:** `tryAcquire` succeeds, then `writePixels` throws (SQLite I/O error, BLOB-too-large, OOM, transient).
- **Trigger:** any unhandled SQL error inside `writePixels` (e.g. row > BLOB cap, see L1).
- **Observable:** client gets `500 storage_failed`, but the cooldown row was already inserted/updated. User must wait 1s before retrying. UX is mildly annoying for humans, fatal for the long-running image-uploader: each failed batch costs both the batch *and* the cooldown slot.
- **Evidence:** `src/durable-objects/canvas-room.js:73` (`tryAcquire` first), `:79` (`writePixels` after) — no compensating "release" path on failure.
- **Severity:** Critical for image upload (silently halves throughput on transient errors); High otherwise.
#### C2. WS broadcast happens before any commit barrier → other clients see pixels server may roll back
- **Scenario:** `writePixels` does N synchronous `INSERT OR REPLACE` calls in a single JS turn. Without an explicit transaction wrapper, each `sql.exec(...)` is its own auto-commit. If chunk K succeeds and chunk K+1 fails (disk pressure, BLOB limit), the partial state is already persisted *and* the broadcast (line 85) has not yet run — but the user sees `500` and may resubmit.
- **Trigger:** any error path on the second (or later) chunk write inside `writePixels`.
- **Observable:** canvas left in half-written state; subsequent `GET /api/canvas` returns it; broadcast never fires so connected WS clients don't see it until they refetch (cache 10s).
- **Evidence:** `src/durable-objects/lib/chunk-storage.js:88-102` — loop has no `transactionSync` despite the comment at L86-87 claiming atomicity by virtue of "no `await`". *No `await` ≠ atomic*; auto-commit per statement still applies.
- **Severity:** Critical. Atomicity claim in the comment is misleading and false in the failure case.
#### C3. NAT / CGNAT collision = group-rate-limit by IP
- **Scenario:** `getUserId` hashes only `cf-connecting-ip`. Mobile carriers, university networks, corporate proxies, and CGNAT all share single egress IPs across thousands of users.
- **Trigger:** any deployment with shared-IP users.
- **Observable:** all users behind the same IP share *one* 1 Hz bucket. First placer "wins" each second; everyone else gets `429`. Effectively unusable on mobile during peak.
- **Evidence:** `src/lib/get-user-id.js:9-23`. No cookie/session/fingerprint augmentation; no per-room or per-IP-group differentiation.
- **Severity:** Critical for product usability (not a security flaw, but a denial-of-service against legitimate users).
---
### HIGH
#### H1. Image-importer DoS / griefing — no per-IP daily quota
- **Scenario:** Per-IP rate limit is 1 batch (= 2048 pixels) / sec ≈ 7.37 M pixels/hour. One IP can repaint 44% of the entire canvas every hour, indefinitely. The image-importer (`src/lib/image-uploader.js:60-122`) is built to do exactly this.
- **Trigger:** anyone running the importer or a custom script with a multi-megapixel image. Multiple users behind the same NAT amplify it (see C3 inverted: NAT *costs* legit users; from the operator side a single bad actor with multiple clients still funnels through the same bucket).
- **Observable:** entire canvas can be overwritten every ~2 hrs by a single attacker. No global cap, no per-day cap, no throttle on "draw image" sessions.
- **Evidence:** no daily/hourly cap anywhere in `src/durable-objects/lib/cooldown-store.js` or `canvas-room.js`. `MAX_BATCH_SIZE = 2048` (constants L12) makes throughput 2048× a naive 1 Hz limit.
- **Severity:** High. Obvious griefing primitive.
#### H2. `cf-connecting-ip` missing → entire dev/preview traffic shares "anon:dev" bucket
- **Scenario:** Wrangler local dev (`wrangler dev`), preview URLs, custom Workers-for-Platforms tunnels, or any non-CF-fronted invocation drops `cf-connecting-ip`.
- **Trigger:** local dev, preview URLs, tests using real DOs.
- **Observable:** all dev traffic shares one cooldown bucket. Manifests as random `429`s when more than one tab is open during dev. Easy to mistake for a real bug.
- **Evidence:** `src/lib/get-user-id.js:11-14`.
- **Severity:** High for DX; not a production bug per se but trips reviewers.
#### H3. WS hibernation: hub state across rehydrate is fine, but no catch-up message protocol
- **Scenario:** Server hibernates DO, drops in-RAM state. Client stays connected (CF keeps the socket). On the next placement, `state.getWebSockets()` returns the rehydrated sockets and broadcast resumes — this part is correct.
- **The bug:** during hibernation gap *or* during reconnect, missed pixels are recovered only by `canvasRenderer.refetchCanvas()` on `onopen` *if* `isReconnect` is true (`App.svelte:122-129`). On the very first connect after a fresh page load, `isReconnect = false` (`:106`), so the initial canvas fetch is the only source of truth. If the canvas fetch happened seconds ago (CDN cache, see H4) and pixels were placed in the gap *between* canvas fetch completion and WS open, those pixels are missed silently until the user causes a refetch or another pixel near them lands.
- **Trigger:** slow page load, two HTTP/1 connection limits, or just unlucky timing.
- **Observable:** persistent stale pixels shown to the user; only fixed by reload or by another nearby placement triggering a render diff.
- **Evidence:** `App.svelte:103-144` does NOT serialize "fetch canvas → open WS"; both happen as separate effects. No version/seq number on broadcast frames to detect gaps.
- **Severity:** High. Common race in r/place clones.
#### H4. Edge cache (`max-age=10, s-maxage=10, stale-while-revalidate=30`) on `/api/canvas` writes a 10-second blind spot
- **Scenario:** Pixel placed at t=0. CF edge has cached canvas from t=−9. New tab loads at t=+0.5: gets the t=−9 snapshot. WS opens at t=+0.6 — but the pixel was already broadcast at t=0, so the new tab will *never* see it via WS, and will see the stale value until either a) cache expiry triggers a refetch or b) the same coord is repainted.
- **Trigger:** any reload during heavy paint activity. Worse with `stale-while-revalidate=30`: total stale window can reach ~40s if the revalidate is delayed.
- **Observable:** users on different tabs/devices see different canvases for tens of seconds. Image uploader's `shouldSkip` predicate (uploader L21-25) reads from this stale view and may "skip" pixels that *aren't* actually the right color, leaving holes.
- **Evidence:** `src/durable-objects/canvas-room.js:38` — `Cache-Control: public, max-age=10, s-maxage=10, stale-while-revalidate=30`.
- **Severity:** High. Visibly degrades multi-client UX; corrupts the importer's resume logic.
#### H5. WS upgrade forwards full request, but DO routes by `url.pathname` after Worker rewrites URL → WS upgrade may miss client IP / origin checks
- **Scenario:** `app.get('/api/ws', ...)` calls `room(c.env).fetch('http://do/ws', c.req.raw)`. The DO's `fetch` switches on `url.pathname` only (canvas-room.js:25). The WS upgrade handler `#handleWsUpgrade()` does *no* origin check, no auth, no IP rate-limit.
- **Trigger:** any client opens a WS to `/api/ws`. `Origin` header is unverified.
- **Observable:** any third-party site can open and hold a WS to the canvas DO. Hibernation lets connections sit cheaply, but every pixel placement broadcasts to all of them, multiplying egress per active user. With N hostile clients, broadcast cost is O(N) per placement.
- **Evidence:** `src/worker.js:67-73`, `src/durable-objects/canvas-room.js:89-94`. No `Origin` allowlist, no max-clients.
- **Severity:** High. Cheap WS-amplification attack on the broadcast hub.
---
### MEDIUM
#### M1. `tryAcquire` `INSERT … VALUES` race — relies on PK conflict throwing, but cursor is not drained on success
- **Scenario:** The SUCCESS branch of the second insert (`cooldown-store.js:45-49`) does not call `.toArray()` on the cursor (compare to the UPDATE branch L34 which *does* drain). On CF DO SQL, statement effects are committed once the cursor is materialized. If the engine deferred the commit until cursor drain and a subsequent `Math.random()` GC sweep fires (L51) and a *second* `tryAcquire` runs concurrently (impossible in a single DO, but possible across DO replays / fast retry) … this is a code-smell, not provably wrong.
- **Trigger:** API-level retry at the millisecond boundary.
- **Observable:** None reproducible from static reading. Drain-symmetry is the safe default.
- **Evidence:** `src/durable-objects/lib/cooldown-store.js:34` (drain) vs `:45-49` (no drain).
- **Severity:** Medium. Flag for fix; low real-world impact since DOs are single-threaded per name.
#### M2. `writePixels` aliasing comment is misleading; `Uint8Array(buf)` does *not* always copy
- **Scenario:** `readChunk` returns either a fresh zero-fill (no row) or wraps the SQL-returned BLOB with `new Uint8Array(blob)` if it's not already a Uint8Array. If the BLOB *is* already Uint8Array, `readChunk` returns it directly (L37-39). Then `writePixels` does `const next = new Uint8Array(buf)`. Per spec, `new Uint8Array(typedArray)` *copies*, so this is fine — but the comment at L91 says "must not alias persisted state, so copy to be safe", suggesting uncertainty.
- **Trigger:** Future refactor that uses `new Uint8Array(arrayBuffer)` instead of `new Uint8Array(typedArray)` would silently introduce aliasing (the latter constructs a view, not a copy, when given an ArrayBuffer).
- **Observable:** would corrupt the SQLite-cached BLOB and produce inconsistent reads.
- **Evidence:** `chunk-storage.js:37-39` (return path), `:92` (consumer).
- **Severity:** Medium (latent footgun, not an active bug).
#### M3. Missing duplicate-coord dedup in batch → user can inflate batch size with redundant pixels
- **Scenario:** Client submits 2048 pixels all at `(0,0)` with random colors. Validation (`worker.js:43-52`) accepts. `writePixels` groups by chunk, so all 2048 hit chunk 0; then iterates `edits` writing 2048 times to `next[0]`. Result: only the last write survives (no functional bug), but you've burned the user's full batch quota on 1 effective pixel.
- **Trigger:** buggy client, or an attacker trying to hide intent in a noisy batch.
- **Observable:** user pays 1 sec cooldown for what looks like 2048 pixels but is 1.
- **Evidence:** `worker.js:36-52` (no Set dedup), `chunk-storage.js:72-80` (last-write-wins per byte).
- **Severity:** Medium UX nuance; the importer's `pixel-buffer.js:23-30` actually *does* dedup client-side, so well-behaved clients are unaffected.
#### M4. Image-importer "progressive skip" reads stale canvas state via `shouldSkip`
- **Scenario:** `shouldSkip` is fed by the client's local canvas view, which is updated from WS broadcasts and the initial fetch. Combined with H4 (10s edge cache), the importer can skip pixels that aren't truly placed yet, leaving holes in uploaded images.
- **Trigger:** Concurrent edits + cached canvas view.
- **Observable:** importer reports "Skipped N already-matching pixels" but the pixels weren't actually placed.
- **Evidence:** `image-uploader.js:64-76`. No server-authoritative readback.
- **Severity:** Medium. Self-inflicted, recoverable by re-running the importer.
#### M5. SQLite per-row BLOB size — DO SQLite cell size limit
- **Scenario:** CF DO SQLite has a per-cell limit (commonly 2 MB hard, often documented around ~2 MB). Each chunk is 64 KB — well under. ✅ Not a bug today. Becomes a problem if `CHUNK_BYTES` is bumped above the cell limit during a "resize redeploy".
- **Trigger:** Future redeploy with `CHUNK_BYTES > 2 MB`.
- **Observable:** writes throw at runtime; canvas frozen.
- **Evidence:** `constants.js:18` — no assertion that `CHUNK_BYTES` ≤ documented cell-size cap.
- **Severity:** Medium (latent; add a static assertion).
#### M6. Per-DO storage cap (CF DO SQLite ~10 GB / instance)
- **Scenario:** Cooldown table grows to ~`active-users` rows; canvas chunks at 256 × 64 KB = 16 MB. Plenty of headroom *unless* users explode (millions of unique IPs/day) and GC at 1% sample rate fails to keep up.
- **Math:** at 1% GC sample rate per `tryAcquire`, expected GC runs/sec = 0.01 × QPS. At low QPS (1-10), GC may run < once/min. Each row ~50-100 bytes; 10 GB cap = ~150 M rows. Not realistic on rPlace traffic, but on a viral spike, possible.
- **Evidence:** `cooldown-store.js:7` (`GC_SAMPLE_RATE = 0.01`), `:36-37, :50-51` (probabilistic).
- **Severity:** Medium (capacity, not correctness).
#### M7. `webSocketClose` re-closes already-closed socket
- **Scenario:** Hibernation API delivers `close` events for *all* terminations including ones the server initiated. The handler at `canvas-room.js:115-121` calls `ws.close(code, reason)` again, on a socket that's already closed.
- **Trigger:** any close event on hibernation-mode WS.
- **Observable:** likely silent (ws.close on closed = no-op or throws caught upstream). Comment says "Required pre-2026-04-07 compat" which today's compat date is `2025-04-01` — so this code path *is* exercised. Confirmed no try/catch around it.
- **Evidence:** `canvas-room.js:120` and `wrangler.json:4` (`compatibility_date: "2025-04-01"`).
- **Severity:** Medium. Add try/catch defensively.
---
### LOW
#### L1. Chunk-boundary math is correct for current dims, but no off-by-one guard if `TOTAL_PIXELS % CHUNK_BYTES != 0`
- **Math:** `4096 × 4096 = 16_777_216`; `16_777_216 / 65536 = 256` exactly. So `chunkSize(255) = min(65536, 16_777_216 - 255*65536) = 65536`. ✅ Today.
- **Risk:** if dims become non-multiples (e.g. `CANVAS_WIDTH = 4097`), `TOTAL_PIXELS = 16_785_409`, `CHUNK_COUNT = ceil(16_785_409 / 65536) = 257`. `chunkSize(256) = 16_785_409 - 256*65536 = 8193`. `readChunk` returns a `Uint8Array(8193)` for the missing row, but `readAllChunks` does `out.set(view, chunkId * CHUNK_BYTES)` where `out` is sized `TOTAL_PIXELS` — write at offset `256*65536 = 16_777_216` of length `8193` ends at `16_785_409` ✅.
- **Where it breaks:** `pixelToChunk` returns `byteOffset = offset % CHUNK_BYTES` — for the last chunk this is fine because writes are bounded by valid (x,y). But if a stored row's BLOB is larger than `chunkSize(chunkId)` (e.g. legacy rows after a *shrink*), `out.set` would overrun. No length-check on the read path.
- **Evidence:** `chunk-storage.js:46-56` — no `subarray(0, chunkSize(chunkId))` clamp.
- **Severity:** Low (current dims safe; flag if shrinking).
#### L2. Validator double-work: edge validates, then DO re-validates
- Edge in `worker.js:36-52`, DO in `canvas-room.js:51-70`. Defensive, not a bug, but increases JSON parse cost twice for every batch.
- **Severity:** Low (perf; acceptable defense-in-depth).
#### L3. `Number.isInteger(p?.x)` accepts `-0` and the same coord as `0`
- Per ES spec `Number.isInteger(-0) === true`, and `-0 < 0 === false`, so `-0` passes through. Harmless (writes byte 0 of chunk 0), but worth noting if anyone ever uses x as a JSON Map key.
- **Severity:** Low.
#### L4. `MAX_BODY_BYTES = MAX_BATCH_SIZE * 64` is a heuristic; large palette indices aren't longer
- 64 bytes/pixel is generous (`{"x":4095,"y":4095,"color":255}` is ~32 chars). Comment says "~64 bytes is generous" — fine. But malicious whitespace-padded JSON (`" x ": 0`) can blow past this *length* before parsing. `c.req.header('content-length')` is client-supplied and may lie.
- **Trigger:** crafted body with bogus `Content-Length`. Hono / `c.req.json()` will read the actual body; if it's larger than declared, behavior depends on the Hono runtime (usually reads what's there). Not a memory exhaustion attack on Workers (req body capped at 100 MB), but it bypasses the early reject.
- **Severity:** Low.
#### L5. WS broadcast: `JSON.stringify` once, send to all — but no backpressure detection
- `canvas-room.js:97-105`: `ws.send(message)` in a loop. No queue length check. If a client is slow / hibernated incorrectly / on flaky cell, sends pile up. CF docs suggest checking `getReadyState` or buffered amount; here we just rely on the catch.
- **Severity:** Low (CF runtime likely drops or queues internally).
#### L6. No cap on number of WebSocket clients per DO
- Combined with H5 (no Origin check), a malicious party can open 10K hibernation sockets cheaply. Each pixel broadcast iterates all of them via `getWebSockets()` — O(N) per place.
- **Severity:** Low standalone, High when combined with H5.
#### L7. Test coverage gaps
- No tests for the DO itself (storage, cooldown, broadcast, hibernation).
- No tests for `chunk-storage.writePixels` (atomicity, multi-chunk batches, boundaries).
- No tests for `cooldown-store.tryAcquire` (race, GC, expiry).
- No tests for `getUserId` (header presence/absence, hashing).
- No integration / WS reconnect tests.
- The single test file (`worker-validation.test.js`) only exercises edge JSON validation — exactly what TypeScript types would catch.
- **Severity:** Low (process), High in aggregate (quality risk).
---
## Test file gaps (worker-validation.test.js)
- Mocks the DO as an empty class; never tests forwarding behavior under failure (DO 5xx, network error from the DO stub).
- No assertion that `getUserId` is called and forwarded in the body.
- No test for `MAX_BODY_BYTES` early reject (`content-length > MAX_BODY_BYTES`).
- No test that the DO response is passed through verbatim — currently only the body is checked, not headers, not status text.
- No test that `pixels` non-object element (e.g. `[null, 1, "x"]`) is rejected with `invalid_pixel`. (Code uses `p?.x` so null/non-object yields `undefined`, fails `Number.isInteger`, returns 400. Good — but untested.)
- No `/api/canvas` test at all.
- Boundary test (L137) only tests upper bound, not `(0, 0, 0)`.
---
## Cross-cutting observations
1. **Cooldown identity is the weakest link.** IP-based hashing collides under NAT (C3) and is shared in dev (H2). Combined with H1 (no daily cap), one IP can either lock out a building or repaint the whole canvas — both bad outcomes from the same input.
2. **The atomicity story is broken.** `writePixels` comment claims atomicity from "no `await`" (chunk-storage.js:86-87) but each `sql.exec` is auto-commit. Wrap the loop in `state.storage.transactionSync(() => { ... })` to actually deliver on the comment. C1+C2 both go away.
3. **Edge cache and WS race (H3+H4) is the canonical r/place clone bug.** Mitigation: serve canvas via the WS first message (snapshot frame) instead of via cached HTTP, *or* attach a `version` (monotonic counter) to broadcast frames and let the client refetch when it detects a gap.
4. **Hibernation API is used correctly.** `acceptWebSocket` (canvas-room.js:92) and `getWebSockets` (`:98`) are right. The `webSocketClose` re-close (M7) is the only smell; everything else aligns with CF docs.
5. **SQLite usage is conservative.** 64 KB BLOBs, single-PK rows, sparse rows. No schema landmines today. Future-proofing: assert `CHUNK_BYTES <= 2*1024*1024` in init.
---
## Suggested priority fixes (not implementing — read-only audit)
1. Wrap `writePixels` in `state.storage.transactionSync` (fixes C2, partially C1).
2. Move `tryAcquire` *after* `writePixels` succeeds, OR add a "release" path on storage failure (fixes C1).
3. Add `Origin` allowlist + max-clients cap on `#handleWsUpgrade` (fixes H5/L6).
4. Drop `s-maxage` to 1-2s (or remove edge cache and serve from DO live), and emit a sequence number on every broadcast frame to detect gaps (fixes H4 and partially H3).
5. Add per-IP daily quota in addition to 1Hz cooldown (fixes H1).
6. Augment `getUserId` with a stable client-side cookie (signed) to break NAT collisions (fixes C3).
7. Add static assertion `CHUNK_BYTES <= 2_000_000` (M5).
8. Add try/catch around `webSocketClose`'s re-close (M7).
9. Drain INSERT cursor symmetrically in `tryAcquire` (M1).
10. Add DO unit tests covering the gaps in L7.
---
## Status: DONE_WITH_CONCERNS
**Summary:** 3 critical, 5 high, 7 medium, 7 low findings across cooldown identity (IP/NAT), atomicity claims that don't match implementation, WS+cache race window, and image-importer DoS surface. Test coverage is limited to edge-validation; DO logic is untested.
**Concerns:**
- The "no await = atomic" comment in `chunk-storage.js:86-87` is wrong; needs `transactionSync` for true atomicity.
- IP-only identity is both a usability bug (NAT) and an abuse vector (no daily cap). These are the same fix from different angles.
- WS broadcast lacks an Origin gate — any site can hold sockets to the canvas DO.
**Unresolved questions:**
1. What is the documented per-cell BLOB limit on CF DO SQLite as of compat date 2025-04-01? (Affects M5 severity if user later resizes chunks.)
2. Is `state.storage.transactionSync` available on the Workers runtime version pinned by `compatibility_date: 2025-04-01`? If not, the atomicity fix needs `transaction(async () => {...})` instead.
3. Is there a planned per-room model (so `idFromName('main')` becomes one of many)? Per-room would naturally lift IP collision pain *if* combined with an account/cookie identity. Without that, sharding doesn't help.
4. Is there a CDN-layer mitigation for H4 (e.g. Cache API key on a freshness query string set by the DO)? The current `Cache-Control` headers will be honored by CF colos; product needs to decide live-fresh vs. cheap.
5. What's the operational policy on importer abuse? A 7M pixel/hr/IP cap is the canvas-rewrite budget; this is a product decision, not just engineering.
plans/reports/docs-manager-260510-0211-rplace-docs-drift.md
-228
View File
@@ -1,228 +0,0 @@
# rplace Documentation Drift Audit
**Date:** 2026-05-10
**Scope:** Verify docs alignment with recent Upstash → DO migration (commits a977adc through c3f7c02)
**Methodology:** Cross-check README, docs/, package.json, wrangler.json, .env.example, and active plan against actual src/ tree
---
## Summary
**Migration Status:** Phases 1–4 complete (files deleted, dependencies removed). Phase 5 partial (deploy done, 7-day observation window).
**Drift Found:** 4 stale/misleading items in README.md and .env.example. Docs in `docs/` directory are **accurate and current**. Migration plan ready for archival.
---
## Drift Findings
### STALE — README.md Project Structure (Lines 79–111)
**Severity:** Stale (factually wrong)
**Current State:**
```md
src/
├── worker.js # ✓ exists
├── admin/
│ └── migrate-from-upstash.js # ✗ DOES NOT EXIST (deleted in Phase 4)
├── durable-objects/ # ✓ exists
│ ├── canvas-room.js # ✓ exists
│ └── lib/
│ ├── schema.js # ✓ exists
│ ├── chunk-storage.js # ✓ exists
│ └── cooldown-store.js # ✓ exists
├── lib/
│ ├── constants.js # ✓ exists
│ ├── canvas-decoder.js # ✓ exists
│ ├── canvas-storage.js # ✗ DOES NOT EXIST (deleted in Phase 4)
│ ├── redis-client.js # ✗ DOES NOT EXIST (deleted in Phase 4)
│ ├── rate-limiter.js # ✗ DOES NOT EXIST (deleted in Phase 4)
│ ├── image-uploader.js # ✓ exists
│ └── get-user-id.js # ✓ exists
```
**Proposed Fix:**
Remove the entire `src/admin/` block and the three legacy lib files from the tree display:
```markdown
## Project Structure
```
src/
├── worker.js # Hono entry — thin proxy + edge validation
├── durable-objects/
│ ├── canvas-room.js # DO: storage + cooldown + WS hub
│ └── lib/
│ ├── schema.js # Idempotent CREATE TABLE
│ ├── chunk-storage.js # BLOB chunk read/write
│ └── cooldown-store.js # Rate-limit acquire + lazy GC
├── lib/
│ ├── constants.js # CANVAS_WIDTH/HEIGHT, CHUNK_BYTES, palette
│ ├── canvas-decoder.js # Raw bytes → RGBA (client-side)
│ ├── image-uploader.js # Browser-side batched uploader
│ ├── get-user-id.js # IP-based identity
│ ├── dither-kernels.js # Dithering algorithms
│ ├── image-color-correction.js # Color-space transform
│ ├── image-to-palette.js # Quantization
│ ├── image-transform.js # Scaling + rotation
│ ├── image-resize.js # Image dimensions
│ ├── image-pipeline.js # Multi-step image processing
│ ├── image-pipeline-client.js # Client-side queue
│ ├── image-pipeline-worker.js # Worker-side handler
│ ├── image-job-storage.js # Job persistence
│ └── pixel-buffer.js # Batch accumulator
├── client/
│ ├── main.js # Svelte mount
│ ├── App.svelte # Root + WebSocket
│ ├── app.css # Global styles
│ └── components/
│ ├── CanvasRenderer.svelte # Canvas + zoom/pan + touch
│ ├── ColorPicker.svelte # Favorites + 256-color grid
│ ├── CanvasControls.svelte # Zoom buttons + coordinates
│ ├── DrawToolbar.svelte # Paint / submit / undo / redo
│ └── ImageImporter.svelte # Image-to-canvas uploader
└── index.html # Vite entry
```
```
**Reason:** Upstash files deleted in commit a977adc. Showing orphaned files confuses developers and suggests the migration is incomplete. Current tree is incomplete (missing image pipeline files); use actual tree from bash scan.
---
### MISLEADING — README.md API Section (Lines 146–151)
**Severity:** Misleading (technically exists but endpoint removed)
**Current Text:**
```markdown
### `POST /admin/migrate-from-upstash` (transitional)
Token-gated one-shot endpoint that pulls the canvas from a legacy Upstash
Redis instance and imports it into the Durable Object. Slated for removal
after the production migration completes (Phase 4 of
[`plans/260509-2309-canvas-on-do-storage`](plans/260509-2309-canvas-on-do-storage)).
```
**Proposed Fix:**
Delete this section entirely. The endpoint was removed in commit a977adc (Phase 4 cleanup). No need to document historical endpoints.
**Reason:** Endpoint no longer exists in worker.js. Documenting it as "slated for removal" when it's already removed is confusing. Developers might spend time looking for it.
---
### STALE — .env.example
**Severity:** Stale (now incorrect for setup)
**Current Content:**
```
UPSTASH_REDIS_REST_URL=
UPSTASH_REDIS_REST_TOKEN=
```
**Proposed Fix:**
Delete file entirely or replace with a comment explaining that no external environment variables are required:
**Option A (Delete):** Remove `.env.example` — the repo has no external secrets now.
**Option B (Keep as placeholder):**
```
# No external secrets required.
# Canvas + cooldown state live inside CanvasRoom Durable Object (SQLite).
# All configuration is in src/lib/constants.js.
```
**Reason:** Current file references Upstash credentials that are no longer needed. New developers will be confused by empty placeholders for deleted services. Phase 4 success criteria explicitly calls for clean `.env.example`.
---
### ACCURATE — docs/ Directory
All docs files checked and found **accurate**:
- **canvas-resize-procedure.md** — correctly describes lazy-init, CHUNK_COUNT derivation, DO storage caps. No Upstash references.
- **deployment-guide.md** — migration section accurately marked `(Optional) One-Shot Migration from Upstash` with clear date context. No Upstash in main deploy flow.
- **system-architecture.md** — correctly describes CanvasRoom DO, SQLite schema, no Upstash. Migration endpoint marked "transitional" and "Removed in Phase 4".
- **code-standards.md** — does not reference Upstash or legacy code. Reflects current arch.
- **references.md** — informational only, no implementation details to drift.
---
## Cross-Check Results
### File Existence Verification
| File Reference | Status | Location |
|---|---|---|
| `src/worker.js` | ✓ Exists | Confirmed, 75 lines |
| `src/durable-objects/canvas-room.js` | ✓ Exists | Confirmed |
| `src/durable-objects/lib/schema.js` | ✓ Exists | Confirmed |
| `src/durable-objects/lib/chunk-storage.js` | ✓ Exists | Confirmed |
| `src/durable-objects/lib/cooldown-store.js` | ✓ Exists | Confirmed |
| `src/lib/constants.js` | ✓ Exists | Confirmed |
| `src/lib/canvas-decoder.js` | ✓ Exists | Confirmed |
| `src/lib/image-uploader.js` | ✓ Exists | Confirmed |
| `src/lib/get-user-id.js` | ✓ Exists | Confirmed |
| `src/admin/migrate-from-upstash.js` | ✗ Deleted | Removed in Phase 4 (a977adc) |
| `src/lib/canvas-storage.js` | ✗ Deleted | Removed in Phase 4 (a977adc) |
| `src/lib/redis-client.js` | ✗ Deleted | Removed in Phase 4 (a977adc) |
| `src/lib/rate-limiter.js` | ✗ Deleted | Removed in Phase 4 (a977adc) |
### Dependencies Verification
| Package | Current | Status |
|---|---|---|
| `@upstash/redis` | Not in package.json | ✓ Removed |
| `ioredis` | Not in package.json | ✓ Removed |
| `hono` | ^4.7.6 | ✓ Present, correct |
| `svelte` | ^5.28.2 | ✓ Present, correct |
### Configuration Verification
| Config Item | File | Status |
|---|---|---|
| DO binding name `CANVAS_ROOM` | wrangler.json line 11 | ✓ Matches docs reference |
| DO class `CanvasRoom` | wrangler.json line 12 | ✓ Matches src/durable-objects/canvas-room.js |
| SQLite migration tag `v1` | wrangler.json line 18 | ✓ Registered for CanvasRoom |
---
## Docs Directory Size Assessment
| File | Lines | Status |
|---|---|---|
| canvas-resize-procedure.md | 57 | ✓ Under 800 LOC limit |
| deployment-guide.md | 115 | ✓ Under 800 LOC limit |
| system-architecture.md | 165 | ✓ Under 800 LOC limit |
| code-standards.md | 51 | ✓ Under 800 LOC limit |
| references.md | 21 | ✓ Under 800 LOC limit |
---
## Plan Archive Status
`plans/260509-2309-canvas-on-do-storage/plan.md` marked `status: in-progress` but phases 1–4 complete and deployed to production.
**Proposed Action:** Update plan.md line 3 to `status: completed` (observing 7-day rollback window before archival).
---
## Unresolved Questions
1. **Image pipeline files** — README was outdated before audit (missing `image-pipeline.js`, `image-pipeline-client.js`, etc.). Was the tree intentionally simplified, or is it an ongoing drift issue unrelated to migration?
2. **.env.example strategy** — Are empty placeholder secrets (Option B) preferable to deletion (Option A) for discoverability?
---
## Recommended Actions (Priority Order)
1. **README.md line 85–97** — Remove `src/admin/` and three legacy lib files from project structure tree.
2. **README.md line 146–151** — Delete `/admin/migrate-from-upstash` API section.
3. **.env.example** — Delete or replace with placeholder comment.
4. **plans/260509-2309-canvas-on-do-storage/plan.md line 3** — Change `status: in-progress` → `status: completed` after 7-day window (ca. May 17, 2026).
---
**Status:** DONE
**Summary:** 4 stale refs flagged (README tree + API section, .env.example, plan status). Docs directory accurate. No broken links or config mismatches. Ready for targeted edits.
plans/reports/researcher-260416-1557-hosting-comparison-rplace.md
-198
View File
@@ -1,198 +0,0 @@
# Hosting Platform Comparison: Vercel vs Netlify vs Cloudflare for r/place Clone
**Date:** 2026-04-16
**Scope:** Real-time collaborative pixel canvas (r/place clone) with Next.js, Upstash Redis, SSE, and potential viral traffic spikes.
---
## Comparison Matrix
| Criterion | Vercel Hobby | Netlify Free | Cloudflare Workers Free |
|-----------|--------------|--------------|-------------------------|
| **Free Tier Invocations/Month** | 1M | 125k | 3M (100k/day) |
| **Function Timeout** | 10s (60s with config) | 10s (26s on Pro) | 10ms CPU per request |
| **SSE/Streaming Support** | ✅ 60s (serverless) / 300s (edge) | ✅ 20 MB payload | ✅ 30s per WebSocket msg |
| **WebSocket Support** | ❌ No | ❌ No | ✅ Yes (Durable Objects) |
| **Cold Start Latency** | ~1s | 3+ seconds | <5ms (V8 isolates) |
| **Bandwidth Included** | 100GB | 100GB | Unlimited |
| **SSE Max Duration** | 60s base, 300s with Edge | 10s (26s Pro) | No limit (WebSocket) |
| **Concurrent Connections** | Framework-limited | High | 6 simultaneous fetches |
| **Upstash Redis** | ✅ REST API | ✅ REST API | ✅ REST API + native SDK |
| **Edge Runtime** | ✅ (300s) | ❌ Via Edge Functions | ✅ (V8 isolate, default) |
| **Durable Objects / Stateful** | ❌ | ❌ | ✅ (SQLite-backed on free) |
| **Cost at 10M Req/Month** | ~$6 (1M free) | $2/100k invocations | $0.30/M (sub-$5) |
---
## Free Tier Breakdown
### Vercel Hobby
- **Invocations:** 1M/month, 100 deployments/day
- **Bandwidth:** 100GB included
- **SSE Support:** Serverless (10s default, 60s configurable) or Edge Functions (25s to first byte, then 300s total)
- **Pain Point:** Timeout too short for long-lived SSE without upgrade. WebSocket not supported.
- **Recommendation:** Viable only with aggressive client-side batching or Edge Functions.
### Netlify Free
- **Invocations:** 125k/month
- **Bandwidth:** 100GB included
- **SSE Support:** 10s timeout by default, extendable to 26s on Pro
- **Edge Functions:** 50ms CPU time (idle time doesn't count, better for SSE)
- **Pain Point:** Lowest free invocation quota; shared 50MB memory default
- **Recommendation:** Weakest choice for real-time pixel canvas due to invocation limits and timeout constraints.
### Cloudflare Workers Free
- **Requests:** 100k/day (3M/month), unlimited after paid
- **CPU Time:** 10ms per request (idle time excluded for WebSocket)
- **WebSocket Support:** Native via Durable Objects; 32 MiB message size
- **Durable Objects:** 5 GB free storage (SQLite-backed)
- **Cold Starts:** <5ms globally
- **Pain Point:** 6 concurrent fetch limit; eventually-consistent KV; 1 write/sec/key limit
- **Recommendation:** Best architectural fit for real-time apps; native WebSocket preferred over SSE.
---
## SSE vs WebSocket: Technical Tradeoffs
**SSE (Vercel, Netlify):**
- Simpler to implement (HTTP-based)
- Max duration: 60–300s depending on platform
- Client reconnection logic required on timeout
- ~3–5KB overhead per connection
- Scaling: 2.5MB canvas = ~833 concurrent viewers at 3KB/sec
**WebSocket (Cloudflare):**
- Bi-directional communication (publish-subscribe patterns)
- No timeout; runs until connection closed
- Lower per-message overhead (~200 bytes)
- Durable Objects provide room-scoped state (excellent for isolated pixel groups)
- Scaling: Single Durable Object handles ~50–100 WebSocket connections
**Recommendation for r/place:** WebSocket + Durable Objects (Cloudflare) is architecturally superior. Eliminates timeout pain, enables efficient room-based scaling, and handles viral traffic spikes better.
---
## Redis vs Alternatives
### Upstash Redis (BITFIELD)
- **All platforms:** Compatible via REST API
- **Performance:** Handles BITFIELD operations (critical for pixel state as bitmaps)
- **Latency:** ~50–100ms globally
- **r/place fit:** Essential for efficient canvas storage (1M pixels = 125KB bitfield)
- **Cost:** Free tier (10k commands), then $0.0001/command
### Cloudflare Workers KV (Alternative)
- **Write Limit:** 1 write/sec/key (dealbreaker for high-frequency updates)
- **No BITFIELD:** Cannot efficiently store pixel bitmaps
- **Eventual Consistency:** Unacceptable for pixel canvas state
- **Verdict:** Not suitable; stick with Upstash Redis
---
## Adoption Risk & Maturity
**Vercel (Lowest Risk)**
- De facto Next.js standard
- Excellent DX, tight framework integration
- Proven at scale (millions of sites)
- **Risk:** SSE timeout constraints; may need Edge Functions workaround for long streams
**Netlify (Medium Risk)**
- Mature platform but slower cold starts
- Free tier invocation quota is limiting for real-time workloads
- **Risk:** Function timeout + low quota = poor scaling for spikes
**Cloudflare Workers (Low Risk, High Reward)**
- Production-ready; powers millions of requests
- OpenNext adapter (1.0-beta) adds Next.js 14/15/16 support
- **Risk:** OpenNext is newer than @vercel/next; Node.js runtime adds ~50ms overhead vs V8 isolates. Adoption of Durable Objects is lower than serverless.
---
## Scaling to Viral Traffic
**Scenario:** r/place unexpectedly spikes from 100 users to 100k users in 1 hour.
| Platform | Behavior | Cost Impact |
|----------|----------|------------|
| **Vercel** | Scales function invocations; may hit free tier quota ($6 overage) | Moderate |
| **Netlify** | Hits 125k invocation cap within minutes; service degradation | Severe (quota suspension) |
| **Cloudflare** | Handles surge gracefully at $0.30/M cost | Minimal ($30 for 100M req) |
**Winner:** Cloudflare. Auto-scaling, low marginal cost, no quota walls.
---
## Next.js App Router on Cloudflare
**OpenNext Cloudflare Adapter (1.0-beta)**
- ✅ Supports Next.js 14, 15, 16
- ✅ App Router fully supported
- ✅ Incremental Static Regeneration (ISR) works
- ❌ Node.js runtime (slower than V8 isolates by ~50ms)
- ⚠️ Beta status; breaking changes possible
**Setup:** `npm install -D @opennextjs/cloudflare` + `wrangler.toml` configuration.
**Gotcha:** SSE doesn't natively work in serverless. Use **Durable Objects WebSocket** instead for real-time updates.
---
## Deployment DX
| Platform | Preview Deploys | Git Integration | Local Dev | CI/CD |
|----------|-----------------|-----------------|-----------|-------|
| **Vercel** | Instant per PR | Native; auto-deploy | `vercel dev` | First-class |
| **Netlify** | Instant per PR | Native; auto-deploy | `netlify dev` | Good |
| **Cloudflare** | Via `wrangler publish` | Requires setup | `wrangler dev` | Requires scripts |
**DX Winner:** Vercel (tightest Next.js integration), but Cloudflare's `wrangler` is improving rapidly.
---
## Recommendation
### For Hobby/MVP (Simplicity First)
**Vercel Hobby** — Start here. Easy Next.js deployment, sufficient free tier for initial testing. **Accept:** SSE timeout constraints; use client-side reconnection logic or split long streams.
### For Scale-Ready Production (Real-Time First)
**Cloudflare Workers + Durable Objects** — Best architecture for r/place:
- ✅ Native WebSocket support (no timeout)
- ✅ Durable Objects for room-based pixel groups
- ✅ Sub-5ms cold starts
- ✅ Scales to viral traffic at <$30/month
- ✅ Upstash Redis compatibility (REST API)
- ⚠️ Requires OpenNext adapter (beta); monitor releases
**Hybrid Approach (Safe):**
Deploy on **Vercel** initially. When traffic patterns stabilize and WebSocket needs are confirmed, migrate API layer to **Cloudflare Workers** while keeping Next.js frontend on Vercel (edge functions for canvas downloads, Workers for real-time WebSocket).
---
## Unresolved Questions
1. **Durable Objects namespace limits:** How many rooms (pixel groups) can a single namespace handle before rate-limiting?
2. **OpenNext adapter stability:** Expected timeline for 1.0 release; any known breaking changes between beta and GA?
3. **Upstash Redis + Durable Objects:** Can Durable Objects efficiently call Upstash REST API, or should canvas state be mirrored in both KV and Redis?
4. **Client payload size:** 2.5MB canvas download on initial load—confirmed acceptable on mobile?
---
## Sources
- [Vercel Limits Documentation](https://vercel.com/docs/limits)
- [Vercel Functions Limitations](https://vercel.com/docs/functions/limitations)
- [Vercel Edge Functions Streaming](https://vercel.com/blog/streaming-for-serverless-node-js-and-edge-runtimes-with-vercel-functions)
- [Netlify Pricing](https://www.netlify.com/pricing/)
- [Netlify Functions Overview](https://docs.netlify.com/build/functions/overview/)
- [Netlify SSE Support](https://edge-functions-examples.netlify.app/example/server-sent-events)
- [Cloudflare Workers Limits](https://developers.cloudflare.com/workers/platform/limits/)
- [Cloudflare Durable Objects Pricing](https://developers.cloudflare.com/durable-objects/platform/pricing/)
- [Cloudflare Durable Objects Limits](https://developers.cloudflare.com/durable-objects/platform/limits/)
- [OpenNext Cloudflare Adapter](https://opennext.js.org/cloudflare)
- [Deploying Next.js with OpenNext on Cloudflare](https://blog.cloudflare.com/deploying-nextjs-apps-to-cloudflare-workers-with-the-opennext-adapter/)
- [Upstash Redis Compatibility](https://upstash.com/docs/redis/sdks/ts/deployment)
- [Upstash + Cloudflare Workers Integration](https://upstash.com/blog/cloudflare-upstash-integration)
- [Cloudflare KV vs Redis Benchmark](https://upstash.com/blog/edgecaching-benchmark)
- [Cloudflare Workers vs Vercel Cold Start Comparison](https://dev.to/dataformathub/cloudflare-vs-vercel-vs-netlify-the-truth-about-edge-performance-2026-50h0)
- [Cloudflare Workers WebSocket Documentation](https://developers.cloudflare.com/workers/runtime-apis/websockets/)
plans/reports/researcher-260418-1217-wplace-autobot-logic.md
-110
View File
@@ -1,110 +0,0 @@
# WPlace-AutoBOT UX Flow Research
**Date:** 2026-04-18
**Scope:** Browser extension bot/uploader UX patterns (non-image-processing)
**Source:** https://github.com/Wplace-AutoBot/WPlace-AutoBOT
---
## Findings by Topic
### 1. Target Position Selection
**Approach:** Click-to-pick mode with paint-interception fallback.
User clicks **"Select Position"** button, enters capture mode (`selectingPosition: true`). In **Auto mode**, any pixel painted on canvas is intercepted via fetch middleware; coordinates reconstructed from tile-local (0–999 within tile) + region offset. In **Assist mode**, overlay preview guides manual placement—no automatic start. Optional manual coordinate input via prompt (format: `x,y`). Captured position stored as both world coordinates and tile-relative for resume compatibility.
**Ref:** `Auto-Image.js:1038–1043, 9412, 9494–9700` | `Art-Extractor.js:130–180` (manual coordinate prompt pattern)
### 2. Upload Queue / Pacing
**Approach:** Sequential per-account dispatch with multi-account round-robin on cooldown.
Core loop: paint up to `paintingSpeed` pixels (default 5, configurable 1–1000), wait for response, repeat. Per-user cooldown enforced server-side (31s default). When charges drop below threshold (`cooldownChargeThreshold`, default 1), bot switches to next account in roster via `accountManager` or waits. Batch size varies: normal mode sends `paintingSpeed` pixels/request; random mode picks `randomBatchMin–randomBatchMax` per cycle. No explicit retry-on-429; instead relies on account rotation. One `sleep()` call per batch loop (no polling).
**Ref:** `Auto-Image.js:37–42 (batch config), 1046–1051 (speed state), 9494–9700 (paintPixels dispatch)` | `utils-manager.js` (smartSave at 25+ pixels, 30s minimum)
### 3. Pause / Resume / Cancel UX
**Approach:** Stop flag with auto-save on user cancel; session resume from localStorage.
**Pause/Cancel:** Stop button sets `state.stopFlag = true` and disables itself; on next cycle check, loop exits, progress auto-saved. Resume available if `savedData.state.paintedPixels > 0`; UI alerts user with timestamp, progress %, and prompts click-to-load. No explicit pause (only stop). Progress persists in localStorage (version 2.2) with compression (bit-packed painted map + optional IndexedDB for large pixel arrays >500KB).
**Display:** Progress bar updates per 10-pixel batch; ETA calculated from `remainingPixels`, `charges`, `cooldown` (formula: time_from_speed + time_from_charges). No live pixel counter by-default, but visible in UI section (`#colorProgress` shows painted/total by color).
**Ref:** `Auto-Image.js:9494–9700 (startPainting/stopBtn handlers), 1650–1728 (saveProgress/loadProgress)` | `utils-manager.js:200–300 (calculateEstimatedTime, formatTime)` | `Auto-Image.js:2700–2810 (progress bar + cooldown UI)`
### 4. Preview / Overlay on Canvas
**Approach:** Static blended overlay with toggle + blue marble effect option.
**Overlay Display:** Composited onto tiles via `OverlayManager` class. Uses `source-over` blending with user-controlled opacity (default 0.6, slider 0–1). Draws processed image onto each tile's canvas as tiles load (tile-by-tile refresh). Blue marble effect enabled by default: scales image 3x, renders only center pixel of each 3×3 block (sparse mosaic).
**Toggle:** Button labeled "Toggle Overlay" disables/re-enables `overlayManager.isEnabled`. To force refresh (cached tiles), script dispatches `wheel` and `resize` events.
**Alignment:** No manual offset in UI (fixed at startPosition); overlay always anchored to placed start position.
**Ref:** `overlay-manager.js` (entire file: toggle(), globalAlpha, blue marble algorithm) | `Auto-Image.js:1058–1059, 4489 (opacity slider + processImage)` | `Auto-Image.js:3310–3330 (slider styling)`
### 5. Polished But Non-Core Features
- **Progressive pixel detection:** One-time scan on session start (top-left to bottom-right) to mark already-painted pixels; skips re-scanning if `preFilteringDone: true`. Avoids redundant requests on resume.
- **Ref:** `Auto-Image.js:9497–9515` | `Auto-Image.js:10418–10480` (pre-filtering logic)
- **Smart auto-save:** Fires only when `paintedPixels >= 25` AND `timeSinceLastSave >= 30s`. Strips pixel data if localStorage quota exceeded; falls back to sessionStorage.
- **Ref:** `utils-manager.js` (shouldAutoSave, performSmartSave)
- **Desktop notifications:** Polls charge status; alerts user when threshold reached. Respects focus state (only notify if tab unfocused). Repeats every 5 min while condition holds.
- **Ref:** `Auto-Image.js:1187–1227 (notificationManager setup)`
- **Multi-language UI:** 13 languages loaded dynamically (en, es, ru, pt, vi, fr, id, tr, zh-CN/TW, ja, ko, uk). Defaults to browser locale or English fallback.
- **Ref:** `Auto-Image.js:449–674` (loadTranslations)
- **Theme system:** 6 built-in themes (Classic, Classic Light, Neon Retro, Acrylic, etc.) with CSS variables + extension-injected stylesheets. Persists in localStorage.
- **Ref:** `Auto-Image.js:170–290 (CONFIG.THEMES), 390–435 (applyTheme)`
- **Area extraction for repair:** Art-Extractor script captures corner pixels (world coords) via fetch interception + fallback manual input. Stores as region + local offset for later repair tasks.
- **Ref:** `Art-Extractor.js:200–500` (pixelCapture, completeAreaCapture)
- **Coordinate generation modes:** Sequential vs. color-by-color; row/column ordering; snake mode (alternating direction per row); configurable block dimensions (row/column skip). Useful for large images to prioritize skin/background.
- **Ref:** `Auto-Image.js:1103–1107` (state.coordinateMode/Direction/Snake/blockWidth/blockHeight)
- **Color palette adaptation:** Auto-detects available colors from canvas metadata; dithering algorithms (Jarvis, Ordered, Floyd–Steinberg) to smooth gradients.
- **Ref:** `image-processor.js` (referenced, not fully shown)
---
## Candidate Imports for Our Site
1. **Progressive pixel detection + pre-filtering** (one-shot scan on first start)
*Rationale:* Avoids re-requesting already-painted pixels on resume; reduces API load by ~10–20% on resumed sessions.
2. **Smart auto-save trigger** (25-pixel threshold + 30s cooldown)
*Rationale:* Balances durability with localStorage quota. Matches your server's 1 req/sec limit naturally.
3. **Multi-account round-robin with charge-aware switching**
*Rationale:* Enables scaled throughput; if user has 3 accounts × 30 charges each = 90 pixels/round = ~3x faster without hitting per-user cooldown. Your Redis can track active account rotation.
4. **Overlay blue marble effect** (sparse 3×3 mosaic)
*Rationale:* Reduces visual noise; makes large overlays readable at zoom-out. Distinctive UX that feels intentional, not a bug.
5. **Progressive save-to-IndexedDB** for >500KB pixel arrays
*Rationale:* Avoids localStorage quota crashes on multi-megapixel jobs. Your worker can offload large arrays to IDB on the first batch to stay under localStorage limits.
6. **Coordinate generation modes** (sequential, color-by-color, snake, block skip)
*Rationale:* Low-cost UX win. Lets users prioritize aesthetically (e.g., paint skin first). Doesn't change core pacing but feels sophisticated.
---
## Architecture Notes for Our Stack
- **Fetch interception** (WPlace-AutoBOT uses it for both pixel painting + assist-mode overlay remapping) is **not portable to a SPA**. Your worker + Svelte client already separates concerns cleanly; keep painting strictly server-side.
- **LocalStorage-based resume** works here because it's a userscript. You'll want **Upstash Redis** to persist user job state (imageData, painted pixels, position, timestamp) server-side. Allows resume across browser sessions & devices.
- **Account rotation** requires a user-managed roster. Store as `user_accounts: [{token, displayName, charges, lastUsed}]` in your user doc or a separate KV namespace.
- **Blue marble effect** is client-side canvas manipulation; keep it in Svelte component for preview. Doesn't change server painting.
---
## Unresolved Questions
1. **Does WPlace-AutoBOT handle 429 (rate limit) responses explicitly?** → No explicit retry-on-429 found in code path. Relies on cooldown + account rotation to stay under limit. If server returns 429, behavior unclear (likely treated as failed batch, continues next cycle). May silently lose pixels.
2. **How does account roster persist?** → Uses browser's `localStorage.getItem("accounts")` + `chrome.storage.local`. No cross-device sync. Unclear how users add new accounts (extension UI must have hidden menu).
3. **Does sketch/undo exist?** → No undo or pixel-level editing found. Progress is forward-only; stop → resume paints remaining pixels in same order.
4. **Can users adjust painting order mid-job?** → `state.coordinateMode` can be changed, but unclear if it applies to remaining pixels or re-orders from scratch. Likely needs restart.
5. **What triggers overlay refresh on tile cache hit?** → Script dispatches synthetic `wheel` + `resize` events. Undocumented; may not work on all canvas implementations (especially if WPlace uses WebGL). Falls back to manual toggle.
---
## Summary
WPlace-AutoBOT's UX is **pragmatic & modular**. It offloads position selection to paint interception, uses localStorage for resume, and abstracts account rotation into the cooldown loop. No fancy pause (only stop) or per-pixel undo. The overlay system is simple (opacity + effect toggle) and tied tightly to tile refresh. The "polish" comes from smart saves, multi-language i18n, theme system, and area extraction tools—features that don't block the core painting loop but improve perceived quality.
For your rplace rebuild, focus on **server-side job persistence** (Upstash), **skip-painted filtering**, and **multi-account routing** in your Worker. The overlay & coordinate modes are nice-to-have, not critical path.
plans/reports/researcher-260418-1217-wplace-design.md
-156
View File
@@ -1,156 +0,0 @@
# Wplace.live UI/UX Design Research
**Date:** 2026-04-18
**Analyst:** Researcher
**Scope:** wplace.live design patterns vs. rplace current implementation
---
## 1. Color Palette Picker
**Wplace.live:** 64-color palette (24 free + 40 premium). Browser extensions provide quick color picker overlays with coordinate display; third-party tools emphasize "easy color identification" via visual feedback. No detailed native UI layout found (grid vs. wheel not specified in accessible sources). Color converter tools support multiple input methods (direct value entry, visual pickers, palette selection) with real-time feedback.
**Your current impl:** 32-color grid, docked bottom, 8-wide × 4-tall layout, 32px swatches, hover scale 1.2, selected state has white border + glow, dark background (rgba 0,0,0 0.85) with blur. **Matches pattern.** You have fewer colors and smaller grid. Wplace's ecosystem relies heavily on *third-party tools* for picker UX (not native), suggesting their built-in picker may be minimal.
**Verdict:** Your picker is more polished than wplace's likely native implementation. Third-party tools suggest the base UI doesn't have an outstanding picker. **No immediate import needed.**
---
## 2. Canvas Controls
**Wplace.live:**
- Zoom: Mouse wheel, Q/E keyboard shortcuts, "Zoom in to see pixels" button (jumps to level 10; pixels only visible at zoom ≥10)
- Pan: WASD keys + mouse drag
- Coordinates: Real-world location display (latitude/longitude); third-party extensions add coordinate search (format: `123,456` or degrees-minutes-seconds)
- **No minimap found**; third-party tools offer "location manager" to bookmark/jump to spots
- Geographic context: Every pixel maps to real-world locations on OpenStreetMap
**Your current impl:** Zoom buttons (+ / −) at bottom-left, mouse wheel zoom, coordinates display, zoom levels 1–19 (user not specified), pan via drag. **Differs:** No keyboard shortcuts (Q/E), no WASD pan, no goto-coordinates input, no location bookmarks. **Lacks:** No real-world coordinate mapping (your canvas is abstract 2048×2048).
**Verdict:** Wplace's keyboard shortcuts (Q/E zoom, WASD pan) + goto-coordinates input are **quick UX wins**. Minimap is provided by third-party only, not native. **Recommend borrowing:** Keyboard pan/zoom shortcuts + coordinate search input.
---
## 3. User Info / Rate-Limit Display
**Wplace.live:** Cooldown timer displayed somewhere (referenced as "1 pixel every 30 seconds"). Pixel charge system with "Pixel Pool" that starts at 30–64 and expands via purchase or leveling. Leaderboards for players, alliances, countries, regions. **No explicit cooldown countdown timer UI details found** in accessible sources; third-party overlays enhance this.
**Your current impl:** Rate limit is 1 request/second (batch-independent), not per-pixel. No visible cooldown timer, no pixel-pool display, no level/purchase system, no leaderboards. **Lacks:** Visible countdown, gamification (levels, inventory), leaderboards.
**Verdict:** Wplace emphasizes **gamification** (levels, pixel pool, leaderboards) and **cooldown transparency** (visible timer). Your app is simpler (no progression system planned?). **Recommend:** Add visible cooldown countdown on next-available timestamp (e.g., "Next paint in 3s"). Leaderboard / progression left to future scope.
---
## 4. Place / Submit Flow
**Wplace.live:** Click-to-draw; pixel placement mechanics include eraser tool (dedicated button or spacebar + cursor). Charge system: clicks spend from pixel pool. No mention of pending buffer, explicit submit, or undo/redo in core flow. **Single-action placement** (no pending buffer like yours).
**Your current impl:** **Two-stage flow:** paint locally (pending buffer), then click Submit to POST batch. Undo/Redo buttons available. Paint mode toggleable. No eraser tool. **Differs significantly:** You have a *pending buffer* (uncommitted pixels), wplace has *immediate placement* with a charge pool.
**Verdict:** Your buffering approach is **safer for batch optimization** (batches up to 2048 pixels, 1 req/sec). Wplace's single-action model feels more responsive but requires careful rate-limiting on the server side. **No import suggested**—your batch model is architecturally sounder for a small canvas. Eraser tool could be added as a future enhancement.
---
## 5. Image Importer / Converter
**Wplace.live:** External pixel-art converters (Floyd-Steinberg, Ordered, Atkinson dithering), grid overlay, drag-and-drop upload, real-time preview, 64-color palette matching, browser-side processing (no server upload). Multiple converters available; UI is clean and intuitive but not deeply detailed in sources.
**Your current impl:** Full image importer (phase 6 completed). Features: dither methods (none, ordered, floyd-steinberg, ordered-64, atkinson), skip-white + paint-transparent toggles, color correction (brightness, contrast, saturation, gamma), resize with aspect-lock, flip/rotate transforms, on-canvas overlay preview with alpha control, drag-and-drop, live batched upload with progress. **Exceeds** wplace converters in sophistication and integration.
**Verdict:** **Your importer is more feature-rich than wplace's external tools.** You already have a competitive advantage. No imports needed. Your in-app integration + color-correction sliders are differentiators.
---
## 6. General Visual Language
**Wplace.live:**
- Dark theme (maps are typically dark by default on MapLibre/OSM)
- Web-based overlay on world map (geographic context is the visual centerpiece)
- Collapsible/draggable UI windows (mentioned in search results)
- Light/dark mode support (browser extensions mention toggle)
- No detailed typography or specific accent-color guidance found
- Onboarding / empty-state: Not documented in accessible sources
**Your current impl:**
- Dark theme only
- Abstract 2048×2048 canvas (no geographic context)
- Fixed layout: toolbar top-right, color picker bottom-center, controls bottom-left, importer panel top-right
- Rgba panels with blur, white accent on selected items
- No onboarding, no empty-state visual
- Mobile: pinch-zoom + touch-drag supported (no specific mobile UI layout)
**Verdict:** Wplace's **draggable/collapsible UI** is a nice flexibility feature but not critical. Your fixed layout is simpler and clearer. **Recommend:** Add a simple onboarding tooltip (first-time visit) explaining paint → submit flow, color picker, and zoom controls. Mobile layout could benefit from a mobile-specific toolbar (e.g., stacked buttons instead of side-by-side).
---
## Candidate Imports for Our Site
### 1. **Keyboard Shortcuts for Pan & Zoom** (small effort)
- **What:** Add Q/E for zoom in/out and WASD for pan (wplace pattern)
- **Why:** Power-user friendly, faster workflow, matches common game/design tool patterns
- **Effort:** Small (2–3 lines per command, keyboard handler already exists)
### 2. **Goto-Coordinates Input** (small effort)
- **What:** Add a text input in canvas-controls to jump directly to x,y (e.g., "256,512" → center canvas there)
- **Why:** QoL improvement for large canvases; wplace's search feature is a third-party add-on, suggesting native feature gap
- **Effort:** Small (input field + canvas center/zoom logic)
### 3. **Visible Cooldown Countdown** (small effort)
- **What:** Display "Next paint in Xs" on the submit button when rate-limited
- **Why:** Reduces user confusion; wplace players expect cooldown visibility
- **Effort:** Small (store next-available timestamp, update UI every 100ms)
### 4. **Simple Onboarding Tooltip** (medium effort)
- **What:** First-time visitor banner explaining: select color → paint → submit, with mouse/touch hints
- **Why:** Wplace relies on external guides; your app should be self-documenting
- **Effort:** Medium (modal/banner component, localStorage to hide after first interaction)
### 5. **Mobile-Optimized Toolbar** (medium effort)
- **What:** Stack or simplify draw toolbar buttons for small screens; ensure color picker is touch-friendly
- **Why:** Wplace is discussed as "viral on TikTok"—heavy mobile use; your importer is on-brand for desktop first
- **Effort:** Medium (responsive breakpoint, grid-to-column layout swap, touch target sizes ≥48px)
### 6. **Eraser Tool (Optional)** (medium effort)
- **What:** Toggle eraser mode (paints transparent or placeholder color) or spacebar + click to erase
- **Why:** Wplace offers this; useful for corrections without undo overhead
- **Effort:** Medium (add mode toggle, update paint logic to handle erase color)
---
## Key Findings
| Aspect | Wplace Pattern | Your Implementation | Gap | Priority |
|--------|---|---|---|---|
| Color picker | 64-color palette (external tools provide UI) | 32-color grid, polished | None—yours is better | — |
| Canvas controls | Q/E zoom, WASD pan, goto-coords | Buttons, mouse-wheel, no keyboard | Keyboard shortcuts missing | Small |
| Cooldown display | Visible countdown (implicit from pooling) | None shown | Transparency gap | Small |
| Place flow | Immediate (charge-based) | Buffered + submit | Architectural difference; yours is safer | — |
| Image importer | External dither converters | In-app, 6 dither methods + color correction | Yours exceeds | — |
| Visual language | Dark, map-centric, collapsible UI | Dark, fixed layout, polished panels | Layout flexibility gap (non-critical) | Large (low ROI) |
| Onboarding | Not found (likely minimal) | None | User confusion risk | Medium |
---
## Sources
- [Wplace.live Guide](https://wplace.life/)
- [Place Live vs. Wplace Live: From Reddit's r/Place to a Global Pixel World](https://wplace.style/blog/reddit-place-vs-wplace)
- [Ultimate Wplace.live Guide — Cooldown, Tools & Hot Regions](https://wplaceartconverter.com/wplace-guide)
- [Complete Wplace.live Location Search Guide](https://wplacepixelconverter.org/blog/complete-wplace-live-location-search-guide/)
- [Wplace.live Extension & Tools](https://wplacetool.com/wplace-extension)
- [Wplace Pixel Art Converter — Free Online Tool](https://wplaceconverter.net/)
- [GitHub - Wplace Tools & Overlays](https://github.com/ethansunray/wplace-tool)
- [Wplace Color Palette Reference](https://wplacepixel.com/wplace-color-palette)
---
## Unresolved Questions
1. **Wplace native UI details:** Direct access to wplace.live blocked (403). Details on built-in color picker layout (grid width, swatches per row) inferred from third-party tools, not official UI. If critical, may need user account or GitHub repo inspection.
2. **Cooldown timer implementation:** Wplace sources mention cooldown system but don't specify countdown display details—inferred as "expected" from community tool discussions.
3. **Mobile layout:** Wplace's viral TikTok popularity suggests strong mobile use, but no mobile-specific UI layout details found in sources.
4. **Onboarding flow:** Neither wplace nor your current app have documented onboarding; gap identified from *absence*, not competitor feature.
---
**Report Status:** Ready for design review. Highest-ROI imports are keyboard shortcuts (#1), goto-coordinates (#2), and cooldown countdown (#3). All are small-effort QoL wins.
plans/reports/researcher-260418-1431-palette-quantization-fast-paths.md
-202
View File
@@ -1,202 +0,0 @@
# Fast Palette Quantization: Research Report
**rPlace 256-Color HSL Palette Optimization Study**
---
## 1. LUT Sizing: 4-bit vs 5-bit vs 6-bit
**Current implementation:** 5-bit LUT (32³ = 32 KB, O(1) lookup per pixel).
**Findings:**
- **5-bit (32³ = 32 KB):** Build cost ~8M ops. Industry standard for fixed small palettes; balances memory, cache-fit, and quantization quality.
- **4-bit (16³ = 4 KB):** Lower memory (1/8 size), faster build (~1M ops), but 50% higher quantization error. Cache-perfect on even old devices. Only viable if image quality acceptable.
- **6-bit (64³ = 256 KB):** 8× larger, marginally better accuracy (~2–5% perceptual improvement in edge cases, not human-visible for HSL wheel). Build ~64M ops. Not worth it for browser memory profile.
**Reference implementations:** pngquant/libimagequant uses adaptive clustering (no dense LUT); GIMP uses octree (tree overhead); Paint.NET provides Median Cut (k-d tree). None use dense RGB LUTs—they optimize for *adaptive* palettes. For *fixed* palettes, dense LUT is superior.
**Recommendation:** **Stick with 5-bit.** Sweet spot. Data structure overhead (octree pointers, k-d tree traversal) beats LUT only when palette is unknown at build time.
---
## 2. Data-Structure Alternatives: k-d Tree, Octree, VP-Tree, Ball Tree
**Pointer chasing vs linear memory:**
- **Octree:** Most common. Divides RGB cube into 8 per level; requires ~log₈(palette_size) traversals per pixel. For 256 colors: 2–3 levels. ~10–20 CPU cycles per lookup (pointer chasing, cache misses). Paper: "Octree Color Quantization" (1988, Gervautz/Purgathofer).
- **k-d tree (Median Cut):** Recursively splits longest axis. More balanced than octree but same fundamental cost. Slightly better spatial locality.
- **VP-Tree / Ball Tree:** Designed for variable-size palettes; overkill for fixed 256. Worse cache behavior than LUT.
**LUT advantage:** Single memory fetch, zero branch prediction. Modern CPUs: ~1–2 cycles (L1 cache hit). For 4096² image: ~16M pixels × 15 cycles (tree) vs ~2 cycles (LUT) = **7–8× speedup**.
**Source:** [Color Quantization | ACM SIGGRAPH Education Committee](https://education.siggraph.org/archive/slide-sets/1995-ColorQuantization), [Cris' Image Analysis Blog | k-d trees](https://www.crisluengo.net/archives/932/).
**Recommendation:** **LUT unbeatable for fixed palette.** Tree structures justified only if palette changes per-image and rebuild cost is amortized.
---
## 3. Perceptual Color Spaces: Oklab, CIELab, YCbCr
**Why it matters:** Quantizing in perceptual space gives visually smoother gradients; RGB space has non-uniform error visibility.
**Findings:**
- **Oklab:** Modern (2020), more uniform than CIELAB. ~4 arithmetic ops to convert RGB→Oklab. ~2–3% perceptual improvement in gradient smoothness. CSS Level 4 standard.
- **CIELAB:** Established, ~10% slower conversion than Oklab. Widely used in quantization literature. Both give similar results for uniform palettes (grayscale + hue wheel).
- **YCbCr:** Luma-chroma separation designed for video; less relevant for palette design. No perceptual uniformity guarantee.
**For HSL-wheel palettes:** HSL construction already uses hue/lightness separation. Oklab adds ~10–15% per-pixel cost (RGB→XYZ→Oklab) but improves only edge cases (smooth gradients). Build-time palette clustering benefits more from perceptual space than quantization pass.
**Source:** [Oklab: A perceptual color space](https://bottosson.github.io/posts/oklab/), [CIELAB Wikipedia](https://en.wikipedia.org/wiki/Oklab_color_space).
**Recommendation:** **Skip for runtime quantization.** Fixed palette already well-designed. If palette changes, do k-means clustering in Oklab, not runtime quantization.
---
## 4. Dithering Parallelization: Floyd-Steinberg, Atkinson, Riemersma
**Challenge:** Error diffusion is inherently serial (each pixel depends on prior error).
**Findings:**
- **Floyd-Steinberg:** Distributes error to 4 neighbors (7/16 weights). ~30% of dithering overhead. Block-based parallelization runs 3–5× faster on GPU (OpenCL); tile-wise (16×16 blocks + boundaries) loses ~5–10% quality at seams.
- **Atkinson:** Smaller kernel (1/8 fractions, only 3 neighbors ahead). ~40% faster than Floyd-Steinberg. Degrades near white/black. Lower feature visibility. Better for parallel tile processing (fewer dependencies).
- **Riemersma (Hilbert curve):** Space-filling curve visit order; errors propagate along curve neighbors. Naturally parallelizable (process independent curve segments). ~5–10% quality loss vs Floyd-Steinberg, but no tile artifacts. ~same speed.
**GPU/CPU parallelism:** Wavefront GPU (fixed warp width) struggles with error diffusion; workaround: Riemersma or block-diagonal processing.
**Source:** [ARM: Accelerating Floyd-Steinberg on Mali GPU](https://developer.arm.com/community/arm-community-blogs/b/mobile-graphics-and-gaming-blog/posts/when-parallelism-gets-tricky-accelerating-floyd-steinberg-on-the-mali-gpu), [Ditherpunk | surma.dev](https://surma.dev/things/ditherpunk/), [High Performance Floyd Steinberg Dithering](https://hal.science/hal-03594790v1/document).
**Recommendation:** **Current Floyd-Steinberg is fine** (not bottleneck for 4096² on modern CPUs). If profile shows dithering dominates, switch to **Atkinson** (simpler, 40% faster) or **Riemersma** (parallelizable, visual trade-off acceptable).
---
## 5. GPU / WebGL / WebGPU Fragment Shaders
**Data transfer bottleneck:** For 4096² RGBA (64 MB), upload + download dominate. Fragment shaders run at full pixel rate (theoretically fast) but I/O overhead kills advantage.
**Findings:**
- **WebGL:** Fragment shader can run palette lookup in ~1 cycle (texture read + bit shift). But uploading 4096² image to GPU = 64 MB transfer. Typical bandwidth: 1–2 GB/s (H.264 codec limit). = 30–60 ms transfer. Shader compute: ~20 ms. Not worth it unless batch-processing multiple images.
- **WebGPU:** Successor to WebGL; compute shaders allow more flexible VRAM management. Similar transfer bottleneck for single-image jobs.
- **Browser quantizers (pixi.js, glfx.js):** pixi.js has ColorMatrixFilter (5×4 matrix for color adjustments); no palette quantization shaders found. glfx.js similarly lacks quantization filters.
**Sweet spot:** GPU only if (a) processing 10+ images in batch, or (b) output stays on GPU (e.g., rendering live to canvas without readback). Single image → CPU faster.
**Source:** [MDN WebGL API](https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/Tutorial/Using_shaders_to_apply_color), [WebGPU Fundamentals](https://webgpufundamentals.org/webgpu/lessons/webgpu-from-webgl.html), [pixi.js Filters](https://pixijs.com/8.x/guides/components/filters).
**Recommendation:** **Skip GPU for single-image quantization.** If batch-importing large image sets, revisit. Current CPU path is already O(1) per pixel.
---
## 6. WebAssembly + SIMD
**Key numbers:**
- **Pure JS:** ~100–200 ns per pixel (4096² image ≈ 3–6 seconds).
- **Wasm:** ~50 ns per pixel (~1.5–2x baseline speedup due to inlining, no JS dispatch).
- **Wasm + SIMD:** ~10–15 ns per pixel (~6–15× improvement over pure JS). pngquant/squoosh reports 1.7–4.5× from SIMD alone; threading adds 1.8–2.9×.
**Libraries:** Squoosh.app uses libimagequant compiled to Wasm + aggressive caching (500ms init → ~50ms amortized). pngquant-wasm available on npm.
**Browser support:** SIMD in WebAssembly widely supported (Chrome 91+, Firefox 79+, Safari 16+, Edge 91+). Zero-copy transfer of ArrayBuffer to Wasm.
**Caveat:** Build cost. Wasm module size ~200–500 KB gzipped. Init latency ~100–500 ms (JIT compilation). Only worthwhile for batch jobs (>10 images) or large single images (>1024²).
**Source:** [Building Squoosh with libimagequant-wasm | DEV Community](https://dev.to/alixwang/building-an-enhanced-squoosh-high-performance-local-image-compression-with-libimagequant-2ja6), [Rust + WASM SIMD Performance | Medium](https://medium.com/@oemaxwell/rust-webassembly-performance-javascript-vs-wasm-bindgen-vs-raw-wasm-with-simd-687b1dc8127b).
**Recommendation:** **Add Wasm + SIMD if image imports are performance bottleneck.** Rough threshold: if users import >5 images/session or images >2048², Wasm pays for init cost. Start with profiling current JS path.
---
## 7. Typed Array Micro-opts: Uint32Array, Uint8Array Packing
**Current pattern:** `rgba[i*4], rgba[i*4+1], rgba[i*4+2]` = 3 array accesses per pixel.
**Findings:**
- **Uint32Array view on same buffer:** Pack RGBA as single 32-bit fetch. One memory access vs four. Benchmark (browser): Uint8Array actually *wins* (counterintuitive). Reason: bit-shift overhead (3 shifts + 3 masks per channel) vs direct indexing. Node.js favors Uint32Array (calculations faster than memory); browsers favor Uint8Array (L1 cache prefetch wins).
- **Typed array perf:** 20% faster I/O vs regular arrays. Pre-allocate Uint8ClampedArray; avoid reallocs.
- **Cache impact:** LUT is 32 KB = fits L1 cache (32–64 KB). RGBA buffer for 4096² = 64 MB = cold main memory. LUT lookup is cache-hot; RGBA fetch is cache-cold. Uint32 vs Uint8 difference negligible relative to LUT fetch cost.
**Source:** [Mozilla Hacks: Faster Canvas Pixel Manipulation](https://hacks.mozilla.org/2011/12/faster-canvas-pixel-manipulation-with-typed-arrays/), [DEV Community: Benchmarking RGBA extraction](https://dev.to/ku6ryo/benchmarking-rgba-extraction-from-integer-4510).
**Recommendation:** **Not a win.** Uint8Array indexing is already optimal on browsers. Focus on keeping LUT hot (it is, 32 KB) and dithering cost (already minimized). Micro-opt doesn't move the needle.
---
## 8. Web Worker Thread
**Doesn't speed up compute** but offloads main thread.
**Pattern:** Post RGBA (transferable), quantize in worker, return indices (transferable). Transfer cost: 32 MB ArrayBuffer = ~6.6 ms (zero-copy). Quantize: ~2–6 seconds. Return: ~6.6 ms. Total: ~7–13 seconds with overhead absorbed by transfer.
**Key:** Use `postMessage(buffer, [buffer])` (transferable) not `postMessage(buffer)` (structured clone). Massive difference (6.6 ms vs 302 ms for 32 MB).
**When to use:** Always, for UX. Main thread stays responsive; users see progress. Compute doesn't accelerate, but perceived responsiveness improves.
**Source:** [Chrome Blog: Transferable Objects](https://developer.chrome.com/blog/transferable-objects-lightning-fast), [MDN: Transferable Objects](https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Transferable_objects).
**Recommendation:** **Already implemented in image-uploader.js** (likely). Keep it. Transfers are negligible cost (<10 ms for 4096²).
---
## Recommendations: Top 3 Next Steps
Given you have 5-bit LUT working well:
### 1. **Profile quantization bottleneck** (LOW COMPLEXITY, HIGH INFO)
Run benchmark: `performance.now()` before/after `rgbaToPalette()` on representative images. Measure:
- LUT build time (one-time, should be <10 ms)
- Quantization time (should be <500 ms for 4096²)
- Dithering time (if enabled, should be 80% of total)
If dithering dominates, **switch to Atkinson** (change one line in `dither-kernels.js`). ~40% speedup, acceptable quality loss.
If quantization is sub-100 ms, **stop here.** Already fast enough for browser.
### 2. **Add Wasm quantizer conditionally** (MEDIUM COMPLEXITY, MEDIUM IMPACT)
If profiling shows >1000 ms on typical images:
- Pull in squoosh's libimagequant-wasm (~200 KB gzip).
- Use for batch imports (>3 images) or large singles (>2048²).
- Keep JS path as fallback.
- Estimated gain: 2–6× depending on image size.
Test on your users' typical workflows before shipping.
### 3. **Move quantization to Worker, keep preview on main** (LOW COMPLEXITY, UX GAIN)
If not already done:
- Quantize in Worker (doesn't speed up, but keeps UI responsive).
- Stream preview/progress to main thread.
- User sees feedback while waiting.
Low engineering cost, high UX win.
---
## Unresolved Questions
1. **Palette responsivity:** Does your HSL wheel design (4 lightness rings × 60 hues) match typical image color distributions? Profiling perceptual loss vs RGB LUT would refine "5-bit is optimal" claim. (Likely not critical; HSL wheel is well-balanced.)
2. **Dithering quality trade-off:** Atkinson reduces error diffusion overhead but visual trade-off on smooth gradients. User testing would validate acceptability.
3. **Batch quantization:** If users regularly import 5+ images, Wasm + SIMD threshold flips to "always use." Requires usage telemetry.
4. **Browser variance:** Uint8Array vs Uint32Array perf difference varies by JS engine (V8, SpiderMonkey, JavaScriptCore). Did not test on Safari/Firefox specifically.
---
## Sources
- [pngquant/libimagequant](https://pngquant.org/lib/)
- [ImageMagick Quantize](https://legacy.imagemagick.org/Usage/quantize/)
- [Paint.NET Quantization](https://github.com/paintdotnet/PaintDotNet.Quantization)
- [Octree Color Quantization | Cubic](https://www.cubic.org/docs/octree.htm)
- [Cris' Image Analysis Blog | k-d trees](https://www.crisluengo.net/archives/932/)
- [Oklab Color Space](https://bottosson.github.io/posts/oklab/)
- [ARM: Accelerating Floyd-Steinberg on Mali GPU](https://developer.arm.com/community/arm-community-blogs/b/mobile-graphics-and-gaming-blog/posts/when-parallelism-gets-tricky-accelerating-floyd-steinberg-on-the-mali-gpu)
- [Ditherpunk | surma.dev](https://surma.dev/things/ditherpunk/)
- [Riemersma Dithering](https://www.compuphase.com/riemer.htm)
- [Atkinson Dithering Wikipedia](https://en.wikipedia.org/wiki/Atkinson_dithering)
- [Building Enhanced Squoosh with libimagequant-wasm](https://dev.to/alixwang/building-an-enhanced-squoosh-high-performance-local-image-compression-with-libimagequant-wasm-2ja6)
- [Rust + WASM SIMD Performance](https://medium.com/@oemaxwell/rust-webassembly-performance-javascript-vs-wasm-bindgen-vs-raw-wasm-with-simd-687b1dc8127b)
- [MDN WebGL API](https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/Tutorial/Using_shaders_to_apply_color)
- [WebGPU Fundamentals](https://webgpufundamentals.org/webgpu/lessons/webgpu-from-webgl.html)
- [Mozilla Hacks: Faster Canvas Pixel Manipulation](https://hacks.mozilla.org/2011/12/faster-canvas-pixel-manipulation-with-typed-arrays/)
- [DEV Community: Benchmarking RGBA extraction](https://dev.to/ku6ryo/benchmarking-rgba-extraction-from-integer-4510)
- [Chrome Blog: Transferable Objects](https://developer.chrome.com/blog/transferable-objects-lightning-fast)
- [MDN Transferable Objects](https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Transferable_objects)
- [pixi.js Filters](https://pixijs.com/8.x/guides/components/filters)
plans/reports/researcher-260509-2246-vercel-migration-feasibility.md
-160
View File
@@ -1,160 +0,0 @@
# Research Report: Can rplace Be Moved to Vercel?
**Date:** 2026-05-09 22:46 (Asia/Saigon)
**Scope:** Feasibility of migrating rplace (Cloudflare Workers + Durable Objects + Upstash) to Vercel.
**Verdict:** **Not a drop-in move. Requires architectural rewrite of the realtime layer.**
---
## Executive Summary
rplace **cannot be lifted-and-shifted** to Vercel. Two hard blockers:
1. **Cloudflare Durable Objects have no Vercel equivalent.** rplace uses a DO (`CanvasRoom`) as the single broadcast hub for all WebSocket clients — a stateful, globally-addressable actor. Vercel does not offer this primitive.
2. **Vercel Functions cannot host WebSocket servers.** Confirmed unchanged in 2026, even with Fluid Compute. Each invocation terminates after responding; no persistent process holds sockets open.
Everything else (Svelte SPA, Vite build, Upstash Redis storage, rate limiter) is portable. The realtime broadcast is the ~20% of the code that drives ~80% of the migration cost.
**Recommended path if migration is mandatory:** Vercel hosts the SPA + HTTP API; offload WS broadcast to a managed realtime provider (Ably / Pusher / Liveblocks / Partykit). Estimated effort: medium (1–3 days), plus a new monthly bill from the realtime provider.
**Recommendation:** Stay on Cloudflare unless there is a non-technical driver (org policy, billing consolidation). The current stack is a near-optimal fit for this workload; Vercel is a strict downgrade for realtime.
---
## Methodology
- Sources: 2 web searches (Vercel WS support 2026, Vercel DO equivalent 2026)
- Code inspected: `src/worker.js`, `src/durable-objects/canvas-room.js`, `src/lib/redis-client.js`, `src/lib/canvas-storage.js`, `src/lib/rate-limiter.js`, `wrangler.json`, `package.json`, `README.md`
- Date: 2026-05-09
---
## Cloudflare Coupling Inventory
| Component | File | Cloudflare Lock-in | Portable? |
|---|---|---|---|
| Worker entry (Hono) | `src/worker.js` | Uses `c.env`, `c.executionCtx.waitUntil` | Rewrite needed |
| Durable Object | `src/durable-objects/canvas-room.js` | `state.acceptWebSocket`, Hibernation API, `WebSocketPair`, `idFromName`/`get` | **No equivalent** |
| Redis client | `src/lib/redis-client.js` | `import { Redis } from '@upstash/redis/cloudflare'` | Trivial — swap to `@upstash/redis` |
| Canvas storage | `src/lib/canvas-storage.js` | Upstash REST only | Yes |
| Rate limiter | `src/lib/rate-limiter.js` | Upstash SET NX EX | Yes |
| Static assets | `wrangler.json` `assets.directory` | CF static binding | Yes (Vercel serves SPA natively) |
| WS upgrade route | `src/worker.js` `GET /api/ws` | Delegates to DO | **No equivalent** |
| `executionCtx.waitUntil` | `src/worker.js` | CF runtime | Vercel has `waitUntil` via `@vercel/functions` |
---
## Hard Blockers
### 1. Durable Objects (the showstopper)
`CanvasRoom` is the single broadcast room. All clients connect to the **same DO instance** (`idFromName('main')`) so a `POST /api/place` on any worker can reach every connected socket via one `room.fetch('/broadcast')` call. This is the entire architectural reason DOs exist.
**Vercel has no actor / single-threaded stateful primitive.** Confirmed 2026: "No equivalent exists on Vercel for Cloudflare Durable Objects." The official Vercel migration KB recommends external state (Redis) + third-party realtime services.
### 2. WebSocket Server Hosting
`webSocketMessage` / `webSocketClose` / `webSocketError` callbacks rely on Cloudflare's **Hibernation API**, which lets sockets survive worker eviction. Vercel Functions terminate per-request — they physically cannot keep a socket open across requests, even on Fluid Compute.
---
## Migration Options (if forced)
### Option A — Hybrid: Vercel + Managed Realtime (recommended if migrating)
```
Browser ──HTTP──▶ Vercel Function (Hono or Next API route)
│
├─▶ Upstash Redis (canvas + cooldown) — unchanged
└─▶ Ably/Pusher/Liveblocks/Partykit ──▶ broadcast to clients
Browser ◀──WS────────── (managed provider connection, not Vercel)
```
- **Code changes:** Replace `broadcastPixels()` body with `await ably.channels.get('canvas').publish(...)`. Delete `canvas-room.js`. Replace WS client connection URL.
- **New cost:** ~$10–50/mo small tier (Ably/Pusher); Partykit free tier may suffice.
- **Effort:** ~1–3 days including testing.
- **Risk:** Two providers to monitor; broadcast no longer co-located with storage.
### Option B — Vercel + SSE (no managed provider)
Replace WebSocket with **Server-Sent Events** + Redis Pub/Sub. Each client opens a long-lived SSE response from a Vercel Function. The function `SUBSCRIBE`s to a Redis channel and streams pixel events.
- **Problem:** Vercel Function max duration is bounded (Fluid Compute extends but is not infinite). Long-lived SSE streams burn function-seconds — billing concern at scale.
- **Bidirectional?** SSE is server→client only. rplace currently broadcasts only, so this is fine.
- **Effort:** ~2–4 days (more plumbing than Option A).
- **Verdict:** Cheaper monthly bill, more code to own.
### Option C — Migrate to Cloudflare Pages instead
If the underlying motivation is "I want a Pages-like static + functions host," Cloudflare Pages with Functions + Durable Objects already does this and the code runs unchanged. Worth confirming the user's actual goal before assuming Vercel.
### Option D — Rejected: Vercel-only with no realtime
Not viable. Polling `GET /api/canvas` (16 MB payload) every few seconds destroys the UX and bandwidth budget. Don't.
---
## Cost / Effort Comparison
| Path | Effort | Monthly Cost Delta | Realtime Quality |
|---|---|---|---|
| Stay on Cloudflare | 0 | $0 | Excellent (current) |
| Vercel + Ably/Pusher | 1–3 days | +$10–50 | Excellent |
| Vercel + Partykit | 2–4 days | $0 (free tier) | Good (Partykit *is* DOs under the hood — ironic) |
| Vercel + SSE/Redis Pub/Sub | 2–4 days | Function-seconds at scale | Acceptable |
| Vercel polling-only | 0.5 day | Bandwidth $$$ | Unacceptable |
---
## Things That Just Work on Vercel
- Svelte 5 + Vite SPA build → Vercel serves `dist/` natively (no `vercel.json` needed for SPA).
- `@upstash/redis` (drop the `/cloudflare` subpath).
- HTTP API routes — Hono runs on Vercel Functions via `@hono/vercel`.
- `executionCtx.waitUntil` → use `import { waitUntil } from '@vercel/functions'`.
- IP-based `getUserId` — Vercel exposes client IP via `x-forwarded-for` / `x-real-ip`.
---
## Concrete Migration Steps (Option A, sketch)
1. `npm i @hono/vercel @vercel/functions ably` (or chosen provider).
2. Move `src/worker.js` → `api/[[...path]].js`, export via `@hono/vercel` adapter.
3. Replace `import { Redis } from '@upstash/redis/cloudflare'` → `'@upstash/redis'`.
4. Delete `src/durable-objects/`, `wrangler.json`, `migrations`.
5. Replace `broadcastPixels()` to publish on Ably channel `canvas`.
6. Update client `src/client/...` to subscribe via Ably SDK instead of `new WebSocket('/api/ws')`.
7. Add `vercel.json` only if SPA fallback routing needs tweaking.
8. Remove `wrangler` from devDeps; add `vercel` CLI for local preview.
9. Move `wrangler secret` env vars → Vercel project env (Upstash creds + Ably key).
10. Run integration tests; the existing testcontainers Redis tests stay valid.
---
## Recommendation
**Don't migrate** unless there is a business/ops reason. The current Cloudflare stack is the right tool — DOs solve exactly the problem (single-room WS broadcast with stateful coordination) that rplace has, in fewer moving parts than any Vercel-shaped alternative.
If migration is mandatory, **Option A (Vercel + Ably or Partykit)** is the lowest-risk path. Plan for ~3 days of work, a new vendor relationship, and minor monthly cost.
---
## Unresolved Questions
1. What is driving the migration request? (Cost? Org consolidation? Curiosity?) The right answer changes per motivation.
2. Is Cloudflare Pages (Functions + DOs, Pages-style DX) acceptable as a middle ground?
3. Acceptable monthly budget for a managed realtime provider vs. function-seconds for SSE?
4. Are there latency requirements that rule out non-edge providers?
---
## Sources
- [Vercel Functions WebSocket Support (KB)](https://vercel.com/kb/guide/do-vercel-serverless-functions-support-websocket-connections)
- [Migrate to Vercel from Cloudflare (Vercel KB)](https://vercel.com/kb/guide/migrate-to-vercel-from-cloudflare)
- [Does Vercel Support WebSockets with Fluid Compute? (Vercel Community, 2025–2026)](https://community.vercel.com/t/does-vercel-support-websockets-now-that-we-have-fluid-compute/27205)
- [WebSockets on Vercel: Why Serverless Functions Can't Host Them (Ably)](https://ably.com/topic/ai-stack/websockets-on-vercel-why-serverless-functions-cant-host-them)
- [How We Built WebSocket Servers for Vercel Functions (Rivet, 2025-10)](https://rivet.dev/blog/2025-10-20-how-we-built-websocket-servers-for-vercel-functions/)
- [Cloudflare Durable Objects Overview](https://developers.cloudflare.com/durable-objects/)
- [Cloudflare Durable Objects vs Liveblocks Broadcast 2026 (Ably)](https://ably.com/compare/cloudflare-durable-objects-vs-liveblocks-broadcast)
- [Cloudflare Workers vs Vercel 2026 (Morph)](https://www.morphllm.com/comparisons/cloudflare-workers-vs-vercel)
plans/reports/researcher-260509-2255-forever-free-hosting.md
-152
View File
@@ -1,152 +0,0 @@
# Research Report: Best Forever-Free Hosting for rplace
**Date:** 2026-05-09 22:55 (Asia/Saigon)
**Scope:** Find the best **truly always-free** (not trial, not credits) cloud hosting for rplace's stack: WS broadcast hub + HTTP API + Redis-like KV + static SPA.
**Verdict:** **Stay on Cloudflare.** It is *the* forever-free fit for this workload. Only realistic alternative is Oracle Cloud Always Free + self-host, with operational cost.
---
## Use Case Constraints (rplace specific)
| Need | Numbers |
|---|---|
| Static SPA | ~1 MB Svelte build |
| HTTP API (Hono) | low QPS, hobby-scale |
| WebSocket broadcast | one global room, all clients fan-out from one place |
| Storage | 16 MB canvas + per-user cooldown TTL keys |
| Egress | up to ~5 MB gzipped per `/api/canvas` (cached 10s) |
| Stateful coordinator | required (broadcast hub) |
---
## Free-Tier Reality Check (May 2026)
| Platform | Forever-Free? | WebSocket Server? | Stateful Actor? | Verdict for rplace |
|---|---|---|---|---|
| **Cloudflare Workers + DO** | ✅ Yes | ✅ Yes (Hibernation API) | ✅ Yes (DO) | **Best fit, current** |
| **Oracle Cloud Always Free** | ✅ Yes | ✅ Yes (real VM) | ✅ Yes (any) | Viable backup; ops cost |
| **Google Cloud Run** | ✅ Yes (180K vCPU-s/mo) | ⚠️ Limited (no long-lived WS as a server, max 60min request) | ❌ | Marginal; cold starts |
| **Vercel Hobby** | ✅ (limits) | ❌ | ❌ | Not viable, see prev report |
| **Netlify Free** | ✅ (limits) | ❌ | ❌ | Not viable, see prev report |
| **Render Free** | ⚠️ 750 hr/mo + auto-sleep | ✅ (when awake) | ❌ | 30–50s cold start kills WS UX |
| **Koyeb Free** | ✅ Yes (1 service) | ✅ | ❌ | Decent backup, single instance limit |
| **Fly.io** | ❌ Removed for new signups in 2026 | — | — | **Out** |
| **Railway** | ❌ Trial credit only ($5/mo) | — | — | **Out** |
| **Heroku** | ❌ Killed free tier 2022 | — | — | **Out** |
| **AWS Free Tier** | ❌ 12 months only | — | — | **Out** |
---
## Why Cloudflare Wins (Numbers)
### Workers Free (always-free)
- **100,000 requests / day** — at 1 req/sec rate-limit, that's 100K user actions/day before hitting the cap. Plenty for hobby.
- **10 ms CPU / request** — broadcasts and BITFIELD ops finish in <1ms.
- **Static assets**: free, unlimited bandwidth.
### Durable Objects Free (since 2024)
- **5 GB SQLite storage** (we use 0 — state is in Upstash).
- **WebSocket Hibernation = idle sockets cost $0 CPU.** Critical: a connected-but-idle client doesn't burn the request quota.
- Available on Workers Free plan with SQLite backend (the only DO option currently used by rplace).
### Upstash Redis Free (always-free)
- **500K commands / month** (~16K/day). Bumped from 10K/day in March 2025.
- **256 MB storage** — canvas is 16 MB, fits 16× over.
- ⚠️ **Possible squeeze:** `/api/canvas` uses 4 GETRANGE = 4 commands per fetch. If the 10s cache-control isn't honored by clients, traffic spikes can chew through 500K/mo. Already mitigated by `Cache-Control: max-age=10, s-maxage=10`.
### Total monthly cost: $0. Forever.
---
## The One Real Alternative: Oracle Cloud Always Free
If you need a non-Cloudflare backup, **Oracle Cloud Always Free** is the only platform offering a *real* VM forever-free that can host a WS server.
| Resource | Limit |
|---|---|
| ARM Ampere A1 | 4 OCPU + 24 GB RAM (split across up to 4 VMs) |
| Block storage | 200 GB |
| Egress | 10 TB/month outbound |
| AMD x86 VM | 2× shape with 1/8 OCPU + 1 GB RAM (small) |
### Pros
- True root access. Run any WS server (Bun, Node, Go, Rust).
- Generous resources — overkill for rplace.
- Forever, not trial.
### Cons (brutal)
- **You become the sysadmin.** Patches, monitoring, TLS, restarts — all yours.
- **Idle reaping**: <10% CPU + <10% network for 7 days → Oracle stops the VM. rplace is bursty hobby traffic, this is real risk. Mitigation: a cron `dd if=/dev/urandom` every 6h or a small load-gen.
- **Single region** — no edge. Latency for users far from your chosen region (vs Cloudflare's ~330 PoPs).
- **Capacity issues** — A1 instances are notoriously hard to provision in popular regions ("Out of Capacity" loops). Plan for retries.
- **Vendor risk** — Oracle has historically been quick to terminate "abusive" free accounts.
### When to choose
Only if Cloudflare becomes unavailable to you (account ban, geographic restriction, org policy). Otherwise the operational debt is not worth it.
---
## Stack Recommendation (Forever-Free)
### Primary (current, optimal)
```
Cloudflare Workers (Hono)
├── Durable Object (CanvasRoom) — WS broadcast hub
└── Upstash Redis Free — canvas BITFIELD + cooldown
```
### Backup (if forced off Cloudflare)
```
Oracle Cloud A1 VM (single instance, 1 OCPU, 6 GB RAM)
├── Caddy (TLS + static SPA)
├── Bun + Hono server (HTTP API + native ws)
└── Upstash Redis Free (or local valkey-server, free)
```
A1 backup loses: edge latency, zero-config TLS, automatic scaling, hibernation-cheap idle WS.
A1 backup gains: full control, no platform-specific lock-in, no vendor-shaped architecture.
---
## What Changed in 2025–2026 (worth knowing)
- **Fly.io removed free tier for new signups** (legacy accounts grandfathered).
- **Railway moved to $5/mo trial credit** model — no longer "always free."
- **Cloudflare DOs now free on Workers Free plan** (SQLite backend), making the rplace stack 100% free where it used to require paid Workers.
- **Upstash bumped Redis free tier** from 10K/day to 500K/month commands.
- **Oracle Cloud expanded A1 outbound** to 10 TB/month.
Net effect: Cloudflare's free-tier moat got **wider**, not narrower.
---
## Recommendation
**Do nothing.** The current Cloudflare Workers + DO + Upstash stack is the unambiguous winner for rplace's exact shape of workload at $0/month forever. Any move is a downgrade in capability or an upgrade in operational burden.
If you specifically want a backup plan documented, set up an **Oracle Cloud A1 VM** in your closest region as a cold-standby. Don't migrate; just keep it provisioned in case of CF account loss.
---
## Unresolved Questions
1. Why is migration on the table? (Cost = $0 already; capability = best-in-class.) The motivation matters more than the answer.
2. Geographic constraints? (Cloudflare is restricted in certain countries/orgs.)
3. Risk tolerance for vendor lock-in vs. operational burden? (CF = locked-in but free; Oracle = portable but ops-heavy.)
4. Is Upstash 500K cmd/mo enough at projected traffic? Worth measuring current `/api/canvas` and `/api/place` rates over a week.
---
## Sources
- [Cloudflare Workers Pricing](https://developers.cloudflare.com/workers/platform/pricing/)
- [Cloudflare Durable Objects Pricing](https://developers.cloudflare.com/durable-objects/platform/pricing/)
- [Which Cloudflare Services Are Free? 2025 Free Tier Guide (DEV)](https://dev.to/ioniacob/which-cloudflare-services-are-free-2025-free-tier-guide-53jl)
- [Oracle Cloud Free Tier (Official)](https://www.oracle.com/cloud/free/)
- [Oracle Cloud Always Free VPS 2026 Real Limits](https://space-node.net/blog/oracle-vps-free-tier-review-2026)
- [Setup Always Free VPS 4 OCPU 24GB RAM Oracle Guide 2026 (Medium)](https://medium.com/@imvinojanv/setup-always-free-vps-with-4-ocpu-24gb-ram-and-200gb-storage-the-ultimate-oracle-cloud-guide-bed5cbf73d34)
- [Upstash Redis Pricing & Limits](https://upstash.com/docs/redis/overall/pricing)
- [Upstash New Pricing Higher Limits (March 2025)](https://upstash.com/blog/redis-new-pricing)
- [Platforms with a Real Free Tier 2026 (Render Blog)](https://render.com/articles/platforms-with-a-real-free-tier-for-developers-in-2026)
- [Free Cloud Deployment Platforms 2026 (SnapDeploy)](https://snapdeploy.dev/blog/free-cloud-deployment-platforms-2026-comparison)
- [Best Always-Free Tier Cloud Platforms (GitHub gist)](https://gist.github.com/hashirahmad/8df502f8d9e3b01f7998c55c22447c4f)
plans/reports/tester-260417-0927-test-suite.md
-353
View File
@@ -1,353 +0,0 @@
# Test Suite Review — rplace
**Date:** 2026-04-17
**Platform:** Windows 11, Node v18+, Docker unavailable
**Test Runner:** Vitest 4.1.4
---
## Executive Summary
Vitest config and 7 unit test files (total 779 LOC) run cleanly. **All 71 tests pass.** 1 integration test file (234 LOC, 11 tests) skipped due to Docker unavailability on Windows CI. No coverage instrumentation configured; no frontend tests exist. Test quality is mixed: encoding edge cases and rate-limit Lua are well-covered; critical findings from code review (retryAfter unit bug, DO hibernation, IP hash collisions, body size limits) have partial or zero test coverage.
---
## Test Execution Results
### Unit Tests (vitest.config.js)
| Config | Total | Passed | Failed | Skipped | Duration |
|--------|-------|--------|--------|---------|----------|
| **Unit** | 71 | 71 | 0 | 0 | 1.23s |
**Test Files (7):**
1. `test/lib/canvas-decoder.test.js` — 15 tests (PASS)
2. `test/lib/canvas-storage.test.js` — 10 tests (PASS)
3. `test/lib/get-user-id.test.js` — 5 tests (PASS)
4. `test/lib/pixel-buffer.test.js` — 27 tests (PASS)
5. `test/lib/redis-client.test.js` — 7 tests (PASS)
6. `test/durable-objects/canvas-room.test.js` — 8 tests (PASS)
7. `test/worker-validation.test.js` — 21 tests (PASS)
**Status:** ✅ All unit tests green.
---
### Integration Tests (vitest.integration.config.js)
| Config | Total | Passed | Failed | Skipped | Error |
|--------|-------|--------|--------|---------|-------|
| **Integration** | 11 | 0 | 0 | 11 | Docker runtime unavailable |
**Test File (1):**
- `test/integration/redis-canvas-roundtrip.test.js` — 11 tests (SKIPPED)
**Failure Details:**
```
Error: Could not find a working container runtime strategy
at getContainerRuntimeClient (node_modules/testcontainers/build/container-runtime/clients/client.js:67)
at GenericContainer.start (node_modules/testcontainers/build/generic-container/generic-container.js:62)
```
**Root Cause:** Testcontainers requires Docker/Podman. Windows CI environment has neither. Expected on CI without Docker daemon.
**Status:** ⏭️ Skipped (infrastructure limitation, not test failure).
---
## Test Quality Assessment
### Strong Coverage Areas
#### 1. BITFIELD Encoding/Decoding (canvas-decoder.test.js)
- ✅ Empty buffer → all zeros
- ✅ Single pixel decode at offset 0
- ✅ Round-trip all 32 color values (0–31)
- ✅ Repeated color patterns over 100 pixels
- ✅ Max color value (31) at various byte offsets
- ✅ RGBA output consistency with COLORS_RGBA lookup
- ✅ Type checks (Uint8ClampedArray)
- ✅ COLORS ↔ COLORS_RGBA consistency (hex→RGB unpacking)
**Assessment:** Excellent. Encoder/decoder bit math is thoroughly tested across boundary conditions (byte offsets, max values, patterns). Code review flagged BITFIELD overflow (M3) as "already guarded by validation" — tests confirm round-trip works but do not test overflow directly (color >= 32 inputs). However, validation in worker.js is tested separately.
#### 2. Canvas Storage (canvas-storage.test.js)
- ✅ Empty array → no Redis call
- ✅ Single pixel BITFIELD command construction
- ✅ Multiple pixels batched in one command
- ✅ Offset arithmetic for boundary pixels (0,0) and (2047,2047)
- ✅ Base64 decode from Redis response
- ✅ Padding short responses to CANVAS_BYTES
- ✅ All byte values (0x00–0xFF) round-trip without corruption
- ✅ Null/empty string handling
**Assessment:** Good. Mocks redis-client; does not hit real Redis. The "all bytes 0–255" test is specifically designed to catch the binary encoding bug mentioned in code review (L1: atob edge case). Base64 round-trip is tested but getFullCanvas payload validation (L1: detecting if response is already binary vs base64) is not.
#### 3. Rate-Limit Lua (redis-canvas-roundtrip.test.js)
- ✅ New user gets full MAX_CREDITS
- ✅ Credit deduction is correct
- ✅ Credit regeneration over time
- ✅ Rejection when insufficient credits
- ✅ Cap at MAX_CREDITS
- ✅ Credit deficit calculation
- ✅ Multiple rapid calls serialize correctly
**Assessment:** Excellent. Lua script atomicity is verified. Tests cover the happy path and edge cases (full spend, regen, cap). **However:** Code review flagged two **Critical** bugs:
- **C1:** `retryAfter` is returned as credit deficit, not seconds. Test at fixed REGEN_RATE=1 does not catch this (deficit=credits by coincidence). **No test with REGEN_RATE ≠ 1.**
- **C2:** Fractional regen (e.g. REGEN_RATE=0.5) is broken due to `math.floor(elapsed * rate)` truncation. **No test with fractional rate.**
#### 4. Worker Validation (worker-validation.test.js)
- ✅ Invalid JSON rejection
- ✅ Missing/empty/non-array pixels rejection
- ✅ Batch size limits (MAX_BATCH_SIZE)
- ✅ Coordinate bounds (x, y in [0, CANVAS_WIDTH/HEIGHT))
- ✅ Negative coordinates rejection
- ✅ Color range validation (0–31)
- ✅ Negative color rejection
- ✅ Non-integer coordinate rejection
- ✅ Non-integer color rejection
- ✅ String coordinate rejection (type coercion blocked)
- ✅ Rate-limit enforcement (429 response)
- ✅ Success case with valid pixel
- ✅ Boundary pixel values (CANVAS_WIDTH-1, CANVAS_HEIGHT-1, MAX_COLORS-1)
**Assessment:** Excellent. Input validation is comprehensive (integer, type, range checks). **However:** Code review flagged:
- **L4:** Body size not capped — test uses default small payloads. Code review recommends rejecting bodies > 4KB before parsing. **No test that sends huge JSON.**
- **M2:** No deduplication in batch — user can send same coordinate 32×. **No test for duplicate-pixel-in-batch detection.**
#### 5. Pixel Buffer (pixel-buffer.test.js)
- ✅ Initial state (empty, no undo/redo)
- ✅ Add stroke increments count
- ✅ Empty stroke ignored
- ✅ Redo cleared on new stroke
- ✅ Input array not held by reference (copied)
- ✅ Undo/redo returns stroke
- ✅ Undo/redo on empty returns null
- ✅ Multiple undo/redo cycles
- ✅ Deduplication: last stroke wins for same coordinate
- ✅ Pixel merge across strokes
- ✅ getColorAt returns -1 for non-pending
- ✅ getColorAt returns latest color
- ✅ getColorAt post-undo behavior
- ✅ getAffectedKeys uniqueness
- ✅ pixelCount with duplicates
- ✅ clear() resets all state
**Assessment:** Excellent. Client-side undo/redo buffer is thoroughly tested (state machine, edge cases, dedup).
#### 6. get-user-id (get-user-id.test.js)
- ✅ anon: prefix present
- ✅ Deterministic for same IP
- ✅ Different IPs → different IDs
- ✅ Missing header fallback to 127.0.0.1
**Assessment:** Minimal but correct. **Critical gap:** Code review flagged **H1** — IP hash collisions at ~65k unique IPs share rate-limit buckets. Test uses 3 hardcoded IPs and assumes no collisions exist. **No collision detection test.** Also **H2** — dev fallback to 127.0.0.1 masks misconfiguration. Fallback is tested but not the consequence (multiple users sharing one bucket).
#### 7. redis-client (redis-client.test.js)
- ✅ POST request with JSON body and auth header
- ✅ Non-ok response (401) throws
- ✅ redisRawBinary uses path-based URL with Upstash-Encoding header
- ✅ URL-encodes special characters (`:` → `%3A`)
- ✅ Returns base64-encoded result string
- ✅ Non-ok response (500) throws
**Assessment:** Good. HTTP request shape and auth are correct. Mocks fetch; does not hit real Upstash. **Gap:** Code review noted (L1) that `atob` may silently decode non-base64 strings that happen to be valid alphabet. The canvas-storage test covers round-trip but not the edge case where input is not base64 at all.
#### 8. CanvasRoom (canvas-room.test.js)
- ✅ Broadcast sends to all connected WebSockets
- ✅ Closes WebSocket on send failure
- ✅ Broadcasts to empty room without error
- ✅ webSocketClose calls ws.close() with code/reason
- ✅ webSocketError closes with error code
- ✅ webSocketMessage ignores (no-op)
**Assessment:** Basic. Covers happy path and error cases. **Critical gap:** Code review flagged **H3** — DO uses `server.accept()` (non-hibernation API), not hibernation API. Tests use mocks that do not exercise hibernation semantics. **No test that verifies hibernation API behavior** (state restoration, event-driven awakening). Tests only cover the handler methods, not the DO lifecycle.
---
### Coverage Gaps vs. Code Review Findings
| Review Finding | Severity | Test Coverage | Gap |
|---|---|---|---|
| **C1: retryAfter unit drift** | Critical | Partial | No test with REGEN_RATE ≠ 1 |
| **C2: Fractional regen broken** | Critical | None | No test with REGEN_RATE < 1 |
| **H1: IP hash collisions** | High | None | No collision test; no bucket-sharing demo |
| **H2: Dev fallback to 127.0.0.1** | High | Tested fallback, not impact | Tests fallback exists; doesn't verify bucket sharing |
| **H3: Non-hibernation DO** | High | None | No hibernation API test |
| **H4: Canvas GET unbounded egress** | High | N/A (perf, not logic) | Not a unit test concern |
| **H5: Broadcast blocks /api/place** | High | Partial | Mock doesn't measure latency; no waitUntil test |
| **L4: Body size not capped** | Low | None | No test sending 100MB JSON |
| **M2: No batch dedup** | Medium | None | No test with duplicate coordinates |
| **M3: BITFIELD color overflow** | Medium | Partial | Validation guarded; no overflow test in storage |
| **M5: Broadcast validates payload** | Medium | None | No test sending invalid pixel shape to `/broadcast` |
---
## Frontend Tests
**Status:** ❌ **No tests exist.**
Files reviewed:
- `src/index.html`
- `src/client/App.svelte`
- `src/client/components/CanvasRenderer.svelte`
- `src/client/components/ColorPicker.svelte`
- `src/client/components/UserInfo.svelte`
**Gaps flagged by code review:**
- **M3:** `applyUpdates` does not validate WS payload bounds/types. **No unit test.**
- **M1:** `onclose` reconnects but never refetches canvas. **No integration test.**
- **M3:** Optimistic UI never rolls back on 429. **No rejection test.**
---
## Coverage Instrumentation
**Status:** ❌ **Not configured.**
No coverage tool (c8, nyc, vitest coverage) is set up. All 71 tests run but coverage % is unknown. Recommended:
```json
{
"test": {
"coverage": {
"enabled": true,
"provider": "v8",
"reporter": ["text", "json", "html"],
"lines": 80,
"functions": 80,
"branches": 75
}
}
}
```
---
## CI/CD Integration
**Package.json scripts present:**
```json
"test": "vitest run --config vitest.config.js",
"test:integration": "vitest run --config vitest.integration.config.js",
"test:all": "vitest run --config vitest.config.js && vitest run --config vitest.integration.config.js",
"test:watch": "vitest --config vitest.config.js"
```
**Status:** ✅ Scripts are wired up. No CI config file found (no `.github/workflows/test.yml` or `.gitlab-ci.yml`), but commands are ready for CI to invoke.
---
## Recommended Test Additions (Priority Order)
### Tier 1: Critical bugs from code review
1. **Rate-limit unit test with REGEN_RATE ≠ 1**
```js
it('correctly calculates retryAfter in seconds (not credits)', async () => {
// REGEN_RATE = 0.5 → 10 seconds elapsed = 5 credits regen
// Check that deficit is divided by rate to convert back to seconds
});
```
2. **Fractional regen rate test**
```js
it('handles fractional regen rate (0.5 credits/sec)', async () => {
// Verify that elapsed * 0.5 doesn't truncate; accrued is computed correctly
});
```
3. **Body size limit test**
```js
it('rejects request body > 4KB', async () => {
const huge = { pixels: Array(50000).fill({ x: 0, y: 0, color: 0 }) };
const res = await app.fetch(postPlace(huge), env);
expect(res.status).toBe(400); // or 413
});
```
### Tier 2: Medium/High gaps
4. **Duplicate pixel deduplication in batch**
```js
it('rejects or dedupes duplicate (x,y) in batch', async () => {
const pixels = [
{ x: 0, y: 0, color: 1 },
{ x: 0, y: 0, color: 2 }, // same coord, different color
];
// Expect only last to be used, or rejection
});
```
5. **Hibernation API lifecycle test** (requires real or stubbed DO runtime)
```js
it('restores WS sessions on DO wake via state.getWebSockets()', async () => {
// This requires a Durable Object test harness from CF
});
```
6. **WebSocket payload validation on `/broadcast`**
```js
it('rejects broadcast with invalid pixel shape', async () => {
const req = new Request('http://internal/broadcast', {
method: 'POST',
body: JSON.stringify([{ x: 'invalid', y: 0, color: 0 }]),
});
const res = await room.fetch(req);
expect(res.status).toBe(400);
});
```
### Tier 3: Frontend tests (new suite needed)
7. **CanvasRenderer.svelte: WS payload validation**
8. **CanvasRenderer.svelte: Optimistic UI rollback on 429**
9. **App.svelte: Canvas refetch on WS reconnect**
---
## Performance Notes
- Unit test suite runs in **1.23 seconds** (fast, good).
- No performance benchmarks run (pixel placement latency, canvas decode speed, Lua script execution time not measured).
- Recommend adding a "perf" test suite that measures:
- Decode 2.6MB canvas cold (should be < 100ms)
- BITFIELD command generation for 32 pixels (should be < 5ms)
- Lua script execution (should be < 50ms on Upstash)
---
## Docker Availability Issue
**Environment:** Windows 11 (Git Bash, no WSL2 Docker)
**Fix for CI:** Docker Desktop or Podman required to run integration tests. Alternative: skip integration tests on Windows CI, run only on Linux CI.
Test file `test/integration/redis-canvas-roundtrip.test.js` is well-written (11 concrete tests on real Redis behavior) and will pass on Docker-capable CI. The tests are not flaky — Docker unavailability is an environmental issue, not a test issue.
---
## Summary of Test Status
| Category | Result | Notes |
|---|---|---|
| **Unit Tests** | ✅ 71/71 PASS | All backends covered; some critical bugs untested |
| **Integration Tests** | ⏭️ 11/11 SKIPPED | Docker unavailable; tests are sound |
| **Frontend Tests** | ❌ 0 TESTS | No Svelte/client tests exist |
| **Coverage Config** | ❌ NOT SET UP | Add c8/v8 provider |
| **CI Scripts** | ✅ WIRED | test, test:integration, test:all scripts ready |
| **Performance Tests** | ❌ NONE | No benchmark suite |
---
## Unresolved Questions
1. **Is integration test skipping acceptable for Windows CI?** Should test:integration be run only on Linux runners, or is mocking Redis sufficient?
2. **Will HiveSystems DO hibernation API tests require CF's test harness?** Current mocks cannot verify DO event-driven wakeup.
3. **Should frontend tests use Vitest + jsdom, or a different framework** (Playwright, JSDOM + component harness)?
4. **Is coverage instrumentation (c8) desired as a gating check** (fail on <80% lines), or informational only?
---
**Status:** DONE_WITH_CONCERNS
**Summary:** 71 unit tests all pass cleanly on 7 files. Integration tests (11 tests) skipped due to Docker unavailability—expected on Windows CI. Test quality is uneven: encoding/decoding, rate-limit Lua, and input validation are well-covered; critical bugs from code review (retryAfter unit bug C1, fractional regen C2, IP collisions H1, body size limit L4, batch deduplication M2, DO hibernation H3) have partial or zero test coverage. Frontend has no tests. No coverage instrumentation configured.
**Concerns:** (a) C1 and C2 (rate-limit unit bugs) are not tested at non-default REGEN_RATE values—will silently fail in production if rate tuning occurs; (b) integration test infrastructure requires Docker for CI; (c) frontend is untested.