feat: Android taxonomy sync + hide built-in default chips - #7
Conversation
Note the accidental close/reopen and that draft/android-category-sync must stay published until upstream merges. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Hey so sorry for the massive history here. This is my attempt to avoid asking for android client changes more than once. Happy to answer questions. |
44a655d to
cfb2c96
Compare
Defensive client support for config-driven categories. Without /taxonomy (upstream today), behavior matches mainline chips and sync. With /taxonomy, honor use_default_categories, taxonomy_version full-resync, and custom chips. Co-authored-by: Cursor <cursoragent@cursor.com>
cfb2c96 to
5975a02
Compare
|
@coderabbitai review |
- 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.
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.
|
Fixed in d629ccf per CodeRabbit's review on the sibling review-only PR: |
…a, remove taxonomy-cache race Independent second-opinion review (Fable 5) on the prior CodeRabbit-driven fix caught a deeper bug: /sync never actually supported the `offset` param the client was already sending — get_posts_since() always queried from the same `since` with no OFFSET, and the endpoint never returned `has_more`. The client's non-advancing-cursor safety net correctly stopped the loop instead of looping forever, but since lastSyncTime never advanced either, every retry hit the identical wall — a guaranteed permanent sync stall for any backlog over 200 changed posts, not just a hypothetical. - backend: get_posts_since()/`/sync` now honor offset and return real has_more (fetch limit+1, trim, compare). Added shortcode as a secondary ORDER BY key since updated_at alone isn't a stable pagination cursor when timestamps tie. - client: syncPosts() now marks a transient fetch failure distinctly (`failed: true`) instead of returning the same shape as a legitimate empty/final page — a network blip could otherwise be mistaken for sync completion and silently advance the cursor past unfetched changes. - HomeScreen: taxonomyRef was a component-level ref reset at the top of every loadPosts() call but only ever read/written within that same call — turned into a plain local variable so overlapping loadPosts calls (focus listener vs. poll-interval refresh) can no longer stomp each other's cache and reintroduce the redundant /taxonomy fetches this PR was written to eliminate. Verified offset/has_more pagination against an in-memory sqlite db (paged fetch of 5 rows at limit=2 returns all rows once, in order, with has_more flipping false on the last page). Typechecked clean.
|
Ran an independent second-opinion review (different model) on top of the CodeRabbit round. It found a deeper issue than the pagination-cap fix addressed: Fixed in 3acb694, pushed to this branch:
Verified the new pagination logic against an in-memory sqlite db (paged fetch returns all rows once, in order, |
|
Independent second-opinion re-verification (same model that caught the /sync offset bug) confirms all four issues are fixed in
This PR is in a good state from my side — ready whenever you want to take a look. |
Thank you for building and maintaining SuperBrain — it's a genuinely great project, and it's a pleasure to get to contribute back to it. No rush at all on review; happy to adjust anything.
What this adds
Defensive, gated support in the Android app for a config-driven category taxonomy, so a fork/deployment that adds
GET /taxonomyon the backend can offer custom categories and always-fresh sync — without any backend change required to merge this, and with zero behavior change for everyone else.Why
Categorization needs vary a lot between deployments (different content domains want different category sets), but the mainline app's categories are currently hardcoded. Rather than carry a growing fork-only diff against
main, this PR proposes landing the client-side plumbing now — entirely inert until a backend opts in — so the taxonomy work stays upstream-compatible from day one instead of drifting further apart over time.Why it's safe to merge before any backend taxonomy work exists
Every new code path is gated behind
isTaxonomyApiActive(taxonomy). Concretely:GET /taxonomy(every deployment today): identical mainline behavior — built-in category chips, early-return after the local SQLite read, delta-only sync (including pull-to-refresh). Nothing observably changes.GET /taxonomy(opt-in, fork/future upstream): always background-sync after the local paint, pull-to-refresh can trigger a full resync,taxonomy_versionchanges trigger an automatic full resync, anduse_default_categories: falseswitches to server-configured chips only./taxonomy404s, times out, or returns malformed data at any point, the client falls back to exact current mainline behavior rather than erroring.Scope
App (client, the original scope of this PR):
superbrain-app/src/constants/categories.ts— default-category fallback plumbingsuperbrain-app/src/screens/HomeScreen.tsx— gated taxonomy fetch/sync integrationsuperbrain-app/src/screens/PostDetailScreen.tsx— configured-category displaysuperbrain-app/src/services/api.ts—getTaxonomy(), paginatedsyncPosts()superbrain-app/src/services/localDb.ts— taxonomy_version persistencesuperbrain-app/src/services/syncService.ts— bounded, safe delta-sync paginationsuperbrain-app/src/services/taxonomySupport.ts— new,isTaxonomyApiActivegate + typessuperbrain-app/src/theme/index.ts— category color fallbackBackend — 2 files, 25 lines, fixing a pre-existing bug this PR is not responsible for (see below):
backend/api.py—/syncnow accepts and honors theoffsetparambackend/core/database.py—get_posts_since()now supports real offset-based pagination with ahas_moresignalThis is not a taxonomy-feature change, and I want to be upfront that it goes beyond "Android app only": while hardening this PR's own delta-sync handling, I found that
/syncnever actually implemented theoffsetquery param the Android app has already been sending on every sync, before this PR touched anything. The backend'sget_posts_since()just ignored it — noOFFSETin the SQL, nohas_morein the response — sooffsetwas silently a no-op.In practice this means: any single delta sync with more than 200 changed posts (the page size the client already uses) has always been unable to fetch past the first page, on the current
mainbranch, independent of this PR. A large batch of changes (e.g. a big playlist import) landing between syncs would get truncated at 200 with no error and no way to catch up. My new client-side pagination-safety net (bounded loop + stall detection, see below) made this concrete: it correctly stops instead of looping forever, but a client-only fix can't retrieve data the server never sends — only a backend fix can.The fix is small and purely additive:
offsetdefaults to0andhas_moreis a new optional field, so nothing changes for any existing caller that doesn't use them. I mention this prominently because I know the intent was to keep this Android-only, and I want the "why" to be clear rather than have it read as scope creep.Client-side hardening (the other 3 of 4 commits)
The original feature commit went through two rounds of independent automated review (CodeRabbit, then a second differently-sourced review as a deliberate second opinion) before I felt comfortable asking for your time on it. Both rounds found real, worthwhile issues, now fixed:
/syncbug above.)lastSyncTime) only advances when a sync cycle genuinely completes; an early stop (cap hit, non-advancing cursor, or a failed page fetch) leaves it untouched so the next sync safely retries the same window instead of silently skipping data.Every fix above was independently re-verified (not just re-read) before this revision, and the app was rebuilt from a clean debug+release APK and confirmed working end-to-end. Happy to walk through any of this in more detail, or to split the backend fix into its own separate PR if you'd rather review/merge it independently of the taxonomy feature — just say the word.
Test plan
/taxonomy): chips and sync match current production APK exactlyuse_default_categories: false; ataxonomy_versionchange triggers a full resynctsc --noEmit) on the current commithas_moreflips correctly on the last page)Thanks again for taking a look — genuinely appreciate the project and your time.