Conversation
add alot of stuff
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update standardizes database and code naming conventions from camelCase to snake_case, adds new utility modules for batching and websocket synchronization, and removes obsolete migration and default cell data files. It also refactors change notification logic for direct polling, updates UI components to match schema changes, and introduces new dependencies and websocket setup. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant DB
participant WebSocket
participant Server
UI->>DB: Perform data operations (insert/update)
UI->>DB: Subscribe to change logs
UI->>WebSocket: Call setupWebsocket(db)
WebSocket->>Server: Open ws://localhost:3000/ws
WebSocket->>DB: getLatestId()
WebSocket->>Server: Send { type: "ping", cursor }
Server-->>WebSocket: Receive { type: "pong" | "request_data" }
WebSocket->>DB: getLatestId() (on message)
WebSocket->>Server: Send { type: "data", cursor, payload }
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (10)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
src/components/GameOfLifeCanvas.tsx (1)
60-76: 🛠️ Refactor suggestion1 ms loop will peg the CPU
setTimeout(runLoop, 1)schedules ±1000 fps which the main thread cannot meet; the browser will continuously thrash, and the DB write volume explodes.- timeoutId = setTimeout(runLoop, 1) + // ~60 fps; switch to requestAnimationFrame for smoother timing if desired + timeoutId = setTimeout(runLoop, 16)Consider
requestAnimationFrameor a throttled interval based on grid size to balance UX and battery.
🧹 Nitpick comments (13)
src/components/Logo.tsx (1)
3-6: Rename propclass→classNamefor clarity and TS intellisense
Using the reserved wordclassas a prop works at runtime, but editors and tooling often autocompleteclassName. Aligning with the broader JSX ecosystem avoids friction and prevents accidental shadowing when destructuring:-interface LogoProps { - size?: 'sm' | 'md' | 'lg' | 'xl' - class?: string -} +interface LogoProps { + size?: 'sm' | 'md' | 'lg' | 'xl' + className?: string +} ... -<div class={`inline-flex items-center ${props.class || ''}`}> +<div class={`inline-flex items-center ${props.className || ''}`}>src/utils/createBatchFlusher.tsx (2)
42-48: Expose async status or return promise for better composability
The returned flusher currently hasvoidreturn type, giving callers no signal when a batch is actually sent/failed. Consider returning the internalflush()promise (or a boolean) so upstream code can await or log back-pressure.
51-65: Suppress swallowed errors inchainify
chainifylogs errors but resolves the chain, potentially hiding failures from callers. Bubble the rejection (or allow opt-in suppression) so error handling remains explicit:-last = result.catch(e => console.log('error during func chain', e)) +last = result.catch(e => { + console.error('error during func chain', e) + throw e +})package.json (1)
31-33: Move build-time tools to devDependencies
madgeis only used for static analysis graphs and never shipped to clients. Keeping it underdependenciesincreases bundle-install size for production deploys.- "madge": "^8.0.0", + }, + "devDependencies": { + "madge": "^8.0.0",(merge with existing
devDependenciesblock)index.html (1)
5-9: Nit: encode the in-line SVG faviconThe un-escaped
<,>, and spaces in the data-URI technically violate the spec and can break in some browsers. Encode the SVG or wrap it inencodeURIComponent()to avoid parsing issues.src/sync/cursor.ts (1)
5-12: Add explicit return type & drop.tssuffixA small polish:
-import * as schema from '../sqlite/schema.ts' +import * as schema from '../sqlite/schema' -export const getLatestId = async (db: SqliteRemoteDatabase<typeof schema>) => { +export const getLatestId = async ( + db: SqliteRemoteDatabase<typeof schema> +): Promise<number> => {• The
.tssuffix is unnecessary with TS path resolution and causes ESM complaints in some bundlers.
• DeclaringPromise<number>makes the public contract clear.src/components/GameOfLifeCanvas.tsx (1)
218-226: Minor: duplicated button classesBoth buttons share identical
bg-gray-700 hover:bg-gray-600 text-white. Extract to a shared class or reuse the existingButtonvariant API to stay DRY.src/routes/Home.tsx (1)
34-48: Large block of commented-out sync code – commit hygieneLeaving ~15 lines of commented code (plus the now-unused imports they require) bloats the bundle and risks merge conflicts. Either:
- gate the experimental websocket flow behind a feature flag and keep it live, or
- remove the dead code and re-introduce it in a dedicated PR.
This also eliminates the presently unused imports (
on,Logo,ky,setupWebsocket, etc.).src/sync/socket.ts (1)
5-29: Return the WebSocket & expose a disposer for callers
setupWebsocketattaches listeners but offers no way for the caller to close or dispose of the socket. That makes unmounting / hot-reloads leak connections.-export async function setupWebsocket(db: …) { +export async function setupWebsocket(db: …): Promise<{ ws: WebSocket; close(): void }> { const ws = new WebSocket('ws://localhost:3000/ws') … - }) + }) + + // give the caller control + const close = () => { + ws.removeEventListener('open', openHandler) + ws.removeEventListener('message', messageHandler) + ws.close() + } + return { ws, close } }Exposing the handle lets route components call
close()from theironCleanup.src/sqlite/index.ts (2)
15-15: Consider making the debug flag configurable.Hardcoding
shouldLogtofalsemakes it difficult to enable logging for debugging in different environments. Consider making this configurable through environment variables or a configuration object.-const shouldLog = false +const shouldLog = globalThis.DEBUG_SQLITE ?? falseOr alternatively:
-const shouldLog = false +const shouldLog = process.env.NODE_ENV === 'development'
277-299: Verify performance impact of the simplified notification system.The removal of the buffered notification system in favor of direct polling could impact performance, especially under high load. The current implementation calls
notifyBatched()after each operation which may cause more frequent database queries.Consider monitoring the performance impact of this change in production environments.
You might want to implement a configurable batching interval if performance becomes an issue:
// Example batching with configurable interval const BATCH_INTERVAL = 100; // ms let batchTimer: number | null = null; const scheduleBatchNotification = () => { if (batchTimer) return; batchTimer = setTimeout(() => { notifyBatched(); batchTimer = null; }, BATCH_INTERVAL); };src/sqlite/schema.ts (1)
51-61: Well-structured cells table implementation.The
cellstable is properly designed with:
- Composite primary key on (x, y) coordinates
- Boolean
alivefield with proper default- Correct type exports
Consider adding an index on the
alivecolumn if you'll be frequently querying for live/dead cells.drizzle/meta/0000_snapshot.json (1)
24-30: Use numeric default for integer column.The
alivecolumn is declared asintegerbut usesfalseas its default, which may lead to ambiguity. It's clearer to use0instead."alive": { "name": "alive", "type": "integer", "primaryKey": false, "notNull": true, "autoincrement": false, - "default": false + "default": 0 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.locklocal.dbis excluded by!**/*.db
📒 Files selected for processing (22)
drizzle/meta/0000_snapshot.json(1 hunks)drizzle/meta/_journal.json(1 hunks)index.html(1 hunks)package.json(1 hunks)src/components/AppIcon.tsx(1 hunks)src/components/ChangeLog.tsx(0 hunks)src/components/GameOfLifeCanvas.tsx(3 hunks)src/components/Logo.tsx(1 hunks)src/components/Users/AddUser.tsx(0 hunks)src/components/Users/UserTable.tsx(0 hunks)src/consts/defaultCells.ts(0 hunks)src/consts/demoData.ts(1 hunks)src/consts/migrations-data.ts(0 hunks)src/hooks/useChangelog.tsx(2 hunks)src/hooks/useLiveUsers.ts(2 hunks)src/routes/Home.tsx(1 hunks)src/sqlite/index.ts(4 hunks)src/sqlite/schema.ts(4 hunks)src/sqlite/triggers.ts(1 hunks)src/sync/cursor.ts(1 hunks)src/sync/socket.ts(1 hunks)src/utils/createBatchFlusher.tsx(1 hunks)
💤 Files with no reviewable changes (5)
- src/components/Users/UserTable.tsx
- src/components/ChangeLog.tsx
- src/components/Users/AddUser.tsx
- src/consts/migrations-data.ts
- src/consts/defaultCells.ts
🧰 Additional context used
🧠 Learnings (1)
src/sync/cursor.ts (1)
Learnt from: ShaulLavo
PR: ShaulLavo/sqlite-sync#1
File: src/sqlite/index.ts:231-231
Timestamp: 2025-06-02T14:37:02.162Z
Learning: In the SQLite change tracking system in src/sqlite/index.ts, the logCursor should track the highest ID seen, not the count of records returned. The getChangeLogsAtCursor function uses WHERE id > lastSeenLogId to return only new records since the cursor, so rows.length represents the count of new records, not the highest ID value.
🧬 Code Graph Analysis (1)
src/hooks/useChangelog.tsx (2)
src/sqlite/schema.ts (1)
ChangeLog(91-91)src/sqlite/client-wasm.ts (2)
batch(195-219)batch(309-314)
🔇 Additional comments (6)
src/hooks/useLiveUsers.ts (1)
14-15: Subscription cleanup captured correctly – nice!Storing the
unsubscribecallback in a local variable eliminates the dangling-listener leak we previously had.src/hooks/useChangelog.tsx (1)
13-14: Lifecycle leak fixed – good alignment with Solid patternsIntroducing
unsubscribeplusonCleanupbrings the hook in line with Solid’s expected resource-management flow. Thanks for tightening this up.Also applies to: 38-40
src/sqlite/schema.ts (2)
40-49: LGTM! Relations properly defined.The relations are correctly defined with proper foreign key references using the updated snake_case column names.
76-84: Excellent type safety improvement.Adding the
OPERATIONSenum and using it for theop_typecolumn improves type safety and prevents invalid operation types from being inserted.drizzle/meta/0000_snapshot.json (1)
4-4:Details
❓ Verification inconclusive
Ensure snapshot id aligns with schema migration.
Confirm that the updated
"id"value matches the new migration snapshot generated by yoursrc/sqlite/schema.tschanges and that there are no lingering references to the old ID insrc/consts/migrations-data.ts.
🏁 Script executed:
#!/bin/bash # Verify no stale snapshot IDs in migration-data.ts rg "10c6b36e-5eb7-4ecf-ad47-a0fe883a8ade" -n src/consts/migrations-data.tsLength of output: 144
It looks like
src/consts/migrations-data.tsisn’t present at the expected path. Let’s first locate where (or if) that file exists in the repo:
🏁 Script executed:
#!/bin/bash # Locate migrations-data.ts in the repository fd migrations-data.tsLength of output: 23
Let’s search the entire repo for the old snapshot ID and also list any migration-related files to confirm the correct path:
🏁 Script executed:
#!/bin/bash # 1) Check for any lingering references to the old snapshot ID rg "10c6b36e-5eb7-4ecf-ad47-a0fe883a8ade" -n . # 2) List all files with “migration” in their name fd migrationLength of output: 142
Let’s verify that the new snapshot ID is actually referenced in your migration files:
🏁 Script executed:
#!/bin/bash # Search for the updated snapshot ID in all migration-related files rg "117096b9-3e0e-4184-8374-9ade719ba54a" -n src/consts/migrations.ts src/sqlite/migrations.ts scripts/generate-migrations.tsLength of output: 125
Ensure snapshot ID is correctly wired into your migrations
I scanned for the old ID (
10c6b36e-5eb7-4ecf-ad47-a0fe883a8ade) and the new one (117096b9-3e0e-4184-8374-9ade719ba54a) across your migration files and found no matches in:
- src/consts/migrations.ts
- src/sqlite/migrations.ts
- scripts/generate-migrations.ts
Please manually verify that:
- Your migrations-data (now in
src/consts/migrations.ts) includes the updated snapshot ID.- There are no lingering references to the old ID elsewhere.
drizzle/meta/_journal.json (1)
8-9:Details
✅ Verification successful
Verify journal entry versioning consistency.
You bumped the journal
"version"to7but kept the entry"version"at"6". Ensure this is intentional (entry version typically maps to the associated snapshot version).
🏁 Script executed:
#!/bin/bash # Check consistency between journal and snapshot versions jq '.version' drizzle/meta/_journal.json jq '.entries[0].version' drizzle/meta/_journal.jsonLength of output: 102
Journal vs. Entry Version Deliberation
The journal’s top-level"version": 7indicates the schema bump for the journal file itself, whereas each entry’s"version"corresponds to its associated snapshot version. Since no new snapshot has been released (still at v6), keeping the entry at"version": "6"is correct.No changes needed here.
1 participant
add alot of stuff
Summary by CodeRabbit