refactor: 8-pass code-quality sweep (DRY, types, unused, slop) + favicon - #25
Merged
Merged
Conversation
Two repeated patterns extracted into small shared helpers: 1. `requireScope(ctx, scope)` in src/mcp/scope.ts replaces the `if (!ctx.scopes.includes(...)) throw new McpError(...)` block that was copy-pasted across 6 tool handlers (find-best-deal, find-products, get-code-for-url, redeem-link, watch-price, get-price-history). The long error-code rationale comment formerly inline in get-price-history now lives once on the helper. 2. `dealIsActive()` in src/db/predicates.ts replaces the 4-clause `eq(deals.isActive, true) + or(isNull(expiresAt), gt(expiresAt, ...))` predicate that appeared in 5 read-side handlers. Standardizes on `new Date()` (find-deals previously used sql\`now()\`; functionally equivalent on a single-node deploy). Net: -26 lines across 7 files, identical behavior. All 166 tests pass.
Inlines the existing .github/assets/mark.svg as a data: URI in the <head>, so no new static-file route or asset pipeline is needed. https://claude.ai/code/session_01YFa2Xf6Bkw3rnR3DXeVtgj
…m-link
The `{ code, title, dealId }` mini-shape was inlined 4× in find-products
(result item, helper return-type, helper map declaration) and 1× as
redeem-link's `topCode`. Promote it to a tiny `CodeRef` interface in a
new `mcp/tools/types.ts` so the two tools share one source of truth.
Lives in its own file (not `tools/index.ts`) to avoid coupling each
tool to the registry barrel. Larger per-tool match shapes — `CodeMatch`
in find-best-deal, `CodeForUrlMatch` in get-code-for-url — carry
UX-specific extras (estimatedDiscountCents, deeplink, expiresAt,
description, discountSummary) and stay tool-local; they deliberately
do not extend `CodeRef` because the layers evolve independently.
No public schema change: the structural shape of every output is
identical, so existing tests using `toEqual({...})` keep passing.
Remove dead test-only cache resetters (resetClerkClientCache, resetEnvCache) that no test actually invokes, and un-export zod-to-json internals (zodObjectToJsonSchema, JsonSchemaObject, AnyJsonSchema) that have no consumers outside their module. Also drop the matching `resetEnvCache: () => undefined` stubs from the two test mocks that referenced it. https://claude.ai/code/session_01YFa2Xf6Bkw3rnR3DXeVtgj
Eight high-confidence tightenings of weak types whose precise type was
recoverable from the upstream library typings:
- env() callers: replace `env() as unknown as Record<string, string |
undefined>` with the native `Env` type (z.infer). Static keys read
cleanly via property access.
- src/landing/stats.ts: drop `(result as unknown as { rows?: unknown[] })`
pessimization; drizzle's `database.execute<T>` already returns
`QueryResult<T>` with a typed `.rows`.
- get-price-history.ts: `sql<Date>` literal instead of
`sql as unknown as Date` — `gt` accepts SQLWrapper directly.
- list-merchants.ts + get-price-history.ts: type conditions array as
`SQL[]` instead of `Array<ReturnType<typeof eq>>` so `sql\`\``
fragments and `or()` results fit without `as unknown as` casts.
- test/auth/middleware.test.ts: parametrize `new Hono<{ Variables }>()`
so `c.get('principal')` is typed without `(c as any)`.
Load-bearing casts left in place (SDK type widening, Proxy stubs, JSON
parse boundaries, Zod `_def` reflection, drizzle Db structural mocks)
with reasons documented in the report.
https://claude.ai/code/session_01YFa2Xf6Bkw3rnR3DXeVtgj
Module headers stripped of marketing-toned narrative ("killer tool",
"closes the buy-side loop", "money path") and replaced with concise
WHAT/WHY notes. Inline narration that re-stated the next line, "v1/v2"
temporal phrasing, and references to retired Stripe billing flows
removed. WHY notes that document real constraints (concurrency, rate
limits, JSON-RPC mapping, Google's retired engine) were left intact.
Also reconciles the auth/types.ts JSDoc per pass-7 flag ("auth + billing
layer" → concise list of what the file exports). Plan union retained:
'starter'|'pro'|'enterprise' still referenced by clerk.defaultScopesFor
and test/auth/middleware.test.ts.
https://claude.ai/code/session_01YFa2Xf6Bkw3rnR3DXeVtgj
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
idanmann10
marked this pull request as ready for review
May 13, 2026 22:03
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
2 participants
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
Eight scoped cleanup passes, run as separate sub-agents in sequence. Each pass had a single concern, a single tool toolbox, and an explicit "stay in your lane" prompt to avoid stepping on the others. Every pass ran
npm run typecheck,npm run lint,npm test, andnpm run buildto green before committing. Three passes (circular deps, try/catch, legacy code) found nothing to do and produced no commit — that's the report, not a regression.Net diff: 29 files, +181 / -250. No behavior changes.
What landed
b08e5a2src/mcp/scope.ts(requireScope) andsrc/db/predicates.ts(dealIsActive). 6 scope guards + 5 active-deal SQL filters collapsed.29a52e1src/mcp/tools/types.tswithCodeRef(4 inline shape sites). Rejected mergingCodeMatch/CodeForUrlMatchand the seller/best-deal/find-products triad — different layers.f30197eresetClerkClientCache,resetEnvCache, and 3 internal zod-to-json helpers. knip + ts-prune + depcheck verified.src/,test/,eval/. No commit.a9422fbas unknown as Record<string, …>env casts, replacedas unknown as Datewithsql<Date>template, retyped Drizzle SQL arrays, removedas anyfromtest/auth/middleware.test.ts. Kept the documentedas unknown ascasts (MCP SDK widening, Zod private-API reflection, Clerk Proxy stub)..catch()s classified. Every one has a named recovery role (external-input parse, finally-cleanup, per-iteration boundary, sanitized request lifecycle). Zero unjustified catches. No commit.a8cebf4Stripe,43144b2deps + Fly,f30197eexports) already swept it. Three flag items forwarded to pass 8 / future work. No commit.206985dPlus one unrelated UI fix that hitched a ride on the same branch:
6035a0e— added theshopdealsmark from.github/assets/mark.svgas the browser-tab favicon (inlined asdata:image/svg+xml— no new route or asset pipeline).Forward flags (out-of-scope for this PR)
Planunion insrc/auth/types.tsstill has'starter' | 'pro' | 'enterprise'—src/auth/clerk.ts:58-62(defaultScopesForswitch) andtest/auth/middleware.test.ts:82still reference them, so removal needs a coordinated code change after the Stripe retirement.src/db/migrations/0001_add_waitlist.sqlis still on disk after the waitlist feature was dropped. Migration history shouldn't be edited; a forward "drop waitlist table" migration would retire the schema cleanly.Test plan
npm run typecheckcleannpm run lintclean (0 warnings,--max-warnings=0)npm test— 166/166 pass across 23 filesnpm run buildcleannpm run dev+ atools/listround-trip to confirm the registry still resolves every tool after the DRY refactorhttps://claude.ai/code/session_01YFa2Xf6Bkw3rnR3DXeVtgj
Generated by Claude Code