feat(newsletters): raise AI generation limits for long-form content#809
Conversation
Long source notes and longer generated newsletters were getting capped before the AI could finish. Raise the input ceilings on raw content (20k -> 50k chars) and system-prompt override (10k -> 20k chars), and add a newsletter-specific output cap of 12k tokens so generation isn't truncated mid-flow. Meeting-agenda generation keeps the shared 4k ceiling -- agendas have a 2k character target and shouldn't drift larger. Signed-off-by: Nirav Patel <npatel@linuxfoundation.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughNewsletter generation AI token limits are increased and centralized. Constants for raw content length, system prompt length, and a new AI output token ceiling are updated in the shared constants file. The AiService imports and applies the newsletter-specific token ceiling to newsletter generation requests. ChangesNewsletter AI Token Limits
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR raises newsletter AI generation limits to better support long-form source content and longer generated newsletter bodies, while preserving the existing shorter cap for meeting agenda generation.
Changes:
- Increased newsletter raw content and system prompt override character limits.
- Added a newsletter-specific AI output token cap.
- Updated newsletter AI generation to use the new newsletter token cap instead of the shared agenda cap.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/shared/src/constants/newsletter.constants.ts |
Defines the higher newsletter input limits and newsletter-specific AI max token constant. |
apps/lfx-one/src/server/services/ai.service.ts |
Uses the newsletter-specific max token cap for newsletter generation while leaving agenda generation unchanged. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Actionable comments posted: 0 |
dealako
left a comment
There was a problem hiding this comment.
Code Review — PR #809
Independent review
Correctness ✅
Every claim in the PR description and inline comments checks out against the code on disk:
AI_REQUEST_CONFIG.MAX_TOKENS = 4_000is untouched;ai.service.tsstill binds it to the meeting-agendachat.completionscall (line 84) — agendas keep the 4k ceiling exactly as stated.NEWSLETTER_AI_MAX_TOKENS = 12_000is bound only to the newsletter completion (line 138) — no blast radius to the agenda path.- The
bodyHtmlJSON schema cap inai.service.ts:157is100_000chars, so 12k output tokens (~48k chars at ≈4 chars/token) land well inside the schema before it would push back. The comment is accurate. - Claude Sonnet 4 supports up to 64k output tokens; 12k is safely within range.
- Input-limit enforcement is symmetric: both constants are imported and validated server-side in
newsletter.controller.ts:429,446and used in the Angular form validators innewsletter-generate-drawer.component.ts:48,52,70. No client/server drift.
Security ✅
The diff is pure numeric-constant adjustments — no new code paths, no new sinks, no new HTTP surface. The systemPromptOverride prompt-injection surface is pre-existing; this PR only raises its character cap from 10k to 20k. Not a new concern, but worth noting for awareness.
Performance / cost 💬 (nit)
Raising the output ceiling from 4k to 12k tokens (~3×) and the input from 20k to 50k chars (~2.5×) meaningfully increases per-generation spend. The /generate route currently relies on the shared API-level rate limiter and doesn't have a per-route budget. That's a pre-existing condition — this PR doesn't make it worse in a novel way — but a future ticket for a newsletter-specific rate limit (or per-user budget) would be worth filing once you see real usage data at the new limits. Not blocking.
Test coverage ✅
No automated tests existed for these numeric caps before this PR, so there's nothing regressed. A future test that asserts newsletter.controller.ts rejects a rawContent > 50k chars and accepts one at exactly 50k would be a nice guard — worth a follow-up, not a blocker here.
Style / consistency ✅
The numeric separator (12_000, 50_000, 20_000) matches the existing convention in ai.constants.ts. The NEWSLETTER_AI_MAX_TOKENS name follows the existing AI_REQUEST_CONFIG naming pattern. Comment quality is high — the inline rationale in both files explains the why clearly.
Documentation ✅
The constant comment correctly cross-references the JSON schema cap and the shared agenda cap. The PR description's test plan covers the three meaningful scenarios. No doc gaps found.
AI comment reconciliation
CodeRabbit — zero actionable comments. Agreed: nothing to add from their pass.
Copilot — reviewed both files, zero comments. Agreed.
My independent pass adds one additional observation (cost/rate-limit nit above) that neither bot surfaced, but it's pre-existing context rather than a fault in this change.
Revision tracking
First review on this PR — d88b8040 is the only commit. No prior review rounds to track.
Summary
Hey Nirav! Clean, surgical change — you isolated the newsletter output cap from the shared agenda cap, verified the model's actual token ceiling, and matched the comment arithmetic to the schema constraints. The server-side validation symmetry with the Angular form validators is exactly right.
Blocking issues: 0 | Minor issues: 0 | Nits: 1 (cost/rate-limit awareness on the increased ceilings — pre-existing, not introduced here).
✅ Approved. No blockers or merge conflicts. The one nit is worth a follow-up ticket when you have usage data at the new limits, but it doesn't need to hold this PR.
Summary
NEWSLETTER_AI_MAX_TOKENS = 12_000). Meeting-agenda generation keeps its 4k shared cap.apps/lfx-one/src/server/services/ai.service.ts(protected). Change is a singlemax_tokensbinding swap on the existing newsletter call — no structural change.Test plan