Files
homefeed/docs/review-2026-07.md
T
Claude 24bf5afb07 Act on review findings: dead code, poe2_primary_currency_name, inefficiencies, custom source type
- Delete dead ArticleCard.svelte and five zero-call-site backend exports
  (getTagsByIds, unclusteredItemsForSources, itemsByCluster, deleteAllContentItems,
  latestArticleInThread)
- Drop poe2_primary_currency_name from the schema entirely (dev DB, no migration
  concerns)
- settings.ts: bind named params instead of 33 positional ?s to remove the
  reorder-and-silently-corrupt risk
- Remove the long-dead stooqToYahooSymbols unconditional startup rewrite
- Share direct-publish logic between runPassthroughCycle/runSynthesisCycle via a new
  publishItemsDirect helper, and cache sourcesDb.listSources() once per synthesis tick
  instead of a per-item getSource() call
- Remove the 'custom' source type (had no adapter of its own, aliased to api)
2026-07-26 16:45:46 +00:00

12 KiB

Homefeed program review — July 2026

Scope: dead code, inefficiency, and UI/system consistency across the whole app (backend + frontend, public site + admin panel), as requested after a long run of incremental feature work. Findings below are left as originally written (historical record); the "Resolved since this report" section tracks what's since been acted on.

Resolved since this report

  • 1.1 ArticleCard.svelte — deleted.
  • 1.2 All five zero-call-site exports (getTagsByIds, unclusteredItemsForSources, itemsByCluster, deleteAllContentItems, latestArticleInThread) — removed.
  • 1.3 poe2_primary_currency_name — dropped from the schema entirely (CREATE TABLE literal + the ALTER TABLE migration line), not just left as an inert column.
  • Inefficiencies — the unconditional stooqToYahooSymbols startup rewrite (long-since a no-op) was deleted outright; settings.ts:updateSettings now binds named $column parameters instead of 33 positional ?s, removing the reorder-and-silently-corrupt risk; runPassthroughCycle/runSynthesisCycle's direct-publish logic now share one publishItemsDirect helper; the per-item sourcesDb.getSource() N+1 in the synthesis tick was replaced with a single sourcesDb.listSources() map reused for both the direct-publish partition and each item's category-rank lookup. (settings.ts's remaining structural size and migrate()'s 410-line mixed-idiom growth were left as-is — real fixes but a larger refactor than this pass warranted.)
  • 'custom' source type — removed entirely: dropped from the Source['type'] union (backend + frontend), the poller's adapter map, and the admin add-source dropdown. It had no adapter of its own (silently aliased to the API adapter) and no admin-facing purpose distinct from api.

Everything above was verified via tsc --noEmit/svelte-check (0 errors on both sides), a full build of both packages, and a live smoke test against a fresh DB (migrate() + a settings roundtrip through the new named-param query, confirming the column is gone and unrelated fields are untouched).

See the staleness/refresh recommendation delivered separately in conversation — that item was discussed, not changed, per the request to advise rather than implement.


1. Dead code

1.1 Frontend

  • frontend/src/lib/components/ArticleCard.svelte — zero references anywhere under frontend/src (confirmed via grep -rn "ArticleCard" frontend/src, zero matches outside its own file). Superseded by ArticleListRow.svelte. Safe to delete.

1.2 Backend

Exported functions with no call sites found anywhere in backend/src:

  • backend/src/storage/db/tags.ts:31getTagsByIds
  • backend/src/storage/db/contentItems.ts:72unclusteredItemsForSources
  • backend/src/storage/db/contentItems.ts:96itemsByCluster
  • backend/src/storage/db/contentItems.ts:121deleteAllContentItems
  • backend/src/storage/db/articles.ts:146latestArticleInThread

1.3 Dead settings field

  • global_settings.poe2_primary_currency_name — left over from before PoE2 moved to per-pair tracking (base/quote currencies with a directly-computed rate, no longer "everything quoted in one reference currency"). The column is harmless — dropping it would mean a SQLite table rebuild for one nullable text column, not worth doing on its own. GlobalSettings.poe2 on the TS side no longer exposes it, so it's already fully inert; only the raw DB column remains as an artifact.

1.4 Over-exported (not dead, but worth a look)

These are only ever called from within their own module, so exporting them invites cross-module coupling that hasn't happened yet but could:

  • backend/src/storage/db/sources.ts:27listEnabledSources
  • backend/src/storage/db/events.ts:46getEvent
  • backend/src/clustering.ts:9strictnessToThreshold

2. Inefficiencies

  • backend/src/storage/db/index.ts:375-382 — the stooqToYahooSymbols stock symbol rewrite runs unconditionally on every server start, forever, with no skip-guard once it's already been applied to a given database. Same category of issue as the unconditional DROP TABLE IF EXISTS admin_users / admin_sessions at lines 25-26 — both re-run one-time work on every boot instead of gating it behind a "have I already done this" check.
  • backend/src/storage/db/settings.ts:updateSettings (lines 77-109) — 33 positional ? SQL parameters matched to 33 arguments passed to .run() in the same order, with nothing enforcing that the two lists stay in sync if either is edited. Not currently broken, but a single misordered edit here would silently write the wrong value into the wrong column with no type error to catch it.
  • Synthesis's 60-second tick calls sourcesDb.listSources() on every iteration, which is redundant if the source list hasn't changed since the last tick — a small, low-priority optimization.
  • runPassthroughCycle and runSynthesisCycle's "direct publish" halves are near-duplicate copy-pasted logic rather than a shared helper.
  • The migrate() function (410 lines) mixes four different guarded/unguarded backfill idioms with no schema-version tracking, which makes it hard to tell at a glance whether a given block still needs to run or is a no-op on any DB that's already current.

3. UI / layout consistency audit (public site)

  • /weather is not reachable from the top nav — it's only linkable via the sidebar widget, unlike every other route (home, categories, events, /more), which are all in top nav.
  • Sidebar widgets don't share a component. Weather, Stocks, and PoE2 each hand-roll their own .widget shell but do converge on the same .head + interval-tag pattern (today, 24h, etc.). BookmarksWidget is the outlier: it lacks that .head/interval wrapper the other three use, so it looks structurally different from its neighbors in the same sidebar.
  • Confirmed: the PoE2 icon-removal and 24h-only simplification from earlier this session are fully complete on the frontend (verified via grep — no remaining icon references).

4. UI / layout consistency audit (admin panel)

  • Near-duplicate CRUD tabs. StocksTab.svelte and BookmarksTab.svelte are close to byte-identical in structure (add row, list, delete button). Poe2Tab.svelte is a variant of the same pattern; WeatherTab.svelte uses a different settings-form pattern entirely. None of the three share a common component, so a fix to one (e.g. the null/ display bug fixed in Poe2Tab this session) doesn't propagate to the others even where the same bug class could exist.
  • Admin tab list has an implicit, unlabeled grouping — Sources & Content / Sidebar widgets (Weather, Stocks, PoE2, Bookmarks) / Integrations (Telegram) / System (Logs, Settings) — but the tab bar renders them as one flat list with no visual separation or heading, so the grouping only exists in the code's mental model, not on screen.
  • /admin itself is a dead redirect-only render — it exists only to bounce to /admin/settings or another tab, with no content of its own.

5. Cross-system / type consistency findings

5.1 Duplicated types

  • WeatherHourEntry, WeatherDayEntry, WeatherCurrentConditions, and WeatherAlert are byte-identical between frontend/src/lib/types.ts and adminTypes.ts — genuine duplication, not just similar shapes.
  • Category and CategoryPriority are same-shape-but-differently-named — worth confirming whether that's intentional (different semantic roles) or just drift from copy-paste.
  • Backend-side: Tweet/Telegram-message inline shapes are duplicated between ContentItem and MergedArticle in backend/src/storage/db/types.ts (lines 78-108 vs. 125-143) rather than being extracted into a shared named interface.

5.2 CSS token drift

  • A 12px "card radius" value is hardcoded across roughly 20 files instead of using the existing var(--radius) token (which is 8px) — meaning cards and the token-driven --radius elements don't actually share one visual language despite looking like they should.
  • LogsTab.svelte:99-101,157 hardcodes #a8710f/#fff for the "warn" log-level color rather than using a token, unlike "err" which correctly uses --text-danger. There's no --text-warning token defined to match.

5.3 Ingestion pipeline (traced end-to-end)

Flow: adapter → poller → priority queue → clustering/synthesis (or direct-publish bypass) → articles table → public feed. The telegram/nitter/youtube direct-publish bypass (skipping clustering/synthesis entirely for those source types) is coherent and already explicitly commented as intentional — not an oversight, despite looking unusual at first glance.

One asymmetry does look like a genuine oversight rather than a documented design choice: the 'custom' source type is excluded from both FOLLOWS_LINK_FOR_FULL_ARTICLE (backend/src/poller.ts:24, which only lists ['rss', 'api']) and from directPublishSourceIds in backend/src/queue/priorityQueue.ts — with no comment anywhere explaining why custom is treated differently from rss, which otherwise behaves the same way a custom source presumably would.

5.4 Frontend staleness beyond the sidebar

This session added a 5-minute invalidate('app:sidebar') timer (frontend/src/routes/+layout.svelte / +layout.ts) so the sidebar widgets stay fresh in a tab left open. That is the only refresh mechanism anywhere in the frontend. Every other route — home (/), /category/[name], /event/[id], /article/[id], /more, and notably /admin/settings — is a pure one-shot load() with no refresh at all. This is fine for content pages (a stale article list is a minor nuisance, fixed by navigating), but is a real staleness risk specifically for /admin/settings: an admin who leaves that page open to watch Logs, AI-status, or Telegram connection status will see indefinitely stale state with no signal that it's stale.

5.5 Public API field leak (most concrete/actionable finding)

backend/src/api/public.ts — the categories, bookmarks, and events routes all explicitly filter their response shape down to public-safe fields:

// lines ~48-51, 57-61, 68-72 — filtered
app.get('/api/events', async () => eventsDb.listEvents().filter(e => e.active).map(({ id, name, ... }) => ({ id, name, ... })));

But stocks and PoE2 do not:

// line 66
app.get('/api/stocks', async () => stocksDb.listStockTickers());

// lines 74-77
app.get('/api/poe2', async () => {
	const { leagueName, updatedAt } = settingsDb.getSettings().poe2;
	return { leagueName, updatedAt, entries: poe2WatchlistDb.listWatchlist() };
});

listStockTickers() and listWatchlist() return the full DB row, including lastError and lastPolledAt — internal poller-diagnostic fields with no reason to be visible to an unauthenticated visitor. This is inconsistent with how every other public route in the same file handles the same concern, and is a straightforward fix: map each entry down to its public-facing fields the same way categories/bookmarks/ events already do.


Appendix: surveyed and found fine

Recorded here so these don't get re-litigated in a future review:

  • Logging — a single shared logger, consistent lowercase tags, and only two console.log calls outside of it, both explicitly commented as intentional exceptions (in index.ts).
  • API route organizationadmin.ts (329 lines / 38 routes) is cleanly sectioned into 11 logical groups; public.ts (78 lines / 9 routes) has no duplicated logic. 52 routes total, no redundant endpoints found.
  • Scheduler cadences — weather (45m), stocks (15m), and PoE2 (1h) all poll immediately on start by design; the ingestion poll/synthesis loop (60s) and retention sweep (1h) deliberately don't, also by design. No redundant external fetches were found across the whole scheduler table.
  • Module organization pattern — the client.ts (raw external API) + poller.ts (orchestration) split is followed consistently for weather, stocks, and PoE2. Telegram is the one deliberate exception (a stateful GramJS client/credentials singleton, ingested through the generic per-source ingestion/poller.ts rather than queue/scheduler.ts) — this is explicitly commented in the code as an intentional divergence, not an inconsistency to fix.