Fix reconcile optimistic role updates against the real paginated cache (closes #243)#311
Merged
Conversation
…PI mocking, and automated type generation
…opment and testing
…PI mocking, and automated type generation
…re flag utilities
into fix-reconcile-optimistic-role-updates-against-the-real-paginated-cache-(closes-Adamantine-guild#243)
… branch Reconciles the optimistic role-assignment work (issue Adamantine-guild#243) with upstream's multi-community restructuring: - lib/query/member-cache.ts: keep the prefix-matching getQueriesData/setQueriesData reconciliation logic, add the optional community param threading into queryKeys.members.all(community) - lib/api/mock.ts: keep setMockRoleMutationFailure() checks in assignRole/removeRole and the resetMockData() toggle reset, combined with upstream's per-community memberStore/ensureAddress signatures - app/[communitySlug]/admin/members/page.tsx: keep toast-parity error handling in the optimistic mutations, scoped to queryKeys.members.all(communitySlug) - app/[communitySlug]/developer/page.tsx: keep the "Simulate Role Mutation Failure" dev-tools toggle alongside upstream's typed customAddress state - test/member-cache.test.ts: add missing './setup-env' import (ConfigError on load) and adapt to queryKeys.members.all() as a function - test/role-assignment-optimistic.test.ts: adapt to queryKeys.members.all() as a function instead of a static array Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
Description
Optimistic updates for role assignment were already substantially built (onMutate/onError/onSettled, snapshot+rollback, toasts, a "Saving" badge) — but the post-success reconciliation path was silently dead in production: it only ever checked the bare
['members']cache key, while the real Members page caches under a composite, filter-dependent key ([...queryKeys.members.all, { searchQuery }]) holding a paginateduseInfiniteQueryshape. Every successful mutation was falling through to a fullinvalidateQueriesrefetch instead of the surgical patch it was built to do — reintroducing the exact round-trip sluggishness this issue exists to eliminate, just moved to after the mutation instead of before. This fixes that seam, bringsremoveRoleto parity withassignRoleon error toasts, and adds a way to actually simulate and test mutation failures in mock mode.Linked Issue
Closes #243
Type of Change
Changes Made
Reconciliation seam (
lib/query/member-cache.ts)reconcileMemberRoleCachenow usesgetQueriesData/setQueriesData(prefix match) instead ofgetQueryData/setQueryData(exact match), so it actually reaches the real composite-keyed, paginated cache entry instead of silently missing it.assign, only patches entries where the address already exists — never fabricates the{ tier: 'free', active: true }placeholder row into an entry that doesn't already contain it (a differently-filtered page, for instance). Falls back toinvalidateQueriesonly when the address isn't found in any entry, preserving the "fetch real data for a brand-new member" correctness guarantee.Toast parity (
app/admin/members/page.tsx)removeRoleMutation'sonErrornever calledaddToast(only set local state) —assignRole's did. Both mutations now show the same error toast (session-expiry vs. generic failure, distinguished the same way), and both show a success toast.Simulate-able mock failures (
lib/api/mock.ts,app/developer/page.tsx)setMockRoleMutationFailure(shouldFail)— a runtime-togglable flag (unlike the module-load-timeNEXT_PUBLIC_MOCK_SESSION_STATE), checked byassignRole/removeRole, throwing a generic500distinct from the existing session-expiry simulation. Cleared byresetMockData()./developerso this is manually triggerable, not just usable from tests.Tests
test/member-cache.test.tsrewritten around the real composite/paginated shape (10 cases) — including the regression test that would have failed against the old bare-key implementation, and the two "don't fabricate into an unrelated filtered entry" cases.test/role-assignment-optimistic.test.ts(new) — integration tests wiring a realQueryClient+ realMockAccessApi+ real reducers/reconciliation together (no React rendering), one test per acceptance criterion: instant optimistic appearance during an artificially slow mutation, rollback via the new failure toggle, and no duplicate/ghost rows after that same mutation succeeds.test/mock-controls.test.ts— 5 new cases for the failure toggle itself.Screenshots / Recordings
Not captured — built and verified via dev-server + curl rather than an interactive browser session. Please add a screenshot/recording of the role-assignment optimistic flow (instant update, then a triggered failure + rollback via the new
/developertoggle) before merging.Test Evidence
NEXT_PUBLIC_MOCK_MODE=true)guildpass-core— not applicable; this is entirely a frontend cache-reconciliation fix, no API contract changesVerified via dev server + curl:
/admin/membersand/developerboth compile clean, and the new toggle renders correctly. Unit suite: 12 new/updated tests, all passing. Full suite shows the exact same 15 pre-existing failing tests present before this branch (confirmed by diffing the failing-test-name list directly, not just the summary count — unrelated to this change: policy-listing contract drift, SIWE nonce, session storage, etc.). Typecheck: zero new errors at both thetest/tsconfig.jsonscope (20-error baseline) and full-project scope (44-error baseline), confirmed by diffing full compiler output before/after, not just eyeballing counts.Checklist
npm run typecheckpasses for this branch's own changes — no new errors introduced;mainalready carries pre-existing errors unrelated to this work (documented inASSUMPTIONS.md)npm run lintpasses on all new/changed files.env.exampleupdated — not needed, no new env vars (the failure toggle is a runtime function, not an env var)Additional Notes
test/member-cache.test.tsonly ever tested the bare-key/plain-array shape, never the real composite/paginated one the page actually uses — worth keeping in mind for future cache-adjacent changes: test against the actual query key shape, not a simplified stand-in.didi168expressed interest in taking this issue — worth checking in before merging in case they'd already started work in parallel.