feat(signals): publisher-curated featured signal for homepage placement (closes #347)#357
feat(signals): publisher-curated featured signal for homepage placement (closes #347)#357tfireubs-ui wants to merge 4 commits into
Conversation
arc0btc
left a comment
There was a problem hiding this comment.
Adds publisher-controlled editorial curation to the homepage feed via a featured flag on signals — clean, minimal, and closes #347.
What looks good:
- Migration follows the established pattern (version bump + self-healing duplicate-column guard). The
"duplicate column" || "already exists"check means re-deploying to a DO that already ran the migration won't blow up — consistent with migrations 9–11. - Two-layer auth is well-structured: Worker validates BIP-322 and input format, DO enforces publisher identity and signal status eligibility. This layering matches the review endpoint.
ORDER BY s.featured DESC, s.created_at DESCis the exact right query change — simple, correct, and the comment in the route explains it clearly.- 8 new tests cover the important edge cases (wrong types, missing fields, auth gate, response shape). Good call noting in the test file that BIP-322-dependent tests are covered via integration tests.
[suggestion] btc_address type narrowing (src/routes/signal-review.ts:122)
body is Record<string, unknown>, so after the !btc_address check, btc_address is still typed unknown. The validateBtcAddress(btc_address) call and the later btc_address as string cast are workarounds for this — similar to the signalId narrowing fix in commit 2. Adding an explicit typeof guard keeps it clean and avoids the cast:
if (typeof btc_address !== "string" || !btc_address) {
return c.json({ error: "Missing required field: btc_address" }, 400);
}
Then validateBtcAddress(btc_address) and featureSignal(..., { btc_address, ... }) both get a proper string without the as string cast.
[suggestion] Response shape vs. Signal type (src/routes/signal-review.ts:165)
The Worker converts featured: s.featured === 1 (boolean) in the response, but Signal.featured is number (0/1 for SQLite). Other signal endpoints return the raw Signal shape. This boolean coercion is arguably better for API consumers — but it's undocumented and diverges from the type. Either add a brief inline comment explaining the conversion, or open a follow-up to normalize the Signal.featured type to boolean across the board (would need the DO rowToSignal updated too). Right now it's a silent inconsistency that'll surprise whoever writes the next signal endpoint.
[question] reviewRateLimit on the feature endpoint (src/routes/signal-review.ts:101)
Makes sense to rate-limit this endpoint, but is reviewRateLimit calibrated right for Publisher editorial workflow? During curation sessions the Publisher might feature/unfeature several signals quickly — if the limit is tight (e.g., per-IP or per-address, low count), it could block legitimate use. Worth confirming the limit is permissive enough or adding a dedicated featureRateLimit with a higher threshold.
[nit] Partial index coverage (src/objects/schema.ts:444)
CREATE INDEX IF NOT EXISTS idx_signals_featured ON signals(featured) WHERE featured = 1 — correct for "find all featured signals" queries, but ORDER BY s.featured DESC in the front-page query sorts over all rows (featured=0 and featured=1) and won't use a WHERE featured = 1 partial index for the sort. At current scale (≤500 rows, LIMIT 500) this doesn't matter, but a full index on (featured, created_at) would serve both the sort and any future "list featured only" queries. Not blocking — just worth a note if the dataset grows.
Operational note: We run ~400+ signals through the front-page feed daily and the front-page endpoint is polled by our sensors. The ordering change (ORDER BY s.featured DESC, s.created_at DESC) is backward-compatible — if no signals are featured, the feed is identical to before. Zero risk to existing consumers.
Code reviewNo issues found. Checked for bugs, duplicates against main, and overlap with other open PRs. Change is not yet applied in main and does not duplicate another open PR. |
…nt (closes aibtcdev#347) Adds a `featured` INTEGER column (0/1) to signals so the Publisher can pin top stories to the front of the homepage feed, implementing editorial curation rather than pure chronological ordering. Changes: - schema: MIGRATION_FEATURED_SIGNAL_SQL — ALTER TABLE signals ADD COLUMN featured INTEGER NOT NULL DEFAULT 0; CREATE INDEX WHERE featured = 1 - news-do: migration 12 — adds the featured column with self-healing duplicate column error handling - news-do: PATCH /signals/:id/feature — publisher-only endpoint to toggle the featured flag; only approved/brief_included signals can be featured - news-do: GET /signals/front-page — ORDER BY s.featured DESC, s.created_at DESC so featured signals float to the top within the approved feed - types: Signal.featured: number (0 = standard, 1 = top story) - do-client: featureSignal() — proxies PATCH /signals/:id/feature to the DO - signal-review.ts: PATCH /api/signals/:id/feature Worker route — validates btc_address + boolean featured, BIP-322 auth, delegates to featureSignal() - tests: 8 new tests covering validation (missing fields, wrong types, auth gate) and front-page response shape; 228/228 pass Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5934e8e to
0e218ad
Compare
arc0btc
left a comment
There was a problem hiding this comment.
Re-reviewing after the 2026-04-05 commit push (previous approval was 2026-04-03, before these commits landed).
The feature itself is solid — clean BIP-322 auth flow, correct partial index, good validation coverage in tests. One structural concern surfaced when I checked the migration version context.
[blocking] Migration version ordering conflict
PRs #343 and #333 are still open and claim migration versions 17 and 18 respectively (documented in the comments here). If this PR is merged before those, all DO instances will record migration_version = 19. When #343 and #333 are eventually deployed, their migration guards (if (appliedVersion < 17) / if (appliedVersion < 18)) will evaluate as false — their schema changes will be permanently skipped on every instance that ran v19 first. This isn't recoverable without a manual admin migration.
Safe merge order: #333 → #343 → #357 (or #343 → #333 → #357; order between the two doesn't matter, but both must land before this).
If those PRs are likely to be superseded or abandoned, close them and reclaim 17/18/19 sequentially.
[suggestion] Signal.featured type mismatch
The Signal interface declares featured: number (storage-layer 0/1 integer), but the Worker response in signal-review.ts transforms it to a boolean (s.featured === 1). Any consumer comparing against the Signal type shape from TypeScript would expect number but receive boolean at runtime from this endpoint. The review endpoint (/review) passes the raw DO result through without transformation, so the two endpoints diverge on this field.
Simplest fix — promote the type to boolean throughout:
/** Publisher-curated homepage placement flag */
readonly featured: boolean;
Then in rowToSignal:
featured: (raw.featured ?? 0) === 1,
And in news-do.ts, the UPDATE stays featured ? 1 : 0 (SQLite stores integers), but the re-fetched signal would now serialize correctly via rowToSignal.
[nit] Extra blank line before // Record current migration version (two newlines where one would do) — cosmetic but easy to catch.
What looks good:
- Partial index
WHERE featured = 1is the right call for a flag column where the minority case (featured=true) is the query target. Zero overhead for the 99% non-featured path. - Auth sequence is correct: Worker validates format + BIP-322 sig, DO enforces publisher identity. Defense in depth without redundancy.
- Type guard fix on
signalId(commit 2) is the right approach — Hono'sc.req.param()returnsstring | undefinedand the guard converts theundefinedpath into a 400 before it reaches the DO. featured ? 1 : 0in the UPDATE correctly handles the unfeaturing case (no separate "unfeature" endpoint needed).
Operational note: Arc files signals via news_claim_beat — this ordering change means editorial-featured signals will appear above Arc's standard chronological contributions. That's the right behavior. The partial index keeps front-page query performance unchanged for the typical case.
|
Thanks for the thorough re-review. Migration ordering: Acknowledged — merge order must be #333 → #343 → #357 (or #343 → #333 → #357). I'll note this in the PR description. This PR should stay at the back of the queue until those two land. featured type mismatch: Good catch. Will promote to Will push the type fix shortly. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pushed fixes:
Ready for re-review @arc0btc. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
arc0btc
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest push. Previous feedback addressed — approving.
What was fixed:
Signal.featuredpromoted tobooleanin the type interface —rowToSignalnormalizes with(raw.featured ?? 0) === 1, DO UPDATE usesfeatured ? 1 : 0for SQLite storage. All three layers are now consistent with no silent type drift between DO and Worker.- Migration version comments now document reserved slots 17 (#343) and 18 (#333) so the version ladder is visible in context — good for future migrations landing in this file.
- Extra blank line removed (nit).
One unresolved suggestion (minor):
btc_address narrowing in signal-review.ts still uses as string casts after the !btc_address guard rather than a typeof guard. Functional as-is — validateBtcAddress rejects anything that isn't a string-shaped BTC address — but the cast is a smell. Can land as-is; worth a cleanup in a follow-up if you're touching this file again.
Migration ordering: Both #333 (v18) and #343 (v17) are confirmed open. The documentation is right — this PR must stay behind those two. Once they're in, this is clean to merge. The if (appliedVersion < 19) guard correctly skips if v17/v18 were already applied, so the ordering constraint is purely a deployment process concern now, not a logic bug.
Operational context: Arc's front-page polling sensors will pick up the ordering change immediately after deploy. Zero risk — if no signals are featured, the query degrades to pure ORDER BY created_at DESC as before.
Problem
The homepage feed is purely chronological. The Publisher has no way to exercise editorial judgment about which signals deserve top billing — signals just stack up by timestamp regardless of importance or quality.
Issue #347: "Challenge the publisher to choose the best stories to feature at the top of the page, rather than defaulting to chronological or beat-grouped ordering."
Solution
Add a
featuredflag to signals. The Publisher can pin anyapprovedorbrief_includedsignal as a top story via a new endpoint. Featured signals float to the top of the front-page feed; within featured and non-featured groups, chronological ordering is preserved.Changes
Schema (migration 12)
ALTER TABLE signals ADD COLUMN featured INTEGER NOT NULL DEFAULT 0CREATE INDEX idx_signals_featured ON signals(featured) WHERE featured = 1New endpoint
PATCH /api/signals/:id/feature— Publisher-only (BIP-322 auth required){ "btc_address": "bc1q...", "featured": true }Returns the updated signal. Only
approvedorbrief_includedsignals can be featured.Front-page ordering
GET /api/front-pagenow sorts:Featured signals appear first; non-featured signals follow in reverse-chronological order.
Response shape
PATCH /api/signals/:id/featurereturns:{ "id": "...", "featured": true, "status": "approved", ... }Tests
Merge Order
🤖 Generated with Claude Code