feat: add devtool autodiscovery#4
Open
quolpr wants to merge 6 commits into
Open
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
✅ Deploy Preview for hyperdb ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds a global ChangesDB Registry & Devtools Discovery
Demo Multi-Driver Persistence
Sequence Diagram(s)sequenceDiagram
participant App
participant initStore as initStore(mode)
participant memDB as In-memory DB
participant persistentDB as Persistent Driver
participant monitor as PersistenceMonitor
App->>initStore: await initStore(getStoredMode())
initStore->>persistentDB: create & load persisted tables
initStore->>memDB: create in-memory DB
initStore->>memDB: hydrate:load projects/tasks
persistentDB-->>memDB: scan byIds indices
memDB->>persistentDB: subscribe to commit ops
memDB->>persistentDB: persist:batch queued ops
persistentDB-->>monitor: report pending/draining state
initStore-->>App: { db: memDB, persistence }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/hyperdb-demo/src/stores.ts`:
- Around line 190-197: The batch is being removed from the pending queue via
pending.shift() before persistBatch succeeds, causing lost batches on failure.
Move the pending.shift() call to execute only after the persistBatch await
completes successfully, either by placing it after the await statement within
the try block or by only removing the batch once persistence is confirmed. This
ensures failed batches remain in the queue and can be retried.
In `@packages/hyperdb-doc/src/content/docs/runtime/db.md`:
- Around line 32-39: The DB options table in
packages/hyperdb-doc/src/content/docs/runtime/db.md is missing documentation for
the `register` option that was introduced in this feature set. Add a new row to
the options table following the same format as the existing entries like
`runtimeRowsValidation`, `freezeArgs`, and `freezeRows`, including the option
name `register`, its default value, and a clear description of what this option
does (controlling the auto-registration behavior of the database).
In `@packages/hyperdb/src/hyperdb/tracing/registry.ts`:
- Around line 79-81: The listener invocation in the registry notification loop
does not isolate failures, causing a throwing listener to abort the entire
registerHyperDB operation and cascade into DB construction failures. Wrap the
listener invocation call in a try-catch block within the for loop that iterates
over the registry.listeners array, and in the catch block, log or handle the
error gracefully without re-throwing so that subsequent listeners are still
notified and the registration completes successfully regardless of individual
listener failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ffa8f06-c0e2-4bf2-9d49-91fedb1ce1be
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
AGENTS.mdCLAUDE.mdpackages/hyperdb-demo/src/BenchmarkApp.tsxpackages/hyperdb-demo/src/db.tspackages/hyperdb-demo/src/main.tsxpackages/hyperdb-demo/src/persistence-context.tsxpackages/hyperdb-demo/src/persistence-monitor.tspackages/hyperdb-demo/src/store-mode.tspackages/hyperdb-demo/src/stores.tspackages/hyperdb-devtool/src/components.test.tsxpackages/hyperdb-devtool/src/components.tsxpackages/hyperdb-devtool/vite.config.tspackages/hyperdb-doc/src/content/docs/database/writing-data.mdpackages/hyperdb-doc/src/content/docs/index.mdxpackages/hyperdb-doc/src/content/docs/runtime/db.mdpackages/hyperdb/devtool.mdpackages/hyperdb/src/hyperdb/runtime/db.tspackages/hyperdb/src/hyperdb/tracing/db-info.tspackages/hyperdb/src/hyperdb/tracing/index.tspackages/hyperdb/src/hyperdb/tracing/registry.test.tspackages/hyperdb/src/hyperdb/tracing/registry.tspackages/hyperdb/src/hyperdb/tracing/store.tspackages/hyperdb/src/react/context.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/hyperdb-demo/src/stores.ts (1)
363-376: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider returning cleanup for resource management.
startPersisting()returns a cleanup function that removes event listeners and the DB subscription. Currently it's discarded, which prevents proper teardown if the store is ever reinitialized without a page reload.For a demo that reloads on mode change this is acceptable, but if reinitialization without reload is ever needed, this would leak resources.
Optional: return cleanup in InitResult
export type InitResult = { db: SubscribableDB; persistence: PersistenceMonitor | null; + cleanup?: () => void; };- startPersisting(persistentDB, memDB, persistence); + const cleanup = startPersisting(persistentDB, memDB, persistence); - return { db: memDB, persistence }; + return { db: memDB, persistence, cleanup };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/hyperdb-demo/src/stores.ts` around lines 363 - 376, The startPersisting() function returns a cleanup function that is currently being discarded, which can cause resource leaks if initStore() is called again without a page reload. Capture the return value from the startPersisting() call, add a cleanup property to the InitResult object being returned, and include this cleanup function in the return statement alongside db and persistence. This allows consumers of initStore() to properly invoke the cleanup function when the store needs to be reinitialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/hyperdb-demo/src/BenchmarkApp.tsx`:
- Around line 181-199: The select element with value={storeMode} and
onChange={handleStoreModeChange} lacks an associated accessible label for screen
readers and accessibility tools. Wrap both the span element containing "Driver"
text and the select element in a <label> element to properly associate the label
with the form control, following the same pattern used for other form controls
in the component.
In `@packages/hyperdb-demo/src/wa-sqlite-worker.ts`:
- Around line 109-131: The message event listener in the worker currently
processes requests concurrently without serialization, which causes SQL
operations to interleave on the same database connection. To fix this, implement
a request queue mechanism that serializes all incoming requests. Create a queue
to hold pending WorkerRequest objects and a processing loop that dequeues and
processes requests sequentially. The event listener should add requests to the
queue instead of immediately executing the async operation, ensuring that each
call to getConnection, execSQL, and readValues completes before the next request
begins execution.
- Around line 53-71: The createConnection promise caching in the getConnection
function causes permanent failures if the initial connection attempt rejects,
since the rejected promise is cached and never retried. To fix this, add error
handling to the connectionPromise assignment in the getConnection function that
resets connectionPromise back to null when createConnection rejects, allowing
subsequent calls to attempt a fresh connection instead of returning the cached
rejected promise.
---
Nitpick comments:
In `@packages/hyperdb-demo/src/stores.ts`:
- Around line 363-376: The startPersisting() function returns a cleanup function
that is currently being discarded, which can cause resource leaks if initStore()
is called again without a page reload. Capture the return value from the
startPersisting() call, add a cleanup property to the InitResult object being
returned, and include this cleanup function in the return statement alongside db
and persistence. This allows consumers of initStore() to properly invoke the
cleanup function when the store needs to be reinitialized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3ba46bb-9f0d-4498-a59a-ecae94ab5994
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.prettierignoreTODO.mdpackages/hyperdb-demo/package.jsonpackages/hyperdb-demo/src/BenchmarkApp.tsxpackages/hyperdb-demo/src/db.tspackages/hyperdb-demo/src/store-mode.tspackages/hyperdb-demo/src/stores.tspackages/hyperdb-demo/src/wa-sqlite-worker.tspackages/hyperdb-demo/src/wa-sqlite.d.tspackages/hyperdb-devtool/src/components.tsxpackages/hyperdb-doc/src/content/docs/guides/in-memory-persistence.mdpackages/hyperdb-doc/src/content/docs/runtime/drivers.mdpackages/hyperdb/src/hyperdb/drivers/sqlite/async-sql-driver.ts
💤 Files with no reviewable changes (1)
- packages/hyperdb/src/hyperdb/drivers/sqlite/async-sql-driver.ts
✅ Files skipped from review due to trivial changes (1)
- .prettierignore
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/hyperdb-demo/src/db.ts
- packages/hyperdb-devtool/src/components.tsx
Comment on lines
+181
to
+199
| <div className="flex min-w-[200px] flex-col justify-center gap-2 border-t border-line bg-base/60 p-6 text-left sm:border-l sm:border-t-0"> | ||
| <span className={LABEL}>Driver</span> | ||
| <select | ||
| value={storeMode} | ||
| onChange={handleStoreModeChange} | ||
| className="h-10 w-full cursor-pointer rounded-md border border-line bg-base px-3 font-mono text-sm text-ink outline-none transition focus-visible:border-signal/60 focus-visible:ring-2 focus-visible:ring-signal/20" | ||
| > | ||
| <option value="idb">IndexedDB</option> | ||
| <option value="idb-inmem">IndexedDB + in-memory</option> | ||
| <option value="wa-sqlite">WA-SQLite OPFS</option> | ||
| <option value="wa-sqlite-inmem">WA-SQLite OPFS + in-memory</option> | ||
| </select> | ||
| <div className="flex items-center gap-2"> | ||
| <span | ||
| className={`size-2 shrink-0 rounded-full ${storageStatus.dot}`} | ||
| /> | ||
| <p className="text-xs text-faint">{storageStatus.text}</p> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Missing accessible label for the driver select.
The <select> element lacks an associated label. Other form controls in this component use a <label> wrapper pattern for accessibility.
Proposed fix
- <div className="flex min-w-[200px] flex-col justify-center gap-2 border-t border-line bg-base/60 p-6 text-left sm:border-l sm:border-t-0">
- <span className={LABEL}>Driver</span>
+ <label className="flex min-w-[200px] flex-col justify-center gap-2 border-t border-line bg-base/60 p-6 text-left sm:border-l sm:border-t-0">
+ <span className={LABEL} aria-hidden="true">Driver</span>
<select
+ aria-label="Driver"
value={storeMode}Or simply wrap the span-and-select portion in a <label> element.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/hyperdb-demo/src/BenchmarkApp.tsx` around lines 181 - 199, The
select element with value={storeMode} and onChange={handleStoreModeChange} lacks
an associated accessible label for screen readers and accessibility tools. Wrap
both the span element containing "Driver" text and the select element in a
<label> element to properly associate the label with the form control, following
the same pattern used for other form controls in the component.
Comment on lines
+53
to
+71
| let connectionPromise: Promise<Connection> | null = null; | ||
|
|
||
| async function createConnection(databaseName: string): Promise<Connection> { | ||
| const module = await SQLiteAsyncESMFactory({ | ||
| locateFile: () => asyncSqlWasmUrl, | ||
| }); | ||
| const sqlite3 = SQLite.Factory(module) as WaSQLiteAPI; | ||
| const vfs = new OriginPrivateFileSystemVFS(); | ||
| sqlite3.vfs_register(vfs, true); | ||
|
|
||
| return { | ||
| sqlite3, | ||
| dbHandle: await sqlite3.open_v2(databaseName), | ||
| }; | ||
| } | ||
|
|
||
| function getConnection(databaseName: string): Promise<Connection> { | ||
| connectionPromise ??= createConnection(databaseName); | ||
| return connectionPromise; |
There was a problem hiding this comment.
Reset cached connection state when initialization fails.
If createConnection(...) rejects once, connectionPromise stays rejected and every later request fails until reload.
Suggested fix
let connectionPromise: Promise<Connection> | null = null;
function getConnection(databaseName: string): Promise<Connection> {
- connectionPromise ??= createConnection(databaseName);
+ connectionPromise ??=
+ createConnection(databaseName).catch((error) => {
+ connectionPromise = null;
+ throw error;
+ });
return connectionPromise;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/hyperdb-demo/src/wa-sqlite-worker.ts` around lines 53 - 71, The
createConnection promise caching in the getConnection function causes permanent
failures if the initial connection attempt rejects, since the rejected promise
is cached and never retried. To fix this, add error handling to the
connectionPromise assignment in the getConnection function that resets
connectionPromise back to null when createConnection rejects, allowing
subsequent calls to attempt a fresh connection instead of returning the cached
rejected promise.
Comment on lines
+109
to
+131
| self.addEventListener("message", (event: MessageEvent<WorkerRequest>) => { | ||
| const request = event.data; | ||
|
|
||
| void (async () => { | ||
| try { | ||
| const connection = await getConnection(request.databaseName); | ||
|
|
||
| if (request.type === "exec") { | ||
| await execSQL(connection, request.sql, request.params); | ||
| self.postMessage({ id: request.id, ok: true }); | ||
| } else { | ||
| const rows = await readValues(connection, request.sql, request.values); | ||
| self.postMessage({ id: request.id, ok: true, rows }); | ||
| } | ||
| } catch (error) { | ||
| self.postMessage({ | ||
| id: request.id, | ||
| ok: false, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| })(); | ||
| }); |
There was a problem hiding this comment.
Serialize worker requests on the shared SQLite connection.
Current message handling runs requests concurrently; SQL calls can interleave on one DB handle and break expected statement/transaction ordering.
Suggested fix
+self.addEventListener("message", (event: MessageEvent<WorkerRequest>) => {
+ const request = event.data;
+ requestQueue = requestQueue
+ .then(() => handleRequest(request))
+ .catch(() => {
+ // keep queue usable after failures
+ });
+});
+
+let requestQueue: Promise<void> = Promise.resolve();
+
+async function handleRequest(request: WorkerRequest): Promise<void> {
+ try {
+ const connection = await getConnection(request.databaseName);
+
+ if (request.type === "exec") {
+ await execSQL(connection, request.sql, request.params);
+ self.postMessage({ id: request.id, ok: true });
+ } else {
+ const rows = await readValues(connection, request.sql, request.values);
+ self.postMessage({ id: request.id, ok: true, rows });
+ }
+ } catch (error) {
+ self.postMessage({
+ id: request.id,
+ ok: false,
+ error: error instanceof Error ? error.message : String(error),
+ });
+ }
+}
-
-self.addEventListener("message", (event: MessageEvent<WorkerRequest>) => {
- const request = event.data;
-
- void (async () => {
- try {
- const connection = await getConnection(request.databaseName);
-
- if (request.type === "exec") {
- await execSQL(connection, request.sql, request.params);
- self.postMessage({ id: request.id, ok: true });
- } else {
- const rows = await readValues(connection, request.sql, request.values);
- self.postMessage({ id: request.id, ok: true, rows });
- }
- } catch (error) {
- self.postMessage({
- id: request.id,
- ok: false,
- error: error instanceof Error ? error.message : String(error),
- });
- }
- })();
-});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/hyperdb-demo/src/wa-sqlite-worker.ts` around lines 109 - 131, The
message event listener in the worker currently processes requests concurrently
without serialization, which causes SQL operations to interleave on the same
database connection. To fix this, implement a request queue mechanism that
serializes all incoming requests. Create a queue to hold pending WorkerRequest
objects and a processing loop that dequeues and processes requests sequentially.
The event listener should add requests to the queue instead of immediately
executing the async operation, ensuring that each call to getConnection,
execSQL, and readValues completes before the next request begins execution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes