Skip to content

Refine Thanks credits and move its footer link - #242

Merged
guohai merged 6 commits into
mainfrom
fix/thanks-community-credits
Oct 4, 2026
Merged

guohai merged 6 commits into
mainfrom
fix/thanks-community-credits

Conversation

@guohai

@guohai guohai commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

Changes

  • Move the single Thanks link to the bottom-left footer beside the community credit.
  • Remove the Stripe, Google, Discord, Anthropic, and MaxMind acknowledgments, plus the interface, platform, and building/shipping sections.
  • Keep evaluation/audio credits, DB-IP attribution and its CC BY 4.0 link, and the Mediabunny license/source notice.
  • Align page metadata and README descriptions, and add content, navigation, and responsive-layout regression tests.
  • Preserve the required MaxMind license notice outside the curated Thanks page only when GeoLite2 is loaded. Identify loaded sources from MMDB headers or legacy filenames, with refresh metadata as a fallback; retain the existing API shape and DB-IP notice, and document this licensing boundary.
  • Keep raw server notices when wording differs from a cached client, handle mixed-source readers, and revalidate a lightweight in-memory notice endpoint every minute. Persistent config retains its one-hour client cache. Admin refresh completion invalidates the notice query, and admin status credits reflect loaded rather than merely configured data.

Validation

  • Final TypeScript checks and 75 focused unit/security tests pass. Scoped lint has no errors and four pre-existing location logging warnings; the production build passes with existing bundle-size and import.meta warnings.
  • The mandatory ./scripts/full-tests-run.sh gate passes on the final commit in a task-owned isolated database/network/runtime: 2,539 unit/integration tests pass (40 skipped), all 29 audio tests pass, and 237 browser tests pass (16 skipped).
  • All GitHub build and automated review checks pass. Final reviews find no blocking or material issues. Legacy attribution, stale public queries, exact-string fallback, admin-status accuracy, persistent-config polling, mixed resolved/unresolved readers, and the route-scanner convention are covered.
  • No backend integrations, dependency packages, production plugin configuration, or other agents' work are changed.

Review Follow-Up

The GeoLite licensing concern is addressed by a source-specific footer notice, without restoring a commercial-company card. MaxMind's GeoLite EULA requires attribution of use: https://www.maxmind.com/en/geolite2/eula. The current production site's public /api/config confirms DB-IP attribution, and that existing notice is preserved.

An earlier full-gate run caught a route-scanner declaration mismatch; the endpoint now uses the established inline async-handler convention, the security scanner tests are unchanged, and the complete final gate passes. Mixed-source notices intentionally retain complete plain text and provider URLs as a safe fallback; optional link styling is non-blocking.

Generated with SMT smt@agora.build

Keep evaluation/audio projects and DB-IP data attribution, remove the requested credits and sections, and place the Thanks link beside the community credit. Add regression coverage for content, navigation, and responsive layout.

🤖 Built with SMT <smt@agora.build>
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

No blocking issues. This PR only changes content and layout, with no server, auth or data-handling code, so there's nothing security-relevant.

Worth a look

  1. The MaxMind GeoLite2 credit is removed, but Vox still uses GeoLite2 data. server/geoip-refresh.ts downloads GeoLite2 when a MaxMind key is configured, and the admin Regions page shows source: "geolite2". MaxMind's GeoLite license has asked for attribution ("This product includes GeoLite2 data created by MaxMind…") when the data is used in a product. If the reason for removing it was "no commercial companies", confirm that no GeoLite2 attribution is still owed. If one is, put it back as a data credit next to DB-IP, which is where the new "Data attribution" group puts it anyway.

  2. The "Data attribution" description now points to a source the page doesn't name. It says "The active source depends on the deployment's configuration," but DB-IP is the only entry left. Readers will wonder what the other source is. Either name GeoLite2 (see point 1) or reword the line to fit a single-entry list.

  3. On mobile the footer credit is no longer centred. The parent row uses flex-col items-center below sm. The new wrapper is w-full flex-wrap with no justify-center, so on narrow screens "© … Built with 💖" and "Thanks" sit flush left while the links row beneath them stays centred. Adding justify-center sm:justify-start to the wrapper would fix it.

Fine as-is

  • tests/e2e/thanks.spec.ts imports client/src/lib/acknowledgments.ts directly. That's safe because the module is dependency-free (no @/ aliases), and tying the link-count assertion to the data source is an improvement over the old > 25 check.
  • The commercial-names E2E check uses new RegExp(\^${name}\b`)` on a fixed, hard-coded list, so there's no regex-injection risk.
  • The ! non-null assertions on boundingBox() come right after toBeVisible() / scrollIntoViewIfNeeded(), which is acceptable in a test.
  • The footer link keeps a single data-testid="link-footer-thanks", and the test confirms there is exactly one, so nothing is duplicated.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

No findings. The changes match the PR description, preserve the required DB-IP and Mediabunny notices, safely handle external links, and add appropriate content/navigation/responsive regression coverage. No security, logic, performance, or maintainability regressions identified.

Keep curated Thanks credits separate from required license notices. Attribute GeoLite2 to MaxMind only when loaded metadata selects that source, preserve the DB-IP notice, and cover metadata changes and footer behavior with regression tests.

🤖 Built with SMT <smt@agora.build>
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Overall this looks good. Moving the GeoIP credit into a shared shared/geoip-attribution.ts is a clean change. Footer credits are now chosen per source, and loading by source refuses lookalike keys such as __proto__ and constructor, with unit tests covering both. No security issues found. A few small points:

1. Admin GeoIP status still treats DB-IP as the only attributed source (server/routes.ts:1200)

  • attribution: source === "dbip" ? DBIP_ATTRIBUTION : null was left alone, so /api/admin/geoip/status returns null for GeoLite2.
  • That now contradicts the new comment in location.ts ("Both DB-IP Lite and GeoLite2 require public attribution") and the new DEPLOYMENT.md text.
  • Nothing breaks today, because admin-regions.tsx:250 also checks for source === "dbip".
  • Suggest attribution: geoipAttributionForSource(source) so the shared helper is the only place this is decided. DBIP_ATTRIBUTION could then be dropped from geoip-refresh.ts.

2. The footer credit fails closed silently when the strings don't match (client/src/components/layout.tsx:68-70)

  • The client finds the credit by exact match on the server's attribution string.
  • If the server's string and the client bundle's string ever differ (a cached old bundle during a deploy, or someone editing the wording on one side), the footer shows no license notice. Before this change, any non-empty attribution was rendered.
  • For a notice that a license requires, it would be sturdier to have /api/config send the source key (geoipSource: "dbip" | "geolite2") and look it up with GEOIP_ATTRIBUTIONS[key]. Another option is to fall back to rendering the raw geoipAttribution string when nothing matches.

3. Databases without a meta file get no attribution (server/location.ts:246-252, pre-existing, but the new docs promise more)

  • The source comes only from geoip-meta.json.
  • If an operator places GeoLite2 or DB-IP .mmdb files in GEOIP_DB_DIR by hand (no refresh, so no meta file), the readers load but no notice appears.
  • The new DEPLOYMENT.md paragraph says the notices are always kept. Either note that this depends on the meta file, or work out the source from the database's metadata (reader.metadata.databaseType) as a fallback.

4. Test nit

  • tests/geoip-attribution.test.ts mocks fs.readFileSync for the whole module and relies on reloadGeoReaders reading only through it. That works today.
  • If more readFileSync calls are added to location.ts (for example, ASN classification), they will quietly get the mocked JSON. It's worth a comment, or matching on the path in mockImplementation.

The e2e coverage is thorough: footer placement, no overflow at 375px, and the notice present or absent per source.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

  • [P1] Legacy GeoLite2 databases can still load without any MaxMind attribution — server/location.ts:246. reloadGeoReaders() explicitly supports existing GeoLite2-City.mmdb / GeoLite2-ASN.mmdb files, but attribution depends exclusively on geoip-meta.json. Older/manual deployments without that newer metadata file get null; if the startup refresh fails, they continue serving GeoLite2 indefinitely without the required notice. Infer geolite2 when the loaded fallback filenames identify it, or persist/migrate source metadata independently of a successful refresh.

  • [P2] Clients can show stale or missing attribution for at least an hour after a runtime source change — client/src/components/layout.tsx:65. Admin refresh can switch the active database between DB-IP and GeoLite2, but /api/config remains fresh in React Query for one hour and the refresh path does not invalidate/refetch it. A mounted client may therefore display the previous provider’s notice—or none—after new data is loaded. Invalidate this query when refresh completes, reduce/refetch the attribution interval, or expose attribution through a separately refreshed query.

Identify loaded providers from MMDB headers or legacy filenames, retain notices when client wording differs, refresh public credits after data-source changes, and align the admin status notice. Add regression coverage for legacy, mixed, missing, and runtime-changing data sources.

🤖 Built with SMT <smt@agora.build>
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Overall: No security issues. The shared attribution table removes the hard-coded DB-IP strings that were spread around, and identifying the source from the mmdb header first is more reliable than relying on geoip-meta.json alone. The tests cover the edge cases well: a stale metadata file, legacy filenames, mixed sources, and __proto__/constructor as the source value. Three things are worth a look:

1. /api/config is now polled every 60s by every visitor (client/src/components/layout.tsx:65-66)

  • Layout wraps every public and console page, so each focused tab now refetches every minute. Before, it fetched about once an hour.
  • Each refetch runs storage.getAllConfig(), which is a DB query, on an unauthenticated endpoint.
  • The attribution only changes after a GeoIP refresh, which happens weekly or when an admin clicks Refresh. The admin page already calls invalidateQueries(["/api/config"]) when lastRefresh.at changes, so admins see the update straight away.
  • Suggestion: drop refetchInterval, or set it to something like 15–60 minutes. A public visitor seeing the old credit for a while after a source switch is harmless.

2. The admin Regions page and the footer can show different credits (server/routes.ts:1180,1200 and client/src/pages/admin-regions.tsx:257)

  • /api/admin/geoip/status works out source with resolveGeoipSource(maxmindKey). That is the source the instance is configured for, not the data that is loaded.
  • The footer, meanwhile, now reads the actual reader headers.
  • Example: an admin saves a MaxMind key but the GeoLite2 download fails, so the DB-IP files stay loaded. The footer correctly credits DB-IP, while the admin page shows the MaxMind/GeoLite2 credit.
  • The admin page also shows a credit when no database is loaded at all, because the old source === "dbip" guard is gone.
  • Suggestion: return getGeoipAttribution() here, or label the field clearly as the configured source.

3. Mixed-source fallback text in the footer (layout.tsx:69-71, 388)

  • When City and ASN come from different providers, geoipAttribution is the two strings joined together. No GEOIP_ATTRIBUTIONS entry matches that, so the footer falls back to raw text with no links.
  • The credit is still legally sufficient, and this only affects manually installed databases.
  • Possible change: have /api/config send the list of sources rather than a joined string, so the client can render one linked credit per provider. Not a blocker.

Minor:

  • tryOpen falls back to the filename only for GeoLite2-*. A City.mmdb with no header falls through to the metadata file, which is the intended behavior. If an unexpected databaseType comes back, a debug log line would help.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

  • Medium — client/src/components/layout.tsx:65: Every open page now polls /api/config once per minute. That endpoint performs storage.getAllConfig() on every request (server/routes.ts:6379), so traffic and database queries scale continuously with active browser tabs merely to detect a GeoIP source that normally changes weekly or through an admin action. This is a significant load regression from the previous one-hour cache. Prefer a substantially longer interval, a lightweight/cacheable attribution endpoint, or invalidation/version signaling that does not query persistent configuration every minute.

Add a public in-memory attribution endpoint, restore the one-hour config cache, and keep active-source credits synchronized across the footer and admin status. Cover endpoint behavior, source removal, and avoiding repeated config requests.

🤖 Built with SMT <smt@agora.build>
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

The change looks sound. I found no security issues: the new /api/geoip/attribution endpoint is unauthenticated, but it only returns a constant credit string. A few minor points:

Logic and edge cases

  1. Mixed sources lose their links (server/location.ts refreshAttribution). If the City and ASN databases come from different sources (for example, the City DB is DB-IP and a stale GeoLite2-ASN.mmdb is still on disk), geoipAttribution becomes both strings joined with a space.

    • The joined text has no separator between the two credits: "…CC BY 4.0 This product includes GeoLite2…". The DB-IP string has no trailing period.
    • In layout.tsx and admin-regions.tsx, GEOIP_ATTRIBUTIONS.find(c => c.attribution === geoipAttribution) then matches nothing. Both pages fall back to the raw text with no provider links.
    • Suggested fix: return a list of sources (or credits) from the server and render one credit per source.
  2. The Thanks page omits MaxMind (client/src/lib/acknowledgments.ts). The "Data attribution" group says "The active source depends on the deployment's configuration", but it now lists only DB-IP. The MaxMind entry was removed in the same PR that makes GeoLite2 attribution required. The footer still covers the license requirement, so this is a consistency gap only.

Performance

  1. The footer polls on every public page (layout.tsx, refetchInterval: 60_000). Every open tab hits /api/geoip/attribution once a minute, purely to catch a change that only happens after an admin refresh or a weekly timer. The endpoint is cheap and in-memory, and background tabs don't poll by default, so it isn't a problem today. But it is constant traffic for a value that almost never changes, and admin-regions.tsx already invalidates the query when lastRefresh.at changes. A plain staleTime with refetch on window focus would cover cross-session freshness without the interval.

Minor

  • The /api/admin/geoip/status route still returns source from resolveGeoipSource(maxmindKey), which is the configured source, while attribution now reflects the loaded databases. During the window before a refresh completes (for example, right after a key is saved), the admin page can show "source: geolite2" next to the DB-IP credit. That is arguably correct, but it may confuse admins; consider labelling the field.
  • The useEffect in admin-regions.tsx also invalidates /api/config on first mount whenever a refresh has happened. That causes one extra fetch and is harmless.

Otherwise, deriving the source from the mmdb databaseType and falling back to the meta file is a good fix for legacy or manually copied databases. Moving the credit strings into shared/geoip-attribution.ts removes the hardcoded DB-IP link that was duplicated across pages.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

  • Medium — server/location.ts:260: refresh metadata is consulted only when none of the loaded readers has a recognized source. If one reader is recognized and the other has an unknown header/canonical filename, metadata is ignored entirely. A mixed installation such as a GeoLite2 City reader plus an unrecognized DB-IP ASN reader with geoip-meta.json identifying DB-IP will emit only the MaxMind notice, omitting required DB-IP attribution. Apply the metadata fallback per unresolved reader, or conservatively add its valid source whenever any loaded reader remains unresolved. Add coverage for one recognized and one unresolved reader with metadata naming the second source.

Conservatively preserve both required notices when one loaded reader identifies its provider and another relies on refresh metadata. Add mixed-source regression cases and separate combined credit text clearly.

🤖 Built with SMT <smt@agora.build>
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

The change is sound overall. The new public /api/geoip/attribution route returns only an in-memory string, so it has no auth or data-exposure risk. Detecting the source from the database header (falling back to the filename, then to geoip-meta.json) is more reliable than the old meta-only lookup. It also fixes a real compliance gap: the GeoLite2 credit required by MaxMind was missing before. I found no blocking issues, only these smaller points:

  1. Mixed-source credit loses its link. In server/location.ts, when the City and ASN databases come from different sources, refreshAttribution joins the two strings with "; ". Neither layout.tsx nor admin-regions.tsx can match that combined string in GEOIP_ATTRIBUTIONS, so they fall back to plain text with no provider link. The license text itself is still complete, so this is cosmetic. A cleaner fix is to send a list of sources (or a list of attributions) and render one credit per source, instead of matching the client side against a joined string.

  2. Every public visitor polls once a minute. layout.tsx sets refetchInterval: 60_000 on the footer query, so every open tab of every public page makes a request each minute for a value that only changes after a GeoIP refresh. That's cheap per request and far below the 1000/15 min rate limit. Still, it's ongoing traffic across all visitors to cover a rare event, and the admin page already invalidates the query after a refresh. A longer interval, or just refetchOnWindowFocus, would probably do. This is a judgment call.

  3. The admin cache invalidation also fires on first load. In admin-regions.tsx, the useEffect keyed on lastRefresh?.at runs when the page first loads with an existing refresh result, not only when a refresh finishes. That makes one extra fetch of /api/config and /api/geoip/attribution per visit. It's harmless; tracking the previous value would skip it if you care.

  4. Unused source field. In server/routes.ts, /api/admin/geoip/status now reports attribution from what the server has actually loaded, while source still reflects which source the configured key would use. These can legitimately disagree, for example when a key is configured but the last download failed. That's arguably more correct than before. However, geoipStatus.source is no longer read in admin-regions.tsx, so check whether anything else still uses the field.

The large cut to the Thanks page (it drops React, Node, Stripe and others) is a product decision rather than a code issue. The README and DEPLOYMENT.md were updated to match.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

No actionable issues found.

The GeoIP attribution logic correctly derives notices from loaded MMDB headers, supports legacy filenames and mixed sources, falls back safely to refresh metadata, and exposes only static or React-escaped content. Admin routes retain authentication, the public endpoint exposes no sensitive data, and the focused tests cover the important source-transition, stale-metadata, missing-reader, caching, navigation, and responsive-layout cases.

Use the established inline async handler form so the admin-route invariant scanner accounts for the new public read-only route without weakening its checks.

🤖 Built with SMT <smt@agora.build>
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Overall the change is sound. I found no security issues and nothing that blocks merging. Notes follow, most important first.

Logic / edge cases

  • Attribution now follows the databases actually loaded, which is an improvement. server/location.ts now works out the credit from each loaded database's own metadata, falling back to the legacy filename and then geoip-meta.json. This fixes a real bug: a partly failed refresh, where City.mmdb was swapped but ASN download threw, left the old meta-based credit attached to a new database. The ordering also holds. refreshGeoipDatabases calls deps.reload() before it sets lastResult, so when the admin page reacts to a new lastRefresh.at, the fetch it triggers sees the updated credit.
  • Mixed sources lose the link. If City and ASN come from different sources, refreshAttribution joins the two credits with "; ". Then GEOIP_ATTRIBUTIONS.find(...) matches neither, in layout.tsx or admin-regions.tsx, so both pages fall back to plain text with no link. Both URLs still appear in the text, so the license notice survives. A list or source array instead of a joined string would keep the links. This case is rare.
  • The admin effect fires on first load too. In admin-regions.tsx, the useEffect on lastRefresh.at runs on mount whenever a past refresh exists. That invalidates /api/config and /api/geoip/attribution on every page visit, not only after a refresh completes. It's harmless (two cheap refetches), but it doesn't match the "Admin refresh completion invalidates both queries" wording in DEPLOYMENT.md. Tracking the previous value in a ref would make it fire only on an actual change.

Performance

  • Every public page now polls. Each one calls /api/geoip/attribution every 60 s through refetchInterval. The endpoint only reads memory and TanStack pauses polling in background tabs, so the cost is small: about 15 requests per 15 minutes per tab, against the 1000-per-IP limit. Still, the credit only changes when the databases reload, which happens weekly or when an admin triggers it. A longer interval, or just staleTime plus refetch-on-focus, would do the job. Low priority.

Code quality

  • /api/config and /api/geoip/attribution now both carry geoipAttribution. The footer prefers the new endpoint and falls back to config; the code comment says the fallback is there for clients and servers mid-upgrade. Consider a follow-up to drop the field from /api/config once the new route has shipped, so there's one source of truth.
  • Thanks page scope. Removing the interface, platform and tooling groups leaves Thanks with only the evaluation and data-attribution groups. The README and meta text now say "projects, libraries, and open data", which still fits what's left. I'm treating that as a deliberate product choice, not a defect.
  • Missing trailing period. In the footer, the GeoLite2 sentence built from geoipCredit ends "available from https://www.maxmind.com" without the period that the canonical attribution string has. Cosmetic only; the required content is all there.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

No material issues found.

The changes preserve authentication on admin GeoIP routes, safely render server-provided notices through React escaping, avoid persistent-config polling, and cover legacy filenames, MMDB metadata, mixed sources, stale notices, and responsive footer behavior with regression tests. I could not rerun tests because the workspace is read-only; assessment is based on the PR diff and surrounding implementation.

@guohai
guohai merged commit fe85ad8 into main Oct 4, 2026
7 checks passed
@guohai
guohai deleted the fix/thanks-community-credits branch October 5, 2026 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant