review-only: taxonomy fix (878a1dc) for CodeRabbit - #10
Conversation
- syncService.ts: the delta-sync pagination loop could spin forever if the server doesn't implement `offset` (this PR adds client-side pagination with no matching backend change). Add a hard page cap plus non-advancing- cursor detection so it terminates safely either way, warning rather than silently duplicating posts. - HomeScreen.tsx: loadCategories() and loadPosts() fired independent, redundant /taxonomy fetches when run in parallel at bootstrap (the existing taxonomyRef caching only deduped within loadPosts's own steps, not across the two functions) -- a real HTTP call doubling plus a narrow race if one fetch succeeded while the other timed out under flaky network. Fetch /taxonomy once in initializeAndLoad and share the single in-flight promise with both, each still resolving it lazily at the point they actually need it (keeps the fast local-data paint path unblocked). - api.ts: getTaxonomy()'s raw response type declared precedence/guidance as required while TaxonomyPayload declares them optional. Align them. TypeScript passes.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughHomeScreen now shares taxonomy requests across category loading and post synchronization. Delta synchronization now bounds pagination and stops when cursors repeat or the page limit is reached. ChangesTaxonomy and synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HomeScreen
participant TaxonomyAPI
participant loadCategories
participant loadPosts
participant syncService
HomeScreen->>TaxonomyAPI: request /taxonomy
HomeScreen->>loadCategories: pass shared taxonomy promise
HomeScreen->>loadPosts: pass shared taxonomy promise
loadPosts->>loadPosts: resolve and cache taxonomy
loadPosts->>syncService: synchronize posts with taxonomy
sequenceDiagram
participant syncService
participant BackendAPI
participant Logger
syncService->>BackendAPI: request delta page with cursor
BackendAPI-->>syncService: return page and next cursor
syncService->>syncService: detect cursor repetition or page limit
syncService->>Logger: log pagination warning
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@superbrain-app/src/services/syncService.ts`:
- Around line 99-102: Update deltaSync so pagination stopping at MAX_SYNC_PAGES
or a repeated cursor does not advance lastSyncTime; retain the original since
value and persist the current offset or cursor as the continuation checkpoint.
Only update lastSyncTime after an empty page or hasMore === false confirms
synchronization completed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 699447a6-ec1e-42a6-adfe-0f0a1922726a
📒 Files selected for processing (3)
superbrain-app/src/screens/HomeScreen.tsxsuperbrain-app/src/services/api.tssuperbrain-app/src/services/syncService.ts
| if (hitPageCap) { | ||
| console.warn( | ||
| `[Sync] Delta sync hit the ${MAX_SYNC_PAGES}-page safety cap — some changes may not be synced this cycle; next sync will continue from the last successful point.` | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not advance the delta timestamp after incomplete pagination.
When the loop stops at the page cap or on a repeated cursor, deltaSync later stores the current time as lastSyncTime. The next /sync request then excludes changes that were not fetched in this cycle. The warning at Line 101 says the next sync continues, but no offset or cursor is persisted.
Persist a continuation checkpoint with the original since value and offset or cursor. At minimum, update lastSyncTime only after an empty page or hasMore === false confirms completion.
Proposed minimum data-loss safeguard
+ let paginationComplete = false;
for (let pageNum = 0; pageNum < MAX_SYNC_PAGES; pageNum++) {
const page = await apiService.syncPosts(since, BATCH_SIZE, offset);
if (page.data.length === 0) {
+ paginationComplete = true;
hitPageCap = false;
break;
}
// ...
if (!page.hasMore) {
+ paginationComplete = true;
hitPageCap = false;
break;
}
}
- await localDb.setLastSyncTime(new Date().toISOString());
+ if (paginationComplete) {
+ await localDb.setLastSyncTime(new Date().toISOString());
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@superbrain-app/src/services/syncService.ts` around lines 99 - 102, Update
deltaSync so pagination stopping at MAX_SYNC_PAGES or a repeated cursor does not
advance lastSyncTime; retain the original since value and persist the current
offset or cursor as the continuation checkpoint. Only update lastSyncTime after
an empty page or hasMore === false confirms synchronization completed.
CodeRabbit review on PR #10: stopping early at the page cap or on a repeated cursor still advanced lastSyncTime to now, silently skipping whatever changes existed past that point on every future sync. Only advance the cursor when pagination genuinely completed (empty page or hasMore === false); otherwise the next sync retries the same window.
|
Closing — this was only a review-vehicle PR to get CodeRabbit reviewing against a real base branch (see original description). Both rounds of fixes it prompted (the pagination-safety cursor bug, and the deeper /sync offset/has_more bug an independent second-opinion review caught) landed directly on #7's branch ( |
1 participant
Review-only vehicle for the 3-file fix in sidinsearch#7 (pagination safety, deduped /taxonomy calls, type tightening). Based on main (byte-identical to the pre-fix state for these files) so the diff is clean — just the fix, no fork-vs-upstream noise. Not intended to merge; will be closed once CodeRabbit's review lands. Real PR: sidinsearch#7
Summary by CodeRabbit
Performance Improvements
Bug Fixes