Dexie 5 API with chained where-clauses and orderBy (rebased)#2253
Draft
dfahlander wants to merge 33 commits intomasterfrom
Draft
Dexie 5 API with chained where-clauses and orderBy (rebased)#2253dfahlander wants to merge 33 commits intomasterfrom
dfahlander wants to merge 33 commits intomasterfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces Dexie.js v4.5.0-alpha.1 with a new "next" module enabling Mango expression-based querying, IndexedDB 3.0 feature detection, direction support for reverse queries, and new public API exports (AnyRange, NeverRange, obsSetsOverlap). Package.json updated with version, new exports, and build tooling adjustments. Changes
Sequence DiagramsequenceDiagram
participant Client as Client Code
participant Collection as Collection<br/>(where/orderBy)
participant FilteredCollection as FilteredCollection<br/>(_build)
participant QueryEngine as executeMangoQuery
participant IndexAnalyzer as Index Analyzer<br/>(findBestIndex)
participant DBCore as DBCore<br/>(IDB 3.0 or cursor)
Client->>Collection: where("name").startsWith("A")
Collection->>FilteredCollection: Create with MangoExpr
FilteredCollection-->>Collection: Return FilteredCollection
Client->>Collection: orderBy("age").desc()
Collection->>Collection: Chain OrderedQueryable
Client->>Collection: toArray()
Collection->>QueryEngine: executeQuery(ctx)
QueryEngine->>IndexAnalyzer: findBestIndex(expr, orderBy)
IndexAnalyzer-->>QueryEngine: {index, ranges, partial}
alt Has suitable index
QueryEngine->>DBCore: Use IDB 3.0 getAll/getAllRecords<br/>with direction & ranges
else No index or complex
QueryEngine->>DBCore: Cursor-based iteration<br/>with in-memory filtering
end
DBCore-->>QueryEngine: Results/Keys
QueryEngine->>QueryEngine: Apply offset/limit<br/>Sort if needed
QueryEngine-->>Collection: Results array
Collection-->>Client: Typed array<T>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (8)
src/classes/version/version.ts (1)
46-50: Normalize trailing commas beforejoin(',')(or add regression coverage).On Line 50, multiline specs that already include trailing commas can produce
,,afterjoin(','). This works only ifparseIndexSyntax()always ignores empty tokens. Consider stripping trailing commas per line (or add targeted regression tests for trailing-comma + comment combinations) to make behavior explicit and stable.Proposed hardening
- const indexSyntax = stores[tableName] - .split('\n') - .map( - line => line.split('#')[0].trim() - ).filter(line => line.length > 0).join(',') || ''; + const indexSyntax = stores[tableName] + .split('\n') + .map(line => line.split('#')[0].trim().replace(/,+$/, '')) + .filter(line => line.length > 0) + .join(',');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/classes/version/version.ts` around lines 46 - 50, The indexSyntax construction can produce empty tokens when lines contain trailing commas (resulting in ",," after join) because it only strips comments and trims; update the mapping in the index construction (the code building indexSyntax in version.ts) to also remove trailing commas from each processed line (e.g., strip trailing commas from the trimmed string) before filtering and joining, and add a focused regression test for parseIndexSyntax()/the index parsing logic that includes combinations of trailing commas and comments to ensure empty tokens are not produced.src/next/lab.md (1)
6-8: Complete the typed DB shape for all declared stores.The type assertion only exposes
friends, butstores()also definescalendarEvents. This makes the example’s typed API incomplete.Proposed doc-snippet fix
const db = new Dexie('name') as Dexie & { friends: Table<Friend, number> + calendarEvents: Table<CalendarEvent, string> };Also applies to: 24-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/next/lab.md` around lines 6 - 8, The type assertion for the Dexie instance only exposes friends, but the stores() call also defines calendarEvents, so update the db typing to include calendarEvents (e.g., change "as Dexie & { friends: Table<Friend, number> }" to include "calendarEvents: Table<CalendarEvent, number>" or the correct entity name used elsewhere) and apply the same complete typed shape to the other example occurrences (lines 24-31) so the typed API matches all declared stores; reference the db variable, Dexie constructor, friends, calendarEvents, and the stores() definition when making the change..gitignore (1)
192-192: Consider a recursive ignore pattern for nested next declarations.If nested declaration outputs are introduced later, this rule won’t catch them.
🧹 Optional update
dist/next/*.d.mts +dist/next/**/*.d.mts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 192, Update the .gitignore entry "dist/next/*.d.mts" to a recursive pattern so nested Next declaration files are ignored; change or add a rule matching nested paths such as using "dist/next/**/*.d.mts" (or a broader "dist/**/next/**/*.d.mts") to ensure any future nested .d.mts outputs are covered.test/tests-getallrecords.js (1)
20-50: Usetry/finallyso DB handles always close on failures.Lines 49/75/98/126 only close on success path. Wrapping each test body in
try/finallywill prevent leaked connections and reduce flaky follow-up failures.Pattern to apply in each test
spawnedTest("reverse toArray() should work correctly", function* () { const db = new Dexie("TestDB-getAllRecords"); db.version(1).stores({ items: '++id, name' }); - - yield db.open(); - - // Add test data - yield db.items.bulkAdd([ - { name: 'Alice' }, - { name: 'Bob' }, - { name: 'Charlie' }, - { name: 'Diana' }, - { name: 'Eve' } - ]); - - // Test forward toArray - const forward = yield db.items.toArray(); - equal(forward.length, 5, "Forward toArray returns 5 items"); - equal(forward[0].name, 'Alice', "First item is Alice"); - equal(forward[4].name, 'Eve', "Last item is Eve"); - - // Test reverse toArray - this should now use getAllRecords() when available - const reverse = yield db.items.reverse().toArray(); - equal(reverse.length, 5, "Reverse toArray returns 5 items"); - equal(reverse[0].name, 'Eve', "First item in reverse is Eve"); - equal(reverse[4].name, 'Alice', "Last item in reverse is Alice"); - - db.close(); + try { + yield db.open(); + // ...existing assertions... + } finally { + db.close(); + } });Also applies to: 52-76, 78-99, 101-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tests-getallrecords.js` around lines 20 - 50, The test body inside spawnedTest("reverse toArray() should work correctly", function* () { ... }) currently calls db.close() only on the success path; wrap the test logic inside a try/finally so that db.close() is always invoked (open the DB with yield db.open() then perform bulkAdd and assertions inside try, and call db.close() in finally). Apply the same try/finally pattern to the other spawnedTest blocks in this file that call db.open() and db.close() so connections are closed on failures as well.src/next/NextDexie.mts (1)
27-30: Reuse oneDexieCollectioninstance per table.A second wrapper is created for the same table, which adds overhead and breaks identity consistency between
tables[]and named properties.♻️ Suggested refactor
for (const table of db.tables) { - nextDexie.tables.push(new DexieCollection(table)); - nextDexie[table.name as any] = new DexieCollection(table) as any; + const wrapped = new DexieCollection(table); + nextDexie.tables.push(wrapped); + nextDexie[table.name as any] = wrapped as any; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/next/NextDexie.mts` around lines 27 - 30, The code creates two DexieCollection wrappers per table causing identity issues; in NextDexie iteration, instantiate one DexieCollection for each table (e.g., const col = new DexieCollection(table)) and then push that single instance into nextDexie.tables and also assign it to nextDexie[table.name] so both references point to the same object; update the loop in NextDexie.mts where nextDexie.tables and nextDexie[table.name] are set to use this single variable (reference DexieCollection and table.name) to ensure consistency.package.json (1)
3-3: For4.5.0-alpha.1, confirm publish automation uses a non-latestdist-tag.This helps prevent accidental promotion of alpha builds to default consumers.
Based on learnings: The Dexie.js project uses the "canary" dist-tag on npm for pre-release versions of dexie-cloud-addon during feature development, before promoting them to the "latest" tag for stable releases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 3, The package.json version is an alpha prerelease (4.5.0-alpha.1) so update the publish automation to avoid using the default "latest" dist-tag: detect prerelease versions by inspecting the package.json "version" field (look for hyphenated identifiers like "-alpha") and call npm publish with an explicit non-latest tag (for example npm publish --tag canary or --tag alpha) in the CI/publish step; ensure the CI logic that runs the publish command (the job that reads package.json and executes npm publish) sets the tag based on the version string so prereleases use the non-latest dist-tag.src/dbcore/dbcore-indexeddb.ts (1)
305-317: Adding an explicit guard forgetAllRecords()would improve defensive coding.The feature detection relies on checking
IDBObjectStorepresence via feature-detection, but callsgetAllRecordsonsourcewhich can be either anIDBObjectStoreorIDBIndex. While the spec definesgetAllRecords()on both interfaces simultaneously and current implementations (Chrome 141+) support both or neither together, an explicit check onsourcewould be more defensive and clearer about intent—especially if you need to support partial implementations in the future or add detailed logging.The fallback to cursor iteration (lines 324–344) handles the unavailable case, so there's no runtime crash risk with the current code.
Optional hardening
- if (hasIdb3Features) { + const canUseGetAllRecords = records && typeof (source as any).getAllRecords === 'function'; + if (hasIdb3Features && (!records || canUseGetAllRecords)) { const options = { query: idbKeyRange, count: nonInfinitLimit, direction: direction || 'next' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dbcore/dbcore-indexeddb.ts` around lines 305 - 317, The code calls getAllRecords on source when hasIdb3Features is true but does not ensure source actually implements getAllRecords; update the conditional that chooses req so it explicitly checks that (source as any).getAllRecords is a function before invoking it (i.e., combine hasIdb3Features with typeof (source as any).getAllRecords === 'function'), and fall back to the existing getAll/getAllKeys/cursor path if not present; update logic around variables/branches that set req (records, getAll, getAllKeys) and keep eventRejectHandler and resolve usage unchanged.src/next/MangoExpression.mts (1)
1-11: Consider fixing the public type name typo (SimleMangoRange→SimpleMangoRange).This is new surface area; correcting now avoids long-lived API misspelling.
✏️ Proposed rename
-export interface SimleMangoRange { +export interface SimpleMangoRange { @@ -export interface MangoRange extends SimleMangoRange { - $inRanges?: SimleMangoRange[]; +export interface MangoRange extends SimpleMangoRange { + $inRanges?: SimpleMangoRange[]; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/next/MangoExpression.mts` around lines 1 - 11, Rename the misspelled exported interface SimleMangoRange to SimpleMangoRange and update MangoRange to extend SimpleMangoRange; then update all references/imports/usages throughout the codebase to the new name to avoid type errors (look for usages of SimleMangoRange and MangoRange in types, exports, and imports) and run the typechecker; if you need to preserve backward compatibility, add a short exported type alias `export type SimleMangoRange = SimpleMangoRange` and mark it as deprecated in comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/next/executeMangoQuery.md`:
- Line 1: Update the documentation so every reference to implementation files
uses the .mts extension instead of .ts; specifically edit executeMangoQuery.md
to replace stale ".ts" links and code references with ".mts" (including any
import/path examples and anchor links) and verify internal anchors and link
targets still resolve after the change. Ensure all occurrences are updated
(including the other noted section) so readers won't hit dead paths when
following examples or references to the implementation.
In `@src/next/executeMangoQuery.mts`:
- Around line 761-764: The fallback in executeCount currently calls
executeQuery(ctx) and uses .length, which breaks when ctx.query.explain is true
because executeQuery can return a QueryPlan; update executeCount to detect if
ctx.query?.explain is true and, when delegating to executeQuery, pass a cloned
context with query.explain set to false (or otherwise ensure explain is
disabled) so executeQuery returns rows; then return the resulting array length.
Refer to executeCount, executeQuery and the query.explain flag when making the
change.
- Around line 987-1007: The fast-path branch in executeMangoQuery (the block
handling "if (!where && orderBy.length === 0 && offset === 0)") ignores the
requested sort direction; update this branch so the direction is propagated to
both the explain path and the actual query call: include the direction value in
the buildQueryPlan metadata and pass direction into the core.query({ ... query:
{ index: schema.primaryKey, range: AnyRange, direction } }) payload (and ensure
limit handling remains unchanged); this ensures descending queries are executed
correctly when where/orderBy are empty.
- Around line 1424-1453: The manual-sort path applies offset/limit to keys
before performing manual ordering, producing incorrect pages for complex
queries; modify the branch that checks needsManualSort so that when true you do
not slice filteredKeys early — call core.getMany with the full filteredKeys,
then call sortResults(values, orderBy, direction, schema), and only after
sorting apply offset and limit to the sorted result set (rather than slicing
selectedKeys beforehand). Keep the existing fast path (when needsManualSort is
false) that uses queryIndex + slice before core.getMany.
- Around line 535-537: The forEach callbacks (the outer results.forEach and
inner res.result.forEach using concise arrow returns) are implicitly returning
values; change both to use block-bodied callbacks that call
keySet.add(toMapKey(key)); (i.e., replace the concise arrow expressions with {
keySet.add(toMapKey(key)); } ) so no value is returned. Ensure you update the
callbacks that reference results, res.result, toMapKey and keySet to use
statement bodies without returning values.
In `@src/next/lab.md`:
- Line 16: Replace the accidental typo "hthos" in the user-facing comment
"indexer hthos and that" with a clear word or phrase (e.g., "things" or "etc.")
in src/next/lab.md so the line reads something like "indexer things and that" or
"indexers, etc."; locate the string "indexer hthos and that" and update it in
that file's content (ensure surrounding punctuation/capitalization remains
consistent).
- Line 36: The example calls db.setLocale with an incorrect ISO code; update the
example so db.setLocale("ge") uses the correct German locale db.setLocale("de")
(locate the usage of db.setLocale in the example and replace "ge" with "de").
In `@src/next/LIMITATIONS.md`:
- Around line 3-6: The documentation contains typographical and API-name
inconsistencies that make behavior ambiguous; update LIMITATIONS.md to correct
spellings and API casing: change "child querys" to "child queries", "querys" to
"queries", "where-criteria" to "where criteria", and normalize "orderby" to
"orderBy"; ensure items 1–4 consistently reference the functions offset, limit,
where(), desc(), and orderBy(), and clarify "primary key" wording for
readability without altering meaning.
In `@src/next/MangoExpression.mts`:
- Around line 66-77: canonicalizeMango() currently treats any non-null object as
an operator bag, corrupting valid direct equality values like arrays, Dates, or
custom objects; update the branch that creates rangeResult to first detect plain
object operator-bags (e.g., Object.getPrototypeOf(value) === Object.prototype or
a dedicated isPlainObject check) and only then iterate keys and apply
mangoRangeAliases, otherwise assign value directly to result[key]; reference the
function canonicalizeMango, the variables value, key, rangeResult, and
mangoRangeAliases when making the change.
- Around line 45-49: The current noneOf and $nin implementations create a
union-of-ranges that can re-include excluded values for multi-value exclusions;
change them to emit a conjunction of inequality checks instead (i.e., for
multiple values produce { $and: values.map(v => ({ $ne: v })) } and for a single
value use { $ne: value }) so that noneOf, $nin produce an $and of $ne clauses
rather than $inRanges with $gt/$lt; update any aliases (noneOf, $nin) and ensure
$inRanges/$gt/$lt/$eq usages remain unchanged for other operators.
In `@src/next/Queryable.mts`:
- Around line 47-50: The constructor(s) for Queryable<T, TKey, TInsertType>
currently accept any numeric input for _offsetValue and _limitValue; before
assigning these to this._offsetValue / this._limitValue validate that the values
are finite integers >= 0 (e.g., use Number.isFinite and Number.isInteger or
Math.floor checks) and reject negatives, NaN, or Infinity by throwing a clear
RangeError/TypeError; update both the constructor at the offset assignment
(constructor(parent: Queryable<...>, _offsetValue: number)) and the
corresponding limit constructor/assignment (the _limitValue path around lines
63-66) to perform the same validation and messaging so invalid values are
rejected early.
In `@src/next/toMapKey.mts`:
- Around line 2-25: toMapKey currently produces colliding outputs (e.g.,
Number(123) and Date(123) both become "123") causing incorrect deduplication;
change toMapKey to emit type-tagged strings for every non-composite case so type
boundaries are preserved (e.g., prefix numbers with "n:", strings with "s:",
dates with "d:" + timestamp, binary with "b64:" + base64, array-views with
"view:b64:", ArrayBuffer with "ab:b64:", arrays as "arr:[" + mappedKeys + "]"
where mappedKeys are the recursively type-tagged results from toMapKey). Update
toMapKey (the function in this diff) so its recursive calls always return string
segments and avoid returning raw numbers or ambiguous plain strings; this will
ensure executeMangoQuery.mts Set-based deduplication sees distinct IndexedDB key
types as distinct keys.
In `@src/next/WhereClause.mts`:
- Around line 88-91: anyOf currently emits a {$in: keys} predicate which
bypasses index planning; change anyOf (in WhereClause.anyOf) to emit the
planner-recognized multi-point predicate {$inRanges: ...} instead of $in so the
planner can create discrete index ranges. Specifically, when constructing the
FilteredCollection in anyOf, convert the keys array into the $inRanges form
(each key represented as a single-point range) and pass that under this._prop
(preserving this._coll and this._op) so the planner can use multi-point index
ranges.
In `@test-debug.html`:
- Around line 17-20: The logger function log currently appends untrusted strings
via output.innerHTML which can execute HTML; change log to create a new DOM node
(e.g., a div or span) and set its textContent to msg (or create a Text node)
then append it to output and add a line break via CSS or an appended text node,
ensuring no use of innerHTML in function log and referencing the output element
and log function when making the change.
In `@test/karma.lambdatest.js`:
- Around line 19-21: The browserVersion value in the Karma LambdaTest config
should be W3C-compatible; change the browserVersion field (in the same object
that contains 'LT:Options') from "144" to "144.0" and, if you want a resilient
fallback, switch to "latest" when a session returns "version not available";
update the browserVersion assignment accordingly so the config uses the decimal
format required by W3C.
In `@test/next/tests-next.ts`:
- Around line 7-23: Tests share a fixed Dexie instance (_db) and DB name
("Apansson") causing state leakage; ensure each test starts with a clean DB by
deleting or clearing the database in setup/teardown: call
Dexie.delete("Apansson") or clear the tables before each test and recreate the
NextDexie(_db) instance, or generate a unique DB name per test; update the test
harness around module("next") to add beforeEach/afterEach hooks that reference
_db, db, and NextDexie to perform the delete/clear and re-initialize the DB so
assertions remain deterministic.
In `@tools/build-configs/rollup.tests.config.js`:
- Around line 33-34: The path check using importer.includes('/next/') in the
rollup resolver conditional is not separator-agnostic; import normalizePath from
'@rollup/pluginutils' and replace the raw check with a normalized form (e.g.,
normalizePath(importer).includes('/next/')) so backslashes on Windows don't
break the matcher; update the top of the file to import normalizePath and adjust
the if in the block that returns { id: 'dexie', external: true } to use the
normalized importer value.
In `@tools/build-next-modules.js`:
- Around line 15-18: The cleanup loop in build-next-modules.js uses
fs.unlinkSync on every entry from existingFiles (joined with targetDir) which
will throw for directories; update the removal logic to stat each entry (use
fs.lstatSync or fs.statSync on path.join(targetDir, file)) and if it's a file
call unlinkSync, otherwise if it's a directory remove it safely (e.g.,
fs.rmSync(path.join(targetDir, file), { recursive: true, force: true }) or a
recursive delete helper) so both files and directories are cleaned without
throwing; keep references to existingFiles, targetDir, fs.unlinkSync and
path.join(targetDir, file) when making the change.
---
Nitpick comments:
In @.gitignore:
- Line 192: Update the .gitignore entry "dist/next/*.d.mts" to a recursive
pattern so nested Next declaration files are ignored; change or add a rule
matching nested paths such as using "dist/next/**/*.d.mts" (or a broader
"dist/**/next/**/*.d.mts") to ensure any future nested .d.mts outputs are
covered.
In `@package.json`:
- Line 3: The package.json version is an alpha prerelease (4.5.0-alpha.1) so
update the publish automation to avoid using the default "latest" dist-tag:
detect prerelease versions by inspecting the package.json "version" field (look
for hyphenated identifiers like "-alpha") and call npm publish with an explicit
non-latest tag (for example npm publish --tag canary or --tag alpha) in the
CI/publish step; ensure the CI logic that runs the publish command (the job that
reads package.json and executes npm publish) sets the tag based on the version
string so prereleases use the non-latest dist-tag.
In `@src/classes/version/version.ts`:
- Around line 46-50: The indexSyntax construction can produce empty tokens when
lines contain trailing commas (resulting in ",," after join) because it only
strips comments and trims; update the mapping in the index construction (the
code building indexSyntax in version.ts) to also remove trailing commas from
each processed line (e.g., strip trailing commas from the trimmed string) before
filtering and joining, and add a focused regression test for
parseIndexSyntax()/the index parsing logic that includes combinations of
trailing commas and comments to ensure empty tokens are not produced.
In `@src/dbcore/dbcore-indexeddb.ts`:
- Around line 305-317: The code calls getAllRecords on source when
hasIdb3Features is true but does not ensure source actually implements
getAllRecords; update the conditional that chooses req so it explicitly checks
that (source as any).getAllRecords is a function before invoking it (i.e.,
combine hasIdb3Features with typeof (source as any).getAllRecords ===
'function'), and fall back to the existing getAll/getAllKeys/cursor path if not
present; update logic around variables/branches that set req (records, getAll,
getAllKeys) and keep eventRejectHandler and resolve usage unchanged.
In `@src/next/lab.md`:
- Around line 6-8: The type assertion for the Dexie instance only exposes
friends, but the stores() call also defines calendarEvents, so update the db
typing to include calendarEvents (e.g., change "as Dexie & { friends:
Table<Friend, number> }" to include "calendarEvents: Table<CalendarEvent,
number>" or the correct entity name used elsewhere) and apply the same complete
typed shape to the other example occurrences (lines 24-31) so the typed API
matches all declared stores; reference the db variable, Dexie constructor,
friends, calendarEvents, and the stores() definition when making the change.
In `@src/next/MangoExpression.mts`:
- Around line 1-11: Rename the misspelled exported interface SimleMangoRange to
SimpleMangoRange and update MangoRange to extend SimpleMangoRange; then update
all references/imports/usages throughout the codebase to the new name to avoid
type errors (look for usages of SimleMangoRange and MangoRange in types,
exports, and imports) and run the typechecker; if you need to preserve backward
compatibility, add a short exported type alias `export type SimleMangoRange =
SimpleMangoRange` and mark it as deprecated in comments.
In `@src/next/NextDexie.mts`:
- Around line 27-30: The code creates two DexieCollection wrappers per table
causing identity issues; in NextDexie iteration, instantiate one DexieCollection
for each table (e.g., const col = new DexieCollection(table)) and then push that
single instance into nextDexie.tables and also assign it to
nextDexie[table.name] so both references point to the same object; update the
loop in NextDexie.mts where nextDexie.tables and nextDexie[table.name] are set
to use this single variable (reference DexieCollection and table.name) to ensure
consistency.
In `@test/tests-getallrecords.js`:
- Around line 20-50: The test body inside spawnedTest("reverse toArray() should
work correctly", function* () { ... }) currently calls db.close() only on the
success path; wrap the test logic inside a try/finally so that db.close() is
always invoked (open the DB with yield db.open() then perform bulkAdd and
assertions inside try, and call db.close() in finally). Apply the same
try/finally pattern to the other spawnedTest blocks in this file that call
db.open() and db.close() so connections are closed on failures as well.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
.gitignorepackage.jsonsrc/classes/collection/collection.tssrc/classes/dexie/dexie-static-props.tssrc/classes/version/version.tssrc/dbcore/dbcore-indexeddb.tssrc/dbcore/virtual-index-middleware.tssrc/functions/quirks.tssrc/index.tssrc/live-query/cache/apply-optimistic-ops.tssrc/live-query/cache/find-compatible-query.tssrc/next/Collection.mtssrc/next/DexieCollection.mtssrc/next/LIMITATIONS.mdsrc/next/MangoExpression.mtssrc/next/NextDexie.mtssrc/next/OrderedQueryable.mtssrc/next/Query.mtssrc/next/Queryable.mtssrc/next/SUGGESTED_INDEXES.mdsrc/next/WhereClause.mtssrc/next/createMangoFilter.mtssrc/next/executeMangoQuery.mdsrc/next/executeMangoQuery.mtssrc/next/index.mtssrc/next/lab.mdsrc/next/toMapKey.mtssrc/next/tsconfig.jsonsrc/public/index.d.tssrc/public/types/dbcore.d.tssrc/public/types/dexie-constructor.d.tssrc/public/types/global.d.tssrc/tsconfig.jsontest-debug.htmltest/karma.lambdatest.jstest/next/tests-next.tstest/tests-all.jstest/tests-getallrecords.jstest/tsconfig.jsontools/build-configs/rollup.tests.config.jstools/build-next-modules.js
| @@ -0,0 +1,335 @@ | |||
| # executeMangoQuery.ts - Architecture Documentation | |||
There was a problem hiding this comment.
Update file references to .mts to keep docs navigable.
Several references point to .ts names while the implementation in this PR uses .mts, so readers will hit dead paths.
📝 Suggested doc patch
-# executeMangoQuery.ts - Architecture Documentation
+# executeMangoQuery.mts - Architecture Documentation
@@
-- `MangoExpression.ts` - Type definitions and canonicalization
-- `createMangoFilter.ts` - In-memory filtering for partial expressions
+- `MangoExpression.mts` - Type definitions and canonicalization
+- `createMangoFilter.mts` - In-memory filtering for partial expressions
@@
-- `toMapKey.ts` - Convert keys to Set-friendly format
+- `toMapKey.mts` - Convert keys to Set-friendly format
@@
-- [MangoExpression.ts](./MangoExpression.ts) - Query language specification
-- [Query.ts](./Query.ts) - Query builder API
+- [MangoExpression.mts](./MangoExpression.mts) - Query language specification
+- [Query.mts](./Query.mts) - Query builder APIAlso applies to: 325-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/next/executeMangoQuery.md` at line 1, Update the documentation so every
reference to implementation files uses the .mts extension instead of .ts;
specifically edit executeMangoQuery.md to replace stale ".ts" links and code
references with ".mts" (including any import/path examples and anchor links) and
verify internal anchors and link targets still resolve after the change. Ensure
all occurrences are updated (including the other noted section) so readers won't
hit dead paths when following examples or references to the implementation.
Comment on lines
+535
to
+537
| results.forEach(res => { | ||
| res.result.forEach((key: any) => keySet.add(toMapKey(key))); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the pattern described in the review comment
rg -nP 'forEach\(\s*[^)]*=>\s*[^{][^;]*\.add\(' src/next/executeMangoQuery.mtsRepository: dexie/Dexie.js
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Read the relevant sections around the mentioned lines
echo "=== Lines 530-545 ==="
sed -n '530,545p' src/next/executeMangoQuery.mts | cat -n
echo ""
echo "=== Lines 550-565 ==="
sed -n '550,565p' src/next/executeMangoQuery.mts | cat -nRepository: dexie/Dexie.js
Length of output: 1131
🏁 Script executed:
#!/bin/bash
# Search for forEach with arrow functions that don't use braces
# This will catch both typed and untyped parameters
rg -nA1 'forEach\([^)]*=>\s*[^{]' src/next/executeMangoQuery.mts | head -30Repository: dexie/Dexie.js
Length of output: 1004
🌐 Web query:
Biome lint/suspicious/useIterableCallbackReturn rule forEach implicit return
💡 Result:
Biome’s lint/suspicious/useIterableCallbackReturn enforces “return consistency” for callbacks passed to iterable methods: methods like map/filter/find/... must return a value, while forEach must not return a value. For forEach, any returned value is ignored (it “goes nowhere”), so it’s usually a bug or misleading. [1][2]
Why you’re seeing it with forEach “implicit return”
An arrow callback like this implicitly returns a value, so Biome flags it:
items.forEach(x => x * 2); // implicit return of x*2Fix options
- Don’t return a value (use a block body):
items.forEach(x => { doSomething(x); });- If you intentionally want a concise expression but no returned value, use
void:
items.forEach(x => void doSomething(x));Biome explicitly treats this as OK. [1]
- If your codebase intentionally returns from
forEachand you don’t want Biome to check it, disable theforEachpart of the rule (available since Biome v2.4.0):
{
"linter": {
"rules": {
"suspicious": {
"useIterableCallbackReturn": {
"options": { "checkForEach": false }
}
}
}
}
}[1]
Sources: Biome rule docs (and option checkForEach) [1]; explanation that forEach return values are ignored [2]; related ESLint rule behavior [3].
[1] https://biomejs.dev/linter/rules/use-iterable-callback-return/
[2] https://stackoverflow.com/questions/41025939/where-does-the-return-value-go-for-a-foreach-callback
[3] https://eslint.org/docs/latest/rules/array-callback-return
Fix forEach callbacks that implicitly return values (Biome lint errors).
The concise callbacks at lines 536 and 556 return Set.add(...) results, triggering lint/suspicious/useIterableCallbackReturn. The Biome rule requires forEach callbacks to not return values, as returned values are ignored.
🧹 Proposed fix
results.forEach(res => {
- res.result.forEach((key: any) => keySet.add(toMapKey(key)));
+ res.result.forEach((key: any) => {
+ keySet.add(toMapKey(key));
+ });
}); keySets.forEach(keySet => {
- keySet.forEach(key => unionKeys.add(key));
+ keySet.forEach(key => {
+ unionKeys.add(key);
+ });
});📝 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.
Suggested change
| results.forEach(res => { | |
| res.result.forEach((key: any) => keySet.add(toMapKey(key))); | |
| }); | |
| results.forEach(res => { | |
| res.result.forEach((key: any) => { | |
| keySet.add(toMapKey(key)); | |
| }); | |
| }); |
🧰 Tools
🪛 Biome (2.4.4)
[error] 536-536: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/next/executeMangoQuery.mts` around lines 535 - 537, The forEach callbacks
(the outer results.forEach and inner res.result.forEach using concise arrow
returns) are implicitly returning values; change both to use block-bodied
callbacks that call keySet.add(toMapKey(key)); (i.e., replace the concise arrow
expressions with { keySet.add(toMapKey(key)); } ) so no value is returned.
Ensure you update the callbacks that reference results, res.result, toMapKey and
keySet to use statement bodies without returning values.
Comment on lines
+761
to
+764
| // Case 3: Complex query or needs filtering - execute and count | ||
| // We need to pass offset/limit to executeQuery to get accurate count | ||
| return executeQuery(ctx).then(results => (results as any[]).length); | ||
| } |
There was a problem hiding this comment.
executeCount() can break when explain is enabled in fallback path.
Fallback counting delegates to executeQuery(ctx). If query.explain is true, executeQuery may return a QueryPlan instead of rows, so .length is unsafe.
🛠️ Proposed fix
// Case 3: Complex query or needs filtering - execute and count
// We need to pass offset/limit to executeQuery to get accurate count
- return executeQuery(ctx).then(results => (results as any[]).length);
+ const nonExplainCtx = query.explain
+ ? { ...ctx, query: Object.assign(new Query(), query, { explain: false }) }
+ : ctx;
+ return executeQuery(nonExplainCtx).then(results => (results as any[]).length);
}📝 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.
Suggested change
| // Case 3: Complex query or needs filtering - execute and count | |
| // We need to pass offset/limit to executeQuery to get accurate count | |
| return executeQuery(ctx).then(results => (results as any[]).length); | |
| } | |
| // Case 3: Complex query or needs filtering - execute and count | |
| // We need to pass offset/limit to executeQuery to get accurate count | |
| const nonExplainCtx = query.explain | |
| ? { ...ctx, query: Object.assign(new Query(), query, { explain: false }) } | |
| : ctx; | |
| return executeQuery(nonExplainCtx).then(results => (results as any[]).length); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/next/executeMangoQuery.mts` around lines 761 - 764, The fallback in
executeCount currently calls executeQuery(ctx) and uses .length, which breaks
when ctx.query.explain is true because executeQuery can return a QueryPlan;
update executeCount to detect if ctx.query?.explain is true and, when delegating
to executeQuery, pass a cloned context with query.explain set to false (or
otherwise ensure explain is disabled) so executeQuery returns rows; then return
the resulting array length. Refer to executeCount, executeQuery and the
query.explain flag when making the change.
Comment on lines
+987
to
+1007
| // Case 1: Simple toArray() - no where, no orderBy, no offset | ||
| if (!where && orderBy.length === 0 && offset === 0) { | ||
| if (explain) { | ||
| return Promise.resolve( | ||
| buildQueryPlan('Full table scan', schema.primaryKey, [AnyRange], { | ||
| limit, | ||
| complexity: 'O(n)', | ||
| notes: ['No filters or ordering specified', 'Scanning primary key'], | ||
| }) | ||
| ); | ||
| } | ||
| return core.query({ | ||
| trans, | ||
| values: true, | ||
| query: { | ||
| index: schema.primaryKey, | ||
| range: AnyRange, | ||
| }, | ||
| limit: limit === Infinity ? undefined : limit, | ||
| }).then(res => res.result); | ||
| } |
There was a problem hiding this comment.
Fast path ignores requested sort direction.
When where is empty and orderBy is empty, descending queries still run in default forward direction because direction is not passed to core.query().
🛠️ Proposed fix
if (!where && orderBy.length === 0 && offset === 0) {
if (explain) {
return Promise.resolve(
buildQueryPlan('Full table scan', schema.primaryKey, [AnyRange], {
+ direction,
limit,
complexity: 'O(n)',
notes: ['No filters or ordering specified', 'Scanning primary key'],
})
);
}
return core.query({
trans,
values: true,
query: {
index: schema.primaryKey,
range: AnyRange,
},
+ direction: dbDirection,
limit: limit === Infinity ? undefined : limit,
}).then(res => res.result);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/next/executeMangoQuery.mts` around lines 987 - 1007, The fast-path branch
in executeMangoQuery (the block handling "if (!where && orderBy.length === 0 &&
offset === 0)") ignores the requested sort direction; update this branch so the
direction is propagated to both the explain path and the actual query call:
include the direction value in the buildQueryPlan metadata and pass direction
into the core.query({ ... query: { index: schema.primaryKey, range: AnyRange,
direction } }) payload (and ensure limit handling remains unchanged); this
ensures descending queries are executed correctly when where/orderBy are empty.
Comment on lines
+1424
to
+1453
| // Use orderBy index if available, otherwise primary key | ||
| const queryIndex = orderByIndex || schema.primaryKey; | ||
| const needsManualSort = orderBy.length > 0 && !orderByIndex; | ||
|
|
||
| // Get all keys in index order | ||
| return core.query({ | ||
| trans, | ||
| values: false, | ||
| query: { | ||
| index: queryIndex, | ||
| range: AnyRange, | ||
| }, | ||
| direction: dbDirection, | ||
| }).then(orderedKeysResult => { | ||
| // Filter by matching keys | ||
| const filteredKeys = orderedKeysResult.result.filter((key: any) => | ||
| matchingKeys.has(toMapKey(key)) | ||
| ); | ||
|
|
||
| // Apply offset and limit | ||
| const selectedKeys = filteredKeys.slice( | ||
| offset, | ||
| offset + limit | ||
| ); | ||
|
|
||
| // Get values | ||
| return core.getMany({ trans, keys: selectedKeys }).then(values => { | ||
| if (needsManualSort) { | ||
| return sortResults(values, orderBy, direction, schema); | ||
| } |
There was a problem hiding this comment.
Manual-sort path paginates too early for complex queries.
In the complex-query branch, offset/limit are applied to keys before manual orderBy sorting. This can return the wrong page.
🛠️ Proposed fix
- // Apply offset and limit
- const selectedKeys = filteredKeys.slice(
- offset,
- offset + limit
- );
+ const keysForFetch = needsManualSort
+ ? filteredKeys
+ : filteredKeys.slice(offset, offset + limit);
// Get values
- return core.getMany({ trans, keys: selectedKeys }).then(values => {
+ return core.getMany({ trans, keys: keysForFetch }).then(values => {
if (needsManualSort) {
- return sortResults(values, orderBy, direction, schema);
+ const sorted = sortResults(values, orderBy, direction, schema);
+ return sorted.slice(offset, limit === Infinity ? undefined : offset + limit);
}
return values;
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/next/executeMangoQuery.mts` around lines 1424 - 1453, The manual-sort
path applies offset/limit to keys before performing manual ordering, producing
incorrect pages for complex queries; modify the branch that checks
needsManualSort so that when true you do not slice filteredKeys early — call
core.getMany with the full filteredKeys, then call sortResults(values, orderBy,
direction, schema), and only after sorting apply offset and limit to the sorted
result set (rather than slicing selectedKeys beforehand). Keep the existing fast
path (when needsManualSort is false) that uses queryIndex + slice before
core.getMany.
Comment on lines
+17
to
+20
| function log(msg) { | ||
| console.log(msg); | ||
| output.innerHTML += msg + '<br>'; | ||
| } |
There was a problem hiding this comment.
Avoid innerHTML in the logger output path.
Line 19 appends dynamic text through innerHTML, which can execute injected markup. Prefer textContent + DOM nodes for safe debug output.
Suggested safe update
function log(msg) {
console.log(msg);
- output.innerHTML += msg + '<br>';
+ const line = document.createElement('div');
+ line.textContent = String(msg);
+ output.appendChild(line);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-debug.html` around lines 17 - 20, The logger function log currently
appends untrusted strings via output.innerHTML which can execute HTML; change
log to create a new DOM node (e.g., a div or span) and set its textContent to
msg (or create a Text node) then append it to output and add a line break via
CSS or an appended text node, ensuring no use of innerHTML in function log and
referencing the output element and log function when making the change.
Comment on lines
+19
to
+21
| browserVersion: "144", | ||
| 'LT:Options': { | ||
| platformName: 'Windows 10' | ||
| platformName: 'Windows 11' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
LambdaTest Chrome browser version 144 Windows 11 supported capabilities
💡 Result:
To run Chrome 144 on Windows 11 on LambdaTest, use the standard W3C fields plus LambdaTest-specific options under LT:Options:
Minimum required capabilities (Selenium 4 / W3C)
{
"browserName": "Chrome",
"browserVersion": "144.0",
"LT:Options": {
"platformName": "Windows 11",
"user": "<LT_USERNAME>",
"accessKey": "<LT_ACCESS_KEY>"
}
}LambdaTest’s Selenium 4 examples use browserName, browserVersion, and platformName (inside LT:Options). [1]
Note: LambdaTest documents Chrome support broadly (e.g., “Chrome 83 & above” for their automation Chrome stack), but whether 144 is selectable depends on what’s currently available in your LambdaTest browser catalog for your plan/region. [2]
Common supported LambdaTest capabilities (add under LT:Options)
- Metadata:
project,build,name[1] - Logs/recording:
network,console,video,visual[3] - Screen:
resolution(Windows 11/10 examples include1920x1080,2048x1536, etc.) [2] - Local testing:
tunnel,tunnelName[2] - Location/timeouts:
geoLocation,idleTimeout[2] - (If needed) Accessibility scan (WebdriverIO integration):
accessibility: trueandaccessibilityOptions[4]
Sources: [1] [2] [3] [4]
🌐 Web query:
LambdaTest Selenium WebDriver browserVersion Chrome 144 Windows 11 compatibility
💡 Result:
LambdaTest’s capability docs indicate that Windows 11 is a supported platform value, and Chrome browserVersion is supported for “Chrome 83 & above” (i.e., any modern Chrome major version, including 144). [1]
So, Chrome 144 on Windows 11 is compatible in principle (Chrome 144 is a real stable release as of January 13, 2026). [2] However, whether "browserVersion": "144.0" works for your specific run still depends on whether LambdaTest currently has Chrome 144 provisioned on its Selenium grid for Windows 11 at the moment you run.
Practical guidance
- If you need exactly Chrome 144, try
"browserVersion": "144.0"with platform Windows 11 (via LambdaTest capability generator), and if the session fails with “version not supported/available”, switch to"browserVersion": "latest"(or"latest-1") to stay on a guaranteed-available image/version. browserVersionis the standard W3C capability Selenium uses for remote sessions. [3]
Sources: [1] [2] [3]
🌐 Web query:
LambdaTest supported browser versions list Chrome Windows 11 2024 2025
💡 Result:
For LambdaTest (TestMu AI) on Windows 11, the published supported-version policy for Chrome (for Cypress testing) is:
- Chrome: version 66 and above (except version 82) on Windows 11. [1]
If you need the exact Chrome builds available in the cloud for specific years (e.g., 2024/2025), LambdaTest provides a “List of Browsers” page where the specific versions are loaded dynamically (i.e., the live catalog of versions you can pick in Manual/Automation/Screenshots). [2]
Use browserVersion: "144.0" instead of "144" for W3C compatibility.
Chrome 144 on Windows 11 is supported per LambdaTest's policy (Chrome 83+), but the exact version must use the W3C format with a decimal point. If the session fails with "version not available," LambdaTest may not have Chrome 144 currently provisioned on its grid; consider using "latest" as a fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/karma.lambdatest.js` around lines 19 - 21, The browserVersion value in
the Karma LambdaTest config should be W3C-compatible; change the browserVersion
field (in the same object that contains 'LT:Options') from "144" to "144.0" and,
if you want a resilient fallback, switch to "latest" when a session returns
"version not available"; update the browserVersion assignment accordingly so the
config uses the decimal format required by W3C.
Comment on lines
+7
to
+23
| const _db = new Dexie("Apansson") as Dexie & { | ||
| friends: Dexie.Table<{id?: number, name: string, age: number, shoeSize: number}, number>; | ||
| } | ||
| _db.version(13).stores({friends: '++id,age,name,[name+age+shoeSize]'}); | ||
| _db.on("populate", function() { | ||
| _db.friends.bulkAdd([ | ||
| {name: "Arne1", age: 42, shoeSize: 46}, | ||
| {name: "Arne2", age: 43, shoeSize: 45}, | ||
| {name: "Arne3", age: 44, shoeSize: 44}, | ||
| {name: "Bengt", age: 36, shoeSize: 43}, | ||
| {name: "Cecilia", age: 25, shoeSize: 38} | ||
| ]); | ||
| }); | ||
| const db = NextDexie(_db); | ||
|
|
||
| module("next"); | ||
|
|
There was a problem hiding this comment.
Reset database state between tests to avoid flaky outcomes.
This suite uses a fixed DB name and shared instance but does not reset state per test. Assertions on exact counts/plans can become order-dependent when prior runs/suites touched the same DB.
💡 Suggested isolation change
-module("next");
+module("next", {
+ beforeEach: async () => {
+ await resetDatabase(_db);
+ clearSuggestedIndexes();
+ },
+ afterEach: () => {
+ clearSuggestedIndexes();
+ }
+});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/next/tests-next.ts` around lines 7 - 23, Tests share a fixed Dexie
instance (_db) and DB name ("Apansson") causing state leakage; ensure each test
starts with a clean DB by deleting or clearing the database in setup/teardown:
call Dexie.delete("Apansson") or clear the tables before each test and recreate
the NextDexie(_db) instance, or generate a unique DB name per test; update the
test harness around module("next") to add beforeEach/afterEach hooks that
reference _db, db, and NextDexie to perform the delete/clear and re-initialize
the DB so assertions remain deterministic.
Comment on lines
+33
to
+34
| if (source === '../..' && importer && importer.includes('/next/')) { | ||
| return { id: 'dexie', external: true }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Rollup resolveId hook importer parameter path separators Windows POSIX
💡 Result:
In Rollup, the resolveId(source, importer) hook receives importer as the resolved id of the importing module, and Rollup does not normalize separators—so on Windows it will typically contain win32 backslashes (\). In Vite, ids are normalized to POSIX slashes (/) (while preserving the Windows drive/volume, e.g. C:/...).[1]
What to do (portable across Windows/POSIX):
- Normalize before comparing / matching paths (e.g. against
include/exclude, caches, string-prefix checks, etc.). - Use
normalizePath:- from
@rollup/pluginutilswhen writing Rollup-compatible plugins, or - from
vitewhen writing Vite plugins (same intent).[1][2]
- from
Example:
import { normalizePath } from '@rollup/pluginutils' // or: from 'vite'
resolveId(source, importer) {
const imp = importer && normalizePath(importer) // converts \ -> /
// now do comparisons using POSIX-style paths
}normalizePath('C:\\proj\\src\\main.ts') → C:/proj/src/main.ts.[2]
Note (Vite-specific): during dev, some resolveId calls may have importer as a generic absolute index.html path (not always the “real” JS importer), so don’t assume it always maps 1:1 to a module file in dev mode.[3]
Sources: Vite plugin API (path normalization + Rollup vs Vite behavior)[1], @rollup/pluginutils normalizePath docs[2], Vite plugin API note about importer in dev[3].
🌐 Web query:
Rollup plugin resolveId importer normalized path separators documentation
💡 Result:
- Rollup does not guarantee normalized path separators in
resolveId(source, importer, …). In particular on Windows, resolved ids (includingimporter) may contain\, while other tooling (notably Vite) normalizes resolved ids to POSIX/separators. [1] - Therefore, for cross-platform Rollup/Vite plugins, normalize before comparing or storing ids (especially when using include/exclude filters, string prefix checks, map keys, etc.). Use
normalizePath()from@rollup/pluginutils(orvite’s equivalent) which converts separators to forward slashes. [2] - Reminder from the
resolveIdhook contract:importeris the fully resolved id of the importing module, and is usuallyundefinedfor entry points. [3]
Example pattern:
import { normalizePath } from '@rollup/pluginutils';
resolveId(source, importer) {
const s = normalizePath(source);
const imp = importer ? normalizePath(importer) : undefined;
// ...compare/use s + imp safely across OSes
}Sources: Vite path normalization note vs Rollup behavior [1], @rollup/pluginutils.normalizePath API [2], resolveId importer definition/entrypoint behavior [3].
🏁 Script executed:
cat -n tools/build-configs/rollup.tests.config.js | head -50Repository: dexie/Dexie.js
Length of output: 2058
🏁 Script executed:
grep -n "normalizePath\|import" tools/build-configs/rollup.tests.config.js | head -20Repository: dexie/Dexie.js
Length of output: 552
Use normalizePath() from @rollup/pluginutils for separator-agnostic path matching.
Line 33 uses importer.includes('/next/'), which relies on forward slashes. On Windows, Rollup's importer parameter can contain backslashes, causing this check to fail. Use the idiomatic Rollup solution:
♻️ Suggested fix
+import { normalizePath } from '@rollup/pluginutils';
import sourcemaps from 'rollup-plugin-sourcemaps'; resolveId(source, importer) {
if (source === 'dexie/next') {
return path.resolve(__dirname, '../tmp/src/next/index.mjs');
}
// Resolve '../..' imports from next modules back to main dexie (external)
- if (source === '../..' && importer && importer.includes('/next/')) {
+ if (source === '../..' && importer && normalizePath(importer).includes('/next/')) {
return { id: 'dexie', external: true };
}
return null; // Let other plugins handle it
}📝 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.
Suggested change
| if (source === '../..' && importer && importer.includes('/next/')) { | |
| return { id: 'dexie', external: true }; | |
| import { normalizePath } from '@rollup/pluginutils'; |
Suggested change
| if (source === '../..' && importer && importer.includes('/next/')) { | |
| return { id: 'dexie', external: true }; | |
| if (source === '../..' && importer && normalizePath(importer).includes('/next/')) { | |
| return { id: 'dexie', external: true }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/build-configs/rollup.tests.config.js` around lines 33 - 34, The path
check using importer.includes('/next/') in the rollup resolver conditional is
not separator-agnostic; import normalizePath from '@rollup/pluginutils' and
replace the raw check with a normalized form (e.g.,
normalizePath(importer).includes('/next/')) so backslashes on Windows don't
break the matcher; update the top of the file to import normalizePath and adjust
the if in the block that returns { id: 'dexie', external: true } to use the
normalized importer value.
Comment on lines
+15
to
+18
| const existingFiles = fs.readdirSync(targetDir); | ||
| existingFiles.forEach(file => { | ||
| fs.unlinkSync(path.join(targetDir, file)); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the actual file content
cat -n tools/build-next-modules.js | head -30Repository: dexie/Dexie.js
Length of output: 1308
🏁 Script executed:
# Check Node.js version requirements
cat package.json | grep -A 5 '"engines"'Repository: dexie/Dexie.js
Length of output: 40
🏁 Script executed:
# Search for any related cleanup or dist/next references
rg "dist/next" tools/ -A 2 -B 2Repository: dexie/Dexie.js
Length of output: 660
🏁 Script executed:
# Look for fs.rmSync usage elsewhere in the codebase
rg "fs\.rmSync|fs\.unlinkSync|rimraf" tools/ -A 2Repository: dexie/Dexie.js
Length of output: 196
🏁 Script executed:
# Check the full file to see what gets copied to dist/next
cat -n tools/build-next-modules.jsRepository: dexie/Dexie.js
Length of output: 2159
🏁 Script executed:
# Check if package.json exists and what Node.js version it targets
cat package.json | jq '.engines // "no engines field"'Repository: dexie/Dexie.js
Length of output: 76
🏁 Script executed:
# Look for any existing dist/next directory structure or references
find . -path "./dist/next" -type d 2>/dev/null || echo "No dist/next directory found"Repository: dexie/Dexie.js
Length of output: 40
Directory cleanup could fail if unexpected subdirectories exist in dist/next.
The cleanup at lines 15-18 uses fs.unlinkSync() on all directory entries without validation, which throws on directories. While the current copy logic only places files into dist/next, the cleanup lacks defensive guards against edge cases (manual additions, or residual directories from failed builds).
More robust cleanup pattern
if (!fs.existsSync(targetDir)) {
fs.mkdirSync(targetDir, { recursive: true });
} else {
- // Clean target directory before copying
- const existingFiles = fs.readdirSync(targetDir);
- existingFiles.forEach(file => {
- fs.unlinkSync(path.join(targetDir, file));
- });
+ // Clean target directory before copying (works for files + subdirectories)
+ fs.rmSync(targetDir, { recursive: true, force: true });
+ fs.mkdirSync(targetDir, { recursive: true });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/build-next-modules.js` around lines 15 - 18, The cleanup loop in
build-next-modules.js uses fs.unlinkSync on every entry from existingFiles
(joined with targetDir) which will throw for directories; update the removal
logic to stat each entry (use fs.lstatSync or fs.statSync on
path.join(targetDir, file)) and if it's a file call unlinkSync, otherwise if
it's a directory remove it safely (e.g., fs.rmSync(path.join(targetDir, file), {
recursive: true, force: true }) or a recursive delete helper) so both files and
directories are cleaned without throwing; keep references to existingFiles,
targetDir, fs.unlinkSync and path.join(targetDir, file) when making the change.
liz709
force-pushed
the
collection-next-II
branch
from
c7318be to
9b75e8f
Compare
- prettier ^3.8.1 added to devDependencies (was only run via npx) - Added pnpm format / format:check scripts - .vscode/settings.json: formatOnSave + prettier as default formatter - .vscode/extensions.json: recommends prettier-vscode extension - .gitignore: allow .vscode/settings.json and extensions.json to be tracked (only launch.json and snippets remain ignored)
Allow omitting comma in multiline schemas
Still no way to import it. Will work on that so we can test it.
…a for a while even if we release new 4.2.x from master.
…ie db instance (currently readonly and limited)
- Add validation to prevent non-virtual compound indexes from being used when range constraint is on the first component - IndexedDB cannot compare single values against arrays, so compound indexes can only be used for ranges when all preceding components have equality constraints - Virtual indexes handle this correctly by translating ranges - Update index preference logic to favor real indexes over virtual indexes when scores are equal - Ensures correct index selection with virtual-index-middleware
- Verify that queries work correctly when only compound indexes exist - Tests that virtual sub-indexes are used as fallback when no explicit index is defined - Ensures startsWith() works with virtual indexes created from compound indexes like [name+age+shoeSize]
liz709
force-pushed
the
collection-next-II
branch
from
9b75e8f to
a57c2a6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
where(), ordering withorderBy(), and directional traversal (asc/desc).NextDexie()wrapper for typed collection access to database tables.Documentation
Chores