Tinybase & ElectricSQL experiment #1518
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
📝 WalkthroughWalkthroughAdds Postgres/ElectricSQL/Drizzle infra; introduces Tailwind/PostCSS and UI deps for desktop2; expands Tauri plugins and initializes local/cloud DB asynchronously; refactors TinyBase into modular stores/persisters/synchronizers and adds multiple new routes, components, hooks, and schema changes; converts db2 plugin APIs to async. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant App as Desktop2 App
participant Tauri as Tauri Runtime
participant DBExt as db2 Extension
participant PG as Postgres
User->>App: Launch
App->>Tauri: init plugins (listener, local-stt, store, windows)
App->>Tauri: spawn async setup task
Tauri->>DBExt: init_local()
DBExt-->>Tauri: log success/error
Tauri->>DBExt: init_cloud("postgres://...")
DBExt->>PG: connect
PG-->>DBExt: connection ready/error
DBExt-->>Tauri: log success/error
sequenceDiagram
autonumber
participant Router
participant NewRoute as /app/new
participant Hybrid as HybridStore
participant NoteRoute as /app/note/$id
Router->>NewRoute: beforeLoad(context.hybridStore)
NewRoute->>Hybrid: setRow("sessions", newId, {title:"new", humanId:"1", createdAt})
NewRoute-->>Router: redirect to /app/note/{newId}
Router->>NoteRoute: render (id)
NoteRoute->>Hybrid: useRow("sessions", id)
UI->>Hybrid: setRow updates (title/raw_md)
sequenceDiagram
autonumber
participant UI
participant Store as MergeableStore
participant Local as Local Persister
participant Cloud as Cloud Persister
UI->>Store: setRow/update
Store-->>Local: auto-persist
alt CLOUD_ENABLED
Store-->>Cloud: save mapped tables
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit ignore |
✅ Actions performedReviews paused. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
apps/desktop2/src/routes/app.organization.$id.tsx (1)
8-12: Duplicate implementation across detail routes.This Component is virtually identical to the one in
apps/desktop2/src/routes/app.human.$id.tsx(reviewed earlier). Both files fetch a row by id and render JSON with the same structure. This duplication should be addressed with a shared abstraction.See the refactoring suggestion in the
app.human.$id.tsxreview for a potential solution.Verify handling of missing/loading data.
Same concern as
app.human.$id.tsx: confirm thathybrid.UI.useRowhandles undefined/loading states gracefully, or add appropriate checks.
🧹 Nitpick comments (16)
packages/db/src/schema.ts (1)
4-10: Consider adding unique constraint to email field.The
export const humans = pgTable(TABLE_HUMANS, { id: uuid("id").primaryKey().defaultRandom(), name: text("name").notNull(), - email: text("email").notNull(), + email: text("email").notNull().unique(), createdAt: timestamp("created_at").notNull().defaultNow(), updatedAt: timestamp("updated_at").notNull().defaultNow(), });apps/desktop2/src/tinybase/store/memory.ts (1)
42-52: Consider omitting relationships and queries hooks for values-only stores.Since this store has no tables (only values), the useCreateRelationships and useCreateQueries hooks create empty objects with no functional benefit. While harmless, omitting them would reduce unnecessary hook overhead.
Apply this diff if you prefer to remove the unused hooks:
- useCreateRelationships( - store, - (store) => createRelationships(store), - [], - ); - - useCreateQueries( - store, - (store) => createQueries(store), - [], - ); - useProvideStore(STORE_ID, store);apps/desktop2/src-tauri/src/lib.rs (1)
67-67: Consider moving import to file scope.The
use tauri_plugin_db2::Database2PluginExt;import is placed inside the spawned async block. While valid, moving it to the top of the file with other imports would improve consistency and readability.+use tauri_plugin_db2::Database2PluginExt; use tauri_plugin_windows::{HyprWindow, WindowsPluginExt}; // ... later in the file ... tokio::spawn(async move { - use tauri_plugin_db2::Database2PluginExt; - if let Err(e) = app_clone.init_local().await {apps/desktop2/src/components/sidebar.tsx (1)
27-34: Consider using Tailwind classes for consistency.The component uses inline styles for layout while the codebase has Tailwind configured (per
globals.css). For better consistency and maintainability, consider refactoring to use Tailwind utility classes.Apply this diff to use Tailwind classes:
- <div - ref={parentRef} - style={{ - width: "250px", - height: "100vh", - overflow: "auto", - }} - > + <div ref={parentRef} className="w-[250px] h-screen overflow-auto">Similarly for the inner container (lines 35-41):
- <div - style={{ - height: `${rowVirtualizer.getTotalSize()}px`, - width: "100%", - position: "relative", - }} - > + <div + className="w-full relative" + style={{ height: `${rowVirtualizer.getTotalSize()}px` }} + >apps/desktop2/src/styles/globals.css (3)
1-1: Consider moving font import to HTML for better performance.Using
@importfor web fonts is synchronous and blocks rendering. For better performance, prefer a<link>tag in your HTML<head>or use Tailwind's font configuration.Example HTML approach:
<link rel="preconnect" href="https://fonts.googleapis.com"> <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin> <link href="https://fonts.googleapis.com/css2?family=Racing+Sans+One&display=swap" rel="stylesheet">Or configure in
tailwind.config.ts:theme: { extend: { fontFamily: { racing: ['"Racing Sans One"', 'sans-serif'], }, }, }
75-75: Redundantuser-select: nonedeclaration.Line 75 sets
user-select: noneonhtml, body, but line 109 applies the same rule globally via@apply select-nonein the@layer base. Consider removing one to avoid redundancy.Apply this diff to remove the redundant declaration:
html, body { height: 100vh; width: 100vw; overflow-y: hidden; - user-select: none; overscroll-behavior: none; }
144-148: Avoid!importantin favor of higher specificity.Line 145 uses
!importantto override the background color. This can lead to maintainability issues and specificity conflicts. Consider increasing selector specificity instead.Apply this diff to remove
!important:-.search-result-current { - background-color: #31e054 !important; +.search-result.search-result-current { + background-color: #31e054; border-radius: 2px; padding: 1px 0; }And ensure the class is applied as
className="search-result search-result-current"when both are needed.apps/desktop2/src/tinybase/cloudSynchronizer.ts (1)
7-7: Consider making ELECTRIC_URL configurable.The Electric SQL URL is hardcoded to
localhost:3001. For production deployments or different environments, this should be configurable via environment variables or a config file.Example approach:
const ELECTRIC_URL = import.meta.env.VITE_ELECTRIC_URL ?? "http://localhost:3001/v1/shape";apps/desktop2/src/tinybase/shared.ts (1)
1-2: Clarify magic number constants.The constants
MergeableStoreOnly = 2andStoreOrMergeableStore = 3are unexplained magic numbers. Consider adding a brief comment explaining their purpose or origin in the TinyBase API.+// TinyBase internal store type discriminators export const MergeableStoreOnly = 2; export const StoreOrMergeableStore = 3;apps/desktop2/src/routes/app.note.$id.tsx (1)
12-26: Review callback logic for partial updates.Both
handleEditTitleandhandleEditRawMdspread the entirerowobject and overwrite a single field. This approach:
- Overwrites all fields even when updating one
- May lose fields if
rowis stale or missing properties- Creates unnecessary dependencies on the full row in the callback deps array
Consider using a simpler callback that only sets the changed field, or verify that spreading is necessary for TinyBase's merge semantics.
const handleEditTitle = hybrid.UI.useSetRowCallback( "sessions", sessionId, - (input: string, _store) => ({ ...row, title: input }), - [row], + (input: string, store) => { + const currentRow = store.getRow("sessions", sessionId); + return { ...currentRow, title: input }; + }, + [], hybrid.STORE_ID, );apps/desktop2/src/tinybase/localPersister.ts (1)
7-20: Document createCustomSqlitePersister arguments.The
createCustomSqlitePersistercall has 10 arguments with several no-op functions (() => {}). While this may match the TinyBase API, the purpose of each argument is unclear.Consider adding a brief inline comment for non-obvious arguments, especially:
- Line 12: Empty function purpose
- Line 13: Unsubscribe function handler
- Line 14-15: Console logging strategy
- Line 16: Final no-op function
- Line 18: null argument
export function createLocalPersister<Schemas extends OptionalSchemas>(store: MergeableStore<Schemas>) { return createCustomSqlitePersister( store, { mode: "json" }, async (sql: string, args: any[] = []): Promise<any> => (await db2Commands.executeLocal(sql, args)), - () => {}, + () => {}, // onIgnoredError - (unsubscribeFunction: any): any => unsubscribeFunction(), + (unsubscribeFunction: any): any => unsubscribeFunction(), // addChangeListener console.log, console.error, - () => {}, + () => {}, // onSqlCommand MergeableStoreOnly, - null, + null, // no custom config ); }apps/desktop2/src/tinybase/cloudPersister.ts (2)
21-21: Align args type with plugin API (Vec on Rust side).Use string[] to avoid accidental non-string bindings flowing into the Tauri command boundary.
- async (sql: string, args: any[] = []): Promise<any[]> => (await db2Commands.executeCloud(sql, args)), + async (sql: string, args: string[] = []): Promise<any[]> => + db2Commands.executeCloud(sql, args),
15-23: Simplify noop listener; avoid async wrappers.The persister isn’t used for load. A sync no-op subscriber matches the local persister pattern and avoids returning a Promise unsubscriber.
- const noopListener = async (_channel: string, _listener: any) => async () => {}; + const noopListener = () => {};apps/desktop2/src/tinybase/store/hybrid.ts (2)
91-120: Make generic explicit to stabilize inference.Help TS infer the schema for the cloud persister by supplying the TABLE_SCHEMA type parameter.
- return createCloudPersister(store as Store, { + return createCloudPersister<typeof TABLE_SCHEMA>(store as Store, { load: { humans: {
83-126: Cloud persister creation runs even when CLOUD_ENABLED=false.useCreatePersister’s factory is still invoked; only the “start” callback is skipped. If the DB2 cloud client is uninitialized, this can still construct a persister wired to a non-ready backend. Gate construction with CLOUD_ENABLED, or no-op the factory when false.
- useCreatePersister( - store, - (store) => { + useCreatePersister( + store, + (store) => { + if (!CLOUD_ENABLED) { + // No-op persister; caller doesn’t auto-start anyway + return createCloudPersister<typeof TABLE_SCHEMA>(store as Store, { load: {}, save: {} } as any); + } const shared = {Note: If the hook requires a real persister, an explicit no-op persister util would be cleaner. Based on learnings.
plugins/db2/src/ext.rs (1)
39-43: Unify logging (prefer tracing over eprintln!).Elsewhere you use tracing; switching this connection error to tracing keeps logs structured and consistent.
- if let Err(e) = connection.await { - eprintln!("connection error: {}", e); - } + if let Err(e) = connection.await { + tracing::error!("connection error: {}", e); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockapps/desktop2/src/routeTree.gen.tsis excluded by!**/*.gen.tspnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.drizzle/store.json(1 hunks)apps/desktop2/package.json(2 hunks)apps/desktop2/postcss.config.mjs(1 hunks)apps/desktop2/src-tauri/Cargo.toml(2 hunks)apps/desktop2/src-tauri/capabilities/default.json(1 hunks)apps/desktop2/src-tauri/src/lib.rs(2 hunks)apps/desktop2/src/components/sidebar.tsx(1 hunks)apps/desktop2/src/main.tsx(1 hunks)apps/desktop2/src/routes/__root.tsx(2 hunks)apps/desktop2/src/routes/app.human.$id.tsx(1 hunks)apps/desktop2/src/routes/app.index.tsx(1 hunks)apps/desktop2/src/routes/app.new.tsx(1 hunks)apps/desktop2/src/routes/app.note.$id.tsx(1 hunks)apps/desktop2/src/routes/app.note.tsx(1 hunks)apps/desktop2/src/routes/app.organization.$id.tsx(1 hunks)apps/desktop2/src/routes/app.settings.tsx(1 hunks)apps/desktop2/src/styles/globals.css(1 hunks)apps/desktop2/src/tinybase/cloudPersister.ts(1 hunks)apps/desktop2/src/tinybase/cloudSynchronizer.ts(1 hunks)apps/desktop2/src/tinybase/index.ts(0 hunks)apps/desktop2/src/tinybase/localPersister.ts(1 hunks)apps/desktop2/src/tinybase/localSynchronizer.ts(1 hunks)apps/desktop2/src/tinybase/shared.ts(1 hunks)apps/desktop2/src/tinybase/store/hybrid.ts(1 hunks)apps/desktop2/src/tinybase/store/local.ts(1 hunks)apps/desktop2/src/tinybase/store/memory.ts(1 hunks)apps/desktop2/src/types/index.ts(1 hunks)apps/desktop2/src/utils.ts(1 hunks)apps/desktop2/tailwind.config.ts(1 hunks)crates/db-core/Cargo.toml(1 hunks)docker-compose.yaml(1 hunks)packages/db/package.json(1 hunks)packages/db/src/index.ts(1 hunks)packages/db/src/schema.ts(1 hunks)plugins/db2/Cargo.toml(1 hunks)plugins/db2/src/commands.rs(2 hunks)plugins/db2/src/error.rs(1 hunks)plugins/db2/src/ext.rs(3 hunks)plugins/db2/src/lib.rs(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop2/src/tinybase/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop2/src/utils.tsapps/desktop2/tailwind.config.tsplugins/db2/src/lib.rsapps/desktop2/src/types/index.tsapps/desktop2/src/components/sidebar.tsxapps/desktop2/src/tinybase/shared.tsapps/desktop2/src/tinybase/localSynchronizer.tsapps/desktop2/src/routes/app.human.$id.tsxplugins/db2/src/commands.rsapps/desktop2/src/routes/__root.tsxpackages/db/src/index.tsapps/desktop2/src/routes/app.note.tsxapps/desktop2/src/routes/app.settings.tsxapps/desktop2/src/tinybase/store/memory.tsplugins/db2/src/error.rsapps/desktop2/src/routes/app.organization.$id.tsxapps/desktop2/src/tinybase/cloudSynchronizer.tsapps/desktop2/src/routes/app.note.$id.tsxapps/desktop2/src/routes/app.new.tsxapps/desktop2/src/tinybase/store/hybrid.tsapps/desktop2/src/tinybase/localPersister.tspackages/db/src/schema.tsapps/desktop2/src/routes/app.index.tsxapps/desktop2/src/tinybase/store/local.tsapps/desktop2/src-tauri/src/lib.rsplugins/db2/src/ext.rsapps/desktop2/src/main.tsxapps/desktop2/src/tinybase/cloudPersister.ts
🧬 Code graph analysis (18)
plugins/db2/src/lib.rs (1)
crates/db-core/src/lib.rs (1)
sync(24-26)
apps/desktop2/src/tinybase/localSynchronizer.ts (1)
apps/desktop2/src/tinybase/shared.ts (1)
BROADCAST_CHANNEL_NAME(4-4)
apps/desktop2/src/routes/app.human.$id.tsx (3)
apps/desktop2/src/routes/app.new.tsx (1)
Route(4-19)apps/desktop2/src/routes/app.organization.$id.tsx (1)
Route(4-6)apps/desktop2/src/routes/app.settings.tsx (1)
Route(3-5)
apps/desktop2/src/routes/__root.tsx (2)
apps/desktop2/src/routes/app.new.tsx (1)
Route(4-19)apps/desktop2/src/types/index.ts (1)
Context(3-5)
apps/desktop2/src/routes/app.note.tsx (3)
apps/desktop2/src/routes/app.index.tsx (1)
Route(3-5)apps/desktop2/src/routes/app.note.$id.tsx (1)
Route(4-6)apps/desktop2/src/components/sidebar.tsx (1)
Sidebar(8-67)
apps/desktop2/src/routes/app.settings.tsx (6)
apps/desktop2/src/routes/app.human.$id.tsx (1)
Route(3-5)apps/desktop2/src/routes/app.new.tsx (1)
Route(4-19)apps/desktop2/src/routes/app.organization.$id.tsx (1)
Route(4-6)apps/desktop2/src/routes/app.index.tsx (1)
Route(3-5)apps/desktop2/src/routes/app.note.$id.tsx (1)
Route(4-6)apps/desktop2/src/routes/__root.tsx (1)
Route(6-6)
apps/desktop2/src/tinybase/store/memory.ts (3)
apps/desktop2/src/tinybase/store/hybrid.ts (2)
STORE_ID(17-17)StoreComponent(73-150)apps/desktop2/src/tinybase/store/local.ts (2)
STORE_ID(7-7)StoreComponent(34-58)apps/desktop2/src/tinybase/localSynchronizer.ts (1)
createLocalSynchronizer(6-10)
apps/desktop2/src/routes/app.organization.$id.tsx (2)
apps/desktop2/src/routes/app.human.$id.tsx (1)
Route(3-5)apps/desktop2/src/routes/app.new.tsx (1)
Route(4-19)
apps/desktop2/src/tinybase/cloudSynchronizer.ts (2)
apps/desktop2/src/tinybase/store/hybrid.ts (1)
Store(69-69)apps/desktop2/src/tinybase/store/local.ts (1)
Store(32-32)
apps/desktop2/src/routes/app.note.$id.tsx (2)
apps/desktop2/src/routes/app.new.tsx (1)
Route(4-19)apps/desktop2/src/routes/app.note.tsx (1)
Route(6-8)
apps/desktop2/src/routes/app.new.tsx (3)
apps/desktop2/src/routes/app.note.$id.tsx (1)
Route(4-6)apps/desktop2/src/routes/__root.tsx (1)
Route(6-6)apps/desktop2/src/utils.ts (1)
id(1-1)
apps/desktop2/src/tinybase/store/hybrid.ts (7)
apps/desktop2/src/tinybase/store/local.ts (4)
STORE_ID(7-7)UI(31-31)Store(32-32)StoreComponent(34-58)apps/desktop2/src/tinybase/store/memory.ts (3)
STORE_ID(12-12)UI(30-30)StoreComponent(32-57)apps/desktop2/src/tinybase/shared.ts (1)
InferTinyBaseSchema(15-18)apps/desktop2/src/tinybase/localPersister.ts (1)
createLocalPersister(7-20)apps/desktop2/src/tinybase/cloudPersister.ts (1)
createCloudPersister(11-30)packages/db/src/schema.ts (3)
TABLE_HUMANS(3-3)TABLE_ORGANIZATIONS(12-12)TABLE_SESSIONS(20-20)apps/desktop2/src/tinybase/localSynchronizer.ts (1)
createLocalSynchronizer(6-10)
apps/desktop2/src/tinybase/localPersister.ts (1)
apps/desktop2/src/tinybase/shared.ts (1)
MergeableStoreOnly(1-1)
apps/desktop2/src/routes/app.index.tsx (1)
apps/desktop2/src/routes/app.new.tsx (1)
Route(4-19)
apps/desktop2/src/tinybase/store/local.ts (4)
apps/desktop2/src/tinybase/store/hybrid.ts (3)
STORE_ID(17-17)Store(69-69)StoreComponent(73-150)apps/desktop2/src/tinybase/store/memory.ts (2)
STORE_ID(12-12)StoreComponent(32-57)apps/desktop2/src/tinybase/localPersister.ts (1)
createLocalPersister(7-20)apps/desktop2/src/tinybase/localSynchronizer.ts (1)
createLocalSynchronizer(6-10)
apps/desktop2/src-tauri/src/lib.rs (1)
plugins/db2/src/lib.rs (1)
init(31-42)
plugins/db2/src/ext.rs (2)
plugins/db2/src/commands.rs (2)
execute_cloud(17-25)execute_local(5-13)crates/db-core/src/lib.rs (1)
conn(17-22)
apps/desktop2/src/tinybase/cloudPersister.ts (1)
apps/desktop2/src/tinybase/shared.ts (1)
MergeableStoreOnly(1-1)
🪛 Checkov (3.2.334)
docker-compose.yaml
[medium] 27-28: Basic Auth Credentials
(CKV_SECRET_4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (53)
crates/db-core/Cargo.toml (1)
14-15: Verify that libsql supports the encryption feature.Ensure that the workspace-defined libsql dependency includes the
encryptionfeature. If libsql doesn't expose this feature, the build will fail when theencryptionfeature is enabled.apps/desktop2/src-tauri/capabilities/default.json (1)
5-5: Verify that all windows require these capabilities.Changing from
["main"]to["*"]grantsdb2:default,windows:default, andopener:defaultpermissions to all windows. If the app creates auxiliary windows (dialogs, popups, etc.) that don't need database access, this may violate the principle of least privilege.apps/desktop2/tailwind.config.ts (1)
1-18: LGTM! Consider verifying font loading.The Tailwind configuration is well-structured with appropriate content globs and type safety via
satisfies Config. The custom font family extension is properly defined.Ensure the "Racing Sans One" font is loaded in your global CSS or HTML. You can verify with:
packages/db/src/schema.ts (1)
1-27: Verify removal of legacy tables across the codebase.The schema removes
notes,tags, andnoteTagstables and replaces them withhumans,organizations, andsessions. Ensure all references to the old tables have been updated.docker-compose.yaml (3)
10-12: Ephemeral database storage using tmpfs.The postgres data directory is mounted as tmpfs, meaning all data will be lost on container restart. This is appropriate for development/testing but should be verified as intentional.
If persistent data is needed at any point, replace tmpfs with a named volume:
- tmpfs: - - /var/lib/postgresql/data - - /tmp + volumes: + - postgres_data:/var/lib/postgresql/dataAnd add the volume definition at the file's end:
volumes: postgres_data:
28-28: ELECTRIC_INSECURE mode enabled.
ELECTRIC_INSECURE: truedisables security checks in ElectricSQL. This is acceptable for local development but must never be used in production environments.Document this as a dev-only setting or add a comment in the compose file to prevent accidental production use.
44-44: Local directory mount for Drizzle Studio.The
./.drizzledirectory is mounted to/appfor Drizzle Studio's store. Ensure this directory exists or is created during setup, otherwise the service may fail to start.Consider adding a note in documentation or a setup script that creates
.drizzledirectory if it doesn't exist, or use a named volume if persistence isn't required.plugins/db2/src/error.rs (1)
7-8: LGTM! Consistent error variant addition.The new
HyprDbErrorvariant follows the same pattern as the existingPostgresError, using#[error(transparent)]and#[from]for ergonomic error conversion. This integrates cleanly with thehypr_db_coreintegration mentioned in the PR.packages/db/package.json (1)
17-18: New drizzle-zod dependency added for schema generation.The addition of
drizzle-zod ^0.8.3enables Zod schema generation from Drizzle ORM table definitions, which aligns with the new schema factory exports mentioned in the AI summary.Verify that
drizzle-orm ^0.44anddrizzle-zod ^0.8.3are compatible versions. Run a quick compatibility check:Based on learnings: drizzle-zod 0.8.3 is the latest version; ensure generated schemas work correctly with your Drizzle ORM dialect (PostgreSQL/MySQL/SQLite).
apps/desktop2/src/utils.ts (1)
1-1: Verify crypto.randomUUID() availability in target runtime.The implementation is clean and uses the standard Web Crypto API. Ensure your target environment (Electron/Tauri WebView) supports
crypto.randomUUID()(available in modern browsers and Node.js 19+).Since this is a Tauri/Electron desktop app, verify the WebView version supports this API:
apps/desktop2/postcss.config.mjs (1)
1-1: Config delegation to shared package.Clean approach to share PostCSS configuration across the monorepo by re-exporting from
@hypr/ui/postcss.config.Verify that
@hypr/ui/postcss.configexists and exports a valid PostCSS configuration:apps/desktop2/src-tauri/Cargo.toml (2)
25-26: LGTM! New Tauri plugin dependencies added.The additions of
tauri-plugin-listenerandtauri-plugin-local-sttare consistent with other workspace-based plugin dependencies and align with the plugin initialization mentioned in the AI summary.
40-40: LGTM! Tracing dependency added for observability.The
tracingworkspace dependency complements the existingsentryintegration (line 37 with "tracing" feature) and supports the enhanced runtime observability mentioned in the AI summary.apps/desktop2/package.json (3)
11-11: LGTM! Typecheck script added.The
typecheckscript enables type checking without emitting files, which is a standard practice for catching type errors during development.
17-22: LGTM! New UI and utility dependencies added.The workspace-based
@hypr/*dependencies and@tanstack/react-virtualalign with the UI framework upgrade described in the AI summary. The postcss/tailwind devDependencies (lines 39-41) complete the styling stack.
39-41: LGTM! Tailwind CSS toolchain added.The
autoprefixer,postcss, andtailwindcssdevDependencies form a complete styling stack that aligns with the new PostCSS and Tailwind configs mentioned in the AI summary.apps/desktop2/src/types/index.ts (1)
1-5: LGTM! Clean context type definition.The
Contexttype withhybridStoreproperty is well-defined and uses a type-only import forHybridStore. This aligns with the router context usage mentioned in the AI summary for__root.tsxandmain.tsx.plugins/db2/src/lib.rs (2)
1-1: LGTM! Async-safe Mutex for db2 plugin.The migration from
std::sync::Mutextotokio::sync::Mutexis essential for the async transformation described in the AI summary. This change enables safe lock holding across await points in the new async trait methods (execute_local/execute_cloudin ext.rs).
13-13: Verify all lock acquisitions are properly awaited.The
ManagedStatenow usestokio::sync::Mutex, which requires.lock().awaitinstead of.lock().unwrap(). Ensure all call sites that acquire this mutex have been updated to use async syntax.plugins/db2/src/commands.rs (2)
10-12: LGTM! Async execution for local database.The addition of
.awaitcorrectly completes the async call chain forexecute_local, aligning with the async trait methods introduced in ext.rs. The error mapping remains appropriate for Tauri command handlers.
22-24: LGTM! Async execution for cloud database.The addition of
.awaitcorrectly completes the async call chain forexecute_cloud, maintaining consistency with theexecute_localpattern and the async migration described in the AI summary.packages/db/src/index.ts (1)
2-3: LGTM!Re-exporting drizzle-zod schema factory functions is a clean way to expose type-safe validation utilities to consumers of this package.
apps/desktop2/src/routes/app.settings.tsx (1)
1-13: LGTM!The route definition and component follow the established pattern for file-based routes. The minimal UI is appropriate for a placeholder settings page.
apps/desktop2/src/routes/app.index.tsx (1)
1-16: LGTM!The simplified component correctly removes legacy store initialization and renders a clean landing page with a link to create a new note. The Tailwind styling is consistent with the new UI direction.
apps/desktop2/src/tinybase/store/memory.ts (1)
1-57: LGTM!The memory store setup correctly defines a values-only schema and wires up synchronization and context provision. The pattern is consistent with other store files in the codebase.
apps/desktop2/src/routes/app.note.tsx (2)
1-42: LGTM!The route correctly composes a layout with a sidebar, nested outlet, and action buttons. The handlers for settings and new note navigation are correctly implemented.
26-39: Verify button positioning intent.The action buttons are rendered in-flow within a flex column. If these are intended as floating action buttons (e.g., fixed to the right edge), you may need to add positioning styles (e.g.,
absolute,fixed,ml-auto, etc.).plugins/db2/Cargo.toml (3)
20-20: LGTM!The encryption feature addition to
hypr-db-coreis straightforward and aligns with the expanded capability for encrypted PostgreSQL interactions mentioned in the summary.
21-21: LGTM!The
tokio-postgresversion 0.7.14 is current and thewith-serde_json-1feature is appropriate for JSON column support. Based on learnings, this release includes compatibility updates and re-exportsfallible-iterator.
23-25: LGTM!The additions of
futures-utilandtracingfrom the workspace are appropriate for supporting asynchronous operations and instrumentation in the db2 plugin.apps/desktop2/src/routes/app.human.$id.tsx (2)
3-5: LGTM!The route declaration follows the standard TanStack Router pattern and is consistent with similar routes in the codebase.
7-11: Reduce duplication across detail routes.This Component is nearly identical to the one in
apps/desktop2/src/routes/app.organization.$id.tsx. The only differences are the table name and variable name. Consider extracting a shared detail component or route helper to eliminate duplication.Example approach:
// shared/DetailRoute.tsx export function createDetailRoute<T extends string>( path: string, tableName: T ) { return createFileRoute(path)({ component: () => { const { id } = Route.useParams(); const row = hybrid.UI.useRow(tableName, id); return <pre>{JSON.stringify(row, null, 2)}</pre>; }, }); }Verify handling of missing/loading data.
The component renders
humanwithout checking if it'sundefinedor still loading. Confirm thathybrid.UI.useRowreturns a safe default or that the consuming code handles missing data gracefully.apps/desktop2/src/main.tsx (4)
1-2: LGTM!The CSS imports are standard and appropriate for styling the application.
8-16: LGTM!The TinyBase imports are structured correctly, bringing in the necessary Provider, hooks, and store components for the hybrid/local/memory store architecture.
19-19: LGTM!Creating the router with an empty context object is appropriate; the actual context (including
hybridStore) is provided later in theAppcomponent.
27-36: Verify StoreComponent initialization order.The
Appcomponent usesuseStores()to access the hybrid store, but theStoreComponentsare rendered as siblings (lines 44-47), not ancestors. This may create a race whereAppmounts before the stores are initialized, causing a blank screen (null return on line 32).Consider restructuring so that
StoreComponentHybrid,StoreComponentLocal, andStoreComponentMemoryare guaranteed to mount and initialize beforeAppattempts to access them, or add a loading indicator instead of returningnull.Type cast suggests type mismatch.
Line 29 uses
as unknown as HybridStore, indicating a type incompatibility between whatuseStores()returns and the expectedHybridStoretype. This double-cast bypasses type safety and may hide runtime errors.apps/desktop2/src/routes/app.organization.$id.tsx (1)
4-6: LGTM!The route declaration follows the established pattern and is consistent with other detail routes.
apps/desktop2/src-tauri/src/lib.rs (2)
43-44: LGTM!The new plugin registrations follow the established pattern and are correctly positioned in the plugin initialization chain.
65-78: Externalize the hardcoded database connection string.Line 73 contains a hardcoded PostgreSQL connection string with sensitive information (username
yujonglee, databasehyprnote_dev, localhost). This should be moved to environment variables or a configuration file to avoid exposing credentials and to support different environments (dev/staging/prod).Apply this approach:
- if let Err(e) = app_clone - .init_cloud("postgresql://yujonglee@localhost:5432/hyprnote_dev") - .await + let conn_str = std::env::var("DATABASE_URL") + .unwrap_or_else(|_| "postgresql://localhost:5432/hyprnote_dev".to_string()); + if let Err(e) = app_clone.init_cloud(&conn_str).awaitVerify downstream handling of DB initialization race.
The database initialization runs asynchronously in a background task while the app continues. If routes or components attempt to access the database before
init_local()andinit_cloud()complete, they may encounter errors or undefined behavior.Confirm that:
- The TinyBase store components and routes handle the case where the DB is not yet initialized.
- There's a mechanism (e.g., ready flag, initialization barrier) to prevent DB access before initialization completes.
apps/desktop2/src/tinybase/localSynchronizer.ts (1)
6-10: LGTM!Clean factory function that appropriately wraps the broadcast channel synchronizer with the shared channel name. The generic constraint and implementation are correct.
apps/desktop2/src/tinybase/cloudSynchronizer.ts (1)
79-84: Verify graceful error handling behavior.When a stream encounters an error (line 79), the Promise resolves with partial messages rather than rejecting. This means
sync()will succeed even if some tables fail to sync, potentially leading to incomplete data without alerting the caller.If this graceful degradation is intentional, consider adding a comment explaining the behavior. Otherwise, consider propagating errors or returning an error status alongside results.
Example of explicit error tracking:
type SyncResult = { data: Record<string, Message[]>; errors: Record<string, Error>; };apps/desktop2/src/routes/app.new.tsx (1)
10-10: Verify hardcodedhumanIdvalue.Every new session is created with
humanId: "1". If this field should reference an actual user or be unique per session, this hardcoded value may cause data integrity issues.Run the following script to check how
humanIdis used elsewhere:apps/desktop2/src/routes/__root.tsx (2)
30-38: LGTM! TinybaseInspector follows established lazy-loading pattern.The lazy-loaded TinybaseInspector correctly mirrors the TanStackRouterDevtools pattern and is properly gated for production.
6-6: Verify downstream usage handles optional hybridStore.The root route uses
Partial<Context>, makinghybridStoreoptional. However,apps/desktop2/src/routes/app.new.tsxaccesseshybridStore!with a non-null assertion, andapps/desktop2/src/routes/app.note.$id.tsxassumes the store is present.Run the following script to check all context.hybridStore usage:
apps/desktop2/src/tinybase/shared.ts (1)
6-18: LGTM! Type utilities correctly implement TinyBase schema inference.The conditional type logic properly maps primitive types to TinyBase cell schemas as documented in the referenced GitHub issue.
apps/desktop2/src/tinybase/store/local.ts (2)
34-58: LGTM! Store lifecycle correctly wires persister and synchronizer.The component properly initializes the local store with auto-persisting and auto-syncing, following the established pattern from other store modules.
9-15: Which ElectricSQL server release or commit date should I reference to fetch the precise metadata table schema? Once you confirm, I’ll verify that yourelectric_metaschema matches it.apps/desktop2/src/routes/app.note.$id.tsx (1)
10-10: Verify row existence before rendering.
useRowmay returnundefinedif thesessionIddoes not exist in the store. Accessingrow.titleandrow.raw_mdat lines 33 and 38 will throw ifrowis undefined.apps/desktop2/src/tinybase/localPersister.ts (1)
11-11: Verify async SQL execution without error handling.The SQL executor
db2Commands.executeLocal(sql, args)is called without try-catch. If execution fails, the error will propagate to the persister. Verify this is the intended error handling strategy for TinyBase persisters.apps/desktop2/src/tinybase/cloudPersister.ts (1)
12-13: Type tightness of store param.PersistedStore<[Schema, NoValuesSchema], typeof MergeableStoreOnly> is correct, but callers should pass a MergeableStore with the same Schema. Recommend documenting at call sites with an explicit generic to help inference.
apps/desktop2/src/tinybase/store/hybrid.ts (1)
137-143: Relationship wiring LGTM.sessionHuman: sessions.humanId → humans.id is consistent with schemas.
plugins/db2/src/ext.rs (2)
126-158: Confirm args typing for query_raw.tokio_postgres::Client::query_raw expects ToSql items; you’re passing Vec. This can work via blanket impls, but please confirm there’s no implicit allocation/conversion cost or type mismatch at call sites (numbers/JSON should be serialized to strings earlier).
Run this to find cloud-execution call sites and ensure args are string arrays:
20-26: Avoid unwrap() in init_local.unwrap will panic on DB creation failure; prefer propagating the error to the caller like other methods. This is a correctness and stability risk.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/db/src/schema.ts (1)
20-27: Add missing updatedAt field and onDelete behavior.The
sessionstable is missing:
updatedAttimestamp field (present inhumansandorganizations)- Foreign key
onDeletebehavior forhumanIdApply this diff:
export const sessions = pgTable(TABLE_SESSIONS, { id: uuid("id").primaryKey().defaultRandom(), - humanId: uuid("human_id").notNull().references(() => humans.id), + humanId: uuid("human_id").notNull().references(() => humans.id, { onDelete: "cascade" }), title: text("title").notNull(), body: text("body").notNull(), createdAt: timestamp("created_at").notNull().defaultNow(), + updatedAt: timestamp("updated_at").notNull().defaultNow(), });Note: Choose appropriate
onDeletebehavior (cascade,set null,restrict) based on your business logic.apps/desktop2/src/tinybase/store/hybrid.ts (1)
52-76: TABLE_SCHEMA has fields not in database.The
TABLE_SCHEMAdefinition includes fields that don't exist in the database:
humans.orgId(line 64)sessions.raw_mdandsessions.enhanced_md(lines 57-58)This will cause the cloud persister to fail when trying to sync these fields.
🧹 Nitpick comments (3)
apps/desktop2/src/routes/app.finder.tsx (1)
5-7: Single-value enum suggests incomplete implementation.The
viewfield has only one possible value"calendar", which makes the enum unnecessary. This likely indicates the route is a work-in-progress.apps/desktop2/src/hooks/useValidatedRow.ts (1)
14-35: Consider stabilizing setField with useRef for currentRow.The
setFieldcallback includescurrentRowin its dependency array, which changes on every row update and causes the callback to be recreated. This can trigger unnecessary re-renders in child components.Consider using a ref for
currentRow:+ const currentRowRef = useRef(currentRow); + currentRowRef.current = currentRow; + const setField = useCallback( <K extends keyof Row>(field: K, value: Row[K]) => { - if (!currentRow) { + if (!currentRowRef.current) { return; } - const nextRow = { ...currentRow, [field]: value }; + const nextRow = { ...currentRowRef.current, [field]: value }; // ... rest of logic }, - [currentRow, schema, onUpdate], + [schema, onUpdate], );apps/desktop2/src/tinybase/store/hybrid.ts (1)
113-114: Complete the condition for row-level security.The commented-out condition on line 113 indicates incomplete row-level security (RLS) implementation. Without this condition, all users may be able to access all data.
Do you want me to help implement the RLS condition based on your authentication setup?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/desktop2/src/routeTree.gen.tsis excluded by!**/*.gen.tspnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/desktop2/package.json(2 hunks)apps/desktop2/src/hooks/useValidatedRow.ts(1 hunks)apps/desktop2/src/routes/app.finder.tsx(1 hunks)apps/desktop2/src/routes/app.human.$id.tsx(1 hunks)apps/desktop2/src/tinybase/store/hybrid.ts(1 hunks)packages/db/src/schema.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop2/src/routes/app.finder.tsxapps/desktop2/src/hooks/useValidatedRow.tspackages/db/src/schema.tsapps/desktop2/src/tinybase/store/hybrid.tsapps/desktop2/src/routes/app.human.$id.tsx
🧬 Code graph analysis (2)
apps/desktop2/src/tinybase/store/hybrid.ts (7)
apps/desktop2/src/tinybase/store/memory.ts (2)
STORE_ID(12-12)StoreComponent(32-57)apps/desktop2/src/tinybase/store/local.ts (3)
STORE_ID(7-7)Store(32-32)StoreComponent(34-58)apps/desktop2/src/tinybase/shared.ts (1)
InferTinyBaseSchema(15-18)apps/desktop2/src/tinybase/localPersister.ts (1)
createLocalPersister(7-20)apps/desktop2/src/tinybase/cloudPersister.ts (1)
createCloudPersister(11-30)packages/db/src/schema.ts (4)
TABLE_HUMANS(3-3)TABLE_ORGANIZATIONS(12-12)TABLE_SESSIONS(20-20)TABLE_EVENTS(29-29)apps/desktop2/src/tinybase/localSynchronizer.ts (1)
createLocalSynchronizer(6-10)
apps/desktop2/src/routes/app.human.$id.tsx (2)
apps/desktop2/src/tinybase/store/hybrid.ts (1)
Human(27-27)apps/desktop2/src/hooks/useValidatedRow.ts (1)
useValidatedRow(6-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (macos, macos-14)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (2)
apps/desktop2/src/routes/app.human.$id.tsx (1)
9-11: Add guard for undefined human data.
hybrid.UI.useRowcan returnundefinedif the row doesn't exist, but the component accesseshuman.nameandhuman.emailwithout checking. This will cause a runtime error if the human is not found.Apply this diff to add a guard:
function Component() { const { id } = Route.useParams(); const human = hybrid.UI.useRow("humans", id, hybrid.STORE_ID); + + if (!human) { + return <div>Human not found</div>; + } const handleUpdate = hybrid.UI.useSetRowCallback(Likely an incorrect or invalid review comment.
apps/desktop2/src/tinybase/store/hybrid.ts (1)
219-220: Remove non-null assertions on indexes and metrics.Similar to the relationships issue, the
!assertions on lines 219-220 assume the hooks never returnnull.Add guards before calling
useProvideIndexesanduseProvideMetrics:+ if (!indexes) { + console.error("Failed to create indexes"); + return null; + } + if (!metrics) { + console.error("Failed to create metrics"); + return null; + } + useProvideStore(STORE_ID, store); useProvideRelationships(STORE_ID, relationships); - useProvideIndexes(STORE_ID, indexes!); - useProvideMetrics(STORE_ID, metrics!); + useProvideIndexes(STORE_ID, indexes); + useProvideMetrics(STORE_ID, metrics);Likely an incorrect or invalid review comment.
yujonglee
force-pushed
the
electricsql-experiment
branch
from
a35ef11 to
0c7b651
Compare
No description provided.