fix: restore and test typed analytics mock accessors - #395
Merged
Lakes41 merged 2 commits intoAug 19, 2026
Conversation
Remove the duplicate untyped declaration that prevented the mock API from parsing while preserving the AnalyticsDataSource contract.
Move the existing analytics fixture suite into the discovered test directory and add focused public-contract coverage for membership trends, role distribution, and resource access.
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
This PR restores the typed analytics accessor implementation in
MockAccessApiand adds focused regression coverage for its public analytics contract.The analytics section of
lib/api/mock.tscontained two consecutive declarations for the sameanalyticsproperty. The second declaration usedanyand left the module syntactically invalid, preventing the test compiler from parsing the mock API.The implementation removes only the duplicate untyped declaration and preserves the existing
AnalyticsDataSourcecontract. It then moves the pre-existing analytics fixture tests into the repository's discoverabletest/directory and adds direct tests for every accessor exposed throughMockAccessApi.analytics.The change intentionally remains small: it does not alter analytics behavior, production API contracts, CI/CD configuration, generated types, application rendering, or the live API implementation.
Linked Issue
Closes #391
Type of Change
Changes Made
public analytics: anydeclaration that preventedlib/api/mock.tsfrom parsing.public analytics: AnalyticsDataSourcedeclaration.lib/api/analytics/mock.test.tstotest/mock-analytics.test.ts, where it is discovered by the existing Node.js test runner configuration.getMembershipTrendgetRoleDistributiongetAccessAttemptsAnalyticsDataSource. If another accessor is exposed later without corresponding test coverage, the suite will fail and require an explicit review.resourceAccesscoverage through the publicgetAccessAttemptsaccessor.newMembersvalue matches the change intotalMembers.member,moderator, andadmin.package.json,test/tsconfig.json, OpenAPI fixtures, generated API types, or UI components.Why This Approach
Several approaches were considered during the investigation:
1. Keep the tests under
lib/api/analytics/and change the test runner configurationThis would make the tests discoverable, but it would expand the change into repository-wide test configuration. The established convention is to place executable tests directly under
test/, and the issue explicitly requests the repository's existing framework and conventions.2. Test only
MOCK_ANALYTICS_SUMMARYand its internal helper functionsThe previous suite already provided strong fixture-level coverage, but those tests could remain green while the public
MockAccessApi.analyticsaccessors returned a different or malformed structure. This would not fully protect the behavior consumed by the application.3. Render the Next.js analytics page
A rendering test would exercise substantially more code, including authentication state, React Query behavior, and component setup. It would be slower, more brittle, and less precise when diagnosing an accessor regression.
4. Directly test the public mock API while preserving the existing fixture tests
This approach was selected because it provides the strongest issue-specific signal with the smallest production change. It follows the repository's Node.js test conventions, exercises the exact API consumed by the analytics page, and avoids coupling the tests to React or Next.js rendering.
The work is separated into two conventional commits so the parser correction can be reviewed independently from the test relocation and coverage changes:
fix: restore typed analytics mock accessortest: cover analytics mock accessorsAcceptance Criteria Verification
AnalyticsDataSourcehave directMockAccessApi.analyticstests. An additional surface test enumerates the three exposed method names.resourceAccessis explicitly covered if exposed through the APIgetAccessAttemptsvalidates the complete public shape and representative values for all three seeded resources.node:test,node:assert/strict,test/setup-env, and the existing top-leveltest/discovery pattern.Screenshots / Recordings
Not applicable. This PR does not modify UI behavior or rendering.
Test Evidence
Focused committed suite
Static validation
git diff --checkalso completed successfully.Repeated and adversarial simulations
The focused suite and accessors were additionally exercised through the following local audit scenarios:
Math.random() = 0Math.random() = 0.999999guildpass-demobuilders-collectivedesign-guildguildpass-hubThese simulations were used as an adversarial local audit. They are not added as expensive repeated CI tests.
Repository-wide test limitation
The repository's current
npm testcommand is:The TypeScript phase reports 76 existing diagnostics and exits before the Node.js runner starts. The diagnostics include unrelated existing issues in:
CommunityStatepersistence compatibilityinvalid_stateAPI error codestest/admin-guard.test.tsxtest/pending-actions.test.tsA fresh partial verification was therefore performed with:
The first command still reports the existing repository diagnostics but emits an updated focused test artifact. The second command passes all 21 analytics tests.
This PR does not claim that the repository-wide
npm testornpm run typecheckcommands are green.Mock/live coverage
MockAccessApiguildpass-core-- not applicable; no live API behavior was changedOut-of-Scope Findings
The adversarial audit identified several existing concerns. They were intentionally not modified because they would change runtime behavior or broaden this focused test-coverage PR.
AbortSignal behavior
The mock analytics accessors accept an
AbortSignal, but currently ignore it. Calling an accessor with an already-aborted signal still resolves normally.Changing this would require defining cancellation semantics for the mock API and adding corresponding error-handling behavior. That should be handled as a separate contract-level change.
Shared mutable response references
getMembershipTrendandgetAccessAttemptsreturn arrays backed by shared fixture references. A consumer can mutate a returned object and affect subsequent calls.Defensive cloning or immutable fixtures would reduce this risk, but either approach changes existing mock behavior and should be reviewed separately. The tests in this PR deliberately avoid asserting this reference identity.
Analytics contract and documentation drift
MockAccessApi.getAnalyticsSummary()exists as a concrete public class method but is not declared on the composedAccessApicontract.The current application consumes
api.analytics.getMembershipTrend,getRoleDistribution, andgetAccessAttempts, while parts ofdocs/architecture.mddescribe a different client-computed analytics flow.The corresponding
LiveAccessApi.analyticsproperty also remains typed asany.Reconciling the legacy summary method, live API typing, consumers, and architecture documentation would be valuable, but it is larger than the focused mock-test issue.
Existing repository validation failures
npm run typecheckcurrently stops at an unrelated unclosed<Card>element inapp/[communitySlug]/admin/settings/page.tsx.The full lint command also reports unrelated existing errors in billing, admin, developer, policy-dialog, admin-module, settings, and wallet files.
These files were not changed by this PR.
Existing dependency advisories
The local dependency installation reported 31 advisories in the existing dependency graph:
It also reported an existing security warning for the installed Next.js version.
No dependency manifests or lockfiles were changed because dependency remediation is unrelated to this issue and may require coordinated breaking upgrades.
Checklist
CONTRIBUTING.mdnpm run typecheckpasses -- blocked by pre-existing repository errors described abovenpm run lintpasses globally -- focused lint passes; global lint is blocked by pre-existing errorsnpm run check-typespassesAdditional Notes
The branch was created from an updated
upstream/mainwith no commit divergence before the implementation began.The PR is intentionally limited to restoring the typed mock accessor and protecting its current public response contract. The broader findings above are documented for maintainers rather than silently bundled into this change.