feat(#193 follow-up): per-Lifebook briefing prose#258
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for per-Lifebook (domain-scoped) twin briefings alongside the existing global briefing flow, wiring it end-to-end across DB schema, repository access, worker generation, API retrieval, and the Lifebook UI surface.
Changes:
- Extend
twin_briefingswith nullabledomain_name+ add repository support for creating and querying per-domain briefings. - Update the briefing generator worker to emit per-Lifebook briefings (best-effort, skipping empty domains) and pass
domaininto the briefing-prose prompt. - Add an API endpoint for “latest per-domain briefing” and render it on the Lifebook page (best-effort fetch + empty state).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/policy-prompts/prompts/briefing-prose/v1.md | Adds optional domain framing/scoping instructions to the briefing prose prompt. |
| packages/db/src/repositories/briefing-repository.ts | Adds domain_name support in create + new per-domain “latest” query; scopes global latest to domain_name IS NULL. |
| packages/db/src/migrations/042-briefing-domain.sql | Adds nullable domain_name column + partial index for per-domain lookups. |
| apps/worker/src/jobs/briefing-generator.ts | Generates per-Lifebook briefings by filtering gathered data by lifebook suggested capabilities. |
| apps/api/src/routes/twin-briefings.ts | Adds GET /lifebook/:domain/latest endpoint returning the latest per-domain briefing. |
| apps/web/public/js/api-client.js | Adds fetchLifebookBriefing() client helper for the new endpoint. |
| apps/web/public/js/pages/lifebook.js | Fetches and renders a per-domain briefing card on the Lifebook page with an empty state. |
| apps/worker/src/tests/briefing-generator.test.ts | Adds tests for per-domain emission, skipping empty domains, and resilience to per-lifebook failures. |
| CHANGELOG.md | Documents the feature, migration, and surfaces. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+218
to
+221
| const filteredPromotions = data.promotionResult.rows.filter((r) => { | ||
| const p = r.payload as Record<string, unknown> | null; | ||
| return inSet(typeof p?.['registryId'] === 'string' ? (p['registryId'] as string) : null); | ||
| }); |
Comment on lines
248
to
250
| const rawData = await gatherBriefingData(userId, cadence); | ||
| const data = scope ? filterDataByLifebook(rawData, scope.registryIds) : rawData; | ||
|
|
Comment on lines
+212
to
+214
| // Empty allow-list = "the domain extractor proposed nothing yet"; | ||
| // pass through unchanged rather than emit an empty briefing. | ||
| return data; |
Comment on lines
64
to
69
| * Return the most recently generated briefing for a user, optionally | ||
| * filtered by cadence. | ||
| * filtered by cadence. Only returns *global* briefings (domain_name | ||
| * IS NULL) — the per-Lifebook query lives in | ||
| * `getLatestForUserDomain()` so the existing surface stays bounded | ||
| * to the historical semantic. | ||
| */ |
Comment on lines
+72
to
+83
| router.get('/lifebook/:domain/latest', async (req, res, next) => { | ||
| try { | ||
| const userId = getUserId(req); | ||
| if (!userId) { | ||
| res.status(400).json({ error: 'userId is required' }); | ||
| return; | ||
| } | ||
| const { domain } = req.params; | ||
| if (!domain || domain.length === 0) { | ||
| res.status(400).json({ error: 'domain is required' }); | ||
| return; | ||
| } |
jayzalowitz
added a commit
that referenced
this pull request
May 12, 2026
…owlist footgun, listForUser scope, +6 route tests Five substantive Copilot findings on PR #258 addressed: 1. tier_promotion rows now flow into per-Lifebook briefings. The prior filter checked payload.registryId, but tier_promotion payloads carry { from, to, reason } and reference the server via server_id. gatherBriefingData now computes a server_id → registry_id map and filterDataByLifebook uses it to attribute promotions to the right lifebook. 2. N+1 query pattern eliminated. The orchestrator calls gatherBriefingData ONCE per user; the bundle is shared across the global briefing and every per-Lifebook briefing via a new `preGathered` parameter on generateBriefingProse. Per-lifebook DB cost dropped from 2 repo calls + 1 query per lifebook to "filter the in-memory bundle." 3. Empty-allowlist footgun fixed. filterDataByLifebook now returns an EMPTY bundle when the lifebook has no suggested_capabilities, not the unfiltered global bundle. Previously a scoped call with an empty allowlist would have written a global briefing under the domain's label. 4. listForUser() defaults to global (domain_name IS NULL) — adding the column would otherwise have silently interleaved per-Lifebook rows into existing twin-briefings history surfaces. New opts.includeDomainScoped lets callers opt in. New listForUserDomain() mirror serves the per-domain history. 5. 6 new route-level tests for GET /lifebook/:domain/latest. Test plan: worker 86/86 still green; api 550/550 (+6 new tests). Build clean across db, worker, api. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closes the last of the three #193 Child 1 follow-ups deferred by PR filter shipped in #257, this one). The weekly briefing worker now emits, in addition to the existing global briefing, one per-Lifebook briefing for each visible domain with activity in the window. The lifebook page renders the per-domain briefing when one exists. Migration 042: - Adds nullable domain_name STRING to twin_briefings. NULL preserves the historical global-briefing semantic. A string scopes the briefing to that lifebook's domain. - Partial index (user_id, domain_name, generated_at DESC) WHERE domain_name IS NOT NULL keeps per-domain queries fast without bloating storage for global rows. Repository: - create() accepts optional domainName. - getLatestForUser() now explicitly scopes to domain_name IS NULL (the historical implicit semantic, made explicit). - New getLatestForUserDomain(userId, domain, cadence?). Worker: - After writing the global briefing, fetch visible lifebooks and emit one per-domain briefing per lifebook with matching events. Events filtered by registry_id ∈ lifebook.suggested_capabilities. - Empty filtered sets skipped (no "nothing happened in Health" rows). - Per-lifebook failures caught and logged so one broken domain doesn't take out the rest. - Prompt carries optional {{domain}} input; the template's {{#if domain}} block adds a one-sentence framing for the scoped domain. Deterministic fallback unchanged. API: - New GET /api/twin-briefings/lifebook/:domain/latest?cadence= returns { briefing: TwinBriefingRow | null }. Frontend: - Lifebook page fetches the per-domain briefing best-effort and renders a briefing card. Friendly empty state when none exists yet. Test plan: 3 new vitest cases in briefing-generator.test.ts (per- Lifebook write, no-events skip, per-lifebook failure resilience). worker 86/86 (+3 new), api 547/547, db 189/189. Workspace 70/70 turbo tasks green; build clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…owlist footgun, listForUser scope, +6 route tests Five substantive Copilot findings on PR #258 addressed: 1. tier_promotion rows now flow into per-Lifebook briefings. The prior filter checked payload.registryId, but tier_promotion payloads carry { from, to, reason } and reference the server via server_id. gatherBriefingData now computes a server_id → registry_id map and filterDataByLifebook uses it to attribute promotions to the right lifebook. 2. N+1 query pattern eliminated. The orchestrator calls gatherBriefingData ONCE per user; the bundle is shared across the global briefing and every per-Lifebook briefing via a new `preGathered` parameter on generateBriefingProse. Per-lifebook DB cost dropped from 2 repo calls + 1 query per lifebook to "filter the in-memory bundle." 3. Empty-allowlist footgun fixed. filterDataByLifebook now returns an EMPTY bundle when the lifebook has no suggested_capabilities, not the unfiltered global bundle. Previously a scoped call with an empty allowlist would have written a global briefing under the domain's label. 4. listForUser() defaults to global (domain_name IS NULL) — adding the column would otherwise have silently interleaved per-Lifebook rows into existing twin-briefings history surfaces. New opts.includeDomainScoped lets callers opt in. New listForUserDomain() mirror serves the per-domain history. 5. 6 new route-level tests for GET /lifebook/:domain/latest. Test plan: worker 86/86 still green; api 550/550 (+6 new tests). Build clean across db, worker, api. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
7fb9d89 to
88ebc30
Compare
This was referenced May 13, 2026
jayzalowitz
added a commit
that referenced
this pull request
May 18, 2026
…le web rendering (#335) * v0.6.54.0 feat(#320): per-Lifebook briefing sections fold + collapsible web rendering New repo method briefingRepository.getLatestPerLifebook(userId, cadence?) uses DISTINCT ON (domain_name) for one round-trip regardless of Lifebook count. Hard-filters domain_name IS NOT NULL so global briefings can never accidentally fold into the sections list. GET /api/twin-briefings/latest grew an additive sections[] field: one entry per visible Lifebook that has a per-domain briefing, ordered by Lifebook importance (core -> secondary -> emerging). Lifebooks without a matching briefing are omitted (no empty slots). Web rendering in twin-briefing.js: per-Lifebook sections render as collapsible <details> elements between the global prose section and the history sidebar. First card open by default; rest collapsed to avoid a wall of text. Native browser collapsing, zero JS state. Backend partitioning shipped earlier in #258; this PR is the API fold + web rendering — the dashboard now has a single round-trip to render the full partitioned view. Backward-additive: existing briefing field unchanged; sections always an array (never undefined). /lifebook/:domain/latest untouched. Updated briefing-routes.test.ts to mock the new lifebookRepository + getLatestPerLifebook defaults so pre-#320 tests still pass. 5 new repo tests + 6 new route tests. 290 db / 675 api tests pass. Closes #320 (no deferred scope — mobile rendering tracked under #179). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#320 post-Copilot): docstring scope, parallel-count comment, test count Three Copilot doc findings, all valid: 1. briefingRepository.getLatestPerLifebook docstring overstated scope — it claimed visibility filtering / importance ordering / null placeholders, but those happen later in the API route's join against listVisible. Rewrote docstring to describe what the method actually does (DISTINCT ON over twin_briefings rows with non-null domain_name) and explicitly note the route handles visibility / ordering / omission. 2. Route comment said 'Two parallel queries' but it's actually three (getLatestForUser, getLatestPerLifebook, listVisible). Corrected. 3. CHANGELOG claimed 7 new route tests; the file has 6. Off-by-one I miscounted. Fixed. No functional changes. --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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
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
Closes the last of the three #193 Child 1 follow-ups that PR #242 explicitly deferred (capabilities Lifebook filter shipped in #256, provenance wing filter shipped in #257, this one). The weekly briefing worker now emits, in addition to the existing global briefing, one per-Lifebook briefing for each visible domain with activity in the window. The lifebook page renders the per-domain briefing when one exists.
All three #193 deferred items are now closed end-to-end.
What changed
Migration
packages/db/src/migrations/042-briefing-domain.sql— adds nullabledomain_name STRINGcolumn totwin_briefings. NULL preserves the historical global-briefing semantic untouched. A string value scopes the briefing to that lifebook's domain (matchinglifebooks.domain_name). Partial index(user_id, domain_name, generated_at DESC) WHERE domain_name IS NOT NULLkeeps per-domain queries fast without bloating storage for global rows.Repository
briefingRepository.create()accepts an optionaldomainName.briefingRepository.getLatestForUser()now explicitly scopes todomain_name IS NULL(the historical method always returned global briefings; the semantic is now explicit instead of implicit).briefingRepository.getLatestForUserDomain(userId, domain, cadence?)for the per-Lifebook surface.Worker
apps/worker/src/jobs/briefing-generator.ts:registry_id ∈ lifebook.suggested_capabilities— the same set the domain extractor proposed for that wing.{{domain}}input; the template's{{#if domain}}block adds a one-sentence framing for the scoped domain. Deterministic fallback unchanged.API
GET /api/twin-briefings/lifebook/:domain/latest?cadence=returns{ briefing: TwinBriefingRow | null }. NULL when no per-domain briefing exists yet — the common case while the worker hasn't run, the domain is too new, or it had zero events in the window.Frontend
apps/web/public/js/pages/lifebook.jsfetches the per-domain briefing best-effort and renders it as a card on the Lifebook page. When no briefing exists yet, a friendly empty state explains when one will appear. The fetch is best-effort: the rest of the page renders even if the briefing endpoint is unavailable.What this PR deliberately does NOT do
twin_briefings.read_atand applies to any briefing — but the lifebook page doesn't currently surface a "mark as read" affordance. That's a follow-up UX consideration.Test plan
briefing-generator.test.ts:pnpm build --concurrency=1clean.node --checkclean on the JS files.Notes for reviewers
getLatestForUser()addsAND domain_name IS NULLto the historical query. This makes the historical contract ("global briefings only") explicit. Existing callers were already getting global briefings by default; this just makes that semantic load-bearing rather than incidental.registry_idmatches multiple lifebooks (unusual but legal), it shows up in every matching domain's briefing. That's intentional — the user wants to see Fitbit data in both Health and (e.g.) Productivity if both have it as a suggested capability.{{#if domain}}template syntax is the existing handlebars dialect; the runPrompt layer already handles conditional blocks. Empty-stringdomainfalls through to the global path.🤖 Generated with Claude Code