Repository navigation
Conversation
Native community client on the contract from obv/burst6-fridgeluck-trunk: Apple sign-in with nonce, token persistence (Keychain on Apple platforms, in-memory on Linux), account restore with single-flight refresh rotation, Tables create/join/leave/share/block flows with offline cache fallback, challenge entry/vote with time gates and the account-age voting gate, persisted writes via OutboxService, and the community_table_link_v1 migration so a newly created Table stays discoverable before its first feed fetch. Portable models/view models/rules are pinned into the community-check Linux mirror by refresh.sh. Co-authored-by: Sam Gu <sgu07966@gmail.com>
|
@codex review |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ced3e104c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| mapFailure(error) | ||
| } | ||
| } | ||
|
|
||
| /// Joiner path: present a join code. The 202 response is pending owner approval; the ask | ||
| /// survives as an outbox row when offline (SPEC 5.1, 8.3). | ||
| public func join(withCode code: String) async { | ||
| failure = nil | ||
| do { | ||
| let join = try await client.joinTable(code: code) | ||
| try? store.setActiveTable(tableId: join.tableId) | ||
| tableId = join.tableId | ||
| state = .askingToJoin(tableName: join.tableName) | ||
| } catch { | ||
| // A 404 covers revoked codes, wrong codes, and blocked pairs alike — the joiner is | ||
| // not told which (§5.1, §6.3); the mock copy is one line for all three. | ||
| mapFailure(error) |
There was a problem hiding this comment.
When createTable or joinTable fails because the device is offline, these catch paths only map the error and never enqueue the operation, even though the surrounding API and join documentation promise that the request survives offline via the outbox. The user's create/join intent is therefore discarded and must be entered again manually.
Useful? React with 👍 / 👎.
| public func leave() async { | ||
| guard let tableId else { return } | ||
| await enqueueTableEvent(.init(event: .leaveTable, tableId: tableId)) | ||
| try? store.clearTableCache(tableId: tableId) | ||
| try? store.clearActiveTable() | ||
| self.tableId = nil | ||
| state = .noTable |
There was a problem hiding this comment.
The backend rejects an owner's leave request (owner cannot leave; delete the table instead), but this method clears the cache and active-table link and switches to noTable immediately after enqueueing, without checking whether the outbox operation succeeded. When an owner taps leave, the server still retains the table while the client loses its only local table reference, so subsequent launches cannot recover or manage that table.
Useful? React with 👍 / 👎.
| let join = try await client.joinTable(code: code) | ||
| try? store.setActiveTable(tableId: join.tableId) | ||
| tableId = join.tableId | ||
| state = .askingToJoin(tableName: join.tableName) |
There was a problem hiding this comment.
After a 202 join response, only the table ID is persisted and no pending-join state or table cache is written. If the app is relaunched before the owner approves, load() uses that ID, GET /tables/{id} returns not found because the joiner is not yet an active member, and with no cache the UI falls into failed instead of restoring the documented askingToJoin state.
Useful? React with 👍 / 👎.
| let session = try await authClient.refreshSession( | ||
| AuthRefreshRequest(refreshToken: tokens.refreshToken)) | ||
| try? await tokenStore.write( | ||
| CommunitySessionTokens( | ||
| accessToken: session.accessToken, refreshToken: session.refreshToken)) | ||
| return session.accessToken |
There was a problem hiding this comment.
The refresh response contains a newly computed CommunityAccount, including the server's current canVoteAt, display name, and adult status, but this path persists only the two tokens and returns the access token. Consequently, after a transparent 401 refresh, SessionViewModel.account remains stale until an explicit restore, so an account that has passed the 24-hour vote gate can continue showing the incorrect “vote counts tomorrow” state (and other server-side account changes are likewise not reflected).
Useful? React with 👍 / 👎.
|
Independent review (Obvious, Autobuild + Max) Verdict: changes neededCI is red on the head commit and one new test-pinned behavior needs rework before this can merge. The core design (transport seam, hash-pinned Linux mirror, contract round-trip tests) is solid and the Linux suite result is genuine — I reproduced 71/71 green — but the Apple side, which this PR's own body names as its check, failed to compile. Findings (ranked)1. CRITICAL — macOS CI fails to compile:
|
…s and transport failures - Leave keeps the local Table until the server accepts; an owner's leave surfaces ownerCannotLeave before any request is sent (the backend refuses it with 409). - Join and createTable transport failures enqueue outbox events so intent survives; a pending join restores as asking-to-join after relaunch (community_table_link_v2). - Restore keeps tokens when the refresh leg could not run and clears them only on a server rejection; refresh persists the fresh account snapshot it carries. - The in-memory token store serializes async access with NSLock.withLock (Apple SDK forbids lock/unlock across await). - Failed no-content responses keep their bodies so typed server errors surface. Review round for PR #173 findings 1-7, each with a failing-first regression test. Co-authored-by: Sam Gu <sgu07966@gmail.com>
- CommunityStore.swift:464: Data(row["payload_json"]) has no exact initializer match on the Apple SDK (resolved only on Linux); read the column typed per the file's existing pattern and treat a NULL payload as no cache. - CommunitySessionService.swift: with try? optional flattening (SE-0230) the paired tokens != nil comparisons were always true (compiler warnings at lines 97 and 107); compare the try? result directly. macOS CI run 38838725369 failed on the initializer; this fixes it. Co-authored-by: Sam Gu <sgu07966@gmail.com>
The transport stub's async send() called NSLock.lock()/unlock() directly, which the Apple SDK forbids from async contexts (macOS CI run 38839520976). Wrap the send body in NSLock.withLock; the synchronous sentCount/at accessors keep their existing locking. Co-authored-by: Sam Gu <sgu07966@gmail.com>
The test calls the @mainactor signInAdult helper, sending the non-Sendable test case across the actor boundary (Swift 6 sendability error at CommunityClientTests.swift:513, macOS CI run 38840329632). Every other test in the class already carries the annotation. Co-authored-by: Sam Gu <sgu07966@gmail.com>
fleet-packet: burst6-fridgeluck-community-client-groups
Branch
obv/burst6-fridgeluck-community-client-groups, headf7fb825, based onobv/burst6-fridgeluck-trunk@90386c6(the integration branch under PR #170). Draft. Review-fix round for the independent review of head1ced3e1— all seven findings addressed, dispositions below.Human author: Sam Gu (sgu07966@gmail.com)
What changed and why
The phone could not talk to the community backend yet. This slice adds the native client path for the three community surfaces — session, Tables, and challenges — on the contract shipped by the burst6 trunk:
apps/ios/FeatureLogic/Community/) — session/auth and typed community errors, token storage (CommunityTokenStore), Tables/shared-cook cards/blocks/reports, and challenges, plus pure challenge rules: entry vs. voting phases, delayed tally visibility, and the account-age voting gate the server enforces (account_too_new→ 403, verified against PR feat(community): cooking groups (tables, invites, shared cooks, blocks) and challenges (entries, packs, voting) #166's backend).apps/ios/Integration/Community/CommunityAPIClient.swift) — session, identity, Tables, cards (GET /v1/tables/{tableId}/cards, the{ cards: [...] }envelope confirmed in PR feat(community): cooking groups (tables, invites, shared cooks, blocks) and challenges (entries, packs, voting) #166 review), blocks (decodes the server's{ blocks: [...] }envelope fromblockListView), challenges, voting, and reporting; unauthenticated auth requests; 2xx-without-body handling (202 delete); typed server-error decoding — including on no-content-success endpoints, which now throw with the response body intact (finding 6).apps/ios/Integration/Community/CommunitySessionService.swift) — Apple sign-in with a nonce, token persistence, account restore, sign-out/delete, and single-flight refresh on 401. The refresh seam now returnsCommunityRefreshOutcome(.rotated/.rejected/.unavailable):.rotatedpersists the new pair and the fresh account snapshot it carries,.rejected(dead token family) clears tokens,.unavailable(transport) keeps them for a later retry (findings 4, 5).apps/ios/Platform/Persistence/Community/KeychainCommunityTokenStore.swift) — Keychain-backed on Apple platforms, in-memory on Linux; the in-memory store serializes async access withNSLock.withLockso nolock()/unlock()call straddles anawait(finding 1).apps/ios/Platform/Persistence/Community/) —CommunityStorecaches table/card feeds and the active Table link; a pending join now persists its table name alongside the link (community_table_link_v2migration) so it restores asaskingToJoinafter relaunch, and offlinejoinByCode/createTableenqueue outbox events so intent survives transport failures (finding 3).OutboxServicegains shared-cook, table-lifecycle, and report sends with retry/conflict behavior preserved; thecommunity_table_link_v1migration keeps a newly created Table discoverable before its first feed fetch.apps/ios/Feature/Community/) — Session, Tables, and Challenges view models: restore/sign-in and account gates, load/create/join/share/leave/block with offline cache fallback, entry/voting states with server-error mapping.leave()keeps the local feed, link, and state until the server accepts, and an owner's leave fails fast with.ownerCannotLeavebefore any request is sent (the backend refuses it with 409,backend/src/tables/index.ts:402) so an owner's device cannot be stranded (finding 2). They use Observation@Observable(not Combine'sObservableObject) so they compile in the Linux mirror; no existing views referenced them.apps/ios/Tools/community-check/) —refresh.shpins the portable sources into the mirror (31 pinned sources).Review round — dispositions for the seven findings (head
f7fb825)Each code finding ships with a regression test written first and verified failing against the pre-fix code (red run recorded before the fixes, then green after):
NSLock.lock()/unlock()from async methods — macOS compile failure (CI run 38812515963, exit 65). Fixed: every store method wraps its critical section inNSLock.withLock { }inside a synchronous closure. Linux cannot catch this class; the hosted macOSbuild-and-testrun on this push is the compile check.leave()cleared local state before acceptance; an owner's leave strands the device — Fixed: local feed/link/state survive unless the leave is accepted; an owner's leave returns.ownerCannotLeavewith no request sent. Regression tests:testOwnerLeaveDoesNotDestroyTheLocalTable(fixture owner Sam; asserts the failure, intact link/cache, and that no leave request is sent) andtestLeaveConflictKeepsTheLocalTable(member fixture, stale role, drain 409 → conflict retained). Both red before the fix.community_table_link_v2), restores asaskingToJoin; offline join/create enqueue outbox rows. Regression tests:testPendingJoinSurvivesRelaunchAsAskingToJoin,testOfflineJoinSurvivesAsAnOutboxAsk,testOfflineCreateTableSurvivesAsAnOutboxRow. Red before the fix.restore()cleared tokens when the refresh leg could not run — Fixed: the client's 401 refresh closure now surfacesCommunityRefreshOutcome; restore's 401 path decides by token presence — tokens cleared mean a rejected family (sign-out), tokens surviving mean the refresh was merely unavailable and the session, tokens, and in-memory account stay for a later retry (SPEC 6.1). Regression test:testRestoreKeepsTokensWhenTheRefreshTransportFails. Red before the fix..rotatedcarries the refresh response's fresh account andrefreshAccessToken()persists it (SPEC 4.3, 6.2). Regression test:testTransparentRefreshRefreshesTheAccountSnapshot. Red before the fix.testTableFullFailureMapsToTheMockCopy. Red before the fix.restore()— finding 4 fixes it, and the claim now describes the fixed behavior.Tests/TasteStore/TasteEventStoreTests.swift(helper return type-> TasteEventat line 49, plus pre-existing LineLength violations at 259/260/291). That file is changed by trunk commit9de2985("fix(ios): cookedEvent helper returns the store record type"), which landed onobv/burst6-fridgeluck-trunkafter this branch was cut at90386c6— verified:git diff 90386c6..f7fb825 --name-only | grep -i tasteis empty (this PR's diff does not include the file), whilegit log 90386c6..origin/obv/burst6-fridgeluck-trunkshows9de2985changing it. The base branch has since advanced to54ce8ca; GitHub's PR file list can attribute those trunk changes to this PR. The LineLength violations pre-date both (the review agrees). Nothing in this PR modifies that file.Note: the base branch moved under this PR (
90386c6→54ce8ca, three commits). This branch is left rebased on its original base; integrating the newer trunk belongs to the integration flow (PR #170).How I checked it (commands and results)
At head
f7fb825(review-fix round):cd apps/ios/Tools/community-check && swift test→ 79 tests, 0 failures, EXIT=0 (six full runs during the round, including one after each macOS-only compile fix; final run atf7fb825). Excerpt below.swift format format --in-placeover the 8 changed repo sources/tests, thenrefresh.shto re-pin the mirror (HASHES.txt in sync).swift format lint --strictover the 16 changed Swift files (8 repo + 8 mirror copies) → clean. A stricter sweep over unrelated mirror files reports pre-existing violations in files this PR does not touch (CookSessionMigrations.swift,RecipeBundleV2.swift,OutboxServiceTests.swift,SharePreviewRedactionTests.swift) — unchanged by this PR.bash apps/ios/Tools/community-check/verify.sh→ EXIT=0, all source/mirror hash pins OK.swiftc -parse apps/ios/Platform/Persistence/Database/Subsystems/CommunityMigrations.swift→ clean.build-and-testis the Apple compile check (finding 1 is Linux-invisible). It surfaced three more Apple-SDK-only compile issues after this branch's Linux checks were green — each fixed and pushed (8d2c14d: async-contextlock()/unlock()in the test transport stub and aData(row[...])initializer that does not resolve on Apple;6426058: scoped locking in the stub;f7fb825: missing@MainActoron one test calling the MainActor sign-in helper), with the failing runs 38838725369, 38839520976, 38840329632 and the green run 38841204310 onf7fb825(build-and-testsuccess, ≈8m14s) as the evidence trail.At implementation head
1ced3e1(pre-review): Linux suite 71 tests, 0 failures; the hosted macOS build failed on the finding-1 compile errors, now addressed.What I could not check
build-and-testworkflow on this push is the check for that (and the first round's run proved it is needed: it caught finding 1).Assumptions
nonceclaim (read from the backend source). I generate a lowercase UUID and send it verbatim; binding it into the Apple request (SHA-256 on Apple's side) is separate wiring not present here.blockListViewreturns{ blocks: [...] }), not from memory.rewireusedtokenStore = tokenStore ?? InMemoryCommunityTokenStore(), which replaced the shared in-memory store on every rebind. Fixed with an explicit nil-check preserving the store, per the helper's documented intent.sentCountincludes the sign-in); the leave request itself is asserted absent.🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact
Linux test excerpt