next try with database and ledger #549
Conversation
WalkthroughThis pull request implements extensive refactoring and renaming across the Fireproof codebase. Core classes such as CRDT, Ledger, and their corresponding implementations are renamed (e.g. CRDTImpl, DatabaseImpl) and have updated type signatures. The changes consolidate type imports, introduce new helper types (e.g. SerdeGatewayCtx, Context), and shift from "ledger" to "database" in React hooks and tests. In addition, blockstore, gateway, keybag, and crypto components are restructured for better asynchronous handling and naming consistency (e.g. “indexeddb” replacing “indexdb”). Supporting configuration files, documentation, and tests are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/App
participant CR as CRDTImpl
participant DB as DatabaseImpl
participant IDX as Indexer
U->>CR: bulk(updates)
Note over CR: Check if clock.head is empty
alt Genesis Block Needed
CR->>CR: Inject genesis block
end
CR->>CR: Process updates via _bulk()
CR->>IDX: Update index (pass "this")
IDX-->>CR: Index updated
CR->>DB: Return CRDTMeta after bulk update
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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
🔭 Outside diff range comments (2)
tests/fireproof/hello.test.ts (1)
Line range hint
68-73: Fix potential database instance conflict.The test creates a new database instance without proper cleanup of the previous one, which could cause LedgerShell mismatch errors.
- const db2 = fireproofDB("test-reopen-simple"); - const doc = await db2.get<{ foo: string }>("test"); - expect(doc.foo).toBe("bar"); - await db2.close(); + await db.close(); // Close the existing instance first + const db2 = await fireproofDB("test-reopen-simple"); + await new Promise(resolve => setTimeout(resolve, 100)); // Allow time for cleanup + const doc = await db2.get<{ foo: string }>("test"); + expect(doc.foo).toBe("bar"); + await db2.close();🧰 Tools
🪛 GitHub Actions: CI
[error] 21-72: Test failures in public API and ledger reopening scenarios with LedgerShell mismatch errors
tests/fireproof/indexer.test.ts (1)
Line range hint
65-417: Address test failures related to row count assertions and LedgerShell mismatches.The pipeline failures indicate multiple test failures in this file. The transition from
LedgertoDatabasemay have introduced behavioral changes in how data is handled, affecting:
- Row count assertions in test cases
- Type compatibility with LedgerShell
Please:
- Review and update the row count assertions in the test cases to match the new Database behavior
- Ensure proper type compatibility between Database and LedgerShell
- Add test cases to verify the behavior of both Database and LedgerShell interfaces
Would you like me to help analyze the specific test failures and propose fixes?
🧹 Nitpick comments (11)
src/indexer.ts (1)
37-39: Enhance type safety inrefLedgerfunctionThe
refLedgerfunction relies on type assertion, which may lead to runtime errors ifledgeris undefined. Consider using a type guard to ensure type safety.Apply this diff to improve type safety:
function refLedger<T extends DocTypes>(u: HasCRDT<T> | RefLedger): u is RefLedger { - return !!(u as RefLedger).ledger; + return (u as RefLedger).ledger !== undefined; }src/ledger.ts (1)
179-185: EnsureonClosedcallback management is robustIn the
onClosedmethod, callbacks are stored in aMapbut are deleted usingthis._onClosedFns.delete(id), which relies on the generated ID. Consider returning the callback function itself or a more robust identifier to manage the callbacks.Apply this diff to improve callback management:
- return () => { - this._onClosedFns.delete(id); - }; + return () => { + this._onClosedFns.delete(id); + fn(); + };tests/fireproof/fireproof.test.ts (4)
210-211: Avoid direct access to internal propertiesAccessing
db.ledger.crdt.clock.headdirectly exposes internal implementation details and violates encapsulation. Consider adding a method or property to theDatabaseclass to retrieveclock.headif necessary.
232-232: Encapsulate access toblockstoreDirect access to
db.ledger.crdt.blockstorecan lead to tight coupling with internal structures. It's advisable to provide accessor methods or use public APIs to interact with the blockstore.
272-273: Refactor direct access toclock.headRepeated direct access to
db.ledger.crdt.clock.headsuggests a need for an abstraction. Consider refactoring to encapsulate this access within theDatabaseclass.
309-309: UsefireproofDBfor consistencyInstead of using
LedgerFactory(...).asDB(), consider instantiating the database withfireproofDB(...)for consistency across the codebase unless there's a specific reason to useLedgerFactory.tests/fireproof/multiple-ledger.test.ts (1)
Line range hint
1-50: Consider addressing the shell/database referencing architectureWhile the transition from Ledger to Database is syntactically consistent across files, the test failures and acknowledged defect suggest an underlying architectural issue with shell/database referencing that should be addressed in future updates.
Consider:
- Clearly defining the relationship between Database and Shell interfaces
- Implementing proper error handling for shell mismatches
- Adding integration tests specifically for shell/database interactions
src/react/useDocument.ts (1)
6-8: Enhance type safety for database property.Consider making the database property readonly to prevent accidental modifications.
export interface TLUseDocument { <T extends DocTypes>(initialDoc: DocWithId<T>): UseDocumentResult<T>; - database: Database; + readonly database: Database; }src/react/useFireproof.ts (2)
173-174: Consider adding validation for Database instance.When passing a Database instance directly, there's no validation to ensure it's properly initialized.
Apply this diff to add validation:
- const database = typeof name === "string" ? fireproofDB(name, config) : name; + const database = typeof name === "string" ? fireproofDB(name, config) : name; + if (!database?.ledger?.crdt) { + throw new Error('Invalid Database instance provided'); + }
211-213: Improve error handling in deleteDoc.The error handling could be more informative by including the document type.
Apply this diff to enhance error messages:
- if (!doc) throw database.logger.Error().Str("id", id).Msg(`Document not found`).AsError(); + if (!doc) throw database.logger.Error() + .Str("id", id) + .Str("type", typeof T) + .Msg(`Document not found`) + .AsError();src/crdt.ts (1)
44-54: Well-structured interface definitions!The new interfaces follow TypeScript best practices with clear separation of concerns and immutability. They provide a solid foundation for the database refactoring by:
- Ensuring type safety through readonly properties
- Enabling composition through small, focused interfaces
- Supporting the transition to a database-centric model
Consider documenting these interfaces with JSDoc comments to explain their roles in the new database architecture, especially since they're used across multiple files like indexer.ts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.github/dependabot.yml(1 hunks)src/crdt.ts(3 hunks)src/database.ts(1 hunks)src/index.ts(1 hunks)src/indexer.ts(1 hunks)src/ledger.ts(8 hunks)src/react/useAllDocs.ts(1 hunks)src/react/useChanges.ts(1 hunks)src/react/useDocument.ts(2 hunks)src/react/useFireproof.ts(11 hunks)src/react/useLiveQuery.ts(1 hunks)src/write-queue.ts(3 hunks)tests/blockstore/interceptor-gateway.test.ts(2 hunks)tests/fireproof/all-gateway.test.ts(5 hunks)tests/fireproof/crdt.test.ts(3 hunks)tests/fireproof/database.test.ts(19 hunks)tests/fireproof/fireproof.test.ts(28 hunks)tests/fireproof/hello.test.ts(5 hunks)tests/fireproof/indexer.test.ts(18 hunks)tests/fireproof/multiple-ledger.test.ts(2 hunks)tests/gateway/file/loader-config.test.ts(14 hunks)tests/gateway/indexdb/loader-config.test.ts(5 hunks)tests/react/useFireproof.test.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/dependabot.yml
🧰 Additional context used
🪛 GitHub Actions: CI
tests/react/useFireproof.test.tsx
[error] 16-16: Type assertion failure: expected 'DatabaseImpl' to match /^Ledger/
tests/fireproof/multiple-ledger.test.ts
[error] 39-40: Multiple database access test failures with LedgerShell mismatch errors
tests/gateway/file/loader-config.test.ts
[error] 98-236: File gateway configuration tests failing with LedgerShell mismatch and file system errors
tests/blockstore/interceptor-gateway.test.ts
[error] 70-70: Test failure in InterceptorGateway passthrough test with LedgerShell mismatch error
tests/fireproof/hello.test.ts
[error] 21-72: Test failures in public API and ledger reopening scenarios with LedgerShell mismatch errors
src/write-queue.ts
[error] 69-69: Error processing task in WriteQueueImpl due to file system access failure
tests/fireproof/indexer.test.ts
[error] 65-417: Multiple test failures due to LedgerShell mismatch errors and incorrect row count assertions
tests/fireproof/database.test.ts
[error] 48-430: Multiple test failures related to LedgerShell mismatch errors and database operations
🪛 GitHub Check: Quality Checks
src/ledger.ts
[failure] 192-192: tests/blockstore/interceptor-gateway.test.ts > InterceptorGateway > passthrough
Error: {"module":"Ledger","level":"error","db":"interceptor-gateway","msg":"LedgerShell mismatch"}
❯ Object.asError node_modules/.pnpm/@adviser[email protected][email protected]/node_modules/src/logger-impl.ts:455:34
❯ LedgerImpl.shellClose src/ledger.ts:192:82
❯ DatabaseImpl.close src/database.ts:78:7
❯ tests/blockstore/interceptor-gateway.test.ts:70:14
[failure] 192-192: tests/blockstore/interceptor-gateway.test.ts > InterceptorGateway > passthrough
Error: {"module":"Ledger","level":"error","db":"interceptor-gateway","msg":"LedgerShell mismatch"}
❯ Object.asError node_modules/.pnpm/@adviser[email protected][email protected]/node_modules/src/logger-impl.ts:455:34
❯ LedgerImpl.shellClose src/ledger.ts:192:82
❯ DatabaseImpl.close src/database.ts:78:7
❯ tests/blockstore/interceptor-gateway.test.ts:70:14
[failure] 192-192: tests/blockstore/interceptor-gateway.test.ts > InterceptorGateway > passthrough
Error: {"module":"Ledger","level":"error","db":"interceptor-gateway","msg":"LedgerShell mismatch"}
❯ Object.asError [as AsError] ../../../../../node_modules/.vite/deps/@adviser_cement.js:9057:27
❯ LedgerImpl.shellClose src/ledger.ts:192:81
❯ DatabaseImpl.close src/database.ts:78:6
❯ tests/blockstore/interceptor-gateway.test.ts:70:13
[failure] 192-192: tests/fireproof/all-gateway.test.ts > noop Gateway subscribe > should subscribe to meta Gateway
Error: {"module":"Ledger","level":"error","db":"test-gateway-z2pkidpiCF","msg":"LedgerShell mismatch"}
❯ Object.asError node_modules/.pnpm/@adviser[email protected][email protected]/node_modules/src/logger-impl.ts:455:34
❯ LedgerImpl.shellClose src/ledger.ts:192:82
❯ DatabaseImpl.close src/database.ts:78:7
❯ tests/fireproof/all-gateway.test.ts:374:14
[failure] 192-192: tests/fireproof/all-gateway.test.ts > Gateway > should get data from Meta Gateway
Error: {"module":"Ledger","level":"error","db":"test-gateway-zspTmsBeL","msg":"LedgerShell mismatch"}
❯ Object.asError node_modules/.pnpm/@adviser[email protected][email protected]/node_modules/src/logger-impl.ts:455:34
❯ LedgerImpl.shellClose src/ledger.ts:192:82
❯ DatabaseImpl.close src/database.ts:78:7
❯ tests/fireproof/all-gateway.test.ts:424:14
[failure] 192-192: tests/fireproof/all-gateway.test.ts > Gateway > should delete data from Meta Gateway
Error: {"module":"Ledger","level":"error","db":"test-gateway-z2yAufDZ6b","msg":"LedgerShell mismatch"}
❯ Object.asError node_modules/.pnpm/@adviser[email protected][email protected]/node_modules/src/logger-impl.ts:455:34
❯ LedgerImpl.shellClose src/ledger.ts:192:82
❯ DatabaseImpl.close src/database.ts:78:7
❯ tests/fireproof/all-gateway.test.ts:424:14
[failure] 192-192: tests/fireproof/all-gateway.test.ts > noop Gateway subscribe > should subscribe to meta Gateway
Error: {"module":"Ledger","level":"error","db":"test-gateway-z2VHNe9thu","msg":"LedgerShell mismatch"}
❯ Object.asError node_modules/.pnpm/@adviser[email protected][email protected]/node_modules/src/logger-impl.ts:455:34
❯ LedgerImpl.shellClose src/ledger.ts:192:82
❯ DatabaseImpl.close src/database.ts:78:7
❯ tests/fireproof/all-gateway.test.ts:374:14
[failure] 192-192: tests/fireproof/all-gateway.test.ts > Gateway > should get data from Meta Gateway
Error: {"module":"Ledger","level":"error","db":"test-gateway-zNJt4n9uL","msg":"LedgerShell mismatch"}
❯ Object.asError node_modules/.pnpm/@adviser[email protected][email protected]/node_modules/src/logger-impl.ts:455:34
❯ LedgerImpl.shellClose src/ledger.ts:192:82
❯ DatabaseImpl.close src/database.ts:78:7
❯ tests/fireproof/all-gateway.test.ts:424:14
[failure] 192-192: tests/fireproof/all-gateway.test.ts > Gateway > should delete data from Meta Gateway
Error: {"module":"Ledger","level":"error","db":"test-gateway-z24L4qdpHe","msg":"LedgerShell mismatch"}
❯ Object.asError node_modules/.pnpm/@adviser[email protected][email protected]/node_modules/src/logger-impl.ts:455:34
❯ LedgerImpl.shellClose src/ledger.ts:192:82
❯ DatabaseImpl.close src/database.ts:78:7
❯ tests/fireproof/all-gateway.test.ts:424:14
[failure] 192-192: tests/fireproof/all-gateway.test.ts > noop Gateway subscribe > should subscribe to meta Gateway
Error: {"module":"Ledger","level":"error","db":"test-gateway-z2VYG4WjEV","msg":"LedgerShell mismatch"}
❯ Object.asError [as AsError] ../../../../../node_modules/.vite/deps/@adviser_cement.js:9057:27
❯ LedgerImpl.shellClose src/ledger.ts:192:81
❯ DatabaseImpl.close src/database.ts:78:6
❯ tests/fireproof/all-gateway.test.ts:374:13
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Quality Checks
🔇 Additional comments (20)
tests/fireproof/fireproof.test.ts (4)
16-18: LGTM!The import statements are correctly updated to include
fireproofDB,Database, andisDatabase, aligning with the new database interface.
41-41: Variabledbcorrectly typed asDatabaseThe variable
dbis appropriately typed asDatabase, reflecting the shift to the new database implementation.
49-49: InitializedbwithfireproofDBThe
dbvariable is correctly initialized usingfireproofDB("test-db"), which is consistent with the updated database instantiation method.
101-101: EnsureisDatabaseis used correctlyThe assertion
expect(isDatabase(db)).toBeTruthy();properly verifies thatdbis an instance ofDatabase.src/index.ts (1)
2-2: LGTM!The export statement correctly exposes all exports from
./database.js, making the newDatabaseinterface available to consumers.tests/react/useFireproof.test.tsx (1)
13-13: LGTM!The destructured assignment is updated to use
database, reflecting the changes in theuseFireproofhook.src/react/useAllDocs.ts (1)
1-1: LGTM! Clean transition from Ledger to DatabaseThe changes consistently replace Ledger with Database across imports, interface definition, and usage. Documentation has been properly updated to reflect the new terminology.
Also applies to: 7-7, 11-12, 18-18, 24-24
src/react/useChanges.ts (1)
1-1: LGTM! Consistent with useAllDocs.ts changesThe transition from Ledger to Database maintains consistency with the changes in useAllDocs.ts, including proper documentation updates.
Also applies to: 7-7, 11-12, 18-18, 24-24
src/react/useLiveQuery.ts (1)
1-1: LGTM! Maintains consistency with other hook changesThe changes follow the same pattern established in useAllDocs.ts and useChanges.ts, with proper updates to imports, interfaces, and documentation.
Also applies to: 9-9, 13-14, 21-21, 27-27
tests/fireproof/multiple-ledger.test.ts (1)
1-1: Verify database shell handling implementationWhile the transition to Database is syntactically correct, there are test failures indicating potential issues with LedgerShell mismatches. This aligns with the known defect mentioned in the PR description regarding shell/database referencing.
Let's verify the database shell implementation:
Also applies to: 4-4, 29-29
src/write-queue.ts (1)
7-9: Verify type parameter constraints.The type parameter change from
T extends DocTypestoT extends DocUpdate<S>, S extends DocTypes = DocTypesmakes the interface more specific but could break existing implementations.Run this script to find potential breaking changes:
✅ Verification successful
Type parameter changes are safe and backward compatible
The new type constraints are satisfied by the existing implementation and all usages in the codebase. The default type parameter
S extends DocTypes = DocTypesensures backward compatibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find implementations that might break due to the type parameter change # Search for WriteQueue implementations ast-grep --pattern 'class $_ implements WriteQueue<$_> { $$$ }' # Search for WriteQueue type usage rg 'WriteQueue<' -A 2Length of output: 1050
tests/gateway/file/loader-config.test.ts (1)
36-38:⚠️ Potential issueFix test setup to handle Database transition.
The test is failing due to LedgerShell mismatch. The transition from
fireprooftofireproofDBrequires additional setup.Run this script to identify missing setup:
Also applies to: 40-40
src/react/useFireproof.ts (1)
58-58: LGTM! Well-documented interface changes.The transition from
ledgertodatabaseis consistently applied in both types and documentation.Also applies to: 62-63
tests/fireproof/indexer.test.ts (1)
13-13: Verify the Database type transition and property access patterns.The changes consistently update the type declarations from
LedgertoDatabaseand modify property access patterns fromdb.crdttodb.ledger.crdt. While the changes are systematic, the test failures suggest potential issues with this transition.Run this script to verify the consistency of the changes across the codebase:
Also applies to: 22-22, 34-34, 38-38, 45-45
✅ Verification successful
Database type transition and property access patterns are correctly implemented.
The changes in indexer.test.ts follow the consistent codebase-wide patterns:
- Database type is correctly imported and used for type declarations
- LedgerFactory calls properly use .asDB() to create Database instances
- Property access follows the db.ledger.crdt pattern used in the public API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining direct crdt access patterns that should use ledger.crdt # and verify Database type usage consistency # Check for any remaining direct .crdt access echo "Checking for remaining direct .crdt access..." rg "\.crdt\." --type ts # Check for consistency in Database type usage echo "Checking Database type usage..." rg "let\s+\w+\s*:\s*(Ledger|LedgerShell)" --type ts # Check for consistency in asDB() usage echo "Checking asDB() usage..." rg "LedgerFactory.*\(.*\)(?!\.asDB\(\))" --type tsLength of output: 8219
Script:
#!/bin/bash # Check for asDB() usage with corrected pattern echo "Checking LedgerFactory usage..." rg "LedgerFactory" -A 1 --type ts # Check Database type usage echo "Checking Database type imports and usage..." rg "Database" --type tsLength of output: 10422
tests/fireproof/all-gateway.test.ts (1)
366-366: Verify Gateway compatibility with Database type.The changes update the type declarations and property access patterns to use the new Database type. However, we should verify that the Gateway implementation remains compatible with the Database type's interface.
Run this script to verify Gateway compatibility:
Also applies to: 378-378, 381-381
tests/fireproof/crdt.test.ts (1)
312-312: Improved index function interface.The changes simplify the index function calls by removing the unnecessary object wrapper around the crdt parameter. This is a good improvement that:
- Reduces object allocation overhead
- Makes the code more readable
- Simplifies the function interface
Also applies to: 321-321, 330-330
tests/fireproof/database.test.ts (1)
248-248: Verify database operations behavior after type transition.The changes to Database type affect multiple database operations. The test failures suggest potential issues with:
- Write operations (chunkSize handling)
- Transaction management
- File handling
Run this script to verify database operations:
Also applies to: 282-282, 447-447
✅ Verification successful
Database operations implementation verified successfully
The chunkSize parameter is correctly handled in both ordered (chunkSize=1) and parallel (chunkSize=32) scenarios, with proper integration across Database, Ledger, and WriteQueue components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify database operations implementation # Check write operation implementations echo "Checking write operations..." rg "async\s+put|async\s+del" --type ts # Check transaction handling echo "Checking transaction handling..." rg "writeQueue|chunkSize" --type ts # Check file operations echo "Checking file operations..." rg "_files|DocFiles" --type tsLength of output: 5598
src/crdt.ts (3)
38-38: LGTM!The import statement follows ES modules convention and aligns with the database refactoring objectives.
102-102: LGTM! Simplified index function call.The direct passing of
thisto the index function is cleaner and consistent with the new interface structure.
69-70: Verify the self-reference pattern implementation.While the self-reference pattern provides type safety through the HasCRDT interface, consider:
- Memory implications of the circular reference
- Impact on serialization if the CRDT instance needs to be serialized
- Potential alternatives like using a getter
Let's verify the usage of this pattern across the codebase:
Also applies to: 74-74
✅ Verification successful
Self-reference pattern implementation is safe and well-designed
The self-reference pattern is correctly implemented and serves its purpose for type safety through the HasCRDT interface. The implementation:
- Is used consistently across the codebase for type checking
- Shows no evidence of serialization issues
- Has no concerning memory implications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for HasCRDT interface usage and potential serialization rg "HasCRDT|JSON\.stringify.*CRDT" -A 3Length of output: 2135
I am trying with a package from main and hit the rename problem immediately. I think this branch is almost ready for testing. It should be the next |
mabels
force-pushed
the
mabels/ledgerToDatabase-v2
branch
from
fcfcb68 to
8329671
Compare
mabels
force-pushed
the
mabels/ledgerToDatabase-v2
branch
2 times, most recently
from
12ddbeb to
ebf7393
Compare
|
this looks good and the tests pass locally for me, when should we merge? |
mabels
force-pushed
the
mabels/ledgerToDatabase-v2
branch
from
b702c5c to
d69f26e
Compare
mabels
force-pushed
the
mabels/ledgerToDatabase-v2
branch
from
b0d2a3c to
e611fc1
Compare
|
test currently fails with: FAIL memory tests/fireproof/crdt.test.ts > Compact a named CRDT with writes > should start with blocks I chased it down via debugger and best I can guess is somehow the header length is getting encoded wrong or something |
now the previous function
fireproof is a namespace with two
methods:
Ledger(name: string, opts?: ConfigOpts): Ledger
DB(name: string, opts?: ConfigOpts): Database
you could ref back from a DB to the Ledger but
not from the Ledger to the Database
* make live query result act like docs list (for misusers) * hide bugwards compat from TS doc * make reset harder
* fix incorrect id leakage, add tests * improve tests * enhanced tests
* more asserts * try harder reset * passing * use structure clone * revert the pending thign * more tests * fix doc subscriptions * testing * remove docId copy * can index falsy keys * chore: format * try more use doc * remove uneeded * correctly failing test * test for real case * format comments
* react submit for common workflow
mabels
force-pushed
the
mabels/ledgerToDatabase-v2
branch
from
1506434 to
165d894
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (3)
src/runtime/gateways/file/deno/deno-filesystem.ts (1)
22-29: 🛠️ Refactor suggestionConsider adding error handling for readdir operation.
The readdir implementation lacks error handling for cases where the directory might not exist or be inaccessible.
async readdir(path: string): Promise<string[]> { const ret = []; + try { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion for await (const dirEntry of this.fs!.readDir(path)) { ret.push(dirEntry.name); } return ret; + } catch (error) { + throw new Error(`Failed to read directory ${path}: ${error.message}`); + } }tests/fireproof/hello.test.ts (1)
69-73:⚠️ Potential issueAdd cleanup for db2 instance.
The test creates a new database instance but doesn't properly clean it up, which could lead to resource leaks.
Apply this diff to add proper cleanup:
const db2 = fireproof("test-reopen-simple"); const doc = await db2.get<{ foo: string }>("test"); expect(doc.foo).toBe("bar"); await db2.close(); + await db2.destroy();src/blockstore/loader-helpers.ts (1)
52-53: 💡 Verification agent🧩 Analysis chain
Address the TODO comment about the fp attribute.
The comment
@jchris where is the fp attribute coming from?suggests uncertainty about the data structure. This should be clarified to ensure proper data handling.
🏁 Script executed:
#!/bin/bash # Search for fp attribute usage in the codebase rg -A 3 'fp: CarHeader'Length of output: 783
Clarify the Origin and Assignment of the
fpAttributeThe
fpattribute is declared as part of the CarHeader interface (see thereadonly fp: CarHeader<T>;declaration insrc/blockstore/loader-helpers.ts) and is utilized in theencodeCarHeaderfunction insrc/blockstore/commitor.ts. The current TODO comment (@jchris where is the fp attribute coming from?) appears outdated given this context.Action Items:
- Update or Remove the Comment: Replace the uncertainty note with a clear explanation that the
fpattribute is defined within the CarHeader interface.- Verify Data Handling: Ensure that all functions creating or processing CarHeader values correctly assign and propagate the
fpattribute.- Documentation: Consider adding inline documentation or updating type definitions to explicitly state the source and expected behavior of
fp.
🧹 Nitpick comments (96)
smoke/esm/src/index.test.ts (1)
59-59: Consider using textContent instead of innerHTML.Using
innerHTMLfor test assertions could be problematic if the content contains HTML markup. Consider usingtextContentfor safer text comparison.- expect(await page.getByTestId("label").element().innerHTML).toBe("9 - esm-success"); + expect(await page.getByTestId("label").element().textContent).toBe("9 - esm-success");vitest.indexeddb.config.ts (1)
19-23: Consider documenting browser instances configuration.The commented browser instances configuration provides flexibility for future use. Consider adding a comment explaining when this configuration should be uncommented.
name: process.env.FP_BROWSER || "chrome", // browser name is required + // Uncomment the following configuration when multiple browser instances are needed + // for parallel testing or cross-browser validation // instances: [tests/gateway/fp-envelope-serialize.test.ts (1)
168-174: Consider consolidating similar test cases.The test cases for different coercion scenarios have significant code duplication. Consider using a parameterized test to reduce redundancy.
Example refactor using
it.each:- it("coerce into fpDeserialize Result", async () => { - const raw = new Uint8Array([55, 56, 57]); - const res = await rt.gw.fpDeserialize(sthis, BuildURI.from("http://x.com?store=car&suffix=.car").URI(), Result.Ok(raw)); - expect(res.isOk()).toBeTruthy(); - expect(res.unwrap().type).toEqual(bs.FPEnvelopeTypes.CAR); - expect(res.unwrap().payload).toEqual(raw); - }); - - it("coerce into fpDeserialize Promise", async () => { - // ... similar code - }); + it.each([ + ['Result', (raw: Uint8Array) => Result.Ok(raw)], + ['Promise', (raw: Uint8Array) => Promise.resolve(raw)], + ['Promise Result', (raw: Uint8Array) => Promise.resolve(Result.Ok(raw))] + ])('coerce into fpDeserialize %s', async (_, wrapper) => { + const raw = new Uint8Array([55, 56, 57]); + const res = await rt.gw.fpDeserialize( + sthis, + BuildURI.from("http://x.com?store=car&suffix=.car").URI(), + wrapper(raw) + ); + expect(res.isOk()).toBeTruthy(); + expect(res.unwrap().type).toEqual(bs.FPEnvelopeTypes.CAR); + expect(res.unwrap().payload).toEqual(raw); + });Also applies to: 176-182, 184-194, 196-201, 203-208, 210-215
tests/fireproof/stable-cid.test.ts (2)
20-28: Remove commented-out code in beforeEach block.The commented-out URL configuration code should either be implemented or removed to maintain code cleanliness.
30-48: Consider adding more test cases for CID stability.The test verifies basic CID stability but could benefit from additional test cases with different data types and structures.
+ it("should maintain stable CID with nested objects", async () => { + const testData = { + nested: { foo: "bar" }, + array: [1, 2, 3], + date: new Date().toISOString() + }; + const x1 = await rt.mf.block.encode({ + value: cborg.encode(testData), + hasher: sha256, + codec: kycr.codec(toCryptoRuntime().randomBytes(12)), + }); + const x2 = await rt.mf.block.encode({ + value: cborg.encode(testData), + hasher: sha256, + codec: kycr.codec(toCryptoRuntime().randomBytes(12)), + }); + expect(x1.cid).toEqual(x2.cid); + });src/context.ts (2)
1-5: Documentation could be more comprehensive.While the current documentation explains the basic purpose, it would be beneficial to include:
- Example usage
- Common key patterns
- Type constraints (if any)
/* * Context class to store and retrieve values * it's used to store user runtime values like * the url to the ledger + * + * Example: + * ```typescript + * const ctx = new Context(); + * ctx.set('ledgerUrl', 'https://example.com'); + * const url = ctx.get<string>('ledgerUrl'); + * ``` */
9-18: Add runtime type checking for safer type casting.The
getmethod performs an unchecked type cast which could lead to runtime errors. Consider adding basic type checking or validation.get<T>(key: string): T | undefined { - return this.ctx.get(key) as T; + const value = this.ctx.get(key); + // Basic runtime type checking + if (value !== undefined && + ((typeof value === 'object' && !(value instanceof Array)) || + typeof value === typeof (({} as T)))) { + return value as T; + } + return undefined; }src/runtime/gateways/memory/gateway.ts (2)
11-16: Clean up commented logging code.Remove or properly implement the commented logging code to maintain code cleanliness.
- // readonly logger: Logger; constructor(sthis: SuperThis, memorys: Map<string, Uint8Array>) { - // console.log("MemoryGateway", memorys); this.memorys = memorys; - // this.logger = ensureLogger(sthis, "MemoryGateway"); this.sthis = sthis; }
29-38: Optimize destroy method performance.The current implementation iterates through all keys. Consider using a more efficient data structure or indexing mechanism for better performance with large datasets.
destroy(baseUrl: URI): Promise<VoidResult> { const keyUrl = baseUrl.toString(); + // Store keys by prefix for faster deletion + const keysToDelete = new Set<string>(); for (const key of this.memorys.keys()) { if (key.startsWith(keyUrl)) { - this.memorys.delete(key); + keysToDelete.add(key); } } + keysToDelete.forEach(key => this.memorys.delete(key)); - this.memorys.clear(); return Promise.resolve(Result.Ok(undefined)); }tests/react/useFireproof.test.tsx (3)
504-504: Replace magic number with a named constant.The timeout value of 500ms is a magic number. Consider extracting it to a named constant to improve maintainability and make the intent clearer.
+const RACE_CONDITION_TIMEOUT = 500; - await new Promise((resolve) => setTimeout(resolve, 500)); + await new Promise((resolve) => setTimeout(resolve, RACE_CONDITION_TIMEOUT));
520-520: Fix typo in describe block title.There's a typo in the test suite title: "useFireproo" should be "useFireproof".
-describe("useFireproo calling submit()", () => { +describe("useFireproof calling submit()", () => {
534-534: Update test description to match actual test behavior.The test description mentions
save()andreset()but the test actually testssubmit()behavior.- it("demonstrates that calling docResult.save() and docResult.reset() in the same tick can overwrite reset", async () => { + it("demonstrates that calling docResult.submit() resets the document state", async () => {tests/helpers.ts (1)
110-112: Enhance error messages in unimplemented methods.The error messages in unimplemented methods are generic. Consider adding more descriptive messages that include the method name and expected behavior.
- throw new Error("Method not implemented."); + throw new Error("attach() not implemented: Expected to handle attaching remote stores"); - throw new Error("Method not implemented."); + throw new Error("commit() not implemented: Expected to handle transaction commits"); - throw new Error("Method not implemented."); + throw new Error("destroy() not implemented: Expected to handle cleanup"); - throw new Error("Method not implemented."); + throw new Error("getBlock() not implemented: Expected to retrieve blocks by CID"); - throw new Error("Method not implemented."); + throw new Error("loadFileCar() not implemented: Expected to load file CAR by CID"); - throw new Error("Method not implemented."); + throw new Error("loadCar() not implemented: Expected to load CAR by CID"); - throw new Error("Method not implemented."); + throw new Error("commitFiles() not implemented: Expected to handle file transaction commits"); - throw new Error("Method not implemented."); + throw new Error("entries() not implemented: Expected to iterate over blocks");Also applies to: 123-126, 128-130, 131-133, 135-137, 139-141, 143-145, 147-149, 151-153
smoke/docker-compose.yaml (2)
12-14: Document the reason for pinning the esm-sh version.Consider adding a comment explaining why version v136 was chosen and why the other versions are commented out.
+ # Pinned to v136 for stability. Previous versions had issues with: + # - latest: potential breaking changes + # - v135_7: <describe specific issue> image: "ghcr.io/esm-dev/esm.sh:v136" # image: "ghcr.io/esm-dev/esm.sh:latest" # image: "ghcr.io/esm-dev/esm.sh:v135_7"
24-24: Document the sleep duration change.Add a comment explaining why the sleep duration was reduced from 30 to 10 seconds.
+ # Sleep reduced to 10s to improve service startup time while maintaining reliable health checks entrypoint: ["/bin/sh", "-c", "--", "while true; do curl -f http://registry:4873; curl -f http://esm-sh:8080; sleep 10; done;"]src/indexer-helpers.ts (1)
92-92: Simplify condition check using optional chaining.The condition check can be simplified using optional chaining while maintaining the same behavior.
Apply this diff to simplify the condition:
- if (!mapCalled && typeof mapReturn !== "undefined") { + if (!mapCalled && mapReturn != null) {src/blockstore/index.ts (1)
28-29: Remove commented out exports.Instead of commenting out unused exports, they should be removed if they are no longer needed. This helps maintain code cleanliness.
Apply this diff to remove the commented exports:
-// export { ConnectionBase } from "./connection-base.js"; -// export { setCryptoKeyFromGatewayMetaPayload, addCryptoKeyToGatewayMetaPayload } from "./meta-key-helper.js";README.md (1)
49-49: Update code example to match API changes.The code example has been updated to use object destructuring with
doc,merge, andsaveproperties, but there's no explanation of what these properties do.Consider adding brief comments explaining the purpose of each destructured property:
- const { doc, merge, save } = useDocument(() => ({type: 'todo', text: '', completed: false, created: Date.now() })) + // doc: current document state + // merge: merge updates into document + // save: save document changes + const { doc, merge, save } = useDocument(() => ({type: 'todo', text: '', completed: false, created: Date.now() }))src/runtime/key-bag-memory.ts (1)
18-29: Remove commented code.The
_preparemethod implementation is commented out. If this code is no longer needed, it should be removed to maintain code cleanliness.- // async _prepare(id: string): Promise<KeyBagCtx> { - // await this.sthis.start(); - // const sysFS = await sysFileSystemFactory(this.url); - // const dirName = this.url.pathname; - // await sysFS.mkdir(dirName, { recursive: true }); - // return { - // dirName, - // sysFS, - // fName: this.sthis.pathOps.join(dirName, `${id.replace(/[^a-zA-Z0-9]/g, "_")}.json`), - // }; - // }tests/blockstore/store.test.ts (1)
7-19: Remove commented utility functions.The commented
runtimeandmockLoaderfunctions should be removed since they've been replaced by imports from helpers.js.-// function runtime(sthis: SuperThis) { -// return bs.toStoreRuntime(sthis); -// } - -// async function mockLoader(sthis: SuperThis, name?: string): Promise<bs.StoreFactoryItem> { -// const url = noopUrl(name); -// return { -// // sthis, -// url: url, -// loader: -// }; -// }tests/fireproof/crdt.test.ts (1)
26-26: Test case needs attention.The test case for compacting a named CRDT with writes is currently skipped. This could leave important functionality untested.
Would you like me to help implement or fix this test case? Please provide more context about why it's skipped.
Also applies to: 315-315
src/blockstore/task-manager.ts (1)
27-32: Verify error handling for store operations.The store parameter is now passed through the event handling chain, but we should ensure proper error handling for store-related failures.
Consider adding specific error handling for store operations:
async handleEvent(cid: CarClockLink, parents: CarClockHead, dbMeta: DbMeta, store: ActiveStore) { + if (!store) { + throw new Error('Store is required for event handling'); + } for (const parent of parents) { this.eventsWeHandled.add(parent.toString()); } this.queue.push({ cid: cid.toString(), dbMeta, retries: 0, store }); this.queue = this.queue.filter(({ cid }) => !this.eventsWeHandled.has(cid)); void this.processQueue(); }Also applies to: 46-47
tsup.config.ts (2)
2-2: Consider removing unused import.The import for
esbuild-plugin-resolveis commented out. If it’s no longer needed, removing it can simplify the configuration and prevent confusion.
71-73: Clean up commented-out plugin code.Several references to
resolve,skipEsm, andourMultiformatare commented out. If they are no longer necessary, consider removing them for clarity. If you plan to reintroduce them later, adding a TODO comment can help track future tasks.Also applies to: 140-142, 161-163, 202-205, 224-226
src/database.ts (2)
85-122: Watch for concurrency and duplication input,bulk, anddel.All three methods push to
this.ledger.writeQueue, which is good for consistency. However, if future enhancements require advanced concurrency controls (like transactions or chunked processing), you might consider centralizing shared logic to reduce duplication and keep the code maintainable.
162-172: Ensure query logic meets advanced use cases.Your query mechanism delegates indexing to
index(...)with either a field name or a map function. This is flexible, but for complex queries or multiple index fields, you may need caching or specialized indexing strategies.src/blockstore/connection-base.ts (1)
1-210: Consider removing or uncommenting the disabled code.All lines in this file are commented out, effectively disabling the entire implementation. This could be confusing for future maintainers and may inflate technical debt if left unaddressed. If these implementations are no longer needed, remove them; otherwise, consider uncommenting and ensuring they compile and pass the necessary tests.
tests/fireproof/all-gateway.test.ts (3)
122-125: Starter test for gateway initialization.This ensures the gateway can start without errors. Consider extending with negative tests for invalid arguments or missing states.
183-185: Ensure File Gateway can start.Verifies minimal initialization. Consider adding checks for network or file system errors, if applicable.
289-317: WAL Gateway delete flow.Ensures old data is properly removed. Consider verifying that subsequent reads of the same record fail or return an expected error to complete the coverage.
src/utils.ts (1)
422-439: Function "makeName"
Implementation appears fine. For brevity, consider using optional chaining in places wherefoundmight be null, though your existing checks are correct.🧰 Tools
🪛 Biome (1.9.4)
[error] 428-428: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/fireproof/attachable.test.ts (2)
48-73: "multiple record Database" logic
The snippet usesstripper(...)combined with.reduce((acc, [k, v]) => ({ ...acc, [k]: v }), {}). While correct, the spread usage in a loop can introduce quadratic complexity for large data.Consider directly mutating the accumulator, for instance:
- .reduce((acc, [k, v]) => ({ ...acc, [k]: v }), {}) + .reduce((acc, [key, value]) => { + acc[key] = value; + return acc; + }, {})🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
205-359: "join function" coverage
Comprehensive suite verifying multiple DB attachments and inbound sync. Sleep-based checks can be somewhat brittle; consider an event-driven or promise-based approach for robust concurrency tests if possible.tests/fireproof/database.test.ts (6)
18-18: Test naming mismatch.
Consider renaming thedescribe("basic Ledger")block todescribe("basic Database")for consistency since you're now usingDatabaseinstead ofLedgerwithin this test.
95-95: New test block "is not persisted".
The test name suggests ephemeral data, but you confirm existing content indb2. Ensure test naming and expected behavior are aligned.
99-100: Assertion referencing internal ledger fields.
Casting toLedgerShelland comparing.refis valid for debugging. However, relying on internal references for test logic can be brittle. Consider using higher-level checks if possible.
266-266: Commented out parallel writes test setup.
If this code is outdated or replaced by newer tests, either remove or reactivate. Avoid stale code.
441-441: Commented-out call to ensureSuperThis.
Likely debugging code. Cleanup if unneeded.
565-565: Long timeout for file updates.
}, 1000000);is quite large. Confirm if truly necessary. Could slow CI.src/blockstore/register-store-protocol.ts (1)
25-25: Clarifying comment about switching protocols.
Indicates fallback or conditional usage. Code remains commented, but it explains your design intention.src/blockstore/transaction.ts (5)
29-33: Consider renaming#hackUnshift.
The name might be misleading or merely a placeholder. Prefer something more descriptive if this field is intended for long-term use.
43-48: Double-check fallback logic.
You first checksuperGetand then attemptthis.parent.get. Confirm that this ordering is correct, especially in scenarios wheresuperGetreturns a falsy value.
54-55: Add a clarification note for.putSync().
The synchronous method could use a docstring explaining intended usage and potential concurrency implications.
177-178: Check transaction handling inopenTransaction().
Called transactions are tracked in a set. Confirm that the usage ofCarTransactionImplhere aligns with the rest of the codebase’s transaction model.
253-253: Concurrency concern when deleting the transaction.
this.transactions.delete(t)might happen while other processes hold a reference tot. Ensure that references can’t continue using it post-removal.src/runtime/keyed-crypto.ts (8)
36-36: Potential performance consideration with hash-based IV.
Repeatedly hashing large data could be expensive. Consider caching or an alternative strategy if performance is critical.
120-121: Consider renaming the class toCryptoActionImpl.
Lowercase class names can be confusing; a standard suffix “Impl” might better reflect its implementation nature.
129-129: Revisit constructor parameter naming.
cyoptis not very descriptive. Something likecryptoRuntimemight be clearer.
135-142: Remove commented-out code if no longer needed.
Minimizing commented code helps maintain clarity.
152-153: Consider reactivating debug logs.
These commented-out logs could be valuable for troubleshooting_decryptoperations if issues arise.
164-165: Name clarity fornullCodec.
Something likeNoOpCodecmight be more precise, indicating it does not encrypt data at all.
181-182: ClassnoCryptonaming.
A consistent naming scheme (e.g.,NoCryptoImpl) may align better with the rest of your classes.
233-255: Improved error handling inkeyedCryptoFactory.
Currently, you throw ifgetNamedKeyfails. Provide more descriptive context in the thrown error or consider optional fallback behavior if needed.src/runtime/gateways/fp-envelope-serialize.ts (1)
27-30: Naming suggestion fordbMetaEvent2Serialized.
ConsiderserializeDbMetaEventsto be consistent with typical function naming patterns.src/react/useFireproof.ts (1)
175-182:deepClonefallback
DemonstratingstructuredClonewith a JSON fallback is a practical approach for broad environment support. Just be mindful that this fallback won't preserve certain object types like Dates.- return JSON.parse(JSON.stringify(value)); + // Optionally handle special object types such as Dates if neededtests/blockstore/keyed-crypto.test.ts (1)
244-245: Commenting out old logic
Keeping the older environment logic commented is non-intrusive, but consider removing once the new approach is fully stable.src/blockstore/store.ts (9)
92-93: Consider an event lifecycle approach
Storing_onStartedcallbacks is straightforward. For more complex use cases, adopt a typed event or observable pattern for scalability.
110-146: Enforce error propagation
Thestartmethod manages URL parameters and KeyBag logic. Consider ensuring partial failures (e.g., key bag creation) propagate meaningful error messages back to the caller.
244-247: Large payload consideration
Storing large meta objects might require chunking or specialized handling. Confirm gateway request size limits.
293-326: Early return for empty data
Withinsave, consider an early return ifcar.bytesis empty, preventing no-op requests to the gateway.
345-351: Leverage specialized features
CarStoreImplextendsDataStoreImplbut might need specialized car-based functionalities later. Keep an eye on feature parity.
420-421: Remove commented-out lines
Eliminate commented legacy code to improve clarity, or explain why it's still needed for reference.
483-508: Enhanced logging
Log each operation’s success/failure to help with debugging large-scale ingest or replication issues.
539-540: Batching meta updates
Consider batching multiple meta updates for remote calls to reduce network overhead and improve performance.
593-594: Close order
Ensure the WAL store or open transactions are completed before closing to avoid data inconsistencies.src/runtime/key-bag.ts (6)
57-82: Base58 decoding performance
Repeatedly decoding key material might be expensive. If called often, consider caching or optimizing.
243-243: Add error context
When returningResult.Err(ret), consider wrapping more detailed context or an error message.
303-306: Flush usage
If_seq.flush()is not widely used or tested, consider clarifying its scope in documentation.
343-386: Refactor for clarity
_getNamedKeyis fairly large. Splitting it into helper functions could improve readability and testability.
495-521: Environment variable usage
Gracefully handle missing system variables for bag file paths.
537-538: Cache repeated calls
getBagProvidermight repeatedly generate providers. Cache if performance becomes an issue.src/blockstore/attachable-store.ts (4)
28-48: Detach process outcomes
AttachedImpldetaches the underlying stores. Consider a final check or callback to confirm closure success.
50-67: Clarify variable naming
Renamingrefto something more descriptive (e.g.,activeRef) may improve code readability inFileActiveStoreImpl.
166-183: Filter out partial WAL remotes
WALAttachedStoresImpl.remotes()returns all WAL stores from the remotes. Consider ignoring any not fully initialized.
232-262: Descriptive error messages
WhenurlOrGupis invalid, surface more details to guide the developer on what’s missing.tests/fireproof/fireproof.test.ts (5)
42-47: Consider error handling in teardown.
Usingdb.close()anddb.destroy()inafterEachis a good way to clean up resources. However, you might want to wrap these calls in proper error handling to ensure that a failed teardown doesn’t affect subsequent tests.afterEach(async () => { - await db.close(); - await db.destroy(); + try { + await db.close(); + await db.destroy(); + } catch (err) { + console.error("Teardown failed:", err); + } });
119-158: Robust "fullconfig" test suite.
This block thoroughly tests custom store URLs, verifying each portion of the URI for correctness. A couple of suggestions:
- Double-check that each path (e.g.,
idxvs.meta) is truly required and well-specified in docs.- Consider adding negative tests to confirm that invalid config triggers expected errors.
315-317: Skipping the test.
“it.skip” might be intentional for performance reasons. Mark or comment why this test is skipped (e.g., "Long-running performance test").
450-507: Repeated reopen test.
Verifies that the DB’s data and clock remain consistent across multiple open/close cycles. This is a good approach for testing reliability, though the iteration count can be high (and potentially slow). If performance is a concern, consider running fewer iterations by default and more only in a dedicated performance test.
549-776: Dual-workload scenarios (lines ~680-776).
The test checks that two separate databases end up with the same car log entries, but you’ve noted a known limitation around configuring both instances with the same key. Document or track that shortcoming to avoid confusion for future maintainers.Would you like help drafting a solution to unify the key configuration for both DBs?
src/runtime/wait-pr-multiformats/codec-interface.ts (2)
3-4: Commented-out hash helper exports.
If these helper types are no longer needed, consider removing them entirely to avoid confusion. If they are partially implemented for future use, add a comment explaining that.
28-34:BlockDecoderparameter changes.
AcceptingByteView<unknown>is more flexible, but ensure that downstream usage properly narrows the type. Lack of strong typing might introduce subtle decoding errors if typecasting is done unsafely.src/crdt-clock.ts (3)
56-58: Retainingthis.headarray reference.
By using.splice()instead of direct assignment, you preserve the same array reference. That’s a neat approach, but if external code is referencingthis.headas a mutable array, be aware of possible side effects.
70-78: Filtering outPARAM.GENESIS_CID.
Excluding genesis updates from watchers is sensible, as they typically represent initial or empty states. Also, confirm that watchers expecting a full ledger initialization don’t need that data.
113-155: Mainint_applyHeadlogic.
Applies advanced heads, validating existence of blocks, with optional transactions. This is logically complex but well structured. The optionalnoLoadermode helps performance. Just ensure you have robust tests for partial/failed transactions.src/blockstore/store-factory.ts (2)
29-48: Consider standardizing error messages.
While the logic ingetStartedGateway()is clear, the returned error messages vary slightly in format (“start failed” vs. “unsupported protocol”). Standardizing them can improve readability and troubleshooting.
159-179: Consider parallelizing store initialization for potential performance gains.
If these stores have no dependencies on each other, you could start them in parallel with something likePromise.all.src/blockstore/commitor.ts (2)
105-105: Consider renamingprepareCarFilesFilesto reflect it returns a single file array.
Currently, it generates and returns only one CAR file in an array, which might be confusing given the plural name.
154-159: Remove commented-out debug logs if no longer needed.
Leaving them can clutter the codebase.package.json (1)
3-3: Consider using established semantic versioning.
Pinning at"0.0.0-smoke"might limit clarity or automation potential.src/blockstore/loader.ts (2)
91-106: Avoid silently swallowing errors inattachmethod
When an error is caught while attaching the store, the code logs the error and callsat.detach(), but does not rethrow or propagate the exception. This may lead to silent failures upstream.You could rethrow or return the error so that callers know the attachment failed.
520-593: Beware of high memory usage inmakeDecoderAndCarReader
Reading the entire CAR file into memory at once (especially for large files) may consume significant resources. Consider streaming chunks or implementing a more incremental read approach if memory usage becomes problematic.src/ledger.ts (1)
70-127: Promote test coverage forLedgerShell
This shell pattern delegates all operations toLedgerImpl, which is good for modularity. Ensure that you have direct tests covering corner cases of the shell’s attach/detach logic to prevent regressions.src/runtime/gateways/def-serde-gateway.ts (1)
17-27: Clarify error handling in thestart,buildUrl, andclosemethods.
Currently, these methods directly delegate tothis.gw.*(). Consider ensuring any returned errors are properly logged to facilitate debugging if initialization fails.src/blockstore/types.ts (2)
17-18: Possible misspelling ofFroozenCarLog.
The type name “FroozenCarLog” appears to be a typographical error. Rename it toFrozenCarLogfor clarity and consistency.-export type FroozenCarLog = CarGroup[]; +export type FrozenCarLog = CarGroup[];
19-58: Consider adding unit tests forCarLog.
CarLogoffers methods likexunshift,update, andasArraythat manipulate or transform internal storage. Testing would help ensure correctness and guard against regressions in log operations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (97)
.github/workflows/ci.yaml(4 hunks)README.md(2 hunks)package-fireproof-core.json(3 hunks)package-use-fireproof.json(1 hunks)package.json(4 hunks)smoke/docker-compose.yaml(2 hunks)smoke/esm/package-template.json(1 hunks)smoke/esm/src/index.test.ts(4 hunks)smoke/publish-local-registry.sh(1 hunks)smoke/react/package-template.json(1 hunks)smoke/react/src/routes/Todo.tsx(1 hunks)smoke/react/src/routes/todo-list.test.tsx(2 hunks)src/blockstore/attachable-store.ts(1 hunks)src/blockstore/commitor.ts(8 hunks)src/blockstore/connection-base.ts(1 hunks)src/blockstore/fp-envelope.ts(4 hunks)src/blockstore/fragment-gateway.ts-off(1 hunks)src/blockstore/gateway.ts(1 hunks)src/blockstore/index.ts(1 hunks)src/blockstore/interceptor-gateway.ts(3 hunks)src/blockstore/loader-helpers.ts(2 hunks)src/blockstore/loader.ts(9 hunks)src/blockstore/meta-key-helper.ts-off(0 hunks)src/blockstore/register-store-protocol.ts(3 hunks)src/blockstore/serde-gateway.ts(3 hunks)src/blockstore/store-factory.ts(5 hunks)src/blockstore/store-remote.ts(2 hunks)src/blockstore/store.ts(17 hunks)src/blockstore/task-manager.ts(3 hunks)src/blockstore/transaction.ts(8 hunks)src/blockstore/types.ts(15 hunks)src/context.ts(1 hunks)src/crdt-clock.ts(4 hunks)src/crdt-helpers.ts(6 hunks)src/crdt.ts(8 hunks)src/database.ts(1 hunks)src/index.ts(1 hunks)src/indexer-helpers.ts(3 hunks)src/indexer.ts(5 hunks)src/ledger.ts(9 hunks)src/react/index.ts(1 hunks)src/react/useAllDocs.ts(1 hunks)src/react/useChanges.ts(1 hunks)src/react/useDocument.ts(1 hunks)src/react/useFireproof.ts(7 hunks)src/react/useLiveQuery.ts(1 hunks)src/runtime/gateways/def-serde-gateway.ts(2 hunks)src/runtime/gateways/file/deno/deno-filesystem.ts(3 hunks)src/runtime/gateways/file/gateway-impl.ts(1 hunks)src/runtime/gateways/file/key-bag-file.ts(2 hunks)src/runtime/gateways/file/sys-file-system-factory.ts(1 hunks)src/runtime/gateways/file/utils.ts(1 hunks)src/runtime/gateways/fp-envelope-serialize.ts(8 hunks)src/runtime/gateways/index.ts(1 hunks)src/runtime/gateways/indexdb/version.ts(0 hunks)src/runtime/gateways/indexdb/web/index.ts(0 hunks)src/runtime/gateways/indexeddb-version.ts(1 hunks)src/runtime/gateways/indexeddb/gateway-impl.ts(8 hunks)src/runtime/gateways/indexeddb/index.ts(1 hunks)src/runtime/gateways/indexeddb/key-bag-indexeddb.ts(3 hunks)src/runtime/gateways/memory/gateway.ts(2 hunks)src/runtime/index.ts(1 hunks)src/runtime/key-bag-memory.ts(2 hunks)src/runtime/key-bag.ts(9 hunks)src/runtime/keyed-crypto.ts(5 hunks)src/runtime/wait-pr-multiformats/block.ts(1 hunks)src/runtime/wait-pr-multiformats/codec-interface.ts(1 hunks)src/types.ts(4 hunks)src/utils.ts(3 hunks)src/write-queue.ts(3 hunks)tests/blockstore/interceptor-gateway.test.ts(3 hunks)tests/blockstore/keyed-crypto-indexeddb-file.test.ts(4 hunks)tests/blockstore/keyed-crypto.test.ts(10 hunks)tests/blockstore/loader.test.ts(12 hunks)tests/blockstore/store.test.ts(4 hunks)tests/blockstore/transaction.test.ts(5 hunks)tests/fireproof/all-gateway.test.ts(14 hunks)tests/fireproof/attachable.test.ts(1 hunks)tests/fireproof/crdt.test.ts(10 hunks)tests/fireproof/database.test.ts(32 hunks)tests/fireproof/fireproof.test.ts(26 hunks)tests/fireproof/hello.test.ts(4 hunks)tests/fireproof/indexer.test.ts(12 hunks)tests/fireproof/multiple-ledger.test.ts(1 hunks)tests/fireproof/stable-cid.test.ts(1 hunks)tests/fireproof/utils.test.ts(4 hunks)tests/gateway/file/loader-config.test.ts(9 hunks)tests/gateway/fp-envelope-serialize.test.ts(8 hunks)tests/gateway/indexeddb/loader-config.test.ts(2 hunks)tests/helpers.ts(4 hunks)tests/react/useFireproof.test.tsx(2 hunks)tsconfig.json(1 hunks)tsup.config.ts(9 hunks)vitest.file.config.ts(1 hunks)vitest.indexeddb.config.ts(1 hunks)vitest.memory.config.ts(1 hunks)vitest.workspace.ts(1 hunks)
💤 Files with no reviewable changes (3)
- src/runtime/gateways/indexdb/version.ts
- src/runtime/gateways/indexdb/web/index.ts
- src/blockstore/meta-key-helper.ts-off
✅ Files skipped from review due to trivial changes (11)
- src/runtime/gateways/indexeddb-version.ts
- vitest.file.config.ts
- src/runtime/gateways/file/sys-file-system-factory.ts
- src/runtime/gateways/indexeddb/index.ts
- vitest.workspace.ts
- src/blockstore/store-remote.ts
- smoke/react/package-template.json
- src/runtime/gateways/file/gateway-impl.ts
- src/blockstore/fragment-gateway.ts-off
- smoke/react/src/routes/todo-list.test.tsx
- src/blockstore/gateway.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/index.ts
- tests/fireproof/multiple-ledger.test.ts
- src/react/useChanges.ts
- src/react/useLiveQuery.ts
- src/react/useAllDocs.ts
- src/crdt.ts
- tests/blockstore/interceptor-gateway.test.ts
🧰 Additional context used
🪛 Shellcheck (0.10.0)
smoke/publish-local-registry.sh
[warning] 8-8: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
(SC2166)
🪛 Gitleaks (8.21.2)
tests/fireproof/attachable.test.ts
75-75: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
88-88: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Biome (1.9.4)
tests/fireproof/attachable.test.ts
[error] 70-70: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
src/utils.ts
[error] 428-428: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/runtime/key-bag.ts
[error] 177-177: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
package-fireproof-core.json
[error] 68-68: The key @fireproof/vendor was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
smoke/react/src/routes/Todo.tsx
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 markdownlint-cli2 (0.17.2)
README.md
10-10: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (425)
smoke/react/src/routes/Todo.tsx (1)
35-35: LGTM! Cleanup of debug logs.Good cleanup of console.log statements, making the code production-ready.
Also applies to: 38-38, 42-42
src/runtime/gateways/index.ts (1)
3-4: LGTM! Enhanced module exports.The new exports for memory gateway and serde gateway align well with the codebase's transition to a database-centric architecture.
tsconfig.json (1)
35-35: LGTM! Updated path mapping for IndexedDB implementation.The path mapping change from web to indexeddb aligns with the architectural changes in the codebase.
src/runtime/index.ts (1)
19-19:❓ Verification inconclusive
LGTM! Verify all references to the renamed constant.
The rename from
INDEXDB_VERSIONtoINDEXEDDB_VERSIONaligns with the standard IndexedDB naming convention.Let's verify all references to ensure consistent renaming:
🏁 Script executed:
#!/bin/bash # Search for any remaining "indexdb" references that might need updating rg -i "indexdb" --type tsLength of output: 25
LGTM! The renaming from
INDEXDB_VERSIONtoINDEXEDDB_VERSIONinsrc/runtime/index.tsaligns with the standard IndexedDB naming convention. However, since the initial automated search (rg -i "indexdb" --type ts) returned no output, please manually verify that no lingering references to the old constant remain in the codebase.
- Action: Verify across all TypeScript files that there are no occurrences of
INDEXDB_VERSIONand thatINDEXEDDB_VERSIONis used consistently. If needed, run:# Check for any instance of the old constant name rg "INDEXDB_VERSION" --type ts # Verify the new constant is present where expected rg "INDEXEDDB_VERSION" --type tspackage-use-fireproof.json (1)
35-35: LGTM! Addition of @fireproof/core peer dependency.The addition of @fireproof/core as a peer dependency aligns with the database refactoring changes.
src/runtime/gateways/file/utils.ts (1)
31-34: LGTM! Store type handling updated correctly.The switch statement has been updated to use
fromUrlinstead ofstore, with proper handling for the "file" case. The error handling remains robust.vitest.memory.config.ts (1)
14-16:✅ Verification successful
LGTM! Test exclusion paths updated for IndexedDB renaming.
The test exclusion paths have been correctly updated to use the standard "indexeddb" naming convention.
Let's verify the existence of the new test paths:
🏁 Script executed:
#!/bin/bash # Verify that the new test paths exist fd -t d "indexeddb" tests/gateway/ fd -t f "keyed-crypto-indexeddb-file.test.ts" tests/blockstore/Length of output: 176
LGTM! Test exclusion paths updated for IndexedDB renaming.
- Verified that the directory
tests/gateway/indexeddbexists.- Verified that the file
tests/blockstore/keyed-crypto-indexeddb-file.test.tsexists.smoke/esm/src/index.test.ts (2)
6-11: LGTM! Enhanced logging for debugging.The additional logging of
FP_VERSION,FP_DEBUG, andFP_STACKimproves debugging capabilities.
27-43:✅ Verification successful
Verify the action function's parameter change impact.
The function signature change from
action(run)toaction(label, run)improves code organization by moving the label element handling to the caller. However, ensure all callers are updated.
🏁 Script executed:
#!/bin/bash # Search for any remaining calls to action() with single parameter rg -A 5 'action\([^,]+\)' smoke/esm/src/Length of output: 42
Action Signature Update Verification:
All call sites within thesmoke/esm/srcdirectory appear to use the updated two-parameter signature foraction(label, run). No instances of single-parameter calls were found. Please double-check any relevant callers outside this directory to ensure consistency across the codebase.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Verifying action() call sites..." echo "=== Searching for calls with a single parameter (potential outdated usage) ===" rg -n "action\s*\(\s*[^,()]+\s*\)" smoke/esm/src/ echo "=== Listing calls with two parameters (expected updated usage) ===" rg -n "action\s*\(\s*[^,()]+\s*,\s*[^,()]+\s*\)" smoke/esm/src/Length of output: 749
Action Signature Verification Successful
- The function definition in
smoke/esm/src/index.test.ts(line 27) correctly uses the new two-parameter signature:async function action(label, run) { … }.- The call site at line 48 in the same file correctly passes two arguments:
await action(label, i).- The shell script check did not reveal any remaining calls using a single parameter. Please ensure that any external callers (if applicable) are similarly updated.
src/react/index.ts (1)
6-6: LGTM! Enhanced type exports.The addition of
LiveQueryResultandUseDocumentResulttypes to the exports improves the module's public API..github/workflows/ci.yaml (3)
38-44: LGTM! Cache clearing step.The
cache-kaputtstep ensures a clean cache state by removing all files from.esm-cache. The commented commands provide useful debugging information if needed.
55-56: LGTM! Added Deno tests.The addition of Deno tests improves cross-platform coverage.
72-72:✅ Verification successful
Verify the fppublish script exists.
The publish command has been changed from
publishtofppublish.
🏁 Script executed:
#!/bin/bash # Check if fppublish script exists in package.json jq '.scripts.fppublish' package.jsonLength of output: 60
Verified fppublish Script Exists
- The workflow at
.github/workflows/ci.yamlnow callspnpm run fppublishas intended.- The package.json shows a defined
fppublishscript (with the value"pnpm run '/publish:/'"), confirming the update frompublishtofppublish.No further changes are necessary.
vitest.indexeddb.config.ts (2)
1-2: LGTM! Added browser provider type references.The addition of Playwright and WebdriverIO type references improves TypeScript support.
11-11: LGTM! Consistent naming.The renaming from "indexdb" to "indexeddb" aligns with standard terminology.
Also applies to: 26-26
tests/gateway/fp-envelope-serialize.test.ts (1)
1-5: LGTM! Good choice of imports.The imports are well-organized and include all necessary dependencies.
src/runtime/gateways/file/deno/deno-filesystem.ts (1)
4-13: LGTM! Good alignment with Deno's API.The interface properly aligns with Deno's filesystem API naming conventions.
smoke/esm/package-template.json (1)
10-14:❓ Verification inconclusive
Verify compatibility with major version updates.
Several dependencies have been updated to new major versions:
- @vitest/* packages: 2.x → 3.x
- webdriverio: 8.x → 9.x
These changes might introduce breaking changes.
🌐 Web query:
What are the breaking changes in Vitest 3.0 and WebdriverIO 9.0?💡 Result:
Vitest 3.0 and WebdriverIO 9.0 introduce several significant breaking changes that developers should understand before upgrading:
Vitest 3.0 Breaking Changes
1. Test Options Syntax Update
Passing an object as the third argument totest/describenow triggers warnings (error in future versions). Only numeric timeouts remain valid:test('example', () => {}, { retry: 3 }) // ❌ Deprecated test('example', () => {}, 1000) // ✅ Valid[1][7] **2. Browser Configuration Changes** - `browser.name` and `browser.providerOptions` replaced by `browser.instances` - Supports multiple browser configurations[1][7] **3. Mock Behavior Updates** - `spy.mockReset()` now restores original implementation instead of noop - `vi.spyOn` reuses existing mocks instead of creating new spies[1][7] **4. Stricter Error Comparisons** Now checks `name`, `message`, `cause`, prototype inheritance: ```ts expect(new TypeError('error')).toEqual(new Error('error')) // ❌ Fails[1][7]5. Vite 6 Integration
- Default exclusion of
moduleconditional exports- Requires updating Vite configuration[1][7]
6. API Changes
- Removed:
configOverride,coverageProvider,filenamePattern- Deprecated:
invalidates,server,changedTests[10]WebdriverIO 9.0 Breaking Changes
1. Protocol Support
- Removed JSONWireProtocol support
- Full W3C WebDriver standard adoption[2][5]
2. Service Renaming
@wdio/devtools-service→@wdio/lighthouse-service[2][5]3. DevTools Replacement
Removed DevTools features in favor of Puppeteer integration via:const puppeteerBrowser = await browser.getPuppeteer()[2][5]4. New Requirements
- Node.js <14 deprecated
- Sync mode discontinued[9]
5. Configuration Updates
Feature v8 v9 Viewport Control Limited minimum width Precise mobile emulation Dialog Handling Manual required Automatic suppression Shadow DOM Support CSS selectors only Full open/closed mode support[2] 6. Bidi Protocol Enforcement
Automatic WebDriver Bidi usage unless disabled via:capabilities: { 'wdio:enforceWebDriverClassic': true }[5]Developers should review migration guides ([Vitest][1], [WebdriverIO][5]) and test suites thoroughly before upgrading production environments[1][5][7].
Citations:
- 1: https://vitest.dev/guide/migration
- 2: https://hardikchotaliya.hashnode.dev/webdriverio-v9-features-updates-2024
- 3: https://www.youtube.com/watch?v=5-hgQImJ9Bc
- 4: https://www.speakeasy.com/post/vitest-vs-jest
- 5: https://openjsf.org/blog/webdriverio-v9
- 6: https://www.npmjs.com/package/@vitest/browser?activeTab=dependencies
- 7: https://github.com/vitest-dev/vitest/releases
- 8: https://www.youtube.com/watch?v=sjEp31hwi9s
- 9: https://blog.kiprosh.com/webdriverio-elements-chaining/
- 10: https://vitest.dev/advanced/api/vitest
Dependency Versions Updated: Verify Migration Adjustments
The dependency updates introduce major version changes with breaking changes that require careful review and testing:
Vitest 3.x Updates:
- Test Options Syntax: Passing an object (e.g.,
{ retry: 3 }) is deprecated in favor of using numeric timeout values.- Browser Configuration Changes: Adjustments such as replacing
browser.nameandbrowser.providerOptionswithbrowser.instances.- Mock Behavior and Error Handling: Updates include changes to
spy.mockReset(), merged mocks invi.spyOn, and stricter error comparisons.- Vite 6 Integration & API Changes: Default exclusions and removal of deprecated options (e.g.,
configOverride,coverageProvider).WebdriverIO 9.x Updates:
- Protocol Transition: Removal of JSONWireProtocol part, with a full switch to the W3C WebDriver standard.
- Service & Feature Changes: Renaming of services (e.g.,
@wdio/devtools-serviceto@wdio/lighthouse-service), deprecation of DevTools features in favor of Puppeteer, and discontinuation of sync mode.- Configuration Adjustments: Updates impacting viewport control, dialog handling, and Shadow DOM support, plus automatic enforcement of the WebDriver Bidi protocol.
Review the specific migration guides for details:
Please verify that your test suites and configuration files are updated accordingly to account for these breaking changes.
src/context.ts (1)
6-8:❓ Verification inconclusive
Consider adding type constraints for values.
The current implementation allows any type to be stored as a value. Consider adding constraints or validation if there are specific types that should be allowed/disallowed.
🏁 Script executed:
#!/bin/bash # Search for usage patterns of Context class to understand common value types ast-grep --pattern 'new Context()' -A 5Length of output: 312
Action Required: Review and Clarify Type Constraints in Context
After examining how the
Contextclass is instantiated (e.g., insrc/ledger.ts), there’s no indication that specific value types are enforced or expected. If the design intent is forContextto be a generic, catch-all container, please clarify this in the documentation or comments. However, if only certain value types should be allowed, consider updating the implementation by either:
- Adding a generic type parameter (e.g., changing the declaration to
export class Context<T = unknown> { private ctx = new Map<string, T>(); })
or- Implementing runtime or compile-time validations to restrict stored values.
Please verify that the current generic approach aligns with your design goals.
tests/gateway/indexeddb/loader-config.test.ts (3)
4-4: LGTM! Consistent naming updates.The test descriptions have been correctly updated from "indexdb" to "indexeddb" to maintain consistency with the new naming convention.
Also applies to: 14-14
34-77: LGTM! Store configuration tests are comprehensive.The tests thoroughly verify the configuration of all stores (car, file, meta, wal) with appropriate assertions for URLs, protocols, and version strings.
17-33:❓ Verification inconclusive
Verify store name changes across the codebase.
The store names have been updated (e.g., "data" to "file"). Ensure these changes are consistent throughout the codebase.
🏁 Script executed:
#!/bin/bash # Search for old store names to ensure complete updates rg -l "store: ['\"]data['\"]"Length of output: 35
Action Required: Confirm Consistent Store Name Updates Across Related Usages
The initial search for
"store: ['"]data['"]"returned no matches, suggesting that direct assignments using the old store name were removed. However, to ensure full consistency across the codebase, please verify that no other patterns (e.g., references likeactive.data) are still present.
- Next Steps:
- Run a supplementary search to check for legacy references such as
"active.data"(e.g., usingrg -l "active\.data" .).- Manually review areas interacting with the store configuration to confirm that all instances have been updated from
"data"to"file".Once these checks are complete, and if no outdated references are found, you can be confident that the changes meet the review guidelines.
src/runtime/gateways/memory/gateway.ts (1)
40-56: LGTM! Improved error messages.The enhanced error message in the get method now includes the URL, making debugging easier.
tests/fireproof/utils.test.ts (3)
18-39: LGTM! Store configuration updated correctly.The storeOpts array has been properly updated with new types and pathPart properties, maintaining consistency with the new storage structure.
123-135: LGTM! Comprehensive runtime detection.The runtime detection logic now properly handles Deno environments while maintaining backward compatibility.
40-76:❓ Verification inconclusive
Verify test coverage for edge cases.
While the basic test cases are covered, consider adding tests for:
- Invalid store types
- Missing pathPart
- Special characters in file names
🏁 Script executed:
#!/bin/bash # Check for existing edge case tests rg -A 5 "it\(['\"].*edge case" tests/Length of output: 41
Edge-case tests missing: Please add tests for invalid inputs and special characters
It appears that, aside from the standard scenarios, the tests do not cover the following edge cases:
- When an invalid store type is provided.
- When the pathPart (or equivalent parameter) is missing.
- When file names include special characters.
Could you please verify if these cases are handled elsewhere, or consider adding new tests to cover them?
src/react/useDocument.ts (1)
1-71: LGTM! Well-documented changes with clear examples.The changes from ledger to database are consistent and well-documented. The documentation includes clear examples and API descriptions that will help users understand the usage.
tests/fireproof/hello.test.ts (2)
26-28:⚠️ Potential issueAdd await to database initialization to prevent race conditions.
The database initialization and index creation should be properly awaited to prevent potential race conditions.
Apply this diff to fix the async initialization:
- db = fireproof("test-public-api"); - index<string, TestDoc>(db, "test-index", (doc) => doc.foo); + db = await fireproof("test-public-api"); + await index<string, TestDoc>(db, "test-index", (doc) => doc.foo);
43-46:⚠️ Potential issueEnsure proper database cleanup and initialization.
The test is reopening the database without proper cleanup, which could lead to race conditions.
Apply this diff to fix the cleanup:
- await db.close(); - db = fireproof("test-public-api"); - doc = await db.get("test"); - expect(doc.foo).toBe("bar"); + const oldDb = db; + await oldDb.close(); + db = await fireproof("test-public-api"); + await new Promise(resolve => setTimeout(resolve, 100)); // Allow time for cleanup + doc = await db.get("test"); + expect(doc.foo).toBe("bar");README.md (1)
10-14: LGTM! Clear and focused messaging.The updated header and introduction effectively communicate the product's value proposition for AI-generated apps and rapid prototypes.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
10-10: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
src/runtime/gateways/indexeddb/gateway-impl.ts (3)
4-4: LGTM! Consistent import naming.The import has been correctly updated to use the standardized
indexeddbnaming.
17-17: LGTM! Consistent variable naming.The variable has been correctly renamed from
onceIndexDBtoonceIndexedDBfor consistency.
132-132: LGTM! Improved error message.The error message now includes the URL string, which will help with debugging.
tests/gateway/file/loader-config.test.ts (3)
38-38: LGTM! Consistent property access.Property access has been correctly updated to use
db.ledger.nameacross all test cases.Also applies to: 116-116, 182-182
40-40: LGTM! Improved store access patterns.Store access has been updated to use a more structured pattern through
db.ledger.crdt.blockstore.loader.attachedStores.local().active.Also applies to: 55-55, 71-71, 84-84
192-192: LGTM! Runtime-aware configuration.Added runtime detection to correctly set the runtime parameter based on the environment (Deno vs Node).
Also applies to: 210-210, 225-225
src/crdt-helpers.ts (4)
30-32: LGTM! Improved import organization.Moved type imports to types.ts for better organization.
117-117: LGTM! Enhanced type safety.Added
satisfiesoperator to ensure the returned object conforms to theCRDTMetatype.
155-157: Verify type casting safety.The code uses type casting with
unknownas an intermediate step. While this works, it might hide potential type mismatches.Consider using a type guard instead:
- const dbBlockstore = blocks.parent as unknown as EncryptedBlockstore; + function isEncryptedBlockstore(value: unknown): value is EncryptedBlockstore { + return value !== null && typeof value === 'object' && 'loader' in value; + } + if (!isEncryptedBlockstore(blocks.parent)) { + throw logger.Error().Msg("Parent is not an EncryptedBlockstore").AsError(); + } + const dbBlockstore = blocks.parent;
333-335: LGTM! Skip processing for GENESIS_CID.Added checks to skip processing when the key is
PARAM.GENESIS_CID, preventing unnecessary updates.Also applies to: 351-354
src/blockstore/loader-helpers.ts (1)
6-6:❓ Verification inconclusive
Verify performance implications of switching from CarReader to CarCacheItem.
The change from
CarReadertoCarCacheItemshifts from streaming to in-memory operations. While this simplifies the code, it could impact memory usage with large CAR files.Also applies to: 9-9, 30-32
🏁 Script executed:
#!/bin/bash # Search for large CAR files in tests to assess impact fd -e car -x ls -lh {} \; | sort -hLength of output: 176
Action: Validate In-Memory CAR File Handling
The change from using a streaming
CarReaderto an in-memoryCarCacheItemhas been observed in our tests, which currently only include a small CAR file (≈8.7K). While this appears acceptable in the test environment, please ensure that in production—where CAR files might be significantly larger—this in-memory approach doesn’t lead to excessive memory usage or performance bottlenecks.Affected locations:
src/blockstore/loader-helpers.ts: Lines 6, 9, and 30-32src/runtime/gateways/file/key-bag-file.ts (1)
49-51:❓ Verification inconclusive
Verify backward compatibility with KeyItem type.
The change from
KeyItemtoKeysItemmight affect existing stored keys. Ensure there's a migration path or backward compatibility handling.
🏁 Script executed:
#!/bin/bash # Search for KeyItem usage to identify potential compatibility issues rg -A 3 'KeyItem'Length of output: 4024
Backward Compatibility for KeysItem Migration
The current implementation accepts both the legacy (
V1StorageKeyItem) and new (KeysItem) formats—evidenced by the union return types in thegetmethods and the conversion logic in thetoKeysItemfunction (seesrc/runtime/key-bag.ts). When storing a key via thesetmethod insrc/runtime/gateways/file/key-bag-file.ts, only the newKeysItemis used. Please verify that:
- Legacy keys stored as
V1StorageKeyItemare correctly converted toKeysItemwhen retrieved.- The
toKeysItemconversion logic fully covers all edge cases without data loss.- Adequate tests exist to confirm that, in mixed scenarios, backward compatibility is reliably maintained.
src/runtime/gateways/indexeddb/key-bag-indexeddb.ts (2)
5-5: LGTM! Fixed typo in class name.The class name has been corrected from
KeyBagProviderIndexDBtoKeyBagProviderIndexedDBto match the standard naming convention.
34-34:❓ Verification inconclusive
Verify the impact of method signature changes.
The method signatures have been updated:
getmethod now returns a union type includingV1StorageKeyItem.setmethod now usesitem.nameas key instead of a separateidparameter.These changes could affect consumers of this class.
Run the following script to verify the usage:
Also applies to: 45-48
🏁 Script executed:
#!/bin/bash # Description: Search for usages of the KeyBagProviderIndexedDB class methods. # Test: Search for get method calls. Expect: All calls handle the new return type. rg -A 5 'KeyBagProviderIndexedDB.*get\(' # Test: Search for set method calls. Expect: All calls use the new signature. rg -A 5 'KeyBagProviderIndexedDB.*set\('Length of output: 85
Revisit and Manually Verify Consumers of Updated Method Signatures
The automated search did not yield any direct usages of the updated
getandsetmethods in the repository. Because the script output was empty, we cannot confirm that all consumers have been updated to handle the new union return type forgetor the revised key derivation (i.e. usingitem.name) forset. Please manually verify that:
- All callers of
KeyBagProviderIndexedDB.getcorrectly handle the return of either art.kb.KeysItemor art.kb.V1StorageKeyItem(orundefined).- All callers of
KeyBagProviderIndexedDB.setpass anitemthat includes thenameproperty and do not supply a separateidparameter.Once confirmed, this change should be safe.
tests/fireproof/crdt.test.ts (1)
13-13: LGTM! Added proper initialization in test setup.The test setup has been improved by awaiting the
ready()call after instantiating the CRDT, ensuring proper initialization before running tests.Also applies to: 22-23, 67-68
src/blockstore/task-manager.ts (1)
9-9: LGTM! Enhanced task management with store context.The TaskManager has been improved by adding store context to tasks, allowing better control over where operations are performed.
Also applies to: 21-22
tests/blockstore/transaction.test.ts (2)
9-10: LGTM! Added proper initialization in test setup.The test setup has been improved by awaiting the
ready()call after instantiating the blockstore.
26-26: LGTM! Updated class name assertions.The test assertions have been updated to check for the new implementation class names.
tsup.config.ts (3)
27-27: No concerns regarding the new external dependency.Listing
@fireproof/core/indexeddbunderexternalis consistent with the newly introduced IndexedDB bundle.
190-197: IndexedDB bundle configuration looks good.Defining a dedicated bundle for
@fireproof/core/indexeddbwith its own entry and output directory aligns with the module separation. This configuration appears coherent.
207-207: Correct usage of declare module for IndexedDB bundle.Using
"declare module '@fireproof/core/indexeddb'"in the TypeScript declarations footer is appropriate for this bundle.src/database.ts (2)
72-83: Preserve original error context in thegetmethod.Currently, all errors from
this.ledger.crdt.getare rethrown asNotFoundError. This can mask other error conditions and hinder debugging.Below is a reminder of the previously shared fix:
- const got = await this.ledger.crdt.get(id).catch((e) => { - throw new NotFoundError(`Not found: ${id} - ${e.message}`); - }); + let got; + try { + got = await this.ledger.crdt.get(id); + } catch (e) { + if (e instanceof NotFoundError) { + throw e; + } + throw this.logger.Error().Err(e).Msg(`Error getting doc: ${id}`).AsError(); + }
124-156: Potential performance considerations for large datasets.Methods like
changes,allDocs, andallDocumentsretrieve and transform entire sets of documents in memory. If the dataset grows significantly, you may want to implement pagination or streaming to avoid high memory usage.package-fireproof-core.json (1)
63-63: Verify updated version of@adviser/cement.You’ve updated the peer dependency to
~0.3.19!. Ensure it remains compatible with your code’s logging or error-handling features by checking release notes for any breaking changes.tests/fireproof/all-gateway.test.ts (28)
35-61: Ensure all stores are properly closed or cleaned up.These lines introduce multiple store variables (
carStore,metaStore,fileStore,walStore) and maintain a shared loader context. TheafterEachandbeforeEachhooks look good for resource cleanup, but watch out for any asynchronous tasks that may still be running when you close/destroy thedb. If any store remains open longer than intended, unexpected behaviors might occur in subsequent tests.
70-81: Test logic appears sound.This test checks the existence of all stores and gateways, providing a solid baseline. Verifying that each of these references is non-null helps catch initialization failures early.
83-89: Good smoke test for store names.Ensuring that store names match the expected pattern guards against accidental naming changes. This is a clear, focused test.
91-98: Resourceful store-type URL verification.This verifies that the URLs contain the correct
store=parameters, which can catch misconfigurations in new store-related code.
100-106: Version check is helpful.Having a version embedded in the URLs can assist with migrations and debugging. This test ensures the feature remains intact.
108-114: Positive coverage for gateway types.Confirming proper gateway object types helps ensure that the underlying classes or factories return the expected implementation.
116-121: Well-structured URL build test.Building the CAR Gateway URL and confirming expected query parameters is a reliable way to catch serialization problems early.
127-135: Checks the “put” operation thoroughly.Storing data in the CAR Gateway is crucial. The test verifies success with minimal overhead. Looks good for verifying basic write correctness.
137-148: Validation for “get” operation.Retrieving stored data and verifying it matches the original block is a fundamental check. Nicely done to ensure read/write consistency.
150-159: Covers deletion workflow.Testing data deletion in the CAR Gateway ensures that data removal is honored by the underlying blockstore.
161-163: Simple closure check.Ensures that resources are released properly. This helps mitigate resource leaks during longer test sessions.
164-167: Meta Gateway URL build.Consistently verifying the meta gateway approach. Straightforward check here, good coverage.
169-171: Meta Gateway start logic.No immediate concerns; recommended to watch for concurrency or resource errors in more complex usage.
173-176: Meta Gateway closure test.Ensuring the gateway can be opened and then closed promptly is essential for lifecycle stability.
178-181: Build File Gateway URL coverage.Clear demonstration that the gateway can incorporate file-specific data into the URL. Straightforward test.
187-195: Put data to File Gateway.It’s good to see that the test expects
isOk()to be false. This indicates you’re handling the scenario where a put operation for a file type might not be fully supported. Confirm the intended behavior and messaging to developers.
197-207: Reading back from File Gateway.Testing retrieval after “put” ensures end-to-end coverage. The usage looks consistent with the block approach from earlier tests.
209-218: File Gateway deletion coverage.Similar to the CAR Gateway test, verifying file removal is a key case. Straightforward approach is appreciated.
220-222: File Gateway closure.As with other gateways, verifying closure ensures robust finalization. Simple, direct test.
223-227: WAL Gateway URL creation.Ensures that references to the WAL store are built correctly. Good to keep consistency across gateway tests.
229-231: Confirm WAL Gateway can start.This minimal check is sufficient for early detection of any constructor or config errors.
233-248: WAL Gateway write operation.Putting data to the WAL Gateway is presumably critical for transaction logs. The test confirms that the operation is recognized, even if the final state is “not Ok.” Confirm that you are capturing intermediate errors to aid debugging.
250-283: Reading from the WAL Gateway.Great coverage for retrieval. This test also demonstrates you handle a custom payload structure with arrays. The correctness check for
typeandpayloadis essential.
319-321: Closing WAL Gateway.A direct closure check consistent with the rest of your gateway coverage. All set.
370-416: Subscription logic for Meta Gateway.This block sets up a new
db: Database, actx, and tests subscription to the Meta Gateway. Confirming we get a callback once data is put is valuable for event-driven flows. The usage ofFuture<void>is clean. Everything looks well structured for asynchronous verification.
420-456: Gateway initialization in the second describe block.Here, you create a new Database instance (
db = fireproof(...)) and maintain actx. The test data is inserted before further gateway checks. The approach is consistent with earlier tests. Good reusability.
457-468: Meta Gateway “get” test.Verifies that retrieving the “main” meta returns a recognized structure for
dbMeta. The test ensures your meta layer can handle newly inserted data from earlier steps. LGTM.
469-475: Meta Gateway “delete” test.Like other deletion tests, you are removing the “main” resource. If “destroy()” or confusion arises, consider clarifying the difference between “delete” vs. a deeper store “wipe.” This is otherwise consistent.
tests/blockstore/loader.test.ts (8)
6-26: Refined imports and transactions type.Switching from
Set<bs.CarTransaction>toSet<CarTransaction>and re-importing the needed symbols from@fireproof/coreclarifies type usage. This is a neat improvement for readability and maintainability.
54-79: Initializing a CarTransaction with the new implementation.The test constructs a
CarTransactionImpland sets up the loader. This looks like a well-organized approach for mocking in-memory block storage. The resource cleanup inafterEachis a good practice to avoid leftover state. No issues spotted here.
83-93: Single commit operation test.Commits to the loader successfully produce a CAR log entry. The subsequent load and
parseCarFilevalidations ensure correctness of stored meta. This is a thoroughly tested path.
103-160: Setup for “two commits” scenario.You set up multiple blocks (
block,block2,block3,block4) and commit them in stages. Pay attention to concurrency if you eventually expand these tests for more parallel operations, but for now, it’s straightforward and well-scoped.
161-170: Second commit verification.Ensures that calling
loadCaron the committed data works as expected. Rightfully checks forparsed.meta.head. Straightforward coverage.
172-184: Compact operation test.Validates your compaction logic by ensuring the old entries get consolidated. Checking the
parsed.compact.lengthis a practical assertion. Good approach.
186-205: Compaction erases old files.An in-depth test verifying that older references in the car log are not loadable post-compaction. This comprehensively covers your log-pruning logic. The crafted approach of verifying an error is correct.
215-279: Index metadata commit test.Your test verifies the entire transaction flow, including encoding an index map. Confirming that the parsed metadata after
parseCarFilematches the original index details is robust validation. Well done.tests/blockstore/keyed-crypto-indexeddb-file.test.ts (5)
1-5: Imports and renamed provider look consistent.
No issues found. The rename fromKeyBagProviderIndexDBtoKeyBagProviderIndexedDBaligns with the standardized “indexeddb” terminology.
7-28: Test block "KeyBag indexeddb and file" with default path appears correct.
The environment detection logic for browser vs. file system is clean and straightforward. Ensure that any additional edge cases (e.g., Node-like browser shims) are covered in other tests if applicable.
29-35: "from env" test flow is clear.
No issues detected. This snippet confirms the proper retrieval of the key bag from environment variables.
36-96: Key retrieval and fingerprint checks are properly validated.
Verifying that the named keys match their persisted state is a good approach. Theawait kb.getNamedKey(name, true)usage to ensure existence vs. creation is clear.
99-129: "KeyedCryptoStore": "no crypto" scenario is well-handled.
The test validates fallback to a non-encrypting approach. No blocking issues found.src/utils.ts (4)
283-285: New Store interface fields
The addition ofpathPartandfromUrlclarifies store usage. Good job making the store’s partition type explicit.
289-308: Refined getStore logic
Switching onfromUrlimproves readability. The fallback throws a meaningful error. This is a concise and clear flow.
440-452: Function "storeType2DataMetaWal"
Straightforward mapping of store types. No issues found.
453-484: Function "ensureURIDefaults"
This function robustly sets default parameters on the URI. The error thrown for missing ledger names is helpful during debugging. Implementation is aligned with typical store usage.tests/fireproof/attachable.test.ts (3)
1-28: "meta check": basic database scenarios
The tests effectively verify empty and populated in-memory DBs. The final expectation for an empty store indicates correct cleanup.
75-88: Potentially flagged "keys" by Gitleaks
These memory-based keys look like random or test values rather than real credentials. Likely a false positive. If they are indeed placeholders, no further action is needed.🧰 Tools
🪛 Gitleaks (8.21.2)
75-75: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
88-88: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
146-203: "activate store" tests
Good job verifying activation via both the store object and store URI. Ensuring correct param expansions inmemory://URIs is crucial for consistent file usage across flows.tests/fireproof/database.test.ts (52)
12-14: Imports alignment looks correct.
No functional issues observed. TheLedgerShellimport is still used later (e.g., line 100).
26-26: Questionable cast to string.
undefined as unknown as stringcan be confusing. If truly optional, consider passingundefinedor a clearly typed parameter. This avoids potential type confusion.
63-64: Consistent test fixture usage.
The calls tosthis.start()and assignment todbare consistent with your setup pattern in other tests. No issues found.
68-68: New test block "should get".
Test logic appears straightforward: retrieving a doc by ID. No findings.
74-74: New test block "should update".
Checks doc update correctness. Looks good.
82-82: New test block "should del last record".
Tests deletion behavior. Seems correct and consistent.
89-89: New test block "has changes".
Verifiesdb.changes([])after creation. Straightforward logic. All good.
112-112: Redeclaring 'let db: Database'.
Repeated pattern across multiple describe blocks. Fine for isolated test contexts.
118-119: Re-invoked test fixture.
sthis.start()+db = fireproof("test-db-name")is consistent with your setup pattern. No issues.
180-180: Test "should del last record".
Behavior matches prior blocks. No concerns.
187-187: Test "has changes".
Asserting thatrows.lengthis 1 after prior puts. Looks accurate.
193-193: Test "should have a key".
Verifies cryptographic blockstore usage. Good coverage for encryption presence.
196-196: Accessingdb.ledger.crdt.blockstore.
Exposes core blockstore internals. Acceptable for advanced testing. No immediate issues.
205-205: Test "should work right with a sequence of changes".
Covers multi-document creation, updates, and deletions. Great coverage for incremental changes.
242-242: Test "should work right after compaction".
Ensures consistency post-compaction. Excellent to verify CRDT and blockstore integrity.
278-278: New let db: Database in 'basic Ledger parallel writes / public ordered'.
Matches prior usage. No concerns.
287-287: Configuring chunkSize=1.
Useful for verifying single-write ordering. Good test coverage for sequential writes.
296-296: Test "should have one head".
Ensures CRDT single head logic. Straightforward assertion. Good usage.
300-300: New test block "has changes ordered".
Checking row order with a final clock comparison. Appropriately tests linearization.
302-302: Comparing clock heads.
Linking local clock anddb.ledger.crdt.clock.head. Basic CRDT state check. Looks correct.
312-312: Describe block "basic Ledger parallel writes / public".
Sets up a new scenario for parallel writes. Good separation from the previous ordered test.
321-321: Writes chunkSize=32.
Similar approach to chunkSize=1 test, verifying concurrency.
329-329: Explicit reference todb.ledger.crdt.
Consistent approach. No issues.
343-343: Test "should del all".
Batch deletion test. Straightforward and complete.
353-353: Test "should delete all in parallel".
Promises handled withPromise.all. Good concurrency check. Looks fine.
366-366: Test "has changes not ordered".
Verifies concurrency effects on changes ordering. Matches the name. Good coverage.
368-368: Comparing local clock to CRDT head again.
Reiterates concurrency assumptions. No further issues spotted.
377-377: Test "should not have a key".
For a public, unencrypted store. Distinguishes private vs. public usage well.
382-382: Accessingdb.ledger.crdt.blockstoreagain.
Similar pattern. No issues.
391-391: Describe block "basic Ledger with subscription".
Introduces subscription-based tests. No immediate concerns.
402-402:beforeEachhooking in test fixture again.
Consistent usage. Established pattern remains correct.
404-404: Initializing db in subscription tests.
Matches your test approach. No issues.
415-415: Test "should run on put".
Checks subscription callback. Good for reactive usage.
428-428: Test "should unsubscribe".
Ensures unsubscribed callback doesn’t trigger. Logical and complete.
438-438: Describe "basic Ledger with no update subscription".
Special scenario for partial subscription usage. Fine approach.
446-446: NewbeforeEachblock.
Nothing unusual. Maintains pattern.
447-447:db = fireproof("factory-name")again.
Repeated test setup. No concerns.
463-463: Extended test timeout.
}, 10000);ensures tests won't prematurely fail under slower IO. Acceptable if needed.
474-474:let db: Databasewith files input.
Continues the consistent pattern. No issues.
483-483:await sthis.start()in file tests.
Aligns with other usage. Approved.
485-485: Explicit naming "fireproof-with-images".
Clarifies data context for file-based tests. Looks good.
497-497: Test "Should upload images".
Minimal check. Possibly consider verifying more details (e.g.,_filesstructure), but not critical.
501-501: Test "Should fetch the images".
Detailed checks on file metadata. Great test coverage for_files.
532-532: Test "should update the file document data without changing the files".
Verifies partial doc updates. Robust coverage.
609-621: Test "no name toStoreURIRuntime(not more)".
Asserts correct default URIs for the "gogo" name across 'data' and 'idx'. Kudos on verifying each parameter.
633-637: Inline object override for store URIs.
Shows custom base URIs for 'data' and 'idx'. Great for advanced config scenarios.
645-647: Assertions for the 'data' property.
Ensures generated URIs contain store keys, suffix, etc. Good thoroughness.
651-653: Assertions for the 'idx' property.
Mirrors the approach for defining index URIs. Consistent and clear.
659-659: Test "default-reg toStoreURIRuntime".
Checks fallback tomurks://whenFP_STORAGE_URLis unset. Good coverage for default logic.
663-664: String changes for protocol and param in default URIs.
Ensures we incorporate "murks://" properly. No issues spotted.Also applies to: 669-670
678-678:it("keyConfigOpts").
Asserts thatkeyConfigOptsreturns consistent store definitions. Thorough test coverage.
686-687: Checked URI strings for 'car' and 'file' store definitions.
Ensures correct store parameters. Implementation line details are consistent with the rest of the file.Also applies to: 694-695
src/runtime/wait-pr-multiformats/block.ts (8)
4-4: Imports from multiformats and codec-interface.
Brings in necessary types for async encoding/decoding. No concerns.Also applies to: 6-6
12-17: New or updated decode input interfaces.
DecodeInputandAsyncDecodeInputunify synchronous vs. asynchronous decoders.- Ensures code readability with typed properties.
Also applies to: 19-24
30-30: Function signature accepting bothAsyncDecodeInputorDecodeInput.
Enhances flexibility to handle both sync and async decoding. Logical approach.
34-40: Conditional hashing logic in decode.
Callscodec.valueToHashBytes(value)if available. Good for specialized hashing flows.
43-43: ReturnsmfBlockwithtoHashassigned tobytes.
Ensures finalBlockViewhas the correct hashed representation. No issues.
46-49: New or updated encode input interfaces.
EncodeInputandAsyncEncodeInputmirror the decode approach.- Clear separation of sync vs. async capabilities.
Also applies to: 55-56, 58-68
70-74:encodefunction overload for sync/async.
Allowscodec.bytesToHashoverrides for advanced hashing flows.Also applies to: 79-79
83-92: Conditional handling ofbytesToHash.
If present, user can produce different bytes for hashing. Otherwise, default approach applies. Straightforward logic.Also applies to: 94-94
src/blockstore/register-store-protocol.ts (2)
3-3: Refined import statements.
- Switches to type imports where appropriate.
- Adds
INDEXEDDB_VERSIONfrom a more consistent location.
No concerns.Also applies to: 5-6, 8-8
132-133: Switched from "indexdb:" to "indexeddb:".
protocol: "indexeddb:"- Setting default URI with
indexeddb://- Using
INDEXEDDB_VERSIONin param- Importing
GatewayImplfrom"@fireproof/core/indexeddb"
All align with consistent naming for IndexedDB.Also applies to: 135-135, 137-137, 142-143
src/blockstore/transaction.ts (10)
17-18: No concerns with the new import statements.
35-35: Confirm that skippingsuper()is intended.
If the parent constructor had important logic, removing the call tosuper()might have unintended side effects.
50-52: Add basic error handling for.put().
If#memblock.putfails internally (e.g., out of memory), you may want to surface an error or handle it gracefully.
58-63: Consider multipleunshiftcalls.
Right now, the error is thrown ifunshiftis called a second time. If you need to support multiple “unshifted” blocks, refactor accordingly. Otherwise, clarify that only one call is expected.
65-71: Validate yield ordering in.entries().
You yield the single#hackUnshiftblock before yielding blocks from#memblock. Ensure this order is correct for your intended logic.
115-115: Introduction ofBaseBlockstoreImpl.
It’s good to see consistent naming here. This class looks like a solid foundation for blockstore functionality.
168-168: Migration toCarTransactionImpl.
ReplacingCarTransactionwithCarTransactionImplis consistent with your new class structure. Verify that all references toCarTransactionelsewhere are updated.
209-209: ExtendingBaseBlockstoreImpl.
EncryptedBlockstorelooks coherent in its inheritance. This design helps maintain code reuse.
238-241: Reintroduce a loader check if needed.
Commenting outif (!this.loader)meansthis.loadercould be undefined in some scenarios. Consider verifying that it’s always available before accessing it.
318-325: Check concurrency inCompactionFetcher.
this.loggedBlocks = new CarTransactionImpl(blocks)could be accessed by multiple processes if compactions run in parallel. Confirm safe usage.src/runtime/keyed-crypto.ts (8)
2-9: Verify new imports usage.
All the newly imported types (e.g.,BytesAndKeyWithIv,CryptoAction) should be utilized consistently throughout the file.
12-12: Check usage of imported types.
Confirm thatAsyncBlockCodecandByteVieware used where appropriate.
19-20: Switch toCryptoActionis consistent.
Renaming from older key-based interfaces toCryptoActionis logical and aligns with the overhaul in cryptographic handling.
23-30: Modular strategy for IV calculation.
Storing generateIV strategies in a record is a straightforward approach to extensibility.
44-45: Ensure IV verification is robust.
Double-check that large or unusual data doesn’t cause unexpected IV mismatches.
156-157: Always await_encrypt.
Make sure every call site of_encryptawaits it properly to handle potential rejections.
194-205: Confirm ephemeral key usage innoCrypto.
Storing an automatically generated key ID might not be necessary unless you plan to handle it distinctly later.
214-215: UsingnullCodecfor no-op encryption.
This is consistent with your no-encryption approach. No issues found.src/runtime/gateways/fp-envelope-serialize.ts (17)
2-2: Verify usage of new types.
CarClockLink,DbMeta, and related imports should be consistently applied in this file.
9-11: Refactor naming fromFPEnvelopeTypetoFPEnvelopeTypes.
While the plural form can group constants, ensure references to the singular type were removed or renamed properly.
43-43:satisfies SerializedMeta.
Leveraging TypeScript’ssatisfiesoperator is a great way to ensure strict type conformance.
73-74:FPEncoderinterface rename.
ReplacingEncoderwithFPEncoderclarifies scope and usage in the Fireproof ecosystem.
80-85:defaultEncoderlogic.
All encoding methods either pass data through or serialize as JSON. Confirm that minimal transformations are indeed the desired behavior.
90-91: Partial usage forpencoder.
Merging user-provided methods withdefaultEncoderis a flexible design approach.
97-97: File envelope type.
No immediate concerns. The usage ofFPEnvelopeTypes.FILEappears consistent.
110-114:decode2DbMetaEventssignature.
Properly handles error results and returnsResult<DbMetaEvent[]>. Ensure thorough coverage in unit tests.
122-123: Reinstate empty metadata checks if needed.
If there’s a legitimate case for empty arrays, consider restoring the relevant checks to handle that scenario gracefully.
177-177:CARDecodeEnvelopetype.
Renaming to match the new envelope-driven approach is consistent.
181-182:FPDecodermirrorsFPEncoder.
Maintaining parallel interfaces for decoding and encoding fosters clarity.
210-210: Partial decoders infpDeserialize.
Similar approach to encoding. This enables easy custom overrides.
212-212: Merging decoders at runtime.
Combining defaults with user-provided decoders is straightforward and helpful.
223-226: Switch statement for store param.
All cases map to a known envelope type. Seems logically consistent.
228-230:WALenvelope decoding.
Ensure thedecode2WalStatemethod is tested adequately for edge cases in WAL data.
232-235:METAcase.
Wrapping the result ofdecode2DbMetaEventsin amakeFPEnvelopeis aligned with the pattern for other envelope types.
236-239: Reject unsupported stores.
Throwing an error for unknownPARAM.STOREensures misconfiguration doesn’t silently fail.src/react/useFireproof.ts (39)
12-12: Use ofDatabasetype import
Replaced the old ledger-based type import withDatabase. This aligns well with your broader shift from "ledger" to "database."
15-15: Refined React imports
This import now consistently uses named hooks from React. Clear and concise.
21-30: Enhanced collection interface
Adding.length,.map(),.filter(),.forEach(), and[Symbol.iterator]()extends the real-time query result object with helpful array-like methods while preserving iteration capability. This approach increases user convenience and code readability.
62-63: Tuple type addition
Defining theUseDocumentResultTuple<T>clarifies the legacy tuple API. This helps maintain backwards compatibility.
64-75: Comprehensive object API
Providing a structured object (UseDocumentResultObject<T>) plus the combined typeUseDocumentResult<T>ensures both legacy tuple support and a robust new API. This is a flexible pattern.
80-81: UseFireproof interface alignment
Exposingreadonly database: Databaseis consistent with the ledger-to-database rename. Good job keeping the interface updated.
85-88: Documentation updates
These lines accurately reflect the usage of the new database-based hooks. The clarity in docstrings will help developers transition seamlessly.
93-105: Detailed usage example
The updated code comments thoroughly demonstrate the newuseDocumentAPI, including object property and tuple destructuring. Excellent example coverage.
108-108: Tuple destructuring doc mention
Highlighting the legacy tuple usage withconst [doc, ...] = todohelps devs maintain or migrate existing code.
138-138: All docs hook comment
Updating the doc commentary to “React hook that provides access to all documents…” ensures consistency with the new database-based naming.
154-154: changes hook doc
Reflects the new ledger → database approach in retrieving the list of new documents. This structural rename helps unify the codebase.
190-190: Updated short summary
“Only use this hook if you need to configure a database name…” reaffirms the new conceptual pivot todatabase.
199-205: Extended documentation
Informing users about the difference between the default database and a custom one is great. This fosters clarity in advanced usage.
208-210: Enhanced function signature
export function useFireproof(name: string | Database = "useFireproof", ...)adds a flexible parameter type that accepts either a string name or an instantiated DB. Solid extension.
211-211:updateHappenedRefusage
Declaring arefto detect local changes prevents overwriting your doc state inadvertently. Good call.
218-218: Default initialDoc
initialDoc = initialDocOrFn ?? ({} as T);sets a fallback to an empty object, ensuring safe usage even if no initial doc is supplied.
221-221: Memoized copy of initialDoc
Creating a deep cloned version for later resets ensures eachuseDocumentstarts fresh.
225-228:refreshcallback
Grabs the latest doc content from the DB and sets it in state. The.catch(() => initialDoc)fallback for missing docs is thoughtful.
230-230: Function signature forsave
Clearer definition ofStoreDocFn<T>helps enforce the doc parameter shape.
232-239:savelogic
ResettingupdateHappenedRef.currentavoids stale doc issues. The logic to track whether_idwas absent is consistent.
245-252:removelogic
Throws explicit errors when_idis missing or doc not found. This is a robust approach for safe removal.
257-260:mergeapproach
Merging partial updates is very convenient for patching docs. Good job marking the ref’s usage to block remote events.
264-267:replaceapproach
Replacing the entire doc keeps a direct, well-defined pathway for full updates.
269-272:reset
Reverts the doc state to its pristine initial condition. Good dynamic usage of the cloned reference.
275-281:updateDoc
Allows toggling between “replace” and “merge” or initiating a refresh or reset. This is a tidy method bridging old/new doc approaches.
283-283: Dependency array
Including[refresh, reset, replace, merge]ensures the callback rebinds correctly if those references change.
285-285: Early return
Skipping subscription logic whendoc._idis missing neatly avoids pointless watchers.
287-295: Subscription
Subscribes to database changes for the doc’s_idand triggers refresh if no local updates overshadow them. Good concurrency approach.
298-299: Initial refresh
Immediately loads doc content on mount, preventing stale data.
301-307:submit
Form-friendly method. Preventing default, saving, and resetting is a nice pattern for standard form usage.
310-319: Return object
Comprehensive, descriptive API object for doc manipulations. Nicely offers a single, cohesive interface.
322-330: Combining tuple & object
Merges the legacy tuple into the same object instance. Clever destructuring approach withSymbol.iterator.
331-331: Cast toUseDocumentResult<T>
Ensures correct type inference for the final returned object.
340-351:useLiveQueryinitial state
Precomputingdocsand setting the array-like helpers in state is consistent with the approach inLiveQueryResult<T>.
357-367:refreshRowsin queries
Retrieves data viadatabase.query, maps rows to docs, then sets an updated result object with array-like methods. This pattern matches your real-time approach.
386-386:database.allDocs<T>(query)
The call to retrieve all docs is consistent with your newDatabaseinterface.
392-392: Subscription for allDocs
Re-subscribing to database changes keeps the docs in sync.
406-406:database.changes<T>
Retrieves incremental changes since a clock head. Good layering of the new approach.
418-418: Final hook return
Exposes all relevant hook methods plus thedatabaseitself, nicely concluding theuseFireproofdefinition.tests/blockstore/keyed-crypto.test.ts (49)
1-1: Added imports
Pulling inStoreType,storeType2DataMetaWal, and others from@fireproof/coreexpands test coverage for blockstore and crypto usage.
4-5: Deprecated imports
Commenting out old DAG-CBOR references indicates a transition tocborg. Good approach for updated serialization logic.
6-6: Usingcborg
Switching tocborgshows a move toward a more standard library for CBOR encoding/decoding.
7-7: New type import
ProvidingKeyBagProviderIndexedDBfrom@fireproof/core/indexeddbreferences the new IndexedDB-driven approach.
9-10:KeyWithFingerPrintreferences
Imports from../../src/blockstore/types.jsandkey-bag.jsalign with the new typed key structure.
14-14: URL changes toindexeddb://
Replacingindexdb://withindexeddb://for clarity is more descriptive and consistent with the rest of the system.
20-20:if (runtimeFn().isBrowser) ...
Ensures environment detecting logic is consistent for key bag usage.
30-30: Expecting updated URL
Verifies the newindexeddb://fp-keybagstring. Pragmatic test for environment-based KeyBag setups.
50-52: Extract testing for named keys
InvokesgetNamedKeywithkeyStrand ensures correct retrieval. Good coverage of the new method signature.
58-63: Extract key material
This block checks the.extract()functionality for deprecated features. The test ensures consistent handling.
64-66: Decoded results validation
Ensuring that the key materials match the originalkeyandkeyStr.
70-75: Warning logs
Leaves a “deprecated internal API” warning. Properly ensures the logs are captured.
85-87:getNamedKeyusage
Checks the scenario where a key is newly created and then re-fetched, verifying no mismatch.
90-91: Implicit key creation
kb.getNamedKey(name2)with no third param ensures the auto-generation logic is tested.
96-98: LocaldiskBagreferences
Captures the direct persistent storage state for newly created keys, ensuring data is physically written.
100-101: IndexedDB path
The_prepare()call is validated to confirm that the record truly exists in the "bag" store.
103-103: Second key check
Ensures the second name is also stored properly in the database.
110-112: Reading from file system
On non-browser environments, confirms we can parse the key bag from JSON.
114-115: Another read from FS
Similarly checks the second key. Good coverage for multiple key entries in file-based mode.
117-119: Fingerprint check
toKeyWithFingerPrintensures the on-disk key matches the one returned byres.Ok().get(). Good validation.
120-122: Created key fingerprint check
Ensures the newly created key has the correct fingerprint in the stored data.
125-127: AES-GCM encryption test
Confirms that data encrypted with one key matches the data encrypted with a second key if they share the same underlying bytes.
129-139: Comparing further encryption paths
Validates encryption using disk-sourced keys matches the memory-based key usage. This ensures round-trip consistency.
134-134: Explicit cast toKeyWithFingerPrint
Allows uniform property access for testing encryption.
198-199: Fingerprint assertion
Ensuring that an upserted key retains matching fingerprint references.
200-205: Key insertion loop
Inserting multiple keys, verifying each unique fingerprint, and ensuring each can be retrieved. This tests repeating upserts thoroughly.
208-209: Multiple key iteration
Double-checks each old key remains accessible while the newest key is always the default.
210-210: Final doc-level check
Verifies the default pointer updates to the last inserted key in the test loop.
214-214: Flushing & verifying
Flushes the key bag changes and re-checks to ensure everything is durable.
239-239: Loader usage
Captures the transition to a mock loader in these tests, ensuring external dependencies are replaced.
266-266: Insecure store iteration
Looping over eachbaseStoreinstance to confirm the store’s crypto mode isnoCryptoif set to “insecure.”
280-280: Ensuring crypto
When not insecure, the store is expected to produce acryptoActioninstance.
283-284: Name-based storeKey parameter
Transforms store type to a coded pattern@test-...@, verifying these computed store keys.
289-289: Random key
Generating random bytes from the keybag ensures we can pass it for store usage.
290-290: Dedicated named key
@heute@used as a store key references a previously created named key, testing cross-referencing.
297-297:cryptoActionconstructor
Checks the updated naming throughout the code.
301-301: IV usage
Injecting a random IV from keybag’s crypto ensures each test encryption is unique.
303-303: Maintaining fingerprint
Ensures the final data can be decrypted with the same fingerprint key object.
311-311: New storeKey param
Allows a direct key string in the URL for encryption.
313-313: Iterating newly attached stores
Validates expanded store creation with the new “insecure” bridging logic as a fallback.
319-319: ExpectingcryptoAction
Ensures cryptographic store usage for this scenario, consistent with the typed keys.
323-323: Retrieving the subtleKey
Confirms the store’s_encryptusage with the derived key fromkc.key.get().
350-350: Explicit IV codec
Creating an encode/decode path verifying that the stored IV is correct.
354-356: Checking CBOR decode
Ensures the raw data plus IV are included in the finalIvKeyIdDatastructure.
357-357:base58btccheck
Matches the real key fingerprint with the block’s key ID.
359-359: Decryption
Ensures the full round trip returns the original data.
364-364: Consistent decode
Confirms we get the same data out, establishing stable encryption across the test.
368-368: Return data check
Verifies that the resulting data block matches the original input size.
373-373: Multiple encodes
Checking that repeated calls produce identical results if the IV is omitted or consistently generated.src/types.ts (20)
3-3: Expandmultiformatsusage
The addition ofBlockandUnknownLinkclarifies references to advanced block or link types.
4-4: Env & logger typed import
Pulling inEnvFactoryOpts,Env, andLoggerfrom@adviser/cementensures consistent environment and logging definitions.
6-26: Blockstore expansions
Bringing in the new store types (e.g.,CarStore,FileStore,MetaStore, etc.) sets a foundation for more specialized store implementations.
29-30:KeyBagOptsimport
Inclusion of the typedKeyBagRuntimeclarifies the shape of the key bag usage in new code.
42-67: UpdatedPARAM
IntroducingSELF_REFLECT,CAR_PARALLEL,CAR_CACHE_SIZE, etc. broadens the specialized param set. Good self-documentation.
68-68:PARAMStype
Defines a union of possible param keys. Great way to keep type checking tight for param usage.
84-84:StoreTypechanges
Allowing"car"and"file"fosters broader store handling beyond the older ledger usage.
354-364:CRDTClockinterface
Exposes onTick/onTock for subscription, plus anapplyHeadmethod. This clarifies how an external caller can embed and manipulate CRDT events.
366-380:CarTransaction
Defines how custom transactions interact with blockstores. Notably includesputSync,unshift, andentries()methods, clarifying transaction flow.
381-414:BaseBlockstore
Outlines the new base blockstore approach, referencing typed transactions. This sets a consistent standard for all blockstore variants.
416-444:CRDTinterface
Details how CRDT logic, includingbulk(),allDocs(),changes(), etc., merges with the new blockstore design. This fosters code synergy between CRDT operations and storage.
450-453:RefLedger
Holds a reference toledger, presumably for bridging older APIs. This is part of your transitional approach towardDatabase.
468-489:CoerceURIandInterceptor,GatewayUrlsParam,GatewayUrls
Better typed gateway definitions for hooking up multi-store attachments. This robust approach ensures each store’s URL is properly intercepted.
491-499:Attachable
Defines an object for registering Gateway URLs and returning them, enabling a flexible plugin approach.
500-517:DataAndMetaAndWalAndBaseStore
Class wrapper unifying multiple store references into a cohesive interface. This is a tidy composition pattern.
519-526:Attached
Defines a status method and cleanup withdetach(). This formalizes how attachments are managed.
528-560:Databaseinterface
Crystallizes high-level methods likeget,put,del,allDocs,query, etc. This is the heart of the ledger → database rebranding.
562-566:WriteQueue
Pushes single tasks or bulks into a queue for persistence. Essential for concurrency-safe writes.
568-582:LedgerOpts
Houses references tometa,gatewayInterceptor,writeQueue,storeUrls,keyBag. Well-structured container for ledger config.
584-629:Ledger
Retains the older ledger-based approach but hidden behind new CRDT logic, bridging older references likesubscribeanddestroywhile supporting the new store infrastructure.src/indexer.ts (3)
15-22: LGTM! Enhanced type safety with new type declarations.The addition of new types improves type safety and modularity by clearly defining the requirements for objects that can be used with the indexer.
42-44: LGTM! Well-implemented type guard function.The
refLedgertype guard function provides a clean way to differentiate between objects with and without a ledger property.
46-64:✅ Verification successful
Verify error handling in the index function.
The error handling has been updated to use
refDb.loggerinstead ofrefDb.crdt.logger. While this change aligns with the new type structure, we should verify that error messages are properly propagated.
🏁 Script executed:
#!/bin/bash # Search for error handling patterns in the codebase rg -A 2 'logger\.Error\(\)\.Msg\(' src/Length of output: 15350
Action: Confirm consistent error propagation in the index function.
After reviewing the error handling pattern in the code—and verifying it against similar implementations across the repository (including in blockstore and CRDT helper modules)—the updated usage ofrefDb.logger.Error().Msg(...).AsError()insrc/indexer.tsis consistent. In particular:
- The check that forbids providing both
mapFnandmetais properly enforced.- The verification that
mapFnis indeed a function is correctly implemented.These align with the repository-wide pattern for error propagation, and there are no indications of issues with the error messages being correctly propagated.
tests/fireproof/indexer.test.ts (2)
23-23: LGTM! Test updates align with architectural changes.The test updates correctly reflect the transition from
LedgertoDatabasetype and the new way to access CRDT through the ledger property.Also applies to: 35-35, 39-39
333-333: LGTM! Consistent CRDT instantiation updates.The changes from
CRDT<TestType>toCRDTImplare consistently applied across all test cases.Also applies to: 334-334, 365-365, 366-366, 380-380, 381-381
src/blockstore/fp-envelope.ts (3)
5-10: LGTM! Improved type safety with constant object pattern.The transition from enum to a constant object with
as constassertion provides better type safety and flexibility. The type extraction pattern is well-implemented.Also applies to: 21-21
36-36: LGTM! Consistent interface updates.The interface property types are consistently updated to use the new constant object, maintaining type safety across the codebase.
Also applies to: 40-40, 44-44, 56-56
109-109: LGTM! Function return values aligned with new type system.The return statements in
Car2FPMsgandFile2FPMsgare correctly updated to use the new constant values.Also applies to: 121-121
src/blockstore/store.ts (20)
11-11: Import alignment verification
No obvious issues, but ensure that theCryptoActiontype is consistently used across the file.
18-18: Ensure consistent usage of the new type
TheDataAndMetaStoretype is introduced. Verify that related references in this file or other files are updated to match this new type.
19-19: Confirm CarStore references
TheCarStoreimport aligns with other code in this file.
20-20: FileStore import addition
TheFileStoreimport seems correct with no immediate issues.
22-22: Check for unused imports
Falsy,PARAM, andStoreTypeare newly imported. Confirm they’re used or remove them to maintain code cleanliness.
106-106: Validate return type usage
Ensure consumers handle the newCryptoActionreturn type appropriately.
214-232: Branch concurrency check
Theloadmethod allows specifying a branch. Verify concurrency-safe behavior if multiple branches are loaded simultaneously.
257-258: Gateway closure
This closure logic looks fine. Just ensure the reference tothis.gatewayis not used afterward to avoid potential runtime errors.
267-270: Optional parameter validation
The constructor’s use of spread syntax withoptsis convenient, but ensure no extraneous properties leak in.
274-277: Validate gateway URL
Double-check that returned URLs frombuildUrlare in the correct format for subsequent calls.
279-279: Verify response casting
Casting the gateway response to a union ofFPEnvelopeFile | FPEnvelopeCaris fine if the object shape is guaranteed. Otherwise, add runtime checks.
328-333: Confirm associated references on removal
Ensure that removing a CID does not leave orphaned references in other structures or logs.
335-336: Consistent closure
The gateway closure mechanism aligns with the base store approach.
342-342: Destruction integrity
When destroying the store, confirm that partial failures are handled, ensuring a complete cleanup of resources.
354-357: FileStore specialization
FileStoreImplmatchesDataStoreImplusage. If advanced file operations are expected, override appropriately.
465-474: Parallel logic caution
Uploading multiple cars in_doProcesscan be done concurrently, but confirm there’s no global state that requires ordered updates.
518-523: Public file usage
publicFilecan expose data. Confirm security policies support public file usage in your environment.
555-560: Validate undefined return
Ifloadreturnsundefinedfor a WAL state, higher layers should handle that gracefully to avoid runtime errors.
572-586: WAL encryption
If WAL content may be sensitive, consider encrypting it or storing it in a secure location.
600-601: Double-check references
After destroying, confirm that no other part of the code attempts to access this store’s resources.src/runtime/key-bag.ts (13)
4-4: Confirm cryptographic interface
Ensure the newly importedCTCryptoKeyaligns with your cryptographic usage.
14-14: Types usage
The additional imports look suitable for the extended key management logic.
21-55: Avoid exposing raw key material
Storing#materialas a string can become a leak risk if ever logged or exposed. Confirm secure handling.
200-206: Simple constructor
The current constructor is minimal. No issues found.
210-231: Deprecated internal API
_deprecated_internal_apican weaken security if turned on inadvertently. Make sure it is only used during development or for testing.
249-253: Invalid storeKey fallback
If the storeKey is malformed, ensure the rest of the function halts cleanly to avoid undefined states.
310-323: Atomic commits
Ensure_upsertNamedKeyis performed atomically if multiple updates happen simultaneously.
388-392: Verified default parameters
No immediate issues with default usage forgetNamedKey(...).
395-405: Read-only V2StorageKeyItem
The interface is well-typed and immutability ensures better safety.
410-411: Simple typed alias
KeyBagFiletyped record usage is straightforward.
422-425: Implementation notes
Implementors ofKeyBagProvidermust handlegetandsetcarefully to avoid concurrency issues.
455-461: Dynamic import for IndexedDB
Consider a fallback or error message if the environment does not support dynamic import orindexeddb:.
545-549: Initialization order
getKeyBagensuressthis.start()is called first. This is good practice to avoid uninitialized runtime references.src/blockstore/attachable-store.ts (10)
1-27: Import statements
These imports appear consistent and well-organized for the newattachable-storelogic.
69-86: CarActiveStore remote filtering
Excluding the current active store from the remotes array is correct; test thoroughly to confirm no edge cases remain.
88-99: Aggregated local store
CarAttachedStoresImplsimply returnslocal()and enumerates remotes. No immediate concerns.
101-112: Parallel approach
FileAttachedStoresImplmatches the pattern inCarAttachedStoresImpl. Looks consistent.
114-132: Meta store logic
Remotes combine the main active store plus everything else. The approach is consistent with other attached stores.
133-144: No issues in MetaAttachedStores
Implementation is parallel to car and file logic.
146-164: Concurrency verification
WALActiveStoreImplmight require concurrency checks to ensure logs are written in a correct order.
185-226: ActiveStoreImpl aggregator
This class provides a unified view over the local/remote car, file, meta, and wal store references. Implementation looks robust.
228-230: Improved type checks
isLoadable()returns true ifarOrLoadablehas the right properties, but also consider verifying data structures further.
264-385: Attach/detach concurrency
AttachedRemotesImplcan attach or detach stores in parallel. If multiple attachments occur simultaneously, ensure that_remotesis not updated in a conflicting way.tests/fireproof/fireproof.test.ts (10)
15-19: Imports from@fireproof/coreseem correct.
All imported symbols (fireproof,Database,isDatabase,PARAM) are referenced in the subsequent code, and there are no apparent discrepancies or unused imports.
48-54: InitializedbinbeforeEach.
Defining a newfireproof("test-db")inbeforeEachensures each test starts with a fresh instance. Verify that other tests do not inadvertently depend on state from previous tests. Looks good overall.
68-74: Great test for functional queries.
The new query approach using a function ((doc) => doc.dream) is intuitive. This test verifies that the query functionality correctly filters documents. No issues found.
81-85: Consistent type usage.
Declaringlet db: Database;keeps the initialization consistent across tests. All other variables (ok,doc,query) align well with the Fireproof types.
92-102: Verifying instance type.
UsingisDatabase(db)is a clear way to confirm the object conforms to theDatabaseinterface. This helps prevent accidental misuse of partial or mock objects.
165-165: Usinglet db: Database;in "basic ledger" block.
Maintains consistency with the rest of the file. No issues.
238-289: Compaction testing approach looks good, but watch concurrency.
The test simulates inserting documents while triggering compaction. This is a valuable integration scenario. Consider verifying that concurrency or batch behavior doesn’t break data integrity if real concurrent writes occur.
352-361: Retrieving changes after large insert.
The logic for verifyingdb.changes()correctness is thorough. Ensure you have additional checks if partial compaction or incomplete writes occur. Overall, the approach is clear.
394-399: Validating the correctness after reopen.
The pattern of reopening the DB and verifying data is a strong integration test. No obvious issues.
522-545: Skipping heavy test.
You’re skipping a slow test that goes through 200 iterations with compaction. This is acceptable for local debugging. Consider enabling it in a CI environment with a specialized long-running job, or keep it as a manual stress test.src/runtime/wait-pr-multiformats/codec-interface.ts (4)
10-15:BlockEncoderupdates.
Makingnameandcoderead-only is a good practice to ensure codec identity remains consistent. The optionalbytesToHashmethod can simplify hashing logic, but be sure to include unit tests if you intend to rely on it for block identification.
17-21:AsyncBlockEncoderconsistency.
Similar toBlockEncoder, the asynchronous version now supports abytesToHashmethod. Verify any existing code that expects synchronous hashing to ensure they gracefully handle async usage.
36-41:AsyncBlockDecoderchanges.
Supporting aPromiseLike<T>decode return type is a clean approach for asynchronous decoding. Same caution as above regarding type safety—ensure you handle untypedByteView<unknown>carefully.
47-48:BlockCodec&AsyncBlockCodec.
Combining encoder/decoder logic under these interfaces is logical. With the three type params (Code,E,D), confirm usage patterns remain consistent across the codebase, especially ifEandDdiffer significantly.src/crdt-clock.ts (8)
6-16: Expanded type imports.
The addition ofDocUpdate,ClockHead, etc., clarifies the CRDT clock logic. Keep an eye on maintenance if the type definitions change elsewhere.
20-29:CRDTClockImpldeclarations.
Readonly arrays/maps forhead,zoomers,watchers, andemptyWatchersreduce the risk of reassignments that break state. This aligns with immutability best practices.
32-33:applyHeadQueueusage.
Wrappingint_applyHeadwith a queue is a solid approach to avoid concurrent head application issues. Just be sure to handle potential queue growth if updates arrive quickly.
47-53: Constructor updates.
Stashingsthisfromblockstorecan simplify cross-class utility usage. Good approach. No immediate concerns.
60-68:applyHeaddefers logic to a queue push.
Good separation of concerns. The function only initiates the queue and yields to the aggregator. Ensure all error states (e.g., invalid heads) propagate meaningful messages back up.
80-87:notifyWatcherslogic.
Skipping notification whenupdates.lengthis zero avoids no-op callbacks. That helps performance. Just ensure watchers that rely on “no updates” calls still have a path (that’s seemingly handled byemptyWatchers).
89-111: Registration patterns withonTick,onTock,onZoom.
Using unique string keys to track watchers and zoomers is straightforward. Return a function to unregister them—a nice pattern to prevent memory leaks.
182-182: Error logging.
Changing from a debug-level to error-level log on line 182 makes sense, as failing to advance the head is critical. Ensure the system recovers or surfaces that failure clearly.src/blockstore/store-factory.ts (7)
4-15: No issues found in these import statements.
Everything looks consistent with the new store implementations.
18-18: No issues with importingSerdeGatewayandSerdeGatewayCtx.
This aligns with the overall serialization/deserialization strategy.
50-63: No issues withcarStoreFactoryconstruction.
Factory pattern is applied consistently forCarStore.
65-78: No issues withfileStoreFactoryconstruction.
Implementation matches the existing store creation pattern.
93-106: No issues withmetaStoreFactoryconstruction.
Implementation looks consistent and straightforward.
121-134: No issues withWALStoreFactoryconstruction.
Same pattern is effectively reused forWALStore.
136-146: No issues withensureStartlogic.
Error handling is clear, and the function returns the newly started store.src/blockstore/commitor.ts (15)
1-1: No issues with the import ofFileTransactionMetaandCarTransaction.
This clarification of transaction types appears correct.
9-13: No issues with addingCarStore,FileStore, andFroozenCarLogimports.
These extended imports maintain clarity and consistent naming.
20-20: No issues with importingByteViewandCID.
Framework usage is consistent with the rest of the codebase.
22-22: No issues withAsyncBlockEncoderimport.
Code reflects a proper asynchronous codec interface.
25-25:CarTransactionImplimport looks good.
Clear naming indicates an implementation detail for car transactions.
26-26: No issues withsha256import.
Switching to a direct named import is straightforward.
28-52:encodeCarFilefunction logic appears correct.
Double iteration to compute CAR size and write blocks is common practice. Hasher usage is consistent withsha256.
55-55:createCarFileproperly delegates toencodeCarFile.
Implementation is succinct and follows the new async encoder approach.
71-71:commitFilestransition toFileStoreis consistent.
Be sure all references to the olderDataStoreusage are removed or updated elsewhere in the code.
82-82: No issues with retrieving the file store codec.
Logic forfileStore.keyedCrypto()is clean.
120-124: No issues withmakeCarHeader.
It properly sets thecarsvs.compactarrays for the header structure.
131-131: No issues switchinghashertosha256.
Hasher usage inencodeCarHeaderis consistent with the rest of the file.
136-136:AsyncBlockEncoderusage inCommitParams.
No issues; the updated interface with the asynchronous block encoder fosters consistency.
153-153: No issues with callingprepareCarFiles.
Code flow for root block is streamlined.
175-205:prepareCarFileschunking logic looks solid.
However, consider validatingthresholdif negative or zero. That scenario could break chunking.package.json (9)
8-10: No issues with the new publish scripts.
Thefppublishworkflow is clear and well-defined.
15-15:build:docsconditional build approach approved.
Ensures that documentation builds only when needed in CI.
18-18:presmokeconditional build approach also approved.
Mirrors the logic used inbuild:docs.
28-28: No issues withtest:browserchanges.
Everything seems consistent with the vitest setup.
61-61: Substitution toesbuild-plugin-replacelooks fine.
No conflicts observed with the existing build pipeline.
63-63: No issues with includingeslint-import-resolver-typescript.
Increases reliability of ESLint import checks.
64-64: No issues with addingeslint-plugin-import.
Helps enforce consistent import conventions.
88-88: Upgrading@adviser/cementto^0.3.20is fine.
Ensure any newer features or breaking changes are tested.
94-94: Addition of@web3-storage/pailis approved.
Be sure to verify compatibility with existing IPFS or CAR logic.src/blockstore/loader.ts (2)
215-258: Safe concurrency approach for merging metadata
Usingthis.isCompactingwithin atry/finallyblock ensures only one metadata merge runs at a time, preventing race conditions. This design is solid, but make sure that any upstream code handles the “cannot merge while compacting” exception gracefully.
320-347: Validatecompactcondition inupdateCarLog
The logic that applies compaction updates the log differently than the non-compact path. Double-check with tests that both paths yield the correct final log, especially regarding ordering of appended groups.src/blockstore/interceptor-gateway.ts (2)
22-58: Consistent context usage inPassThroughGateway
Replacingsthiswithctx: SerdeGatewayCtxthroughout the pass-through methods clarifies the gateway’s responsibilities. Ensure thatctxis consistently populated with essential data (e.g.,loader) for error handling and logging.
66-174: Check for null references insubscribe
WithinInterceptorGateway.subscribe(), you callctx.loader.sthis.logger. Confirm that bothctx.loaderandctx.loader.sthisare never null, or handle the possibility with a fallback to avoid runtime exceptions.src/ledger.ts (2)
164-177: InvestigateLedgerShell mismatcherror
The error is thrown ifshellClose()is called on a shell that isn’t inthis.shells. This is still causing test failures per earlier reviews. Confirm that shells are always added and removed consistently, or handle unknown shells more gracefully.Would you like help in diagnosing shell lifecycle flow or creating additional test coverage for this?
306-309: Implementation detail infireproof
WrappingLedgerFactoryinsideDatabaseImplis a neat approach to unify ledger-based logic with a database abstraction. Consider adding minimal integration tests that confirmfireproofinitializes the new DB consistently with expected store URLs.src/runtime/gateways/def-serde-gateway.ts (4)
2-6: Looks good on import updates.
All import changes align well with the new types and context-based approach.
29-30: Assess potential memory usage ofthis.subscribeFn.
subscribeFncan accumulate callbacks. Confirm that unsubscribing always occurs to avoid indefinite retention of references and potential memory leaks.
31-43: Validate the subscription trigger logic for META envelopes.
After callingfpSerialize, the code conditionally triggers the subscription callback ifenv.type === FPEnvelopeTypes.META. This is a good approach for meta-only updates. Just confirm all intended types are covered.
83-93: Confirm consistent use ofctx.loaderindelete,destroy, andgetPlain.
All three methods rely onthis.gw.*()withsthisfromctx.loader. Ensure upstream callers always supply a valid loader.src/blockstore/types.ts (2)
379-381: Ensure the revisedloaded()method usage is tested.
Theloaded()method now returns aFuture<void>instead of aPromise<void>. Validate that existing code and tests handle this type difference properly.
400-406: Validate newly introducedstart(dam: DataAndMetaStore):
This signature is new inBaseStoreand might cause breakage with older implementations that do not pass aDataAndMetaStore. Confirm that all extending stores implement this correctly.src/blockstore/serde-gateway.ts (2)
20-24: Context-based approach improves maintainability.
The newSerdeGatewayCtxconsolidates loader, encoder, and decoder parameters, making the interface cleaner. Good use of typed context.
98-111: Monitor consistency acrossSerdeGatewayInterceptormethods.
All methods now acceptctx: SerdeGatewayCtx. Ensure any code intercepting gateway calls is updated to match the new parameter structure.
| private async toKeysItem(ki: V1StorageKeyItem | KeysItem | undefined): Promise<KeysItem | undefined> { | ||
| if (!ki) return undefined; | ||
| if ("key" in ki) { | ||
| const fpr = (await toKeyWithFingerPrint(this, ki.key)).Ok().fingerPrint; | ||
| return { | ||
| name: ki.name, | ||
| keys: { | ||
| [fpr]: { | ||
| key: ki.key, | ||
| fingerPrint: fpr, | ||
| default: true, | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
| // fix default | ||
| let defKI: V2StorageKeyItem | undefined; | ||
| let foundDefKI = false; | ||
| for (const i of Object.entries(ki.keys)) { | ||
| if (i[0] !== i[1].fingerPrint) { | ||
| // eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
| delete ki.keys[i[0]]; | ||
| ki.keys[i[1].fingerPrint] = i[1]; | ||
| this.logger.Warn().Str("name", ki.name).Msg("fingerPrint mismatch fixed"); | ||
| } | ||
| if (defKI === undefined) { | ||
| defKI = i[1]; | ||
| } | ||
| if (!foundDefKI && i[1].default) { | ||
| defKI = i[1]; | ||
| foundDefKI = true; | ||
| } else { | ||
| (i[1] as { default: boolean }).default = false; | ||
| } | ||
| } | ||
| if (defKI) { | ||
| ki.keys["*"] = defKI; | ||
| } | ||
| return { | ||
| name: ki.name, | ||
| keys: ki.keys, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use undefined assignment instead of delete
The delete operator can hurt performance. Assigning to undefined is often recommended.
- delete ki.keys[i[0]];
+ ki.keys[i[0]] = undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async toKeysItem(ki: V1StorageKeyItem | KeysItem | undefined): Promise<KeysItem | undefined> { | |
| if (!ki) return undefined; | |
| if ("key" in ki) { | |
| const fpr = (await toKeyWithFingerPrint(this, ki.key)).Ok().fingerPrint; | |
| return { | |
| name: ki.name, | |
| keys: { | |
| [fpr]: { | |
| key: ki.key, | |
| fingerPrint: fpr, | |
| default: true, | |
| }, | |
| }, | |
| }; | |
| } | |
| // fix default | |
| let defKI: V2StorageKeyItem | undefined; | |
| let foundDefKI = false; | |
| for (const i of Object.entries(ki.keys)) { | |
| if (i[0] !== i[1].fingerPrint) { | |
| // eslint-disable-next-line @typescript-eslint/no-dynamic-delete | |
| delete ki.keys[i[0]]; | |
| ki.keys[i[1].fingerPrint] = i[1]; | |
| this.logger.Warn().Str("name", ki.name).Msg("fingerPrint mismatch fixed"); | |
| } | |
| if (defKI === undefined) { | |
| defKI = i[1]; | |
| } | |
| if (!foundDefKI && i[1].default) { | |
| defKI = i[1]; | |
| foundDefKI = true; | |
| } else { | |
| (i[1] as { default: boolean }).default = false; | |
| } | |
| } | |
| if (defKI) { | |
| ki.keys["*"] = defKI; | |
| } | |
| return { | |
| name: ki.name, | |
| keys: ki.keys, | |
| }; | |
| } | |
| private async toKeysItem(ki: V1StorageKeyItem | KeysItem | undefined): Promise<KeysItem | undefined> { | |
| if (!ki) return undefined; | |
| if ("key" in ki) { | |
| const fpr = (await toKeyWithFingerPrint(this, ki.key)).Ok().fingerPrint; | |
| return { | |
| name: ki.name, | |
| keys: { | |
| [fpr]: { | |
| key: ki.key, | |
| fingerPrint: fpr, | |
| default: true, | |
| }, | |
| }, | |
| }; | |
| } | |
| // fix default | |
| let defKI: V2StorageKeyItem | undefined; | |
| let foundDefKI = false; | |
| for (const i of Object.entries(ki.keys)) { | |
| if (i[0] !== i[1].fingerPrint) { | |
| // eslint-disable-next-line @typescript-eslint/no-dynamic-delete | |
| ki.keys[i[0]] = undefined; | |
| ki.keys[i[1].fingerPrint] = i[1]; | |
| this.logger.Warn().Str("name", ki.name).Msg("fingerPrint mismatch fixed"); | |
| } | |
| if (defKI === undefined) { | |
| defKI = i[1]; | |
| } | |
| if (!foundDefKI && i[1].default) { | |
| defKI = i[1]; | |
| foundDefKI = true; | |
| } else { | |
| (i[1] as { default: boolean }).default = false; | |
| } | |
| } | |
| if (defKI) { | |
| ki.keys["*"] = defKI; | |
| } | |
| return { | |
| name: ki.name, | |
| keys: ki.keys, | |
| }; | |
| } |
| async removeCidsForCompact(cid: AnyLink, store: ActiveStore) { | ||
| const carHeader = await this.loadCarHeaderFromMeta( | ||
| { | ||
| cars: [cid], | ||
| }, | ||
| store, | ||
| ); | ||
| for (const cids of carHeader.compact) { | ||
| for (const cid of cids) { | ||
| await (await this.carStore()).remove(cid); | ||
| await this.attachedStores.local().active.car.remove(cid); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against empty array access in removeCidsForCompact
previousCompactCid[0] can cause runtime errors if previousCompactCid is empty or undefined.
- await this.removeCidsForCompact(previousCompactCid[0], this.attachedStores.local())
+ if (previousCompactCid && previousCompactCid.length > 0) {
+ await this.removeCidsForCompact(previousCompactCid[0], this.attachedStores.local());
+ }Committable suggestion skipped: line range outside the PR's diff.
| "browser": "./web/index.js", | ||
| "require": "./web/index.cjs", | ||
| "types": "./web/index.d.ts" | ||
| "./indexeddb": { |
There was a problem hiding this comment.
do you think this is a public api change for downstream code? does it change the import?
There was a problem hiding this comment.
no it's only used internally after runtime detection.
Summary by CodeRabbit
New Features
Refactor
Documentation
CI/CD