Repository navigation
Feat/refactor sqlite - #15
Conversation
…e interface for improved state management
…d corresponding tests
…instead of undefined
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the SQLite implementation by replacing the complex external store synchronization approach with a simpler internal state management system. The changes remove dependency on use-sync-external-store and implement direct iterator-based pagination with reactive updates.
Key changes:
- Replaced external store subscription pattern with direct state management using useRef and useReducer
- Simplified the API by removing search ID-based caching in favor of direct iterator management
- Added a new
useSqliteCounthook for reactive counting
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
packages/core/sqlite/use-sqlite.ts |
Complete refactor replacing external store with iterator-based pagination and direct state management |
packages/core/sqlite/use-sqlite-count.ts |
New hook for reactive counting of table items with filtering support |
packages/core/sqlite/table/table.types.ts |
Moved SqlSeachOptions type definition and added Key export |
packages/core/sqlite/table/table.ts |
Renamed DEFAULT_STEP_SIZE to DEFAULT_PAGE_SIZE for consistency |
packages/core/sqlite/create-sqlite.ts |
Simplified SyncTable interface by removing complex search ID management |
packages/core/sqlite/__tests__/use-sqlite.test.tsx |
Updated tests to work with new API and handle undefined return values |
packages/core/sqlite/__tests__/use-slite-count.test.tsx |
New test file for the useSqliteCount hook |
packages/core/scheduler.ts |
Enhanced scheduler to pass accumulated values to onScheduleDone callback |
Comments suppressed due to low confidence (1)
packages/core/sqlite/tests/use-slite-count.test.tsx:1
- The filename has a typo: 'slite' should be 'sqlite'. The file should be renamed to
use-sqlite-count.test.tsx.
import { act, renderHook } from '@testing-library/react-hooks'
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let isDone = false | ||
| for (let index = 0; index < pageSize; index++) { | ||
| const result = await iterator.next() | ||
| result.value |
There was a problem hiding this comment.
This line has no effect. The result.value is accessed but not used or assigned. Either remove this line or assign it to a variable if it's needed for processing.
| result.value |
|
|
||
| const searchId = useMemo(() => generateCacheKey({ ...options, select: undefined }), [options]) | ||
| const componentId = useId() | ||
| // const [items, setItems] = useState<undefined | (Document | Selected)[]>() |
There was a problem hiding this comment.
Remove commented-out code. These leftover comments from development should be cleaned up before merging.
| // const [items, setItems] = useState<undefined | (Document | Selected)[]>() |
| const [, rerender] = useReducer((c: number) => c + 1, 0) | ||
| const keysIndex = useRef(new Map<Key, number>()) | ||
| const iteratorRef = useRef<AsyncIterableIterator<{ doc: Document; meta: { key: Key } }>>() | ||
| // const pageRef = useRef(0) |
There was a problem hiding this comment.
Remove commented-out code. These leftover comments from development should be cleaned up before merging.
| // const pageRef = useRef(0) |
| useLayoutEffect(() => { | ||
| state.updateSearchOptions(searchId, { ...options, select: undefined }) | ||
| const updateIterator = useCallback(() => { | ||
| // eslint-disable-next-line sonarjs/no-unused-vars |
There was a problem hiding this comment.
Remove commented-out code. These leftover comments from development should be cleaned up before merging.
| // eslint-disable-next-line sonarjs/no-unused-vars |
| const nextPage = useCallback(async () => { | ||
| const isDone = await fillNextPage(false) | ||
| rerender() | ||
| // console.log('nextPage isDone', isDone) |
There was a problem hiding this comment.
Remove commented-out code. These leftover comments from development should be cleaned up before merging.
| // console.log('nextPage isDone', isDone) |
| if (itemsRef.current === undefined) { | ||
| nextPage() | ||
| } |
There was a problem hiding this comment.
This condition will always be true since itemsRef.current is set to undefined on line 164. This appears to be redundant logic that should be simplified or removed.
| if (itemsRef.current === undefined) { | |
| nextPage() | |
| } | |
| nextPage() |
| if (keysIndex.current.has(result.value.meta.key)) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The logic skips duplicate keys but doesn't decrement the loop counter. This could result in fewer items than the requested page size when duplicates are encountered. Consider adjusting the loop logic to ensure the correct page size is maintained.
| while ((itemsRef.current?.length ?? 0) < newLength) { | ||
| await fillNextPage(false) |
There was a problem hiding this comment.
This while loop could potentially run indefinitely if fillNextPage doesn't increase the array length (e.g., when all remaining items are duplicates). Consider adding a break condition or maximum iteration limit to prevent infinite loops.
| while ((itemsRef.current?.length ?? 0) < newLength) { | |
| await fillNextPage(false) | |
| { | |
| const MAX_ITERATIONS = 100; | |
| let iterations = 0; | |
| while ((itemsRef.current?.length ?? 0) < newLength && iterations < MAX_ITERATIONS) { | |
| await fillNextPage(false); | |
| iterations++; | |
| } | |
| if (iterations === MAX_ITERATIONS) { | |
| // Optionally log a warning to help with debugging | |
| // eslint-disable-next-line no-console | |
| console.warn('Reached maximum iterations in fillNextPage loop. Possible duplicate or data issue.'); | |
| } |
…loops and add iteration limit
No description provided.