Skip to content

feat: add clickable GitHub profile links to contributors - #218

Open
me4502 wants to merge 4 commits into
SpongePowered:mainfrom
me4502:feature/clickable-github-profile-links
Open

feat: add clickable GitHub profile links to contributors#218
me4502 wants to merge 4 commits into
SpongePowered:mainfrom
me4502:feature/clickable-github-profile-links

Conversation

@me4502

@me4502 me4502 commented Jul 18, 2026

Copy link
Copy Markdown

Description

This PR adds GitHub profile links for contributors to each version, allowing better attribution, as well as consistency for people who accidentally commit under multiple emails. It's done transparently, so any commits that do not have this data will fallback to the old system, making this non-breaking.

The only caveat here is that it does add requests to the GitHub API. For rate limit purposes, I've allow an optional GitHub token to be supplied to benefit from higher limits, however it'll work fine by default with the lower limits. If it fails, it'll just not store the data. Main goal with this is to keep it non-breaking, and never let these additional requests hold back a build, eg during a GitHub outage.

Type of Change

  • feat: New feature (non-breaking change which adds functionality)
  • fix: Bug fix (non-breaking change which fixes an issue)
  • docs: Documentation update
  • refactor: Code refactoring (no functional changes)
  • perf: Performance improvement
  • test: Adding or updating tests
  • chore: Maintenance tasks (dependencies, tooling, etc.)
  • ci: CI/CD configuration changes
  • BREAKING CHANGE: This change breaks backwards compatibility

Testing

  • Tests pass locally with go test ./...
  • Added/updated tests for new functionality
  • Generated code is up to date (go generate ./...)

Commit Message Format

Checklist

  • My code follows the project's style guidelines
  • I have run go generate ./... to update generated code
  • I have updated documentation as needed
  • My changes do not introduce new warnings or errors
  • I have added tests that prove my fix/feature works
  • All tests pass locally

@me4502
me4502 force-pushed the feature/clickable-github-profile-links branch from d954449 to f37a147 Compare July 18, 2026 07:30
@gabizou

gabizou commented Jul 27, 2026

Copy link
Copy Markdown
Member

This is really awesome, thank you for the PR. I hope that the codebase was simple enough to consider the feature. I'd like to explore the current workflows and how this PR currently suggests changes.

pr-proposed
Ok, so what's wrong with resolving inside the store activities?

A few things, and they all end up fighting the goals in the description:

  • Failures are silent and permanent. The lookup is best-effort inside an activity that effectively succeeds with or without data. In a VersionSync, once a version is marked with a valid enrichedAt, that version effectively no longer has any work to be performed. If there's an active GitHub outage (as much as we don't want to have outages) or intermittent connectivity, the activity is still marked as successful. Only by a break-glass reindex would the commit author's GitHub user be resolved.
  • Backfilling is a manual effort Rewriting attribution on the versions already in the database is the actual point of the feature, and nothing in this shape ever re-touches an enriched row. Hand-written SQL against commit_body is not a path I want us on.
  • One activity, two side effects. The DB write is now coupled to a network fan-out, which is exactly why the PR had to raise StoreChangelog to a 2 minute timeout and carve a 5 second persistence reserve out of the deadline. Traditionally in Temporal, this violates idempotency rules due to the work being retried with the same input, and somewhat keeps a DB transaction open longer than it needs to be. Ideally, we'd be performing the work up front in the activity and persisting results in to the DB in one shot at the end. Likewise, if there's a batch of data to resolve, we'd ideally use activity heartbeats to be able to resume work in worst case scenarios.
  • Cache is limited to a singular instance I understand we could easily use something like Redis to keep a multi-instanced timer to help with rate limits but in this case, the sync.Map wouldn't help and doesn't endure between pod restarts/redeployments, contrary to what ideally we'd be using to schedule with an activity likely in Temporal where we could allow for Temporal's own scheduler handle the rate limits.
So where does it go instead?

Resolution becomes its own thing entirely, three pieces:

  1. The store activities revert to pure DB writes (30s timeouts again).
  2. One new activity, ResolveGitHubAuthorsBatch(versionIDs), owns all GitHub interaction, backed by a persistent github_user_cache table (email → login, with negative entries on a TTL) that survives deploys and is shared by every worker. Both it and the store merge take the row FOR UPDATE (read: there's a latent last-writer-wins race on commit_body today that this feature would have started exercising).
  3. A singleton GitHubAuthorResolutionWorkflow runs on its own Temporal schedule, every 2 minutes with Overlap: SKIP. Each run grabs one keyset page of enriched versions missing a resolution marker (newest first), resolves the whole page in one batch activity with bounded retries, stamps the marker, and continues-as-new with the cursor. On an empty page, it will complete until the next tick starts a new one.
new-proposed

A version that fails resolution is skipped without stamping its marker, so the next tick's query picks it right back up. A rate-limited 403 comes back as a retryable error with the retry delay taken from X-RateLimit-Reset instead of a blind exponential. In short, durable retries live where Temporal can actually honor them, and the sync pipeline is none the wiser.

Why its own schedule instead of kicking it off from the sync?

Mostly because the resolver doesn't actually need to be told anything. The work discovery is a query against the marker, and the workflow re-runs that query at the start of every run, so any nudge from the sync tree would only be telling it something it's about to find out on its own. Ideally, we let Temporal's scheduler do what it's designed for here: Overlap: SKIP means a tick can never stack on top of a run that's still working, similar to what we already do for version syncs.

There's also a mechanical reason: separating the concern of signaling a workflow from multiple sources makes operation easier to consider when we can leverage Temporal's own clock for scheduling.

And the versions already in the database?

This ends up being the nicest property of the design: the backfill is the steady-state loop. The marker query doesn't care when a row was enriched, so the first unpaused run walks the entire history newest-first and drains it in roughly 130 pages, while newly synced versions keep landing in the first page and never end up stuck behind the backlog.

I sized this against our staging database before settling on the shape: across the three artifacts there are ~6.4k enriched versions carrying ~48k author entries, but only 191 unique author emails, and 44 of those are users.noreply.github.com addresses that resolve locally without an API call. That leaves just over a hundred GitHub API calls for the entire history of the project, deduped across artifacts by the cache. Even the unauthenticated 60/hour budget would absorb that within a few hours of backoff, and with a token it's a matter of minutes. The rollout then becomes fairly boring, in a good way: ship with the schedule created paused, confirm GITHUB_TOKEN is on the worker, and unpause once. Your older commits would resolve to your current GitHub login through the same code path as new data, and anyone who renames their account later effectively gets the same fix for free.

So, what does this mean for the PR?
  • Keep: the githubapi package, the domain/OpenAPI/api.gen.go additions, the frontend fallback chain with the escaped profile links, and the token config. All of that lands as-is.
  • Move: resolveCommitInfoAuthors / resolveChangelogAuthors out of the store activities and into the new batch activity.
  • Drop: the 2 minute StoreChangelog timeout and the 5 second persistence reserve, since neither is needed once the stores go back to being pure writes.
  • Add: the github_user_cache migration, the resolution marker, the workflow + schedule + registration, and the docs updates. I'm more than happy to stack these on this PR or take them as follow-up commits on my end, whichever you'd prefer.

TLDR; all of the client/domain/frontend work is great as-is, the GitHub calls move out of the store activities into a dedicated singleton workflow on its own schedule with a persistent cache, and the historical backfill (the actual reason for the feature) falls out of the same loop for free.

@me4502

me4502 commented Jul 28, 2026

Copy link
Copy Markdown
Author

Thank you! I've updated the PR to work similar to the suggested design.

There's one part to note, which is that due to the usage of a partial index, there's a hardcoded schema constant (1) a few times throughout the schemas/queries. I've made sure a test is in place to verify that these don't drift, but it's potential risk if another spot is added without being added to the test (eg, test drift).

It's been a fair while since I've worked with Postgres, so I might be wrong, but it might be potentially resource intensive to not use a partial index here due to it being unindexed JSON data.

Copilot did have the following suggestion, but I felt I'd stick with what I was familiar with for now:

There's also a design-level alternative: keep the query as "marker missing" only, and when the logic changes ship a migration that clears the marker ( commit_body - 'authorResolution' ). The index predicate becomes stable, no literal, no drift, steady-state index still empty. Cost is a bulk rewrite of ~6.4k rows at migration time, and it moves staleness out of the query — which is a deviation, since the diagram explicitly specifies "marker missing/stale" as query semantics.

Please let me know if you're happy with the current setup, or would prefer an alternative (either the Copilot suggestion or another) :)

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.

2 participants