Skip to content

Never show a fabricated GitHub star count - #277

Draft
evnchn wants to merge 1 commit into
mainfrom
fix/github-stars-no-false-zero
Draft

Never show a fabricated GitHub star count#277
evnchn wants to merge 1 commit into
mainfrom
fix/github-stars-no-false-zero

Conversation

@evnchn

@evnchn evnchn commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Verdict: nicegui.io stops rendering a fabricated 0 GitHub Stars. An unknown count now hides the stat tile instead of showing a plausible-looking wrong number, and a failed fetch retries in a minute (backing off) rather than waiting out the full hour. Fixes zauberzeug#6211. Draft — the maintainers picked no fix direction yet, so this implements directions 1 + 2 from the issue and leaves direction 3 (auth token / shared cache) alone.

Motivation

website/github_stars.py had three compounding properties, reported in zauberzeug#6211 with 58/58 region-correlated samples against production:

  1. the failure fallback was a wrong number, not an absent one — the seed is the literal string '0';
  2. the cache that would rescue it (app.storage.general) is per-machine, so a healthy Fly machine's value never reaches a failing one;
  3. a failed fetch got no short retry — one transient failure cost a full hour of 0.

Result: everyone routed to the Tokyo machine saw a confident 0, while San Jose served 16k+.

Implementation

  • Unknown is empty, not zero. GitHubStars.string seeds to ''; the sponsors stat tile binds its visibility to the value being non-empty, so an unknown count hides the tile rather than showing a wrong number. The nav badge's label simply renders empty next to the GitHub icon.
  • Retry with backoff. A failed fetch drops the timer interval to 60 s and doubles it after each further failure up to the hourly cadence, then starts over; a success resets it to hourly. That is ~3.9 requests/hour in the worst case — well under GitHub's 60/hour unauthenticated limit, which is the leading hypothesis for the original failure.
  • The dataclass default moved from an import-time storage read to an explicit constructor argument. Same behaviour for the module's singleton, and it makes the fresh-machine seed deterministic for tests.
Empirical before/after — the real component, fetch forced to fail

A throwaway script served the real sponsors_section with github_stars.httpx swapped for a client that raises ConnectError, with no .nicegui/ cache present (a cold machine whose fetch fails — the production condition). The served payload was curled and inspected.

Before (upstream main): the tile is visible and renders a bare 0.

"13":{"tag":"div","class":["nicegui-column","items-center","gap-0"],"children":[14,15,16]}
"15":{"tag":"div","text":"0","class":["text-[1.5rem]"]}

After (this branch): the tile carries hidden and the label is empty.

"13":{"tag":"div","class":["nicegui-column","items-center","gap-0","hidden"],"children":[14,15,16]}
"15":{"tag":"div","text":"","class":["text-[1.5rem]"]}

Server log confirms the failure path ran: failed to fetch GitHub star count.

Note: an earlier run of the same probe accidentally proved the happy path too — the real hourly timer fired at startup, fetched successfully, and rendered 16k+ with the tile visible.

Regression tests — confirmed to fail without the fix

tests/test_website_github_stars.py, 3 tests. With the source reverted to upstream behaviour but the new symbols kept (so the tests can still be collected), all three fail for the right reasons:

FAILED test_failed_fetch_leaves_the_count_unknown
  AssertionError: an unknown count must render as nothing, not as a plausible number
FAILED test_repeated_failures_retry_soon_and_then_back_off
  assert [3600, 3600, ...00, 3600, ...] == [60, 120, 240...60, 1920, ...]
FAILED test_successful_fetch_restores_the_hourly_interval
  assert 3600 == 60
3 failed in 0.05s

With the fix applied:

tests/test_website_github_stars.py ...                                   [100%]
3 passed in 0.01s

Wider slice, checking the new module-level import does not leak into neighbours (145 passed covers test_user_simulation, test_user_simulation_context, test_binding, test_storage, test_timer; the 74 passed run adds this file alongside test_user_simulation and is clean, where importing the website package instead produced an ERROR — see the next fold):

tests/test_user_simulation.py tests/test_user_simulation_context.py tests/test_binding.py \
  tests/test_storage.py tests/test_timer.py            ->  145 passed in 76.70s
tests/test_website_github_stars.py tests/test_user_simulation.py  ->   74 passed in  7.40s

Linters on the touched files: ruff clean, mypy clean, pylint 10.00/10. Pre-commit hooks passed on commit.

Why the test loads the module by path instead of importing it

import website.github_stars runs website/__init__.py, which eagerly imports the documentation tree; doc.auto_execute pre-renders each page inside a dummy_client(), and each of those constructs an Outbox whose loop() coroutine is deferred via app.on_startup. client.delete() does not close it, so when the test plugin's nicegui_reset_globals calls app.reset() those ~26 coroutines are dropped unawaited. Under filterwarnings = ['error'] pytest surfaces them as an ExceptionGroup at the setup of whichever test runs next — measured: importing the package from a test made tests/test_user_simulation.py::test_module_import_isolation_first_test ERROR with 27 sub-exceptions, in a run that is otherwise green.

Rather than widen this PR into the library, the test loads website/github_stars.py directly with importlib (the module has no relative imports) and temporarily swaps background_tasks.create_or_defer so its own hourly timer is closed instead of deferred — otherwise the suite would fire a real request at api.github.com.

Flagging the underlying leak as a separate finding, not fixed here: any future test that imports anything under website/ will hit it. The fix belongs in app.reset() / the deferred-task path (close pending coroutines instead of dropping them), with its own test.

Second-opinion review (Codex, gpt-5.2-codex — different lineage)

Prompted adversarially ("assume this diff is broken and try to refute it"), pointed at the specific risks: timer-interval mutation from inside the timer's own callback, retry rate vs. GitHub's limit, the unbound nav-badge label, the dataclass default change, the test preamble, and CI portability.

Verdict: no BLOCKER findings, one SHOULD-FIX, 8/10 shippable.

Retry interval sits exactly on the unauthenticated quota (SHOULD-FIX) — A 60s retry is defensible for transient network failures, but it is brittle for the suspected failure mode: unauthenticated GitHub API rate limiting. GitHub's unauthenticated REST limit is 60/hour/IP, so one unhealthy process retrying once per minute consumes essentially the whole IP budget. Multiple Fly machines or processes behind shared egress could keep each other rate-limited. […] I'd prefer exponential backoff with jitter, or at least something like 60s -> 5m -> 15m -> 1h until success. On success, resetting to 3600s is correct.

Acted on — the flat 60 s retry became the doubling ladder described above, with a test asserting the ladder and the resulting request rate.

Its other conclusions, verified against the source:

Mutating timer.interval inside _fetch is correct with current Timer._run_in_loop: it reads self.interval after the callback returns […] Negative sleep is not a bug here; asyncio.sleep(delay <= 0) yields immediately.

The first immediate invocation is also okay. timer = app.timer(...) assigns after constructing the timer, but the callback coroutine cannot execute until the event loop gets control […] so the global timer exists by then.

Leaving the header badge label empty […] does not look like a correctness defect. It may leave an icon-only GitHub pill with a little extra spacing on desktop, but it avoids the false value. NIT at most.

The test preamble restores background_tasks.create_or_defer in finally and closes the timer coroutine, so I don't see state leakage.

Caveat on this evidence: Codex reviewed by reading, not running — its environment lacked the project dependencies.

Deliberately not done
  • Direction 3 from the issue (authenticate the API call, share the cache via NICEGUI_REDIS_URL, or bake the count in at deploy time) — that is a deployment/infrastructure decision for the maintainers, not a code change, and directions 1 + 2 fix the user-visible symptom regardless of why the fetch fails.
  • Confirming the root cause. The rate-limit hypothesis is still unconfirmed; it needs fly logs on the nrt machine grepped for failed to fetch GitHub star count. This PR is deliberately agnostic to the cause.
  • A visibility bind on the nav badge (website/header.py:151) — an empty label beside the GitHub icon already degrades acceptably, and hiding it would change the header's layout.
  • Fixing the website-package import leak described above — real, but a library-level change that does not belong in a website bugfix.

Progress

Seed the count as an empty string instead of '0' and hide the tile
while the count is unknown, so a failed fetch renders nothing rather
than a confident but wrong number.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NYtshdRxziWhERLt5AupKv
@evnchn
evnchn force-pushed the fix/github-stars-no-false-zero branch from 02ca2fa to 065d9e9 Compare August 1, 2026 02:54
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.

Website shows "0 GitHub Stars" on some Fly machines: failed API fetch falls back to a literal 0

2 participants