Repository navigation
Simplify codebase: deduplicate abstractions, remove dead code and over-testing - #274
Merged
Merged
Conversation
- Drop unused devDeps eslint-plugin-jest and tsx - Drop JSX compiler options (no .tsx files in the repo) - Remove dead exports: extensions v1 getLatestRelease and its lib barrel re-export, stats ChartDataset/ChartData, lib normalizePublicCacheKey, unused IEnvironment.has API (interface + both adapters) - Count only src/ toward coverage thresholds instead of also counting test files - Fix stale comment referencing a renamed DeveloperProfilesDatabase method - Trim tests that only re-proved Hono dispatch or delegation semantics; the middleware and adapters are covered end-to-end elsewhere
- preview-github-budget.ts becomes previews/v1/budget.ts: the DO encodes previews-specific rate-limit policy and never belonged under lib/adapters; app/index.ts keeps the re-export wrangler's main requires - Extract the stats dashboard HTML into stats/v1/dashboard.ts so index.ts holds only reviewable API code (byte-identical payload)
- moderatorActorError() replaces two inline 27-line actor-check blocks in db/extensions.ts (relistBlockedError, moderatorCorrectBlockedError) - createCursorCodec<T>(guard) in db/cursor.ts: five per-module encode/guard/decode triplets collapse onto the shared envelope - parseListRow/publishedContent merge into one listContent() projection so the public and owned payload producers cannot drift - keysetPage() extracts the thrice-repeated over-fetch/hasMore/cursor tail - listPayload()/listErrorStatus() in routes/errors.ts replace the copy-pasted pagination envelope and INVALID_CURSOR ternary in nine list routes; c.json stays at call sites for Hono's OpenAPI typing - notifyAuthor() in routes/notify.ts replaces the seven revalidate-then-?notify-gated send blocks in moderation/ownership - sha256Hex() moves to src/lib/hash.ts (three sites in this service) - Drop the requireAuthAllowInactive alias; account.ts uses requireAuth - Cross-link the twin publish-UPDATE statements in approve() and moderatorCorrect(); document the double truncation as deliberate belt-and-braces for legacy rows Net -184 lines; behavior preserved (799 tests pass)
- Shared release-response module (hasNoReleases, response builders, githubErrorDetails) for versions/v1 and stats/v1, which serve the same blob through the same envelope - Remove the stats->versions preReadReleases tristate threading: stats reverts to plain getReleases, costing one cold-path KV read that singleFlight coalesces anyway - Drop stats' dead updateCache knob and versions' unreachable-state checks (empty :version param, null lastKey after the releases guard) - Replace the FIFO-evicting parse memo with a single-slot memo: the blob is byte-stable on a warm isolate, so one slot covers the real workload - parseLegacyPagination in src/lib centralizes the opt-in limit/offset policy duplicated across extensions/v1 and central-alerts/v1; drop central-alerts' typeof-caches guard for a runtime that cannot occur - extensions/v1: shared databaseError/notFound helpers and one extensionError response helper replace three copy-pasted blocks - Logger emits [service] + message + structured context argument instead of interpolating one string: Workers observability already stamps timestamp/severity, and the JSON argument keeps fields filterable - previews: rate-limit bucket key is CF-Connecting-IP verbatim (IPv6 clients limited per address, not per /64); the hand-rolled IPv4/IPv6 parser is gone - previews: routes/main.ts reuses its own cachedLookup/readCachedValue instead of re-implementing the read/negative-cache policy with a second sentinel - github-errors: drop the ErrorPriority enum, getMostCriticalError, and never-read details field; the sole call site reports the first non-validation error Behavior notes: log line format changes (structured context is now a separate console argument); preview rate-limit buckets are per-IP address rather than per-IPv6-/64.
- Seven suites re-declaring the octokit module-shape mock inline now use test/mocks/octokit.ts, the shared factory that already existed for it - Merge test/app/index.test.ts into test/integration/app.test.ts: the two suites booted the full app against the same surface; the two content assertions the merged suite lacked (latest release resolves, alerts non-empty) move into the surviving suite - test-types' VersionsResponse/VersionResponse/ChangelogResponse/ UpdateResponse aliases were ApiResponse<T> restated four times over; tests use ApiResponse<...> directly, and stats' local duplicate of the envelope is gone
include: ["src/**"] also matched src READMEs, which istanbul cannot instrument; glob to src/**/*.ts. Thresholds still clear (91.6/85.3/ 95.7/92.4 against 80/75/80/80) now that test files no longer dilute the measurement.
…nups Preview budget identity (review follow-up): - Restore clientBudgetIdentity: IPv6 aggregated to /64 (address rotation inside a /64 shares one allowance), IPv4-mapped forms share the native IPv4 bucket, unparseable addresses share one conservative bucket. The raw-header simplification from the previous commit traded this away. - The derived identity is SHA-256 hashed before it reaches the DO, so storage rows hold digests, never raw client IPs. Residue and dead code: - Merge account.ts's duplicate lib/auth imports; drop the unused maxLimit parameter from parseLegacyPagination; use listPayload for public-extensions' empty-list shortcut; correct the logger comment (Workers Logs serializes console arguments into message text) - Delete dead ExtensionListResponseSchema/OwnedExtensionListResponseSchema and the unconsumed SPDX_LICENSE_IDS re-export; drop versions' never-overridden keyGenerator option and the sortedReleases alias Small dedups: - cursorPaginationFields fragment shared by the three paginated query schemas (fields spread, not schema extension: CursorPaginationQuerySchema is strictObject and must stay so). Doc-only delta: the revisions queue cursor param gains the shared OpenAPI description. - templates.ts subject labels compose displayLabel instead of restating it - previews artifacts catch blocks pass the already-classified error on instead of classifying twice - stats notes the accepted cold-path KV-serialization trade-off; routes/notify.ts documents why it is a shared helper module
authErrorResponses(forbidden?) in schemas/common.ts replaces the 31 copies of the 401 entry and the 22 copies of the 5-line 403 spread across 34 route definitions; ModeratorForbiddenResponse carries the 12-copy moderator wording. Bespoke 403 descriptions become an explicit override after the spread. Verified byte-identical generated OpenAPI document across the conversion (sha-256 of the doc before and after). The helper deliberately omits 'as const': zod-openapi derives a handler's permitted statuses from the literal numeric keys of the responses map, and const-widened string keys silently drop 401/403 from the typed response union.
- versions/v1/middleware.test.ts kept only the headers this service's own middleware decides (Vary suppression on GitHub failure, /update no-store); the hono cors/trailing-slash/etag/bearer-auth/pretty-json re-tests are gone - CORS/ETag stay covered at the integration layer - integration/versions keeps the four discriminating blocks (/update rewrites the KV blob, cross-endpoint consistency, refresh changes the blob, cold-start concurrency); warm-cache/auth/retry duplicates of the service suite are gone - integration/app.test.ts: the two identical KV assertions merge into one, the third copies of update-auth and the response-format loop are dropped, and the vacuous [204,405] OPTIONS and weak GET loops go; CORS/ETag remain as the surviving coverage - Dead test infrastructure: zero-importer exports mockComposerJsonOldVersion, createMockFetchResponse, GitHubRelease/GitHubAsset/GitHubContentResponse, MockFetchResponse, TestEnv, CentralAlert re-export, FetchSpy, KVPutSpy - Tautologies trimmed: github-errors constructor describe (classify block already exercises every default) and the null-coercion case; context.test.ts reduced to the throw plus one smoke; CF adapter to one delegation smoke; node environment adapter to one smoke; node cache loses the closed-DB and file-durability re-proofs of node:sqlite - previews cache.test.ts keeps only the negative-sentinel behavior; the positive/corrupt/TTL paths are asserted through real routes with exact KV writes in the sibling suites
Unreachable-in-practice states (src guards remain; the tests only pinned states the extensions-site minter structurally cannot emit): - bearer-assertion: fractional NumericDates, non-60s lifetimes, and the legacy-token case go, along with the includeContext helper option that existed solely for it; the digest matrix reduces to the missing-digest shapes. iss/aud/purpose/ver stay - minter/API version skew and sibling verifier traffic are real. Over-specific assertions loosened to their actual contracts: - resource-bounds: the EXPLAIN test asserted a hand-copied query that could drift from the real one silently - deleted (the index is pinned by schema + migration snapshot); SQL-text regexes replaced by the behavioral read-only row check they approximated; the log-redaction test asserts the property, not the console argument layout; the stream pull-count pin dropped (413 + cancel is the contract) - email/moderation-notify: exact HTML entity numberings dropped where the ASCII-safe regex and raw-unicode exclusions already pin the mojibake contract; sender URL order loosened to membership - logger: per-level dispatch and per-key-literal redaction cases consolidated; JSON.stringify re-proof dropped - versions: retry describe loses its /latest duplicates (the retry precedes the alias branch) and the two '404'-named tests now correctly say 503; mirror-matrix it.each reduced to one path x both warm orders; credentials-matrix reduced to one route (stats copy removed; the mechanism lives in lib/cache.test.ts); cache TTL asserted present, not a pinned constant - central-alerts: the non-discriminating body-equality cache test, trailing-slash re-tests, app-level 404 copy, and ISO-datetime fixture test removed; extensions/v1 trailing-slash re-test removed - commit.test.ts: duplicate artifact-name call-shape assertion dropped; stats drops the Chart.js CDN URL pin - lib/cache: string-identity key test dropped Stale comments fixed (apply-migrations path, removed test/app reference).
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
api | fee1bea | Oct 09 2026, 12:49 AM |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 82 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
…torations
Review-driven src changes:
- previews readCachedValue returns a tagged result (hit/negative/miss)
so no cached value can collide with a sentinel
- stats re-overlaps its two KV reads on the cold path without restoring
the getReleases tristate parameter; a failed stats read is a miss
- logger emits one structured object ({service, message, context}) -
Workers Logs indexes top-level fields of object arguments, keeping
service and context keys filterable
- pagination uses Number.isSafeInteger for offset: 1e100 is an integer
but an out-of-range SQLite OFFSET turned into a 500
- stats dashboard grids use minmax(min(100%, 600px), 1fr) so charts do
not overflow narrow screens
- comment corrections: authErrorResponses' as const note (verified the
real failure mode empirically), notify.ts scope, RELEASE_CACHE_KEY
ownership, node adapter portability claim, templates sentence break
Review-driven test changes:
- Restored assertions dropped too aggressively: HTML entity encodings
(moderation-notify, email), the uppercase-SHA artifact-name query,
the unavailable-is-never-cached contract, the OPTIONS preflight
answer, file-cache persistence across reopen, expired write-event
pruning (seeded and asserted on rows), fractional-NumericDate and
lifetime verifier guards, and the full digest format matrix
- Fixed a bug in the report-read-only check: the baseline snapshot was
taken after the call it guards
- versions TTL asserted positive and bounded; /latest mirror-trust pair
retained (separate cache registration); integration/versions casts
corrected to single-release responses; node env test save/restores
process.env in a finally
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 82 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
…erving on KV failure - previews main download path: a cached literal null (valid JSON from something other than cachedLookup) dereferenced download_url and 500'd; treat it as absent and fall through to the R2 head - stats: getReleases is deferred so a valid stats cache hit is served even when the shared releases read rejects - the serialized-reads fix had regressed the old Promise.all tolerance for that failure. No usable cache and a failed releases read now propagates as before - central-alerts: restore the CacheStorage fallback as a deliberate degrade-to-uncached path for runtimes without caches (portability seam), not as a can't-happen guard - github-errors classifier tests assert each branch's errorCode - the codes surface in API error envelopes and the removed constructor describe had been their only coverage - stats: restored the credentials/edge-entry test across both stats registrations (route-level wiring the shared lib tests cannot see), plus a new cached-stats-served-despite-releases-read-failure test
…ument
The shared cursorPaginationFields fragment carries the cursor param
description that /extensions and the moderator list have always
documented; spreading it into RevisionQueueQuerySchema silently added
that description to /revisions and (via the derived history query) to
/extensions/{id}/revisions, changing the generated OpenAPI document
despite the byte-identical guarantee.
RevisionQueueQuerySchema now restates just the cursor field without the
description - validation stays in lockstep with the shared fragment and
the inline field carries a comment explaining why it must not adopt the
shared metadata. Verified: the generated OpenAPI document is
byte-identical to the pre-PR document (extracted and structurally
compared against a dump at the merge base).
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
The fallback test's second request was served from the edge entry warmed by its own first request (publicResponseCache caches the 200 under the public key), so getStats never ran and the rejected gh-fossbilling-releases read went unexercised - the test passed even if the cached-stat fallback regressed. Reset the edge namespace after seeding the KV value, and assert the failing key was actually read so the rejection path is provably exercised. Verified by mutation: moving the releasesFailure rethrow ahead of the cache-serving branch makes the test fail.
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
1 participant
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.
Summary by cubic
A cleanup pass that deduplicates repeated abstractions, removes dead code and unused dependencies, and trims tests that only re-proved library or framework behavior. About 800 tests still pass; behavior is unchanged except for the logger format and preview rate-limit identity hashing noted below. Two rounds of review feedback were folded in: assertions dropped too aggressively were restored, and cache, pagination, and failure-path handling were hardened.
Code and test consolidation
createCursorCodec,listPayload,authErrorResponses,sha256Hex,parseLegacyPagination,notifyAuthor, release-response builders) replace copy-pasted per-module implementations.ErrorPriority,IEnvironment.has, stats update knob), and unused devDependencies (eslint-plugin-jest,tsx) are removed.test/app/index.test.tsmerges intotest/integration/app.test.ts; seven suites share a single octokit mock factory; coverage thresholds now measuresrc/**/*.tsonly.authErrorResponsesconversion; the revisions cursor param also keeps its own description when the shared pagination schema fragment is adopted.nullas absent and falls through to the R2 head instead of 500ing.1e100would have turned an out-of-range SQLite OFFSET into a 500.caches.errorCode, which surfaces in API error envelopes.Behavior changes
{service, message, context}object argument instead of one interpolated string; Workers Logs indexes the object's fields so they stay filterable.Written for commit fee1bea. Summary will update on new commits.